Merge bitcoin/bitcoin#27501: mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0

67b7fecacd0489809690982c89ba2d0acdca938c [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow)
c1061acb9d502cdf8c6996c818d9a8a281cbe40c [functional test] prioritisation is not removed during replacement and expiry (glozow)
0e5874f0b06114d9b077e0ff582915e4f83059e6 [functional test] getprioritisedtransactions RPC (glozow)
99f8046829f699ff2eace266aa8cea1d9f7cb65a [rpc] add getprioritisedtransactions (glozow)
9e9ca36c80013749faaf2aa777d52bd07d9d24ec [mempool] add GetPrioritisedTransactions (glozow)

Pull request description:

  Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`.

  Motivation / Background
  - `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted.
  - Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen.
  - Since #8448, `mapDeltas` is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart.
  - Note the removal due to block/conflict is only done when `removeForBlock` is called, i.e. when the block is received. If you load a mempool.dat containing `mapDeltas` with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them.
  - Related: #4818 and #6464.
  - There is no way to query the node for not-in-mempool `mapDeltas`. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat.
  - Calling `prioritisetransaction` with an inverse value does not remove it from `mapDeltas`, it just sets the value to 0. It disappears on a restart (`LoadMempool` checks if delta is 0), but that might not happen for a while.

  Added together, if a user calls `prioritisetransaction` very regularly and not all those transactions get mined/conflicted, `mapDeltas` might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are.

ACKs for top commit:
  achow101:
    ACK 67b7fecacd0489809690982c89ba2d0acdca938c
  theStack:
    Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c
  instagibbs:
    code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c
  ajtowns:
    ACK 67b7fecacd0489809690982c89ba2d0acdca938c code review only, some nits

Tree-SHA512: 9df48b622ef27f33db1a2748f682bb3f16abe8172fcb7ac3c1a3e1654121ffb9b31aeaad5570c4162261f7e2ff5b5912ddc61a1b8beac0e9f346a86f5952260a

2 files changed, 76 insertions, 1 deletions

diff --git a/test/functional/mempool_expiry.py b/test/functional/mempool_expiry.py
index 15a5f765df..18b3a8def4 100755
--- a/test/functional/mempool_expiry.py
+++ b/test/functional/mempool_expiry.py
@@ -12,7 +12,10 @@ definable expiry timeout via the '-mempoolexpiry=<n>' command line argument
 
 from datetime import timedelta
 
-from test_framework.messages import DEFAULT_MEMPOOL_EXPIRY_HOURS
+from test_framework.messages import (
+    COIN,
+    DEFAULT_MEMPOOL_EXPIRY_HOURS,
+)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -37,6 +40,10 @@ class MempoolExpiryTest(BitcoinTestFramework):
         parent_utxo = self.wallet.get_utxo(txid=parent_txid)
         independent_utxo = self.wallet.get_utxo()
 
+        # Add prioritisation to this transaction to check that it persists after the expiry
+        node.prioritisetransaction(parent_txid, 0, COIN)
+        assert_equal(node.getprioritisedtransactions()[parent_txid], { "fee_delta" : COIN, "in_mempool" : True})
+
         # Ensure the transactions we send to trigger the mempool check spend utxos that are independent of
         # the transactions being tested for expiration.
         trigger_utxo1 = self.wallet.get_utxo()
@@ -79,6 +86,9 @@ class MempoolExpiryTest(BitcoinTestFramework):
         assert_raises_rpc_error(-5, 'Transaction not in mempool',
                                 node.getmempoolentry, parent_txid)
 
+        # Prioritisation does not disappear when transaction expires
+        assert_equal(node.getprioritisedtransactions()[parent_txid], { "fee_delta" : COIN, "in_mempool" : False})
+
         # The child transaction should be removed from the mempool as well.
         self.log.info('Test child tx is evicted as well.')
         assert_raises_rpc_error(-5, 'Transaction not in mempool',
diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py
index a4481c15a0..099c0e418c 100755
--- a/test/functional/mining_prioritisetransaction.py
+++ b/test/functional/mining_prioritisetransaction.py
@@ -30,6 +30,33 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         ]] * self.num_nodes
         self.supports_cli = False
 
