Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

araujo88
Copy link
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
Copy link
Contributor

DrahtBot commented Feb 23, 2024

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.

@araujo88 araujo88 force-pushed the rpc/removeprunedfunds-should-take-an-array-of-txids branch from da1a32a to 10a5742 Compare February 23, 2024 02:20
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it breaks RPC compatibility, needs release-notes.

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved
@fjahr
Copy link
Contributor

fjahr commented Feb 23, 2024

Concept ACK

@araujo88 araujo88 force-pushed the rpc/removeprunedfunds-should-take-an-array-of-txids branch from 10a5742 to 771920d Compare February 23, 2024 22:28
doc/release-notes-29468.md Outdated Show resolved Hide resolved
doc/release-notes-29468.md Outdated Show resolved Hide resolved
@araujo88 araujo88 force-pushed the rpc/removeprunedfunds-should-take-an-array-of-txids branch from 771920d to 4fa1ad3 Compare February 24, 2024 03:31
@luke-jr
Copy link
Member

luke-jr commented Mar 4, 2024

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

@araujo88
Copy link
Contributor Author

araujo88 commented Mar 4, 2024

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
Copy link
Member

luke-jr commented Mar 4, 2024

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
Copy link
Contributor Author

araujo88 commented Mar 4, 2024

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?

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants