rpc, doc: clarify wallet version in getwalletinfo help #32603
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32603. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
There was a problem hiding this comment.
ACK 21ddfbf
modulo nit suggestion, and PR/commit title s/wallet, rpc/rpc, doc/
(suggest not linking to https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2853743852 in the commit message, as every push here adds a GitHub notification in that pull)
I noted in review of a previous PR 32553. Relaying the same information in the RPC help that's present in a code comment: https://github.com/bitcoin/bitcoin/blob/af65fd1a333011137dafd3df9a51704fd319feb4/src/wallet/wallet.h#L809-L810
| @@ -40,7 +40,7 @@ static RPCHelpMan getwalletinfo() | |||
| { | |||
| { | |||
| {RPCResult::Type::STR, "walletname", "the wallet name"}, | |||
| {RPCResult::Type::NUM, "walletversion", "the wallet version"}, | |||
| {RPCResult::Type::NUM, "walletversion", "the current wallet format (the oldest client version guaranteed to understand this wallet)"}, | |||
There was a problem hiding this comment.
I don't think this is right. The current wallet version is 169900 (for a freshly created descriptor wallet). However, I don't think a Bitcoin Core 16.99 client can open descriptor wallets at all.
Generally, all modules in Bitcoin Core moved away from the client version to use a module-specific and module-internal version. (You can see this when you look at all other files written (fee estimates, mempool, addr ...). Also the p2p version, but that again has been replaced by feature-messages.)
There was a problem hiding this comment.
Hmm I see. Thanks for highlighting. Descriptor wallets were indeed introduced after v16.99.
I'm not sure then what does the "client version" here refers to. Maybe it is referring to a wallet client somehow because as mentioned above that versioning has moved to being module specific.
I will dig more to see how this can be made clearer.
There was a problem hiding this comment.
Are you still working on this, or can this be closed?
There was a problem hiding this comment.
Closing this for now.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
I noted in review of a previous PR here: #32553 (review).
Relaying the same information in the RPC help that's present in a code comment:
bitcoin/src/wallet/wallet.h
Lines 809 to 810 in af65fd1