diff --git a/doc/release-notes-27501.md b/doc/release-notes-27501.md new file mode 100644 index 000000000000..386a00fb34d3 --- /dev/null +++ b/doc/release-notes-27501.md @@ -0,0 +1,3 @@ +- A new `getprioritisedtransactions` RPC has been added. It returns a map of all fee deltas created by the + user with prioritisetransaction, indexed by txid. The map also indicates whether each transaction is + present in the mempool. diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 1e0dedae76fe..91a353b21fc3 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -527,6 +527,40 @@ static RPCHelpMan prioritisetransaction() }; } +static RPCHelpMan getprioritisedtransactions() +{ + return 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, "txid", "", { + {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in duffs"}, + {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"}, + }} + }, + }, + RPCExamples{ + HelpExampleCli("getprioritisedtransactions", "") + + HelpExampleRpc("getprioritisedtransactions", "") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + NodeContext& node = EnsureAnyNodeContext(request.context); + CTxMemPool& mempool = EnsureMemPool(node); + UniValue rpc_result{UniValue::VOBJ}; + for (const auto& delta_info : mempool.GetPrioritisedTransactions()) { + UniValue result_inner{UniValue::VOBJ}; + result_inner.pushKV("fee_delta", delta_info.delta); + result_inner.pushKV("in_mempool", delta_info.in_mempool); + rpc_result.pushKV(delta_info.txid.GetHex(), result_inner); + } + return rpc_result; + }, + }; +} + // NOTE: Assumes a conclusive result; if result is inconclusive, it must be handled by caller static UniValue BIP22ValidationResult(const BlockValidationState& state) @@ -1110,6 +1144,7 @@ void RegisterMiningRPCCommands(CRPCTable& t) {"mining", &getnetworkhashps}, {"mining", &getmininginfo}, {"mining", &prioritisetransaction}, + {"mining", &getprioritisedtransactions}, {"mining", &getblocktemplate}, {"mining", &submitblock}, {"mining", &submitheader}, diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 97580e867908..ad8e84e58618 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -136,6 +136,7 @@ const std::vector RPC_COMMANDS_SAFE_FOR_FUZZING{ "getnetworkinfo", "getnodeaddresses", "getpeerinfo", + "getprioritisedtransactions", "getrawaddrman", "getrawmempool", "getrawtransaction", diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 37c1eb10dedc..997232205807 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1806,8 +1806,17 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD } ++nTransactionsUpdated; } + if (delta == 0) { + mapDeltas.erase(hash); + LogPrint(BCLog::MEMPOOL, "PrioritiseTransaction: %s (%sin mempool) delta cleared\n", hash.ToString(), it == mapTx.end() ? "not " : ""); + } else { + LogPrint(BCLog::MEMPOOL, "PrioritiseTransaction: %s (%sin mempool) fee += %s, new delta=%s\n", + hash.ToString(), + it == mapTx.end() ? "not " : "", + FormatMoney(nFeeDelta), + FormatMoney(delta)); + } } - LogPrint(BCLog::MEMPOOL, "PrioritiseTransaction: %s fee += %s\n", hash.ToString(), FormatMoney(nFeeDelta)); } void CTxMemPool::ApplyDelta(const uint256& hash, CAmount &nFeeDelta) const @@ -1826,6 +1835,22 @@ void CTxMemPool::ClearPrioritisation(const uint256& hash) mapDeltas.erase(hash); } +std::vector CTxMemPool::GetPrioritisedTransactions() const +{ + AssertLockNotHeld(cs); + LOCK(cs); + std::vector result; + result.reserve(mapDeltas.size()); + for (const auto& [txid, delta] : mapDeltas) { + const auto iter{mapTx.find(txid)}; + const bool in_mempool{iter != mapTx.end()}; + std::optional modified_fee; + if (in_mempool) modified_fee = iter->GetModifiedFee(); + result.emplace_back(delta_info{in_mempool, delta, modified_fee, txid}); + } + return result; +} + const CTransaction* CTxMemPool::GetConflictTx(const COutPoint& prevout) const { const auto it = mapNextTx.find(prevout); diff --git a/src/txmempool.h b/src/txmempool.h index 0ecf46d66dd9..08f94c60320e 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -558,6 +558,19 @@ class CTxMemPool void ApplyDelta(const uint256& hash, CAmount &nFeeDelta) const EXCLUSIVE_LOCKS_REQUIRED(cs); void ClearPrioritisation(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs); + struct delta_info { + /** Whether this transaction is in the mempool. */ + const bool in_mempool; + /** The fee delta added using PrioritiseTransaction(). */ + const CAmount delta; + /** The modified fee (base fee + delta) of this entry. Only present if in_mempool=true. */ + std::optional modified_fee; + /** The prioritised transaction's txid. */ + const uint256 txid; + }; + /** Return a vector of all entries in mapDeltas with their corresponding delta_info. */ + std::vector GetPrioritisedTransactions() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + /** Get the transaction in the pool that spends the same prevout */ const CTransaction* GetConflictTx(const COutPoint& prevout) const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 14a79964e222..49a9dee28c2a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1374,11 +1374,14 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c { LOCK(cs_wallet); - int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1; // If number of conflict confirms cannot be determined, this means // that the block is still unknown or not yet part of the main chain, // for example when loading the wallet during a reindex. Do nothing in that // case. + if (m_last_block_processed_height < 0 || conflicting_height < 0) { + return; + } + int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1; if (conflictconfirms >= 0) return; diff --git a/test/functional/mempool_expiry.py b/test/functional/mempool_expiry.py index 15a5f765df82..c864f2e6ee0b 100755 --- a/test/functional/mempool_expiry.py +++ b/test/functional/mempool_expiry.py @@ -12,7 +12,10 @@ 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 @@ def test_transaction_expiry(self, timeout): 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, 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 @@ def test_transaction_expiry(self, timeout): 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 e0ce8b17ac9e..b1fc8b65f9f4 100755 --- a/test/functional/mining_prioritisetransaction.py +++ b/test/functional/mining_prioritisetransaction.py @@ -29,6 +29,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, -delta) + assert_equal(node.getprioritisedtransactions(), {}) + def test_diamond(self): self.log.info("Test diamond-shape package with priority") self.nodes[0].setmocktime(self.mocktime) @@ -82,6 +88,13 @@ def test_diamond(self): 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"]) @@ -90,17 +103,26 @@ def test_diamond(self): 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]) @@ -112,6 +134,10 @@ def run_test(self): # Test `prioritisetransaction` invalid extra parameters assert_raises_rpc_error(-1, "prioritisetransaction", self.nodes[0].prioritisetransaction, '', 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) @@ -159,9 +185,18 @@ def run_test(self): sizes[i] += mempool[j]['vsize'] assert sizes[i] > MAX_BLOCK_SIZE # 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(txids[0][0], 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) @@ -181,6 +216,7 @@ def run_test(self): # 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(high_fee_tx, -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()) @@ -200,6 +236,7 @@ def run_test(self): 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 @@ -217,16 +254,22 @@ def run_test(self): # to be the minimum for a 1000-byte transaction and check that it is # accepted. self.nodes[0].prioritisetransaction(tx_id, 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. self.nodes[0].setmocktime(self.mocktime) template = self.nodes[0].getblocktemplate() self.nodes[0].prioritisetransaction(tx_id, -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(self.mocktime+10) new_template = self.nodes[0].getblocktemplate() diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 38a48fc9f657..9be4ed4c6d93 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -402,6 +402,65 @@ def test_dump_createfromdump(self): self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump') assert not os.path.isdir(os.path.join(self.nodes[0].datadir, "regtest/wallets", "badload")) + def test_chainless_conflicts(self): + self.log.info("Test wallet tool when wallet contains conflicting transactions") + self.restart_node(0) + self.generate(self.nodes[0], 101) + + def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + self.nodes[0].createwallet("conflicts") + wallet = self.nodes[0].get_wallet_rpc("conflicts") + def_wallet.sendtoaddress(wallet.getnewaddress(), 10) + self.generate(self.nodes[0], 1) + + # parent tx + parent_txid = wallet.sendtoaddress(wallet.getnewaddress(), 9) + parent_txid_bytes = bytes.fromhex(parent_txid)[::-1] + conflict_utxo = wallet.gettransaction(txid=parent_txid, verbose=True)["decoded"]["vin"][0] + + # The specific assertion in MarkConflicted being tested requires that the parent tx is already loaded + # by the time the child tx is loaded. Since transactions end up being loaded in txid order due to how both + # and sqlite store things, we can just grind the child tx until it has a txid that is greater than the parent's. + locktime = 500000000 # Use locktime as nonce, starting at unix timestamp minimum + addr = wallet.getnewaddress() + while True: + child_send_res = wallet.send(outputs=[{addr: 8}], options={"add_to_wallet": False, "locktime": locktime}) + child_txid = child_send_res["txid"] + child_txid_bytes = bytes.fromhex(child_txid)[::-1] + if (child_txid_bytes > parent_txid_bytes): + wallet.sendrawtransaction(child_send_res["hex"]) + break + locktime += 1 + + # conflict with parent + conflict_unsigned = self.nodes[0].createrawtransaction(inputs=[conflict_utxo], outputs=[{wallet.getnewaddress(): 9.9999}]) + conflict_signed = wallet.signrawtransactionwithwallet(conflict_unsigned)["hex"] + conflict_txid = self.nodes[0].decoderawtransaction(conflict_signed)["txid"] + # Dash has no RBF, so the conflicting tx cannot be broadcast while the parent is in + # the mempool. Mine it directly into a block instead, which conflicts the parent and + # the child just the same. + self.generateblock(self.nodes[0], output=def_wallet.getnewaddress(), transactions=[conflict_signed]) + assert_equal(wallet.gettransaction(txid=parent_txid)["confirmations"], -1) + assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) + assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) + + self.stop_node(0) + + # Wallet tool should successfully give info for this wallet + expected_output = textwrap.dedent(f'''\ + Wallet info + =========== + Name: conflicts + Format: {"sqlite" if self.options.descriptors else "bdb"} + Descriptors: {"yes" if self.options.descriptors else "no"} + Encrypted: no + HD (hd seed available): yes + Keypool Size: {"2" if self.options.descriptors else "1"} + Transactions: 4 + Address Book: 4 + ''') + self.assert_tool_output(expected_output, "-wallet=conflicts", "info") def test_nonhd(self): self.log.info('Check non-hd wallet') @@ -427,6 +486,7 @@ def run_test(self): self.test_wipe() self.test_nonhd() self.test_dump_createfromdump() + self.test_chainless_conflicts() if __name__ == '__main__': ToolWalletTest().main() diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 98da6442a467..0899c7d3cb5f 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -650,6 +650,52 @@ def test_hybrid_pubkey(self): wallet.unloadwallet() + def test_conflict_txs(self): + self.log.info("Test migration when wallet contains conflicting transactions") + def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + wallet = self.create_legacy_wallet("conflicts") + def_wallet.sendtoaddress(wallet.getnewaddress(), 10) + self.generate(self.nodes[0], 1) + + # parent tx + parent_txid = wallet.sendtoaddress(wallet.getnewaddress(), 9) + parent_txid_bytes = bytes.fromhex(parent_txid)[::-1] + conflict_utxo = wallet.gettransaction(txid=parent_txid, verbose=True)["decoded"]["vin"][0] + + # The specific assertion in MarkConflicted being tested requires that the parent tx is already loaded + # by the time the child tx is loaded. Since transactions end up being loaded in txid order due to how both + # and sqlite store things, we can just grind the child tx until it has a txid that is greater than the parent's. + locktime = 500000000 # Use locktime as nonce, starting at unix timestamp minimum + addr = wallet.getnewaddress() + while True: + child_send_res = wallet.send(outputs=[{addr: 8}], options={"add_to_wallet": False, "locktime": locktime}) + child_txid = child_send_res["txid"] + child_txid_bytes = bytes.fromhex(child_txid)[::-1] + if (child_txid_bytes > parent_txid_bytes): + wallet.sendrawtransaction(child_send_res["hex"]) + break + locktime += 1 + + # conflict with parent + conflict_unsigned = self.nodes[0].createrawtransaction(inputs=[conflict_utxo], outputs=[{wallet.getnewaddress(): 9.9999}]) + conflict_signed = wallet.signrawtransactionwithwallet(conflict_unsigned)["hex"] + conflict_txid = self.nodes[0].decoderawtransaction(conflict_signed)["txid"] + # Dash has no RBF, so the conflicting tx cannot be broadcast while the parent is in + # the mempool. Mine it directly into a block instead, which conflicts the parent and + # the child just the same. + self.generateblock(self.nodes[0], output=def_wallet.getnewaddress(), transactions=[conflict_signed]) + assert_equal(wallet.gettransaction(txid=parent_txid)["confirmations"], -1) + assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) + assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) + + wallet.migratewallet() + assert_equal(wallet.gettransaction(txid=parent_txid)["confirmations"], -1) + assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) + assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) + + wallet.unloadwallet() + def run_test(self): self.generate(self.nodes[0], 101) @@ -667,6 +713,7 @@ def run_test(self): self.test_direct_file() self.test_migrate_raw_p2sh() self.test_hybrid_pubkey() + self.test_conflict_txs() if __name__ == '__main__': WalletMigrationTest().main()