rpc: Manual prune lock management (Take 2)#34534
rpc: Manual prune lock management (Take 2)#34534fjahr wants to merge 3 commits intobitcoin:masterfrom
Conversation
These are helpful for RPC methods to manage prune locks manually.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
df7a8c4 to
f192b06
Compare
|
Fixing CI failure: Forgot to set |
luke-jr
left a comment
There was a problem hiding this comment.
Incomplete review. Basically, the original PR interface was way better.
src/rpc/blockchain.cpp
Outdated
| "These locks are used internally by indexes and can also be set manually via the setprunelock RPC.\n", | ||
| {}, | ||
| RPCResult{ | ||
| RPCResult::Type::ARR, "", "", |
There was a problem hiding this comment.
This was an Object for future extensibility. No reason to lose that.
There was a problem hiding this comment.
The RPC is called listprunelocks, I don't see how it could be extended to return anything else aside from a list of locks without that other thing feeling out of place. Can you give some specific ideas that you have? At least I would want to give the RPC a different name then (getpruneinfo or so) but potentially these ideas should rather be in a separate RPC. I prefer consistency and so I really like that we could be consistenty with the banning RPC.
There was a problem hiding this comment.
listprunelocks could return a "blockman epoch" in the future, if there was need to further distinguish rpc results. (Yes, this is made up, but with an array, this is impossible, with an object, this is trivial and harmless)
There was a problem hiding this comment.
What is a "blockman epoch"? I don't think I have heard of that before.
There was a problem hiding this comment.
I guess TIL per this comment that the JSON object thing is in the dev notes, fair enough, I have changed it to return an object.
f192b06 to
0032cee
Compare
Is this just about #34534 (comment) or did you mean something else? |
These allow for manual management of prune locks by users and follow the UX pattern of setban/listbanned. Co-authored-by: Luke Dashjr <[email protected]>
0032cee to
b7fc779
Compare
Do you remember at least who mentioned these users/use cases? |
This looks potentially interesting, but I don't think we should be implementing RPCs when we don't know anyone that would actually be using them? Since the previous PR has been open since 2020 already without users chiming in, it seems like there's no real demand for this? |
|
This is generally useful in the wallet and for any wallets that leverage Bitcoin Core. The fundamental issue is that if the wallet is not loaded while the node is running, blocks necessary for rescanning when the wallet is loaded may be pruned away. The wallet definitely should be using prune locks, but it's also necessary to have a way to for users to remove prune locks that are no longer relevant, e.g. if they unload a wallet that they don't plan to load on that node again. External wallets that use Bitcoin Core should also find this useful as they can then set prune locks that match the scan state of the external wallet. |
Done, replace that vague part of the description with a better motivation paragraph. |
This is a refreshed version of #19463, which was closed due to inactivity.
Motivation: On pruned nodes, if a wallet is not loaded while the node is running, blocks necessary for rescanning when the wallet is loaded may be pruned. This PR allows users and external wallet software to manually manage prune locks via RPC, so they can protect blocks needed for future rescans and also clean up locks that are no longer relevant (e.g. after permanently unloading a wallet from a node). This is useful both for Bitcoin Core's own wallet and for external wallets that use Bitcoin Core as a backend. Usage of temporary prune locking may also be helpful in other functional test scenarios but I didn't spend time to check where might apply.
This PR leverages the existing prune locks system we already have. The interface of the RPCs of the old PR has been slightly updated, it now matches the pattern used by
setban/listbannedexactly. The locks are prefixed to distinguish them from the systems locks and prevent naming colissions. They are not persisted across restarts.