From e5b7785447fc130e8eb6d1a5e8ff051c68b0e8c6 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 25 Jun 2026 14:55:55 -0700 Subject: [PATCH 1/2] test: wallet: resend: avoid internal behavior via removeprunedfunds Currently this test makes assumptions about `listtransaction` exposing a wallet-internal data structure `mapWallet` That is not guaranteed or enforced anywhere. Ideally, this would live in a unit test instead of a functional test, but as a half-measure to simplify the test, just check the behavior 10 times, if there are any dependencies on random ordering inside of a wallet data structure, this is likely to catch them. --- .../wallet_resendwallettransactions.py | 101 +++++++----------- 1 file changed, 36 insertions(+), 65 deletions(-) diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index 1b15fa17e25..b72c79d3620 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -5,8 +5,6 @@ """Test that the wallet resends transactions periodically.""" import time -from decimal import Decimal - from test_framework.blocktools import ( create_block, ) @@ -16,8 +14,6 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_raises_rpc_error, - get_fee, - try_rpc, ) # 36 hours is the upper limit of the resend timer, see CWallet::SetNextResend() @@ -83,72 +79,47 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): peer_second.wait_for_broadcast([txid]) self.log.info("Chain of unconfirmed not-in-mempool txs are rebroadcast") - # This tests that the node broadcasts the parent transaction before the child transaction. - # To test that scenario, we need a method to reliably get a child transaction placed - # in mapWallet positioned before the parent. We cannot predict the position in mapWallet, - # but we can observe it using listreceivedbyaddress and other related RPCs. - # - # So we will create the child transaction, use listreceivedbyaddress to see what the - # ordering of mapWallet is, if the child is not before the parent, we will create a new - # child (via bumpfee) and remove the old child (via removeprunedfunds) until we get the - # ordering of child before parent. - child_inputs = [{"txid": txid, "vout": 0}] - child_txid = node.sendall(recipients=[addr], inputs=child_inputs)["txid"] - # Get the child tx's info for manual bumping - child_tx_info = node.gettransaction(txid=child_txid, verbose=True) - child_output_value = child_tx_info["decoded"]["vout"][0]["value"] - # Include an additional 1 vbyte buffer to handle when we have a smaller signature - additional_child_fee = get_fee(child_tx_info["decoded"]["vsize"] + 1, Decimal(0.00001100)) - while True: - txids = node.listreceivedbyaddress(minconf=0, address_filter=addr)[0]["txids"] - if txids == [child_txid, txid]: - break - # Manually bump the tx - # The inputs and the output address stay the same, just changing the amount for the new fee - child_output_value -= additional_child_fee - bumped_raw = node.createrawtransaction(inputs=child_inputs, outputs=[{addr: child_output_value}]) - bumped = node.signrawtransactionwithwallet(bumped_raw) - bumped_txid = node.decoderawtransaction(bumped["hex"])["txid"] - # Sometimes we will get a signature that is a little bit shorter than we expect which causes the - # feerate to be a bit higher, then the followup to be a bit lower. This results in a replacement - # that can't be broadcast. We can just skip that and keep grinding. - if try_rpc(-26, "insufficient fee, rejecting replacement", node.sendrawtransaction, bumped["hex"]): - continue - # The scheduler queue creates a copy of the added tx after - # send/bumpfee and re-adds it to the wallet (undoing the next - # removeprunedfunds). So empty the scheduler queue: + # We cannot predict the ordering in mapWallet of parent and child, so + # try a few times to get both. + evict_time = 0 + for _ in range(10): + child_inputs = [{"txid": txid, "vout": 0}] + child_txid = node.sendall(recipients=[addr], inputs=child_inputs)["txid"] + # Get the child tx's info for manual bumping + entry_time = node.getmempoolentry(child_txid)["time"] + + # tx must be at least 5 minutes older than the last block to be rebroadcast + block_time = entry_time + 5 * 60 + 1 + node.setmocktime(block_time) + block = create_block(int(node.getbestblockhash(), 16), height=node.getblockcount() + 1, ntime=block_time) + block.solve() + node.submitblock(block.serialize().hex()) + # Set correct m_best_block_time, which is used in ResubmitWalletTransactions node.syncwithvalidationinterfacequeue() - node.removeprunedfunds(child_txid) - child_txid = bumped_txid - entry_time = node.getmempoolentry(child_txid)["time"] - # tx must be at least 5 minutes older than the last block to be rebroadcast - block_time = entry_time + 6 * 60 - node.setmocktime(block_time) - block = create_block(int(node.getbestblockhash(), 16), height=node.getblockcount() + 1, ntime=block_time) - block.solve() - node.submitblock(block.serialize().hex()) - # Set correct m_best_block_time, which is used in ResubmitWalletTransactions - node.syncwithvalidationinterfacequeue() + evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5 + # Flush out currently scheduled resubmit attempt now so that there can't be one right between eviction and check. + with node.assert_debug_log(['resubmit 2 unconfirmed transactions'], timeout=2): + node.setmocktime(evict_time) + node.mockscheduler(60) - evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5 - # Flush out currently scheduled resubmit attempt now so that there can't be one right between eviction and check. - with node.assert_debug_log(['resubmit 2 unconfirmed transactions'], timeout=2): - node.setmocktime(evict_time) - node.mockscheduler(60) + # Evict these txs from the mempool + indep_send = node.send(outputs=[{node.getnewaddress(): 1}], inputs=[indep_utxo]) + node.getmempoolentry(indep_send["txid"]) + assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid) + assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid) - # Evict these txs from the mempool - indep_send = node.send(outputs=[{node.getnewaddress(): 1}], inputs=[indep_utxo]) - node.getmempoolentry(indep_send["txid"]) - assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid) - assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid) + # Rebroadcast and check that parent and child are both in the mempool + with node.assert_debug_log(['resubmit 2 unconfirmed transactions'], timeout=2): + node.setmocktime(evict_time + RESEND_TIMER_LIMIT) + node.mockscheduler(60) + node.getmempoolentry(txid) + node.getmempoolentry(child_txid) - # Rebroadcast and check that parent and child are both in the mempool - with node.assert_debug_log(['resubmit 2 unconfirmed transactions'], timeout=2): - node.setmocktime(evict_time + RESEND_TIMER_LIMIT) - node.mockscheduler(60) - node.getmempoolentry(txid) - node.getmempoolentry(child_txid) + # clear mempool + self.generate(node, 1, sync_fun=self.no_op) + parent_utxo, indep_utxo = node.listunspent()[:2] + txid = node.send(outputs=[{addr: 1}], inputs=[parent_utxo])["txid"] self.log.info("Test rebroadcast of transactions received by others") # clear mempool From f280f5eb47497f53ff997e7d3bec9667cb60339a Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 25 Jun 2026 22:23:59 +0000 Subject: [PATCH 2/2] wallet: rpc: deprecate removeprunedfunds This RPC has no helpful use while being both dangerous and a maintenance burden. Despite what the name says, it allows the deletion of arbitrary transactions, and `importprunedfunds` does not allow the importing of transactions not belonging to the user, and `listtransactions` does not list transactions not belonging to the wallet, so this RPC can only be used to delete transactions actually belonging to the wallet, and in the unlikely event that transactions not belonging to the wallet are present, they cause no harm except for occupying a few bytes on the users disk. --- doc/release-notes-removeprunedfunds.md | 6 ++++++ src/wallet/rpc/backup.cpp | 5 +++++ test/functional/rpc_deprecated.py | 14 +++++++++++++- test/functional/wallet_importprunedfunds.py | 1 + 4 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 doc/release-notes-removeprunedfunds.md diff --git a/doc/release-notes-removeprunedfunds.md b/doc/release-notes-removeprunedfunds.md new file mode 100644 index 00000000000..0e0d0ceb3c9 --- /dev/null +++ b/doc/release-notes-removeprunedfunds.md @@ -0,0 +1,6 @@ +Updated RPCs +------------ + +- The `removeprunedfunds` RPC has been deprecated and will be removed in the +next major release. In order to continue using it, `bitcoind` must be started +with the `-deprecatedrpc=removeprunedfunds` option. diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 396be628259..acfb78547a3 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -95,6 +95,7 @@ RPCMethod removeprunedfunds() { return RPCMethod{ "removeprunedfunds", + "(DEPRECATED) This feature will be removed in the next major release. Start bitcoind with the `-deprecatedrpc=removeprunedfunds` option in order to use this.\n" "Deletes the specified transaction from the wallet. Meant for use with pruned wallets and as a companion to importprunedfunds. This will affect wallet balances.\n", { {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex-encoded id of the transaction you are deleting"}, @@ -110,6 +111,10 @@ RPCMethod removeprunedfunds() std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return UniValue::VNULL; + if (!pwallet->chain().rpcEnableDeprecated("removeprunedfunds")) { + throw JSONRPCError(RPC_METHOD_DEPRECATED, "DEPRECATION WARNING: This feature will be removed in the next major release. Start bitcoind with the `-deprecatedrpc=removeprunedfunds` option in order to use this."); + } + LOCK(pwallet->cs_wallet); Txid hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))}; diff --git a/test/functional/rpc_deprecated.py b/test/functional/rpc_deprecated.py index 2f86954f22b..dd7f6002474 100755 --- a/test/functional/rpc_deprecated.py +++ b/test/functional/rpc_deprecated.py @@ -4,6 +4,8 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test deprecation of RPC calls.""" from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_raises_rpc_error + class DeprecatedRpcTest(BitcoinTestFramework): def set_test_params(self): @@ -26,7 +28,17 @@ class DeprecatedRpcTest(BitcoinTestFramework): # Please don't delete nor modify this comment self.log.info("Tests for deprecated RPC methods (if any)") - self.log.info("Currently no tests for deprecated RPC methods") + if self.is_wallet_compiled(): + self.log.info("Tests for deprecated wallet-related RPC methods (if any)") + self.nodes[0].createwallet("ancient_wallet") + wallet = self.nodes[0].get_wallet_rpc("ancient_wallet") + + self.log.info("Test removeprunedfunds deprecation") + assert_raises_rpc_error( + -32, "Start bitcoind with the `-deprecatedrpc=removeprunedfunds`", + wallet.removeprunedfunds, + "fakeargument" + ) if __name__ == '__main__': diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py index 95e2a5b3a4f..6c1c640645f 100755 --- a/test/functional/wallet_importprunedfunds.py +++ b/test/functional/wallet_importprunedfunds.py @@ -25,6 +25,7 @@ class ImportPrunedFundsTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 2 + self.extra_args = [["-deprecatedrpc=removeprunedfunds"]] * 2 def skip_test_if_missing_module(self): self.skip_if_no_wallet()