+    def clear_prioritisation(self, node):
+        for txid, info in node.getprioritisedtransactions().items():
+            delta = info["fee_delta"]
+            node.prioritisetransaction(txid, 0, -delta)
+        assert_equal(node.getprioritisedtransactions(), {})
+
+    def test_replacement(self):
+        self.log.info("Test tx prioritisation stays after a tx is replaced")
+        conflicting_input = self.wallet.get_utxo()
+        tx_replacee = self.wallet.create_self_transfer(utxo_to_spend=conflicting_input, fee_rate=Decimal("0.0001"))
+        tx_replacement = self.wallet.create_self_transfer(utxo_to_spend=conflicting_input, fee_rate=Decimal("0.005"))
+        # Add 1 satoshi fee delta to replacee
+        self.nodes[0].prioritisetransaction(tx_replacee["txid"], 0, 100)
+        assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : False}})
+        self.nodes[0].sendrawtransaction(tx_replacee["hex"])
+        assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : True}})
+        self.nodes[0].sendrawtransaction(tx_replacement["hex"])
+        assert tx_replacee["txid"] not in self.nodes[0].getrawmempool()
+        assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : False}})
+
+        # PrioritiseTransaction is additive
+        self.nodes[0].prioritisetransaction(tx_replacee["txid"], 0, COIN)
+        self.nodes[0].sendrawtransaction(tx_replacee["hex"])
+        assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : COIN + 100, "in_mempool" : True}})
+        self.generate(self.nodes[0], 1)
+        assert_equal(self.nodes[0].getprioritisedtransactions(), {})
+
     def test_diamond(self):
         self.log.info("Test diamond-shape package with priority")
         mock_time = int(time.time())
@@ -84,6 +111,13 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         raw_after = self.nodes[0].getrawmempool(verbose=True)
         assert_equal(raw_before[txid_a], raw_after[txid_a])
         assert_equal(raw_before, raw_after)
+        prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
+        assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})
+        assert_equal(prioritisation_map_in_mempool[txid_c], {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True})
+        # Clear prioritisation, otherwise the transactions' fee deltas are persisted to mempool.dat and loaded again when the node
+        # is restarted at the end of this subtest. Deltas are removed when a transaction is mined, but only at that time. We do
+        # not check whether mapDeltas transactions were mined when loading from mempool.dat.
+        self.clear_prioritisation(node=self.nodes[0])
 
         self.log.info("Test priority while txs are not in mempool")
         self.restart_node(0, extra_args=["-nopersistmempool"])
@@ -92,17 +126,26 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         self.nodes[0].prioritisetransaction(txid=txid_b, fee_delta=int(fee_delta_b * COIN))
         self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_1 * COIN))
         self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_2 * COIN))
+        prioritisation_map_not_in_mempool = self.nodes[0].getprioritisedtransactions()
+        assert_equal(prioritisation_map_not_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : False})
+        assert_equal(prioritisation_map_not_in_mempool[txid_c], {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : False})
         for t in [tx_o_a["hex"], tx_o_b["hex"], tx_o_c["hex"], tx_o_d["hex"]]:
             self.nodes[0].sendrawtransaction(t)
         raw_after = self.nodes[0].getrawmempool(verbose=True)
         assert_equal(raw_before[txid_a], raw_after[txid_a])
         assert_equal(raw_before, raw_after)
+        prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
+        assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})
+        assert_equal(prioritisation_map_in_mempool[txid_c], {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True})
 
         # Clear mempool
         self.generate(self.nodes[0], 1)
+        # Prioritisation for transactions is automatically deleted after they are mined.
+        assert_equal(self.nodes[0].getprioritisedtransactions(), {})
 
         # Use default extra_args
         self.restart_node(0)
+        assert_equal(self.nodes[0].getprioritisedtransactions(), {})
 
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
@@ -115,6 +158,10 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         # Test `prioritisetransaction` invalid extra parameters
         assert_raises_rpc_error(-1, "prioritisetransaction", self.nodes[0].prioritisetransaction, '', 0, 0, 0)
 
+        # Test `getprioritisedtransactions` invalid parameters
+        assert_raises_rpc_error(-1, "getprioritisedtransactions",
+                self.nodes[0].getprioritisedtransactions, True)
+
         # Test `prioritisetransaction` invalid `txid`
         assert_raises_rpc_error(-8, "txid must be of length 64 (not 3, for 'foo')", self.nodes[0].prioritisetransaction, txid='foo', fee_delta=0)
         assert_raises_rpc_error(-8, "txid must be hexadecimal string (not 'Zd1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000')", self.nodes[0].prioritisetransaction, txid='Zd1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000', fee_delta=0)
