docs: Improve keypoolrefill RPC docs #32429

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

2. 
   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.

3. DrahtBot added the label Docs on May 6, 2025
4. 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
5. 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.
6. w0xlt force-pushed on May 6, 2025
7. w0xlt force-pushed on May 6, 2025
8. 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:

   0                "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
9. 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
10. rkrux approved
11. 
    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.

12. docs: Improve `keypoolrefill` RPC docs ff35a4b021
13. w0xlt force-pushed on May 7, 2025
14. brunoerg approved
15. 
    brunoerg commented at 5:48 pm on May 7, 2025: contributor

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

    ACK ff35a4b021e12ae33e28c01ffeeb2c1916f7a487
18. achow101 merged this on May 7, 2025
19. achow101 closed this on May 7, 2025

    ---

Contributors
w0xlt DrahtBot maflcko brunoerg rkrux achow101

Review Requested
rkrux

Labels
Docs