Split up rpcwallet #23667

meshcollider · 2021-12-03T23:12:15Z

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.

DrahtBot · 2021-12-04T05:03:04Z

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.

shaavan

crACK 889d32f

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.

ryanofsky

Code review ACK 889d32f. 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.

achow101 · 2021-12-07T20:47:07Z

ACK 889d32f

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.

meshcollider · 2021-12-07T22:55:03Z

Thanks for the review!

Rebased and moved abortrescan as suggested by @achow101

achow101 · 2021-12-08T00:31:04Z

ACK b36e738

ryanofsky

Code review ACK b36e738, verified move-only again

fanquake · 2021-12-08T03:33:30Z

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.

Sjors · 2021-12-08T05:32:06Z

Oh hi... (but yeah, it's worth it)

utACK b36e738

also deal with deprecated "addresses" field from upstream

288f99a MOVEONLY: Move abortrescan from backup.cpp to transactions.cpp (Samuel Dobson) 7a47e08 Remove unused imports from rpc/wallet and reorder RPCs (Samuel Dobson) 4fceeb3 MOVEONLY: Move rpcwallet to rpc/wallet (Samuel Dobson) de401aa MOVEONLY: Move spending RPCs to spend.cpp (Samuel Dobson) 5dbd8fe MOVEONLY: Move balance and utxo RPCs to coins.cpp (Samuel Dobson) c0f59cc MOVEONLY: Move address related functions from rpcwallet to addresses.cpp (Samuel Dobson) 5c38fef MOVEONLY: Move transaction related wallet RPCs to transactions.cpp (Samuel Dobson) Pull request description: 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. ACKs for top commit: achow101: ACK 288f99a ryanofsky: Code review ACK 288f99a, verified move-only again Tree-SHA512: 6695fa23bbe9822c7497db7660f44c3dcb01dfa7276f5830a6b7c73c6b5fa04e04fcd4821bf0e6392e7659ed91a277ced85bd8f77477d785bca4e84a93fe791e

meshcollider added Refactoring Wallet labels Dec 3, 2021

meshcollider changed the title ~~MOVEONLY: Move transaction, address, and balance RPCs out of rpcwallet~~ Split up rpcwallet Dec 6, 2021

shaavan approved these changes Dec 6, 2021

View reviewed changes

This was referenced Dec 7, 2021

rpc, gui: bumpfee signer support #21576

Merged

Implement BIP 370 PSBTv2 #21283

Open

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

DrahtBot added the Needs rebase label Dec 7, 2021

ryanofsky approved these changes Dec 7, 2021

View reviewed changes

meshcollider added 7 commits December 8, 2021 11:40

MOVEONLY: Move transaction related wallet RPCs to transactions.cpp

f7646b4

MOVEONLY: Move address related functions from rpcwallet to addresses.cpp

7b45f5c

MOVEONLY: Move balance and utxo RPCs to coins.cpp

9ce521a

MOVEONLY: Move spending RPCs to spend.cpp

8e30875

MOVEONLY: Move rpcwallet to rpc/wallet

e116b97

Remove unused imports from rpc/wallet and reorder RPCs

d794d0d

MOVEONLY: Move abortrescan from backup.cpp to transactions.cpp

b36e738

DrahtBot removed the Needs rebase label Dec 7, 2021

meshcollider requested a review from maflcko December 8, 2021 01:42

ryanofsky approved these changes Dec 8, 2021

View reviewed changes

fanquake merged commit fa3fb46 into bitcoin:master Dec 8, 2021

meshcollider deleted the 202111_split_walletrpc_total branch December 8, 2021 03:48

meshcollider mentioned this pull request Dec 8, 2021

[RFC] Splitting up rpcwallet #23622

Closed

8 tasks

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 8, 2021

Merge bitcoin#23667: Split up rpcwallet

0ba383b

also deal with deprecated "addresses" field from upstream

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 8, 2021

Merge bitcoin#23667: Split up rpcwallet

7b766a5

also deal with deprecated "addresses" field from upstream

bitcoin locked and limited conversation to collaborators Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Split up rpcwallet #23667

Split up rpcwallet #23667

Uh oh!

meshcollider commented Dec 3, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Dec 4, 2021 •

edited

Loading

Uh oh!

shaavan left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

achow101 commented Dec 7, 2021

Uh oh!

meshcollider commented Dec 7, 2021

Uh oh!

achow101 commented Dec 8, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

fanquake commented Dec 8, 2021

Uh oh!

Sjors commented Dec 8, 2021 •

edited

Loading

Uh oh!

Uh oh!

Split up rpcwallet #23667

Split up rpcwallet #23667

Uh oh!

Conversation

meshcollider commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 7, 2021

Uh oh!

meshcollider commented Dec 7, 2021

Uh oh!

achow101 commented Dec 8, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Dec 8, 2021

Uh oh!

Sjors commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

meshcollider commented Dec 3, 2021 •

edited

Loading

DrahtBot commented Dec 4, 2021 •

edited

Loading

Sjors commented Dec 8, 2021 •

edited

Loading