[mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0

It's unnecessary to keep the data around, as it doesn't do anything. If
prioritisetransaction is called again, we'll make a new entry in
mapDeltas.

These entries are only deleted when the transaction is mined or conflicted
from a block (i.e. not in replacement or eviction), are persisted in
mempool.dat, and never expire. If node operators use the RPC to
regularly prioritise/de-prioritise transactions, these (meaningless)
map entries may hang around forever and take up valuable mempool memory.

Author: glozow

src/txmempool.cpp

@@ -870,8 +870,17 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
             }
             ++nTransactionsUpdated;
         }
+        if (delta == 0) {
+            mapDeltas.erase(hash);
+            LogPrintf("PrioritiseTransaction: %s (%sin mempool) delta cleared\n", hash.ToString(), it == mapTx.end() ? "not " : "");
+        } else {
+            LogPrintf("PrioritiseTransaction: %s (%sin mempool) fee += %s, new delta=%s\n",
+                      hash.ToString(),
+                      it == mapTx.end() ? "not " : "",
+                      FormatMoney(nFeeDelta),
+                      FormatMoney(delta));
+        }
     }
-    LogPrintf("PrioritiseTransaction: %s fee += %s\n", hash.ToString(), FormatMoney(nFeeDelta));
 }
 
 void CTxMemPool::ApplyDelta(const uint256& hash, CAmount &nFeeDelta) const

test/functional/mining_prioritisetransaction.py

@@ -30,6 +30,12 @@ def set_test_params(self):
         ]] * 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()
@@ -108,6 +114,10 @@ def test_diamond(self):
         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"])
@@ -135,6 +145,7 @@ def test_diamond(self):
 
         # Use default extra_args
         self.restart_node(0)
+        assert_equal(self.nodes[0].getprioritisedtransactions(), {})
 
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
@@ -202,10 +213,18 @@ def run_test(self):
                 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})
+        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)
 
@@ -277,8 +296,8 @@ def run_test(self):
         template = self.nodes[0].getblocktemplate({'rules': ['segwit']})
         self.nodes[0].prioritisetransaction(txid=tx_id, fee_delta=-int(self.relayfee*COIN))
 
-        assert tx_id in self.nodes[0].getprioritisedtransactions()
-        assert_equal(self.nodes[0].getprioritisedtransactions()[tx_id]["fee_delta"], 0)
+        # 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']})