docs: Improve `keypoolrefill` RPC docs #32429

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:keypoolrefill_docs changing 1 files +2 −1

w0xlt commented at 7:15 PM on May 6, 2025: contributor

Update keypoolrefill RPC docs to make it clear that descriptor wallets have 4 ScriptPubKeyManagers by default and each of them is updated in this command, as suggested #29924 (comment)

Closes https://github.com/bitcoin/bitcoin/issues/29924
DrahtBot commented at 7:15 PM on May 6, 2025: contributor



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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32429.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK brunoerg, achow101

Stale ACK rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Docs on May 6, 2025
in src/wallet/rpc/addresses.cpp:340 in 9e20f6ba68 outdated
```
 335 | @@ -336,7 +336,9 @@ RPCHelpMan addmultisigaddress()
 336 |  RPCHelpMan keypoolrefill()
 337 |  {
 338 |      return RPCHelpMan{"keypoolrefill",
 339 | -                "\nFills the keypool."+
 340 | +                "\nRefills each `ScriptPubKeyManager` keypool in the wallet up to the specified number of new keys.\n"
 341 | +                "By default, descriptor wallets have four `ScriptPubKeyManagers` (`wpkh`, `tr`, `pkh`, and `sh-wpkh`), each with 1000 entries.\n"
```
maflcko commented at 8:43 PM on May 6, 2025:

Should probably use DEFAULT_KEYPOOL_SIZE instead of hardcoded 1000? Same for the OUTPUT_TYPES? Also, ScriptPubKeyManagers was never used in user-facing docs, except for one error message. Maybe just use "active ranged descriptor" (or similar)?

w0xlt commented at 11:37 PM on May 6, 2025:

Done (for DEFAULT_KEYPOOL_SIZE and ScriptPubKeyManagers). Regarding OUTPUT_TYPES, I think it may need to be refactored to include it in the RPC documentation due to static initialization. But for now, I left keypoolrefill exactly as getnewaddress.

w0xlt commented at 3:17 AM on May 7, 2025:

OUTPUT_TYPES handled in https://github.com/bitcoin/bitcoin/pull/32432

Type	Reviewers
ACK	brunoerg, achow101
Stale ACK	rkrux

in src/wallet/rpc/addresses.cpp:341 in 9e20f6ba68 outdated

 335 | @@ -336,7 +336,9 @@ RPCHelpMan addmultisigaddress()
 336 |  RPCHelpMan keypoolrefill()
 337 |  {
 338 |      return RPCHelpMan{"keypoolrefill",
 339 | -                "\nFills the keypool."+
 340 | +                "\nRefills each `ScriptPubKeyManager` keypool in the wallet up to the specified number of new keys.\n"
 341 | +                "By default, descriptor wallets have four `ScriptPubKeyManagers` (`wpkh`, `tr`, `pkh`, and `sh-wpkh`), each with 1000 entries.\n"
 342 | +                "Legacy wallet typically uses a single `LegacyScriptPubKeyMan` (which manages both external and internal keypools)\n"+

maflcko commented at 9:07 PM on May 6, 2025:

also, now that bdb was removed, no need to add docs for it?

brunoerg commented at 10:22 PM on May 6, 2025:

Agreed. Otherwise, #28710 would have to drop it.

w0xlt commented at 11:37 PM on May 6, 2025:

Done. Thanks.

w0xlt force-pushed on May 6, 2025
w0xlt force-pushed on May 6, 2025

in src/wallet/rpc/addresses.cpp:339 in cd00fb85e9 outdated

 335 | @@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
 336 |  RPCHelpMan keypoolrefill()
 337 |  {
 338 |      return RPCHelpMan{"keypoolrefill",
 339 | -                "\nFills the keypool."+
 340 | +                "\nRefills each descriptor keypool in the wallet up to the specified number of new keys.\n"

maflcko commented at 7:46 AM on May 7, 2025:

                "Refills each descriptor keypool in the wallet up to the specified number of new keys.\n"

nit: This is trimmed anyway.

w0xlt commented at 5:45 PM on May 7, 2025:

Done in https://github.com/bitcoin/bitcoin/pull/32429/commits/ff35a4b021e12ae33e28c01ffeeb2c1916f7a487

in src/wallet/rpc/addresses.cpp:340 in cd00fb85e9 outdated

 335 | @@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
 336 |  RPCHelpMan keypoolrefill()
 337 |  {
 338 |      return RPCHelpMan{"keypoolrefill",
 339 | -                "\nFills the keypool."+
 340 | +                "\nRefills each descriptor keypool in the wallet up to the specified number of new keys.\n"
 341 | +                "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +

maflcko commented at 8:02 AM on May 7, 2025:

I don't think to_string is allowed, see #32434

Could use ToString?

w0xlt commented at 5:45 PM on May 7, 2025:

Done in https://github.com/bitcoin/bitcoin/pull/32429/commits/ff35a4b021e12ae33e28c01ffeeb2c1916f7a487

rkrux approved
rkrux commented at 10:49 AM on May 7, 2025: contributor

ACK cd00fb85e9e0997120715e68374462cff0dad7eb given the suggestions are addressed.

I checked that by default there are 4 scriptpubkeymans setup on the first run of the wallet inside SetupDescriptorScriptPubKeyMans. Also, ack because the lack of this information has caused an issue to be created.
docs: Improve `keypoolrefill` RPC docs ff35a4b021
w0xlt force-pushed on May 7, 2025
brunoerg approved
brunoerg commented at 5:48 PM on May 7, 2025: contributor

code review ACK ff35a4b021e12ae33e28c01ffeeb2c1916f7a487
DrahtBot requested review from rkrux on May 7, 2025
achow101 commented at 9:44 PM on May 7, 2025: member

ACK ff35a4b021e12ae33e28c01ffeeb2c1916f7a487
achow101 merged this on May 7, 2025
achow101 closed this on May 7, 2025

Contributors

Review Requested
rkrux

Labels

Docs

Linked (view graph)

#29924 `keypoolrefill` doesn't fill keypool to specified parameter #32432 wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them

docs: Improve `keypoolrefill` RPC docs #32429

Code Coverage & Benchmarks

Reviews