wallet: Delete `removeprunedfunds` RPC #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.~~

Edit: This will be done in a follow-up, all this PR does is delete the removeprunedfunds RPC.

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, achow101, w0xlt, polespinasa
Concept ACK	rkrux

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.

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

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

davidgumberg commented at 1:51 AM on November 5, 2026:

Good point, I've avoided modifying this for now so marking as resolved.

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

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).

davidgumberg commented at 6:04 PM on November 4, 2026:

I think this is fine as-is, the point of the release-notes guidance is to avoid conflicts between branches, and using the PR number is one way to accomplish that, I dislike using the PR number because it means having to open a PR before adding the release note.

pablomartin4btc commented at 8:17 PM on November 4, 2026:

Yeah, np, I think it's easier to find a release note for a particular PR or to check if a release note was missed and we need to add one. Not a big deal anyways, just a small nit.

I dislike using the PR number because it means having to open a PR before adding the release note.

I do that :)

davidgumberg commented at 1:51 AM on November 5, 2026:

I've avoided the deletion of deletion stuff for now, that can be handled in another PR since I think it will require some invasive changes to migration code, so I'll mark this as resolved.

maflcko commented at 9:32 AM on November 6, 2026:

nit: leftover, see the ralph review: https://git.fish.foo/bitcoin/bitcoin/pulls/36333#author-ralph

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

  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")

davidgumberg commented at 1:50 AM on November 5, 2026:

Thank you, taken.

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

 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?

davidgumberg commented at 1:47 AM on November 5, 2026:

Good catch, I've kept this test and changed it to avoid removeprunedfunds.

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.

rkrux commented at 12:36 PM on November 4, 2026: contributor

Concept ACK

in src/wallet/wallet.cpp:3990 in b43647960e

3983 | -    if (txids_to_delete.size() > 0) {
3984 | -        if (auto res = RemoveTxs(local_wallet_batch, txids_to_delete); !res) {
3985 | -            return util::Error{_("Error: Could not delete watchonly transactions. ") + util::ErrorString(res)};
3986 | -        }
3987 | -    }
3988 | -

achow101 commented at 7:20 PM on November 4, 2026:

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

I don't think that we should drop this. Having extra transactions in the wallet could have unexpected side effects, as noted in https://github.com/bitcoin/bitcoin/pull/36333/changes/b43647960e7e98c40f3c671209d9041ffddda811#r4341195658

We could still drop RemoveTxs and EraseTx if instead of deleting from the database, we never add unnecessary txs in the first place. This could be achieved by using the generic all record prefix delete within LegacyDataSPKM::DeleteRecordsWithDB. We already do a full tx record rewrite in migration, so there would not need to be any changes for ensuring the records are written.

davidgumberg commented at 1:46 AM on November 5, 2026:

I've removed the deletion of deletion stuff from this branch so I'll mark this as resolved, let's discuss in a follow-up PR.

davidgumberg force-pushed on Nov 5, 2026

davidgumberg commented at 1:53 AM on November 5, 2026: contributor

Pushed to rebase, address feedback, and reduce scope of this PR to only removing the removeprunedfunds RPC. Deleting the deletion calls cleanly without maybe having leftover stuff in the migrated wallet may require some somewhat invasive changes to migration, so I want to do that in a separate focused PR.

wallet: rpc: Delete removeprunedfunds 3d8c55734e

doc: release note for removeprunedfunds rpc deletion ec7f64d02e

davidgumberg force-pushed on Nov 5, 2026

DrahtBot removed the label Needs rebase on Nov 5, 2026

davidgumberg renamed this:
~~wallet: Delete `removeprunedfunds` and delete transaction delete code~~
wallet: Delete `removeprunedfunds` RPC
on Nov 5, 2026

pablomartin4btc commented at 7:59 PM on November 5, 2026: member

re-ACK ec7f64d02e370e04354e64fa745fca240bcb3cbe

Thanks for taking the suggestions.

Removed RemoveTxs and EraseTx deletion, will be done in a separate PR/ follow-up.

DrahtBot requested review from rkrux on Nov 5, 2026

achow101 commented at 11:03 PM on November 5, 2026: member

ACK ec7f64d02e370e04354e64fa745fca240bcb3cbe

w0xlt commented at 11:22 PM on November 5, 2026: contributor

ACK ec7f64d02e370e04354e64fa745fca240bcb3cbe

polespinasa commented at 11:46 PM on November 5, 2026: member

untested ACK ec7f64d02e370e04354e64fa745fca240bcb3cbe

Code lgtm.

achow101 merged this on Nov 6, 2026

achow101 closed this on Nov 6, 2026