rpc: method removeprunedfunds should take an array of txids #29468

pull araujo88 wants to merge 1 commits into bitcoin:master from araujo88:rpc/removeprunedfunds-should-take-an-array-of-txids changing 5 files +32 −14

araujo88 commented at 1:33 am on February 23, 2024: contributor

This PR addresses issue #29466 by updating the removeprunedfunds RPC method to accept an array of transaction IDs (txids). This enhancement allows for batch removal of transactions, improving usability for pruned wallets. The change includes updates to the RPC method signature and the corresponding functional test adjustments to align with the new input format. This feature simplifies wallet management by enabling more efficient transaction handling.
DrahtBot commented at 1:33 am on February 23, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers

Concept NACK luke-jr

Concept ACK fjahr, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.
DrahtBot added the label RPC/REST/ZMQ on Feb 23, 2024
araujo88 force-pushed on Feb 23, 2024
DrahtBot added the label CI failed on Feb 23, 2024

Type	Reviewers
Concept NACK	luke-jr
Concept ACK	fjahr, BrandonOdiwuor

in src/wallet/rpc/backup.cpp:397 in 10a57427ba outdated

397+            + HelpExampleRpc("removeprunedfunds", "'[\"a8d0c0184dde994a09ec054286f1ce581bebf46446a512166eae7628734ea0a5\", \"9414f1681fb1255bd168a806254321a837008dd4480c02226063183deb100204\"]'")
398                 },
399         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
400 {
401-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
402+std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);

furszy commented at 2:21 am on February 23, 2024:

unneeded change.

furszy commented at 2:22 am on February 23, 2024: member

As it breaks RPC compatibility, needs release-notes.
DrahtBot removed the label CI failed on Feb 23, 2024

in test/functional/wallet_importprunedfunds.py:126 in 10a57427ba outdated

119@@ -120,13 +120,13 @@ def run_test(self):
120         assert_equal(address_info['ismine'], True)
121 
122         # Remove transactions
123-        assert_raises_rpc_error(-4, f'Transaction {txnid1} does not belong to this wallet', w1.removeprunedfunds, txnid1)
124+        assert_raises_rpc_error(-4, f'Transaction {txnid1} does not belong to this wallet', w1.removeprunedfunds, [txnid1])
125         assert not [tx for tx in w1.listtransactions(include_watchonly=True) if tx['txid'] == txnid1]
126 
127-        wwatch.removeprunedfunds(txnid2)
128+        wwatch.removeprunedfunds([txnid2])

brunoerg commented at 5:03 pm on February 23, 2024:

You could test it with more than one tx

fjahr commented at 9:57 pm on February 23, 2024: contributor

Concept ACK
araujo88 force-pushed on Feb 23, 2024

in doc/release-notes-29468.md:6 in 771920d009 outdated

0@@ -0,0 +1,6 @@
1+# RPC `removeprunedfunds` update
2+
3+- The `removeprunedfunds` RPC method has been enhanced to accept an array of transaction IDs (`txids`).
4+- This update allows for the batch removal of transactions, facilitating more efficient management of pruned wallets.
5+- To remove multiple transactions, users can now provide an array of hex-encoded transaction IDs.
6+- The client conversion table (`src/rpc/client.cpp`) has been updated to reflect this enhancement, ensuring proper handling of the new input format.

furszy commented at 11:16 pm on February 23, 2024:

last sentence is not needed. Release-notes are meant to be for end users, not for other contributors.

in doc/release-notes-29468.md:5 in 771920d009 outdated

0@@ -0,0 +1,6 @@
1+# RPC `removeprunedfunds` update
2+
3+- The `removeprunedfunds` RPC method has been enhanced to accept an array of transaction IDs (`txids`).
4+- This update allows for the batch removal of transactions, facilitating more efficient management of pruned wallets.
5+- To remove multiple transactions, users can now provide an array of hex-encoded transaction IDs.

furszy commented at 11:17 pm on February 23, 2024:

this last sentence is redundant.

DrahtBot added the label CI failed on Feb 24, 2024
rpc: method removeprunedfunds should take an array of txids 4fa1ad30d7
araujo88 force-pushed on Feb 24, 2024
DrahtBot removed the label CI failed on Feb 24, 2024
araujo88 requested review from furszy on Feb 24, 2024
araujo88 requested review from brunoerg on Feb 24, 2024
luke-jr commented at 6:29 pm on March 4, 2024: member

Probably better to just remain backward compatible, at least for a release or two…?
araujo88 commented at 6:32 pm on March 4, 2024: contributor

Probably better to just remain backward compatible, at least for a release or two…?

It could handle both cases I guess (passing a string as arg and an array of strings).
luke-jr commented at 6:32 pm on March 4, 2024: member

Actually, looking at the implementation deeper, I’m not sure we gain anything from this change. Just use batch RPC calls?

Concept NACK for now.
araujo88 commented at 6:35 pm on March 4, 2024: contributor

Actually, looking at the implementation deeper, I’m not sure we gain anything from this change. Just use batch RPC calls?

Concept NACK for now.

Doing batches of RPC calls is definitely slower than if we handled this case internally. So how is that not a benefit?
BrandonOdiwuor commented at 10:46 am on April 10, 2024: contributor

Concept ACK
DrahtBot added the label CI failed on Aug 23, 2024
DrahtBot removed the label CI failed on Aug 27, 2024
stickies-v commented at 2:45 pm on October 15, 2024: contributor

Doing batches of RPC calls is definitely slower than if we handled this case internally.

Do you have any performance analysis to look into the difference between both approaches? That would be helpful to assess the merit of this PR.
achow101 commented at 2:45 pm on October 15, 2024: member

Are you still working on this?

The comment about backwards compatibility was never addressed.
DrahtBot added the label CI failed on Oct 24, 2024
DrahtBot removed the label CI failed on Oct 25, 2024

Contributors
araujo88 DrahtBot furszy brunoerg fjahr luke-jr BrandonOdiwuor stickies-v achow101

Review Requested
furszy brunoerg

Labels
RPC/REST/ZMQ

rpc: method removeprunedfunds should take an array of txids #29468

Code Coverage

Reviews

Conflicts