Skip to content

multiprocess: add interfaces::ExternalSigner class #23004

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
laanwj merged 1 commit into from
Nov 15, 2021
Merged

multiprocess: add interfaces::ExternalSigner class #23004

laanwj merged 1 commit into from
Nov 15, 2021

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Sep 17, 2021 •
edited
Loading

Add interfaces::ExternalSigner class to let signer objects be passed between processes and let signer code run in the original process where the object was created.

This PR is part of the process separation project.

@ryanofsky ryanofsky mentioned this pull request Sep 17, 2021
Draft
@benthecarman
Copy link
Contributor

benthecarman commented Sep 17, 2021

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 18, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

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.

This was referenced Sep 18, 2021
Merged
Merged
@Sjors
Copy link
Member

Sjors commented Sep 29, 2021 •
edited
Loading

External signers can in principle be used without wallet support, though currently only the enumeratesigners RPC call works (not very useful). See rpc_signer.py. This is consistent with RPC calls like signrawtransactionwithkey which also don't need a wallet. This is why I initially picked the Node interface. Not sure how much this matters, or even if anyone cares about using external signers without a (watch-only) wallet.

@ryanofsky
multiprocess: add interfaces::ExternalSigner class
a032fa3 
Add interfaces::ExternalSigner to let signer objects be passed between
processes and signer code to run in the original process, without
multiple processes linking and running signer code.
@ryanofsky ryanofsky changed the title multiprocess: Run external signer in wallet not node process multiprocess: add interfaces::ExternalSigner class Oct 6, 2021
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Oct 6, 2021

External signers can in principle be used without wallet support, though currently only the enumeratesigners RPC call works (not very useful). See rpc_signer.py. This is consistent with RPC calls like signrawtransactionwithkey which also don't need a wallet. This is why I initially picked the Node interface. Not sure how much this matters, or even if anyone cares about using external signers without a (watch-only) wallet.

Fair enough. I moved the signers list method back to the Node interface instead of adding it to the WalletClient interface so the signer code will keep running in the node process instead of the wallet process and there's no behavior change. This makes the PR smaller, and if there is ever any reason to switch which process signing code runs in, it can be moved later.

Now this PR only adds the interfaces::ExternalSigner class, which is needed so there is some way of passing signers between processes.

Rebased c74e570 -> e3afe66 (pr/ipc-signer.1 -> pr/ipc-signer.2, compare) due to conflict with #22951, and also moved list signers method back to the Node interface as sjors suggested

meshcollider
meshcollider reviewed Oct 9, 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.

utACK e3afe66

@hebasto
Copy link
Member

hebasto commented Oct 23, 2021

Concept ACK.

hebasto
hebasto approved these changes Oct 23, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK e3afe66, I have reviewed the code and it looks OK. Also compiled with both configure options: --enable-external-signer and --disable-external-signer.

ryanofsky
ryanofsky commented Oct 25, 2021
View reviewed changes
Copy link
Contributor Author

@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.

Thanks for review!

Updated e3afe66 -> a032fa3 (pr/ipc-signer.2 -> pr/ipc-signer.3, compare) with suggestion

hebasto
hebasto approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK a032fa3

@laanwj
Copy link
Member

laanwj commented Nov 15, 2021

Concept and code review ACK a032fa3

@laanwj laanwj merged commit 1ba7412 into bitcoin:master Nov 15, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2021
@laanwj
Merge bitcoin#23004: multiprocess: add interfaces::ExternalSigner class
c5c7df6
@bitcoin bitcoin locked and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@meshcollider meshcollider meshcollider left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

interfaces Wallet

Projects

Process Separation
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ryanofsky @benthecarman @DrahtBot @Sjors @hebasto @laanwj @meshcollider @fanquake