Skip to content

wallet: Split stuff from rpcwallet #23602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Nov 30, 2021
Merged

wallet: Split stuff from rpcwallet #23602

maflcko merged 1 commit into from
Nov 30, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 26, 2021

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.

@maflcko maflcko force-pushed the 1905-walletSplitRpc branch from fa8cbeb to fafa637 Compare November 26, 2021 13:48
wallet: Split signmessage from rpcwallet
fae2392 
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
@maflcko maflcko force-pushed the 1905-walletSplitRpc branch from fafa637 to fae2392 Compare November 26, 2021 13:51
@laanwj
Copy link
Member

laanwj commented Nov 26, 2021

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?

@maflcko
Copy link
Member Author

maflcko commented Nov 26, 2021

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.

@laanwj
Copy link
Member

laanwj commented Nov 26, 2021

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.

@maflcko
Copy link
Member Author

maflcko commented Nov 26, 2021

make a bigger plan

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

@maflcko maflcko changed the title wallet: Split signmessage from rpcwallet wallet: Split stuff from rpcwallet Nov 26, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 27, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

This was referenced Nov 27, 2021
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Open
Merged
Merged
Merged
@laanwj
Copy link
Member

laanwj commented Nov 28, 2021

Concept ACK

@meshcollider
Copy link
Contributor

meshcollider commented Nov 29, 2021 •
edited
Loading

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

@meshcollider meshcollider mentioned this pull request Nov 29, 2021
Closed
8 tasks
@maflcko maflcko force-pushed the 1905-walletSplitRpc branch from fa24419 to fae2392 Compare November 29, 2021 16:45
@maflcko
Copy link
Member Author

maflcko commented Nov 29, 2021

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

ryanofsky
ryanofsky approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fae2392. Confirmed move only

meshcollider
meshcollider reviewed Nov 30, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fae2392

@maflcko
Copy link
Member Author

maflcko commented Nov 30, 2021

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

@maflcko maflcko merged commit ffdf8ee into bitcoin:master Nov 30, 2021
@maflcko maflcko deleted the 1905-walletSplitRpc branch November 30, 2021 12:13
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 30, 2021
Merge bitcoin#23602: wallet: Split stuff from rpcwallet
be41ed1
@ryanofsky ryanofsky mentioned this pull request Dec 2, 2021
Merged
RandyMcMillan pushed a commit to RandyMcMillan/mempool-tab that referenced this pull request Dec 23, 2021
Merge bitcoin/bitcoin#23602: wallet: Split stuff from rpcwallet
028193e 
47d672d wallet: Split signmessage from rpcwallet (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  ryanofsky:
    Code review ACK 47d672d. Confirmed move only
  meshcollider:
    Code review ACK 47d672d

Tree-SHA512: 250445cd544e39376f225871270cdcae462f16cfd9d25ede4b148e915642bfac9ee7ef3e8eccdd2443dc74dbf794d3bcd5fe5c58b1d05a2dcec70b8e03b37dff
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 6, 2022
@PiRK
wallet: Split signmessage from rpcwallet
3ca879e 
Summary:
> 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.

This is a backport of [[bitcoin/bitcoin#23602 | core#23602]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12774
@bitcoin bitcoin locked and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@meshcollider meshcollider meshcollider left review comments

@Wazirabad2 Wazirabad2 Wazirabad2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @laanwj @DrahtBot @meshcollider @ryanofsky @Wazirabad2