@@ -127,6 +174,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         # Test `prioritisetransaction` invalid `fee_delta`
         assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", self.nodes[0].prioritisetransaction, txid=txid, fee_delta='foo')
 
+        self.test_replacement()
         self.test_diamond()
 
         self.txouts = gen_return_txouts()
@@ -165,9 +213,18 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
                 sizes[i] += mempool[j]['vsize']
             assert sizes[i] > MAX_BLOCK_WEIGHT // 4  # Fail => raise utxo_count
 
+        assert_equal(self.nodes[0].getprioritisedtransactions(), {})
         # add a fee delta to something in the cheapest bucket and make sure it gets mined
         # also check that a different entry in the cheapest bucket is NOT mined
         self.nodes[0].prioritisetransaction(txid=txids[0][0], fee_delta=int(3*base_fee*COIN))
+        assert_equal(self.nodes[0].getprioritisedtransactions(), {txids[0][0] : { "fee_delta" : 3*base_fee*COIN, "in_mempool" : True}})
+
+        # Priority disappears when prioritisetransaction is called with an inverse value...
+        self.nodes[0].prioritisetransaction(txid=txids[0][0], fee_delta=int(-3*base_fee*COIN))
+        assert txids[0][0] not in self.nodes[0].getprioritisedtransactions()
+        # ... and reappears when prioritisetransaction is called again.
+        self.nodes[0].prioritisetransaction(txid=txids[0][0], fee_delta=int(3*base_fee*COIN))
+        assert txids[0][0] in self.nodes[0].getprioritisedtransactions()
 
         self.generate(self.nodes[0], 1)
 
@@ -187,6 +244,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         # Add a prioritisation before a tx is in the mempool (de-prioritising a
         # high-fee transaction so that it's now low fee).
         self.nodes[0].prioritisetransaction(txid=high_fee_tx, fee_delta=-int(2*base_fee*COIN))
+        assert_equal(self.nodes[0].getprioritisedtransactions()[high_fee_tx], { "fee_delta" : -2*base_fee*COIN, "in_mempool" : False})
 
         # Add everything back to mempool
         self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
@@ -206,6 +264,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         mempool = self.nodes[0].getrawmempool()
         self.log.info("Assert that de-prioritised transaction is still in mempool")
         assert high_fee_tx in mempool
+        assert_equal(self.nodes[0].getprioritisedtransactions()[high_fee_tx], { "fee_delta" : -2*base_fee*COIN, "in_mempool" : True})
         for x in txids[2]:
             if (x != high_fee_tx):
                 assert x not in mempool
@@ -223,10 +282,12 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         # to be the minimum for a 1000-byte transaction and check that it is
         # accepted.
         self.nodes[0].prioritisetransaction(txid=tx_id, fee_delta=int(self.relayfee*COIN))
+        assert_equal(self.nodes[0].getprioritisedtransactions()[tx_id], { "fee_delta" : self.relayfee*COIN, "in_mempool" : False})
 
         self.log.info("Assert that prioritised free transaction is accepted to mempool")
         assert_equal(self.nodes[0].sendrawtransaction(tx_hex), tx_id)
         assert tx_id in self.nodes[0].getrawmempool()
+        assert_equal(self.nodes[0].getprioritisedtransactions()[tx_id], { "fee_delta" : self.relayfee*COIN, "in_mempool" : True})
 
         # Test that calling prioritisetransaction is sufficient to trigger
         # getblocktemplate to (eventually) return a new block.
@@ -234,6 +295,10 @@ class PrioritiseTransactionTest(BitcoinTestFramework):
         self.nodes[0].setmocktime(mock_time)
         template = self.nodes[0].getblocktemplate({'rules': ['segwit']})
         self.nodes[0].prioritisetransaction(txid=tx_id, fee_delta=-int(self.relayfee*COIN))
+
+        # Calling prioritisetransaction with the inverse amount should delete its prioritisation entry
+        assert tx_id not in self.nodes[0].getprioritisedtransactions()
+
         self.nodes[0].setmocktime(mock_time+10)
         new_template = self.nodes[0].getblocktemplate({'rules': ['segwit']})