From 3a118e19e100110300d3290d4c1434f963721d94 Mon Sep 17 00:00:00 2001 From: kevkevin Date: Mon, 13 Nov 2023 12:48:16 -0600 Subject: [PATCH 1/5] test: Directly constructing 2 entry map for getprioritisedtransactions Directly constructing the map in the assertion instead of indexing by txid so that we wouldn't miss new entries in future regressions. --- test/functional/mining_prioritisetransaction.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py index 099c0e418c0..12be53af6b6 100755 --- a/test/functional/mining_prioritisetransaction.py +++ b/test/functional/mining_prioritisetransaction.py @@ -111,9 +111,7 @@ 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}) + assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True}, 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. @@ -126,17 +124,13 @@ 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}) + assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : False}, 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}) + assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True}, txid_c: {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True}}) # Clear mempool self.generate(self.nodes[0], 1) From 2fca6c2dd03c3955d86efb0b8d2a7961e42115fd Mon Sep 17 00:00:00 2001 From: kevkevin Date: Mon, 13 Nov 2023 13:39:29 -0600 Subject: [PATCH 2/5] rpc: changed prioritisation-map -> "" prioritisation-map gets eaten by the help generator to be "" so we are setting to "" to begin with --- src/rpc/mining.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 76170c32018..2f1872c29b9 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -487,7 +487,7 @@ static RPCHelpMan getprioritisedtransactions() "Returns a map of all user-created (see prioritisetransaction) fee deltas by txid, and whether the tx is present in mempool.", {}, RPCResult{ - RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by txid", + RPCResult::Type::OBJ_DYN, "", "prioritisation keyed by txid", { {RPCResult::Type::OBJ, "txid", "", { {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"}, From 252a86729a15e47ed168d8da7c4a8d6113673909 Mon Sep 17 00:00:00 2001 From: kevkevin Date: Mon, 8 Jan 2024 19:01:45 -0600 Subject: [PATCH 3/5] rpc: renaming txid -> transactionid renamed to transactionid because it is named this way in getrawmempool and getmempoolancestors --- src/rpc/mining.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 2f1872c29b9..0949ddb5767 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -489,7 +489,7 @@ static RPCHelpMan getprioritisedtransactions() RPCResult{ RPCResult::Type::OBJ_DYN, "", "prioritisation keyed by txid", { - {RPCResult::Type::OBJ, "txid", "", { + {RPCResult::Type::OBJ, "", "", { {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"}, {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"}, }} From cfdbcd19b32fd63954d7947dcc639aef291fb6b2 Mon Sep 17 00:00:00 2001 From: kevkevin Date: Mon, 8 Jan 2024 18:59:39 -0600 Subject: [PATCH 4/5] rpc: exposing modified_fee in getprioritisedtransactions Instead of having modified_fee be hidden we are now exposing it to avoid having useless code --- src/rpc/mining.cpp | 4 ++++ test/functional/mempool_expiry.py | 5 +++-- test/functional/mining_prioritisetransaction.py | 14 +++++++------- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 0949ddb5767..6484b9f3cab 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -492,6 +492,7 @@ static RPCHelpMan getprioritisedtransactions() {RPCResult::Type::OBJ, "", "", { {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"}, {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"}, + {RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "modified fee in satoshis. Only returned if in_mempool=true"}, }} }, }, @@ -508,6 +509,9 @@ static RPCHelpMan getprioritisedtransactions() UniValue result_inner{UniValue::VOBJ}; result_inner.pushKV("fee_delta", delta_info.delta); result_inner.pushKV("in_mempool", delta_info.in_mempool); + if (delta_info.in_mempool) { + result_inner.pushKV("modified_fee", *delta_info.modified_fee); + } rpc_result.pushKV(delta_info.txid.GetHex(), result_inner); } return rpc_result; diff --git a/test/functional/mempool_expiry.py b/test/functional/mempool_expiry.py index 18b3a8def43..5eebe43488a 100755 --- a/test/functional/mempool_expiry.py +++ b/test/functional/mempool_expiry.py @@ -36,13 +36,14 @@ class MempoolExpiryTest(BitcoinTestFramework): node = self.nodes[0] # Send a parent transaction that will expire. - parent_txid = self.wallet.send_self_transfer(from_node=node)['txid'] + parent = self.wallet.send_self_transfer(from_node=node) + parent_txid = parent["txid"] 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}) + assert_equal(node.getprioritisedtransactions()[parent_txid], { "fee_delta" : COIN, "in_mempool" : True, "modified_fee": COIN + COIN * parent["fee"] }) # Ensure the transactions we send to trigger the mempool check spend utxos that are independent of # the transactions being tested for expiration. diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py index 12be53af6b6..19179d1e3c9 100755 --- a/test/functional/mining_prioritisetransaction.py +++ b/test/functional/mining_prioritisetransaction.py @@ -45,7 +45,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework): 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}}) + assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : True, "modified_fee": int(tx_replacee["fee"] * COIN + 100)}}) 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}}) @@ -53,7 +53,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework): # 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}}) + assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : COIN + 100, "in_mempool" : True, "modified_fee": int(tx_replacee["fee"] * COIN + COIN + 100)}}) self.generate(self.nodes[0], 1) assert_equal(self.nodes[0].getprioritisedtransactions(), {}) @@ -111,7 +111,7 @@ 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) - assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True}, txid_c: {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True}}) + assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True, "modified_fee": int(fee_delta_b*COIN + COIN * tx_o_b["fee"])}, txid_c: {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True, "modified_fee": int((fee_delta_c_1 + fee_delta_c_2 ) * COIN + COIN * tx_o_c["fee"])}}) # 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. @@ -130,7 +130,7 @@ 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) - assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True}, txid_c: {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True}}) + assert_equal(self.nodes[0].getprioritisedtransactions(), {txid_b: {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True, "modified_fee": int(fee_delta_b*COIN + COIN * tx_o_b["fee"])}, txid_c: {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True, "modified_fee": int((fee_delta_c_1 + fee_delta_c_2 ) * COIN + COIN * tx_o_c["fee"])}}) # Clear mempool self.generate(self.nodes[0], 1) @@ -211,7 +211,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework): # 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, "modified_fee": int(3*base_fee*COIN + COIN * 1 * base_fee)}}) # 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)) @@ -258,7 +258,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}) + assert_equal(self.nodes[0].getprioritisedtransactions()[high_fee_tx], { "fee_delta" : -2*base_fee*COIN, "in_mempool" : True, "modified_fee": int(-2*base_fee*COIN + COIN * 3 * base_fee)}) for x in txids[2]: if (x != high_fee_tx): assert x not in mempool @@ -281,7 +281,7 @@ class PrioritiseTransactionTest(BitcoinTestFramework): 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}) + assert_equal(self.nodes[0].getprioritisedtransactions()[tx_id], { "fee_delta" : self.relayfee*COIN, "in_mempool" : True, "modified_fee": int(self.relayfee*COIN + COIN * tx_res["fee"])}) # Test that calling prioritisetransaction is sufficient to trigger # getblocktemplate to (eventually) return a new block. From 0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e Mon Sep 17 00:00:00 2001 From: kevkevin Date: Tue, 14 Nov 2023 16:11:25 -0600 Subject: [PATCH 5/5] test: Assert that a new tx with a delta of 0 is never added --- test/functional/mining_prioritisetransaction.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py index 19179d1e3c9..c5f34e3ecbf 100755 --- a/test/functional/mining_prioritisetransaction.py +++ b/test/functional/mining_prioritisetransaction.py @@ -263,6 +263,12 @@ class PrioritiseTransactionTest(BitcoinTestFramework): if (x != high_fee_tx): assert x not in mempool + + self.log.info("Assert that 0 delta is never added to mapDeltas") + tx_id_zero_del = self.wallet.create_self_transfer()['txid'] + self.nodes[0].prioritisetransaction(txid=tx_id_zero_del, fee_delta=0) + assert tx_id_zero_del not in self.nodes[0].getprioritisedtransactions() + # Create a free transaction. Should be rejected. tx_res = self.wallet.create_self_transfer(fee_rate=0) tx_hex = tx_res['hex']