wallet: Split stuff from rpcwallet #23602

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1905-walletSplitRpc changing 4 files +73 −59
  1. MarcoFalke commented at 12:08 PM on November 26, 2021: member

    rpcwallet is the file that takes longest to compile, especially with sanitizers enabled it can take several 10s of seconds.

    Allow faster incremental and parallel builds by starting to split it up. First, split off signmessage, which is unrelated to other stuff such as wallet file handling, wallet encryption, tx creation, or wallet status/info.

  2. MarcoFalke added the label RPC/REST/ZMQ on Nov 26, 2021
  3. MarcoFalke added the label Wallet on Nov 26, 2021
  4. MarcoFalke added the label Refactoring on Nov 26, 2021
  5. MarcoFalke force-pushed on Nov 26, 2021
  6. wallet: Split signmessage from rpcwallet
    Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
    fae239208d
  7. MarcoFalke force-pushed on Nov 26, 2021
  8. laanwj commented at 3:14 PM on November 26, 2021: member

    First, split off signmessage, which is unrelated to other stuff such as wallet file handling, wallet encryption, tx creation, or wallet status/info.

    It seems that way! I don't understand how it can be unrelated to wallet encryption. It uses a private key to sign, right?

  9. MarcoFalke commented at 3:27 PM on November 26, 2021: member

    It seems that way! I don't understand how it can be unrelated to wallet encryption. It uses a private key to sign, right?

    I'd say encryption related are only the (two?) passphrase related calls. Otherwise all send* RPCs are also encryption related, no? Though, I am happy to put all HELP_REQUIRING_PASSPHRASE RPCs into one file, as long as the file is split up in some way.

  10. laanwj commented at 3:30 PM on November 26, 2021: member

    No, I'm fine with this (though I'm not sure it goes far enough, if you want to split up rpcwallet.cpp it might make sense to make a bigger plan), it was just a surprise, I see the EnsureWalletIsUnlocked(*pwallet); now.

  11. Wazirabad2 approved
  12. MarcoFalke commented at 4:31 PM on November 26, 2021: member

    make a bigger plan

    Sure, made another commit. Happy to add as many as reviewers would like me to add.

  13. MarcoFalke renamed this:
    wallet: Split signmessage from rpcwallet
    wallet: Split stuff from rpcwallet
    on Nov 26, 2021
  14. DrahtBot commented at 5:28 AM on November 27, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  15. DrahtBot cross-referenced this on Nov 27, 2021 from issue Add external signer taproot support by Sjors
  16. DrahtBot cross-referenced this on Nov 27, 2021 from issue Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference by mjdietzx
  17. DrahtBot cross-referenced this on Nov 27, 2021 from issue wallet: allow psbtbumpfee to work with txs with external inputs by achow101
  18. DrahtBot cross-referenced this on Nov 27, 2021 from issue wallet: Allow users to specify input weights when funding a transaction by achow101
  19. DrahtBot cross-referenced this on Nov 28, 2021 from issue psbt: Taproot fields for PSBT by achow101
  20. DrahtBot cross-referenced this on Nov 28, 2021 from issue psbt: Actually use SIGHASH_DEFAULT for PSBT signing by achow101
  21. DrahtBot cross-referenced this on Nov 28, 2021 from issue rpc: Allow walletprocesspsbt to sign without finalizing by achow101
  22. DrahtBot cross-referenced this on Nov 28, 2021 from issue rpc, gui: bumpfee signer support by Sjors
  23. DrahtBot cross-referenced this on Nov 28, 2021 from issue Implement BIP 370 PSBTv2 by achow101
  24. DrahtBot cross-referenced this on Nov 28, 2021 from issue wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack
  25. DrahtBot cross-referenced this on Nov 28, 2021 from issue rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs by MarcoFalke
  26. DrahtBot cross-referenced this on Nov 28, 2021 from issue refactor: Extract RipeMd160 by Empact
  27. laanwj commented at 3:22 PM on November 28, 2021: member

    Concept ACK

  28. meshcollider commented at 12:58 AM on November 29, 2021: contributor

    Concept ACK, but I too think we could definitely go even further than this. Things like

    • move wallet/rpcdump.cpp => wallet/rpc/backup.cpp and combine with backupwallet and restorewallet RPCs.
    • introduce wallet/rpc/util.cpp and move the helper functions there, like GetWalletForJSONRPCRequest and EnsureWalletIsUnlocked, etc.
    • introduce wallet/rpc/addresses.cpp and move getnewaddress, getrawchangeaddress, setlabel, listaddressgroupings, addmultisigaddress, keypoolrefill, newkeypool, getaddressinfo, getaddressesbylabel, listlabels, and walletdisplayaddress there.
    • introduce wallet/rpc/transactions.cpp and move listreceivedbyaddress, listreceivedbylabel, listtransactions, listsinceblock, gettransaction, abandontransaction, and rescanblockchain there.
    • introduce wallet/rpc/utxos.cpp and move getreceivedbyaddress, getreceivedbylabel, getbalance, getunconfirmedbalance, lockunspent, listlockunspent, getbalances, and listunspent there.
    • Move the walletpassphrase, walletpassphrasechange, walletlock, and encryptwallet RPCs to wallet/rpc/encrypt.cpp.
    • This would leave rpcwallet.cpp with only the getwalletinfo, listwalletdir, listwallets, loadwallet, setwalletflag, createwallet, unloadwallet, sethdseed , and upgradewallet RPCs, which are the "core" wallet management RPCs, so it makes sense. It could be moved to the wallet/rpc/ directory though under a different name.

    Obviously this would be a lot of movement, so it would be worth getting more comments on beforehand. I am happy to open an RFC issue and make these changes over a number of PRs to break it up if needed. Let me know your thoughts.

    EDIT: opened #23622

  29. meshcollider cross-referenced this on Nov 29, 2021 from issue [RFC] Splitting up rpcwallet by meshcollider
  30. DrahtBot added the label Needs rebase on Nov 29, 2021
  31. MarcoFalke force-pushed on Nov 29, 2021
  32. MarcoFalke commented at 4:46 PM on November 29, 2021: member

    I've removed fa24419d22 again to avoid the conflicts. This can be done as a second step later. I think it is pretty clear that this will be done in smaller steps according to the plan above by @meshcollider

  33. DrahtBot removed the label Needs rebase on Nov 29, 2021
  34. ryanofsky approved
  35. ryanofsky commented at 6:46 PM on November 29, 2021: contributor

    Code review ACK fae239208d3676452a755f736ee5aaa17adeb493. Confirmed move only

  36. meshcollider commented at 1:30 AM on November 30, 2021: contributor

    Code review ACK fae239208d3676452a755f736ee5aaa17adeb493

  37. MarcoFalke commented at 12:09 PM on November 30, 2021: member

    This has two ACKs and no conflicts. Seems good to merge now.

  38. MarcoFalke merged this on Nov 30, 2021
  39. MarcoFalke closed this on Nov 30, 2021
  40. MarcoFalke deleted the branch on Nov 30, 2021
  41. sidhujag referenced this in commit be41ed1f74 on Nov 30, 2021
  42. ryanofsky cross-referenced this on Dec 2, 2021 from issue Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces by ryanofsky
  43. RandyMcMillan referenced this in commit 028193e29d on Dec 23, 2021
  44. Fabcien referenced this in commit 3ca879e45a on Dec 6, 2022
  45. bitcoin locked this on Dec 8, 2022
Contributors
MarcoFalkelaanwjWazirabad2DrahtBotmeshcolliderryanofsky
Labels
RefactoringWalletRPC/REST/ZMQ
Linked (view graph)
#23497 Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces#23622 [RFC] Splitting up rpcwallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC