wallet: Split stuff from rpcwallet #23602

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: member

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

    Conflicts

    No conflicts as of last run.

15. 
    laanwj commented at 3:22 pm on November 28, 2021: member

    Concept ACK
16. 
    meshcollider commented at 0: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

17. DrahtBot added the label Needs rebase on Nov 29, 2021
18. MarcoFalke force-pushed on Nov 29, 2021
19. 
    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
20. DrahtBot removed the label Needs rebase on Nov 29, 2021
21. ryanofsky approved
22. 
    ryanofsky commented at 6:46 pm on November 29, 2021: member

    Code review ACK fae239208d3676452a755f736ee5aaa17adeb493. Confirmed move only
23. 
    meshcollider commented at 1:30 am on November 30, 2021: contributor

    Code review ACK fae239208d3676452a755f736ee5aaa17adeb493
24. 
    MarcoFalke commented at 12:09 pm on November 30, 2021: member

    This has two ACKs and no conflicts. Seems good to merge now.
25. MarcoFalke merged this on Nov 30, 2021
26. MarcoFalke closed this on Nov 30, 2021

    ---

27. MarcoFalke deleted the branch on Nov 30, 2021
28. sidhujag referenced this in commit be41ed1f74 on Nov 30, 2021
29. RandyMcMillan referenced this in commit 028193e29d on Dec 23, 2021
30. Fabcien referenced this in commit 3ca879e45a on Dec 6, 2022
31. DrahtBot locked this on Dec 8, 2022

Contributors
MarcoFalke laanwj Wazirabad2 DrahtBot meshcollider ryanofsky

Labels
Refactoring Wallet RPC/REST/ZMQ