#36333 wallet: Delete `removeprunedfunds` and delete transaction delete code
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36333 wallet: Delete `removeprunedfunds` and delete transaction delete code | Waiting on author | 1 | Strong | 5 + 2 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Removes the `removeprunedfunds` RPC and deletes all underlying wallet transaction-removal code (`CWallet::RemoveTxs` and `WalletBatch::EraseTx`). Because the deletion infrastructure is removed, legacy-to-descriptor wallet migration no longer erases watch-only transactions from the migrated non-watch-only wallet. Deprecation warnings, test cases for transaction deletion, and release notes are updated accordingly.
Problem: The `removeprunedfunds` RPC was deprecated in v32 and retained dangerous transaction deletion logic that required complex synchronization between in-memory structures and wallet storage. Furthermore, during legacy wallet migration, deleting watch-only transactions from the main wallet added database overhead and complexity for no practical benefit.
Evidence of importance:
- The RPC was deprecated in v32 (PR #35605).
- Author commit message (c7dfd82edb): "There is never a good reason to delete tx data from a wallet, so delete these functions."
- Author PR description: "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."
Scope: Deletes the RPC endpoint, wallet internal removal routines, and associated functional/unit tests.
Risk of a mistake: medium (wallet, wallet_storage, rpc_api). Leaving unowned transactions in migrated wallets or modifying wallet database manipulation code could break wallet accounting or transaction reporting if any RPCs fail to filter by IsMine.
Changed since the previous assessment: Added release notes documenting the removal of `removeprunedfunds`.
Reviewability
Paused · Waiting on author
Open objections from pablomartin4btc with no author reply for 11 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: rebase and address review comments 2026-10-21
Agreement
Strong
substantive support, no open objection (pablomartin4btc)
Thread notes: pablomartin4btc cr ACKed while leaving non-blocking test suggestions and noting a rebase is needed.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| pablomartin4btc (member) | suggestion, maintenance | Test the new migration behavior in wallet_migration.py and mention in the PR description or release note that kept watch-only txs count in txcount and are rebroadcast if unconfirmed | open | answered | no | 2026-10-21: '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' |
| pablomartin4btc (member) | suggestion, maintenance | Keep coverage for importprunedfunds with no wallet outputs by importing into a fresh wallet with the key instead of dropping the test | open | answered | no | 2026-10-21: 'Could it be kept by importing into a fresh wallet that has the key, instead of dropping the coverage?' |
Support:
- w0xlt (contributor) (Concept ACK, 2026-09-30): (no reason given) [not substantive]
- pablomartin4btc (member) (ACK, 2026-10-21) [read]: Checked that nothing in the tree still refers to `removeprunedfunds`, `RemoveTxs` or `EraseTx`.
Participants:
- crooksmadison69-cmd (none, neutral, 1 statements 2026-09-25 to 2026-09-25): left a single-word inline comment
- w0xlt (contributor, support, 1 statements 2026-09-30 to 2026-09-30): Concept ACK
- pablomartin4btc (member, support, 5 statements 2026-10-21 to 2026-10-21): cr ACK; checked for references to removed symbols and left suggestions on tests and notes, needs rebase
Files
109 lines under test/bench/ci.
- src/wallet/wallet.cpp +0/-70
- test/functional/wallet_importprunedfunds.py +1/-56
- src/wallet/rpc/backup.cpp +0/-39
- src/wallet/test/wallet_tests.cpp +0/-34
- test/functional/rpc_deprecated.py +1/-13
- src/wallet/walletdb.cpp +0/-7
- doc/release-notes-removeprunedfundsremoval.md +5/-0
- src/wallet/wallet.h +0/-4
- test/functional/wallet_migration.py +0/-4
- src/wallet/rpc/wallet.cpp +0/-2
- src/wallet/walletdb.h +0/-1
Card
The short text the ranking pass compares across PRs.
Deletes the `removeprunedfunds` RPC (deprecated in v32) and completely removes all transaction deletion logic from the wallet memory structures and database layer. Legacy wallet migration will no longer delete watch-only transactions from the migrated main wallet, keeping non-owned transactions stored harmlessly without affecting balances. This permanently removes obsolete and potentially problematic transaction-erasure routines across the wallet subsystem.
Data
thread read through 2026-10-21 (8 statements), head 360625ff4e
labels: Wallet, Needs rebase
Categories
Wallet
P3 · cleanup
Removing the deprecated `removeprunedfunds` RPC and associated transaction deletion logic simplifies wallet state synchronization and database handling. The change eliminates fragile deletion code that maintained synchronization between in-memory structures and storage, but addresses no active critical fund-safety defect.
Membership: Carries the Wallet label and removes internal transaction deletion routines and deprecated RPCs from src/wallet/.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1