wallet: Delete `removeprunedfunds` and delete transaction delete code #36333

davidgumberg commented at 12:54 AM on September 25, 2026: contributor

Deprecated in v32 (https://github.com/bitcoin/bitcoin/pull/35605), this deletes the useless and harmful removeprunedfunds RPC.

It also removes deleting of watchonly transactions from the not-watchonly migrated wallet, which in theory we should clean up, but in practice this does nothing for the user, the only observable change is that if the user knows the txid they can do gettransaction and the migrated wallet will return the transaction information even though it does not have keys for the transaction, AFAICT for both GUI and RPC having irrelevant transactions inside of wallet is harmless and does not affect the output of anything, since most stuff is gated behind IsMine() anyways.

Removing both the RPC and the deletion of watchonly tx'es allows deleting the logic for transaction removal and the associated tests.

LLM Disclosure: Not used to write this PR or PR description, but I recently replaced clang on my machine with gemma-4-26B directly generating machine code.

DrahtBot renamed this:
~~wallet: Delete `removeprunedfunds` and delete transaction delete code~~
wallet: Delete `removeprunedfunds` and delete transaction delete code
on Sep 25, 2026

DrahtBot added the label Wallet on Sep 25, 2026

DrahtBot commented at 12:54 AM on September 25, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36333
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36333.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36333#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept ACK	w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36481 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36481.svg"> (wallet, rpc, gui: Allow users to manually rebroadcast a wallet transaction by achow101)
#36440 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36440.svg"> (test: speed up functional tests by removing avoidable waits by ViniciusCestarii)
#36257 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36257.svg"> (qa: assert_equals -> assert_true/assert_false by hodlinator)
#36167 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36167.svg"> ([RFC] Enable -Wunused by fanquake)
#36068 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36068.svg"> (fuzz: reuse one fuzzed wallet across inputs by brunoerg)
#35786 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35786.svg"> (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35569 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35569.svg"> (Encapsulation for CTransaction by purpleKarrot)
#35302 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35302.svg"> (Silent Payments: Sending (take 2) by Eunovo)
#34909 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/34909.svg"> (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

davidgumberg force-pushed on Sep 25, 2026

DrahtBot added the label CI failed on Sep 25, 2026

DrahtBot commented at 12:59 AM on September 25, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/36079638513/job/107898503317 LLM reason (✨ experimental): CI failed due to ruff Python lint errors (F401 unused imports) in test/functional/rpc_deprecated.py and test/functional/wallet_importprunedfunds.py.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

in src/wallet/rpc/wallet.cpp:1072 in 34f9514634 outdated

1068 | @@ -1069,7 +1069,6 @@ RPCMethod walletdisplayaddress();
1069 |  
1070 |  // backup
1071 |  RPCMethod importprunedfunds();
1072 | -RPCMethod removeprunedfunds();

crooksmadison69-cmd commented at 1:17 AM on September 25, 2026:

Gettransaction

bitcoin blocked a user on Sep 25, 2026

DrahtBot removed the label CI failed on Sep 25, 2026

maflcko added this to the milestone 33.0 on Sep 25, 2026

DrahtBot added the label Needs rebase on Sep 26, 2026

w0xlt commented at 1:30 AM on September 30, 2026: contributor

Concept ACK

davidgumberg force-pushed on Oct 9, 2026

davidgumberg commented at 1:05 AM on October 9, 2026: contributor

Rebased to fix merge conflicts.

wallet: rpc: Delete removeprunedfunds 045a0c0c58

wallet: migration: don't prune unnecessary tx'es from DB

While this is nice in theory, it accomplishes very little in practice
for the user, and deleting this allows removal of all transaction
deleting logic.

b43647960e

wallet: qa: Prune unused functions for tx removal

There is never a good reason to delete tx data from a wallet, so delete
these functions.

c7dfd82edb

doc: release note for removeprunedfunds rpc deletion 360625ff4e

davidgumberg force-pushed on Oct 9, 2026

DrahtBot added the label CI failed on Oct 9, 2026

DrahtBot commented at 1:18 AM on October 9, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/37868051410/job/113619418552 LLM reason (✨ experimental): CI failed because Ruff’s Python lint check found an unused find_vout_for_address import in test/functional/wallet_importprunedfunds.py.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label Needs rebase on Oct 9, 2026

DrahtBot removed the label CI failed on Oct 9, 2026

DrahtBot added the label Needs rebase on Oct 13, 2026

DrahtBot commented at 2:13 AM on October 13, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/wallet/wallet.cpp:3907 in 360625ff4e

3905 | @@ -3968,7 +3906,6 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
3906 |                  watchonly_batch->WriteFullTx(data.watchonly_wallet->mapWallet.at(hash));
3907 |                  // Mark as to remove from the migrated wallet only if it does not also belong to it

pablomartin4btc commented at 7:43 PM on October 21, 2026:

Since these txs are no longer deleted, they stay in the migrated wallet. Besides gettransaction, they now count in getwalletinfo's txcount and get rebroadcast by the spendable wallet if unconfirmed. Not a blocker, but maybe worth mentioning in the PR description/release note, and testing the new behavior in wallet_migration.py instead of only dropping the old assertions.

nit: this is not outdated... perhaps it can be something like:

                // Nothing more to do if it does not belong to the migrated wallet

in doc/release-notes-removeprunedfundsremoval.md:1 in 360625ff4e

pablomartin4btc commented at 8:00 PM on October 21, 2026:

nit: according to the release notes guidance this filename should be release-notes-36333.md. Also since we have "New RPCs" section, perhaps a "Removed RPCs" section would make this more noticeable than listing it as an update (feel free to ignore, as previous removals have gone under "Updated RPCs" or "Wallet"). Last thing, not sure if it's worth mentioning, but this also changes migration: watch-only txs are now kept in the migrated wallet (e.g. they're counted in getwalletinfo's txcount).

in test/functional/rpc_deprecated.py:29 in 360625ff4e

  36 | -            assert_raises_rpc_error(
  37 | -                -32, "Start bitcoind with the `-deprecatedrpc=removeprunedfunds`",
  38 | -                wallet.removeprunedfunds,
  39 | -                "fakeargument"
  40 | -            )
  41 | +        return

pablomartin4btc commented at 8:04 PM on October 21, 2026:

nit: before the deprecation commit there were log lines here; restoring them would be cleaner.

        self.log.info("Tests for deprecated RPC methods (if any)")
        self.log.info("Currently no tests for deprecated RPC methods")

in test/functional/wallet_importprunedfunds.py:136 in 360625ff4e

 128 | -        spend_raw = w1.gettransaction(spend_txid)['hex']
 129 | -        spend_proof = self.nodes[0].gettxoutproof([spend_txid])
 130 | -        w1.removeprunedfunds(spend_txid)
 131 | -        assert not [tx for tx in w1.listtransactions() if tx['txid'] == spend_txid]
 132 | -        w1.importprunedfunds(spend_raw, spend_proof)
 133 | -        assert [tx for tx in w1.listtransactions() if tx['txid'] == spend_txid]

pablomartin4btc commented at 8:11 PM on October 21, 2026:

nit: this block tests importprunedfunds for a spending tx with no wallet outputs (#21647, added in c2da52d366); it only used removeprunedfunds to take the tx out of the wallet first. Could it be kept by importing into a fresh wallet that has the key, instead of dropping the coverage?

pablomartin4btc commented at 8:12 PM on October 21, 2026: member

cr ACK 360625ff4e67205d4d1714e00aedbf8be45bb18d

Checked that nothing in the tree still refers to removeprunedfunds, RemoveTxs or EraseTx.

Left some inline nits, no blockers. Needs rebase.