From 0758986600dd44f52c7e7cdb233ce33ab1590e30 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Sun, 27 Sep 2026 23:51:16 -0500 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#27501: mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Dash adaptations: - src/txmempool.cpp: the two new PrioritiseTransaction log lines use LogPrint(BCLog::MEMPOOL, ...) instead of upstream's unconditional LogPrintf, because Dash deliberately gated this message behind the mempool log category in commit a8bef50052 ("Silence/tweak some log output", dash#4102); the old single LogPrint line it replaces was already BCLog::MEMPOOL - src/rpc/mining.cpp: fee_delta RPCResult description says "in duffs" instead of "in satoshis", matching the neighbouring prioritisetransaction help in Dash - src/test/fuzz/rpc.cpp: "getprioritisedtransactions" inserted before Dash's "getrawaddrman" entry to keep the list alphabetical (Dash has getrawaddrman here, upstream at this commit did not) - test/functional/mempool_expiry.py: node.prioritisetransaction(parent_txid, COIN) — Dash's prioritisetransaction RPC has no legacy `dummy` second argument, so upstream's 3-arg call form is dropped to 2 args - test/functional/mining_prioritisetransaction.py: clear_prioritisation() calls node.prioritisetransaction(txid, -delta) (2 args) for the same reason - test/functional/mining_prioritisetransaction.py: kept Dash's self.mocktime / plain getblocktemplate() at the end of run_test instead of upstream's mock_time = int(time.time()) and getblocktemplate({'rules': ['segwit']}) — Dash has no segwit and this file has no `time` import; only the new `assert tx_id not in getprioritisedtransactions()` lines were taken from that hunk - test/functional/mining_prioritisetransaction.py: pre-existing positional prioritisetransaction() calls left as-is where upstream's hunk only used them as context Not applicable to Dash (intentionally omitted): - test/functional/mining_prioritisetransaction.py: test_replacement() and its run_test() call site: the test relies on replace-by-fee (sending a higher-feerate tx spending the same input and expecting the first to leave the mempool). Dash has no RBF — src/validation.cpp rejects any mempool conflict outright with "txn-mempool-conflict" and the comment "RBF doesn't exist in Dash" — so the scenario cannot be reproduced in any Dash counterpart. The helper clear_prioritisation() introduced by the same hunk IS kept, since test_diamond uses it. The additive-delta behaviour test_replacement also covered is still exercised by test_diamond (two deltas applied to txid_c). --- doc/release-notes-27501.md | 3 ++ src/rpc/mining.cpp | 35 +++++++++++++++ src/test/fuzz/rpc.cpp | 1 + src/txmempool.cpp | 27 +++++++++++- src/txmempool.h | 13 ++++++ test/functional/mempool_expiry.py | 12 +++++- .../mining_prioritisetransaction.py | 43 +++++++++++++++++++ 7 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 doc/release-notes-27501.md 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/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() From 3f26cb6d5d70ee46b8267060a663f6f2cbcb9f91 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Sun, 27 Sep 2026 23:54:42 -0500 Subject: [PATCH 2/2] Merge bitcoin/bitcoin#28542: wallet: Check for uninitialized last processed and conflicting heights in MarkConflicted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 782701ce7d31919dba2241ee43b582d8ae5a2541 test: Test loading wallets with conflicts without a chain (Andrew Chow) 4660fc82a1f5cf6eb6404d5268beef5919581661 wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow) Pull request description: `MarkConflicted` assumes that `m_last_block_processed_height` is always valid. However it may not be valid when a chain is not attached, as happens in the wallet tool and during migration. In such situations, when the conflicting height is also negative (which occurs on loading when no chain is available), the calculation of the number of conflict confirms results in a non-negative value which passes the existing check for valid values. This will subsequently hit an assertion in `GetTxDepthInMainChain`. Furthermore, `MarkConflicted` is also only called on loading a transaction whose parent has a stored state of `TxStateConflicted` and was loaded before the child transaction. This depends on the loading order, which for both sqlite and bdb depends on the txids. We can avoid this by explicitly checking that both `m_last_block_processed_height` and `conflicting_height` are non-negative. Both `tool_wallet.py` and `wallet_migration.py` are updated to create wallets with a state that triggers the assertion. Fixes #28510 ACKs for top commit: ryanofsky: Code review ACK 782701ce7d31919dba2241ee43b582d8ae5a2541. Nice catch, and clever test (grinding the txid) furszy: ACK 782701ce Tree-SHA512: 1344e0279ec5413a43a2819d101fb571fbf4821de2d13958a0fdffc99f57082ef3243ec454c8343f97dc02ed1fce8c8b0fd89388420ab2e55618af42ad5630a9 Dash adaptations: - test/functional/wallet_migration.py: conflict was only in run_test() — kept Dash's self.test_hybrid_pubkey() and appended self.test_conflict_txs() (upstream's neighbouring self.test_addressbook() does not exist in Dash) - test/functional/tool_wallet.py + wallet_migration.py: wallet.send(..., add_to_wallet=False, locktime=...) rewritten as wallet.send(..., options={"add_to_wallet": False, "locktime": locktime}) because Dash's send RPC still takes a plain "options" OBJ, not upstream's OBJ_NAMED_PARAMS (matches existing Dash tests e.g. wallet_fundrawtransaction.py, rpc_coinjoin.py) - test/functional/tool_wallet.py + wallet_migration.py: Dash has no RBF (validation.cpp rejects with txn-mempool-conflict), so the conflicting tx cannot be broadcast while the parent sits in the mempool; instead its txid is taken from decoderawtransaction and it is mined straight into a block with self.generateblock(node, output=def_wallet.getnewaddress(), transactions=[conflict_signed]) — same end state (parent and child conflicted at -1, conflict tx at 1 confirmation), and mining to the default wallet keeps the 'conflicts' wallet at 4 transactions / 4 address-book entries - test/functional/tool_wallet.py: expected 'Keypool Size' for the descriptor case is 2 instead of upstream's 8 — upstream's 8 is 1 key per active descriptor over Bitcoin's 8 active descriptors (4 output types x internal/external); Dash has only p2pkh, so GetActiveScriptPubKeyMans() yields 2 descriptors. The legacy value stays 1 (external pool drained by the last getnewaddress, internal pool holding the returned change key), since that code path is identical in Dash --- src/wallet/wallet.cpp | 5 ++- test/functional/tool_wallet.py | 60 +++++++++++++++++++++++++++++ test/functional/wallet_migration.py | 47 ++++++++++++++++++++++ 3 files changed, 111 insertions(+), 1 deletion(-) 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/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()