Split up rpcwallet #23667

1. 
   meshcollider commented at 11:12 pm on December 3, 2021: contributor

   This is the rest of #23622, to split up rpcwallet into smaller, more logical parts.

   This will have a lot of conflicts but let’s just get it over and done with.

2. meshcollider added the label Refactoring on Dec 3, 2021
3. meshcollider added the label Wallet on Dec 3, 2021
4. 
   DrahtBot commented at 5:03 am on December 4, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #23676 (rpc: correct getnewaddress/getrawchangeaddress address_type helptext by brianddk)
   • #23662 (rpc: improve getreceivedby{address,label} performance by theStack)
   • #23644 (wallet: Replace confusing getAdjustedTime() with GetTime() by MarcoFalke)
   • #23599 (Tidy up RPCTxSerializationFlags by MarcoFalke)
   • #23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments [WIP; TOO MANY CONFLICTS] by MarcoFalke)
   • #23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
   • #23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
   • #23367 (Optimize coin selection by dropping BnB upper limit by S3RK)
   • #23341 (RPC: Better safety with newkeypool command and wallet backups by meshcollider)
   • #23202 (wallet: allow psbtbumpfee to work with txs with external inputs by achow101)
   • #23201 (wallet: Allow users to specify input weights when funding a transaction by achow101)
   • #23113 (Add warnings to createmultisig and addmultisig if using uncompressed keys by meshcollider)
   • #23083 (rpc: Fail to return undocumented or misdocumented JSON by MarcoFalke)
   • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
   • #22807 (RPC: Add universal options argument to listtransactions by kristapsk)
   • #22776 (rpc/wallet: add optional transaction(s) to getbalances by kallewoof)
   • #22775 (rpc: Add option to list transactions from oldest to newest in listtransactions RPC command by ben-kaufman)
   • #22751 (rpc/wallet: add simulaterawtransaction RPC by kallewoof)
   • #22558 (psbt: Taproot fields for PSBT by achow101)
   • #22514 (psbt: Actually use SIGHASH_DEFAULT for PSBT signing by achow101)
   • #22341 (rpc: add getxpub by Sjors)
   • #21928 (wallet: allow toggling external_signer flag by Sjors)
   • #21576 (rpc, gui: bumpfee signer support by Sjors)
   • #21283 (Implement BIP 370 PSBTv2 by achow101)
   • #20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)
   • #20583 (rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs by MarcoFalke)
   • #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
   • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

   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.

5. meshcollider renamed this:
   MOVEONLY: Move transaction, address, and balance RPCs out of rpcwallet
   Split up rpcwallet
   on Dec 6, 2021
6. shaavan approved
7. 
   shaavan commented at 12:39 pm on December 6, 2021: contributor

   crACK 889d32f7f2b6577d6ef367efecfc2f55916c3070

   I have reviewed each commit on this PR, and I agree with the changes. Though I have not thoroughly tested, I was able to run bitcoind and bitcoin-cli on this PR successfully.

   However, merging this PR now would not be effective as there are still too many conflicts with this PR. Though I understand that this would be an ineffective way, I would suggest you further split this PR.

8. DrahtBot added the label Needs rebase on Dec 7, 2021
9. ryanofsky approved
10. 
    ryanofsky commented at 7:30 pm on December 7, 2021: member

    Code review ACK 889d32f7f2b6577d6ef367efecfc2f55916c3070. Easy review. Just function moves then file move.

    This will have a lot of conflicts but let’s just get it over and done with.

    Thank you! It’s less annoying to deal with conflicts in one PR than multiple PRs, IMO.

    However, merging this PR now would not be effective as there are still too many conflicts with this PR. Though I understand that this would be an ineffective way, I would suggest you further split this PR.

    I do think fact that these are clean moves with no changes should makes conflicts easier to resolve. And overall conflicts should be less of a burden if they only have to resolved once, instead of multiple times after different PRs.

11. 
    achow101 commented at 8:47 pm on December 7, 2021: member

    ACK 889d32f7f2b6577d6ef367efecfc2f55916c3070

    Nice straightforward moveonly commits. Although this has a lot of conflicts, rebasing shouldn’t be that difficult. Git should be able to identify the moves and apply the changes accordingly, so it won’t be that bad.

    One thought on the RPC grouping: abortrescan should be in rpc/transactions.cpp to be with rescanblockchain.

12. MOVEONLY: Move transaction related wallet RPCs to transactions.cpp f7646b407f
13. MOVEONLY: Move address related functions from rpcwallet to addresses.cpp 7b45f5c059
14. MOVEONLY: Move balance and utxo RPCs to coins.cpp 9ce521a61b
15. MOVEONLY: Move spending RPCs to spend.cpp 8e30875fde
16. MOVEONLY: Move rpcwallet to rpc/wallet e116b9747d
17. Remove unused imports from rpc/wallet and reorder RPCs d794d0da8f
18. MOVEONLY: Move abortrescan from backup.cpp to transactions.cpp b36e738285
19. 
    meshcollider commented at 10:55 pm on December 7, 2021: contributor

    Thanks for the review!

    Rebased and moved abortrescan as suggested by @achow101

20. DrahtBot removed the label Needs rebase on Dec 7, 2021
21. 
    achow101 commented at 0:31 am on December 8, 2021: member

    ACK b36e738285b111a184c67fd845404850c34333a3
22. meshcollider requested review from MarcoFalke on Dec 8, 2021
23. ryanofsky approved
24. 
    ryanofsky commented at 3:02 am on December 8, 2021: member

    Code review ACK b36e738285b111a184c67fd845404850c34333a3, verified move-only again
25. 
    fanquake commented at 3:33 am on December 8, 2021: member

    However, merging this PR now would not be effective as there are still too many conflicts with this PR. Though I understand that this would be an ineffective way, I would suggest you further split this PR.

    I agree with the other comments in regards to just “doing this now”. A PR like this is always going to have a large number of conflicts, and splitting it up into many more PRs is just going draw out the rebasing, and create a lot more (repo) noise.

    Note that nearly half of the conflicting PRs actually belong to the PR author and the two reviewers who have already ACK’d this change. If you include Marco (who was also planning on making these changes) then it’s more than half, so the “rebasing burden” isn’t actually as bad as it may look.

    I’m going to merge this now.

26. fanquake merged this on Dec 8, 2021
27. fanquake closed this on Dec 8, 2021

    ---

28. meshcollider deleted the branch on Dec 8, 2021
29. 
    Sjors commented at 5:32 am on December 8, 2021: member

    Oh hi… (but yeah, it’s worth it)

    utACK b36e738285b111a184c67fd845404850c34333a3

30. sidhujag referenced this in commit 0ba383bd11 on Dec 8, 2021
31. sidhujag referenced this in commit 7b766a50f2 on Dec 8, 2021
32. RandyMcMillan referenced this in commit b7e95a5f43 on Dec 23, 2021
33. DrahtBot locked this on Dec 8, 2022

Contributors
meshcollider DrahtBot shaavan ryanofsky achow101 fanquake Sjors

Review Requested
MarcoFalke

Labels
Refactoring Wallet