multiprocess: add interfaces::ExternalSigner class #23004

1. 
   ryanofsky commented at 2:47 am on September 17, 2021: member

   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.

2. fanquake added the label interfaces on Sep 17, 2021
3. fanquake added the label Wallet on Sep 17, 2021
4. fanquake added this to the "In progress" column in a project

   ---

5. 
   benthecarman commented at 4:16 pm on September 17, 2021: contributor

   Concept ACK
6. 
   DrahtBot commented at 2:28 am on September 18, 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:

   • #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.

7. 
   Sjors commented at 9:14 am on September 29, 2021: member

   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.
8. DrahtBot added the label Needs rebase on Oct 5, 2021
9. multiprocess: add interfaces::ExternalSigner class

   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.

   a032fa30d2
10. ryanofsky force-pushed on Oct 6, 2021
11. ryanofsky renamed this:
    multiprocess: Run external signer in wallet not node process
    multiprocess: add interfaces::ExternalSigner class
    on Oct 6, 2021
12. 
    ryanofsky commented at 2:59 am on October 6, 2021: member

    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 c74e5706a46d063adbdef93b1cf2582086a78382 -> e3afe660a44aec9cc6ce6a2e04461fdf5ade2483 (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

13. DrahtBot removed the label Needs rebase on Oct 6, 2021
14. 
    meshcollider commented at 1:00 pm on October 9, 2021: contributor

    utACK e3afe660a44aec9cc6ce6a2e04461fdf5ade2483
15. 
    hebasto commented at 2:50 pm on October 23, 2021: member

    Concept ACK.
16. in src/node/interfaces.cpp:77 in e3afe660a4 outdated

    71+{
    72+public:
    73+#ifdef ENABLE_EXTERNAL_SIGNER
    74+    ExternalSignerImpl(::ExternalSigner signer) : m_signer(std::move(signer)) {}
    75+    std::string getName() override { return m_signer.m_name; }
    76+    ::ExternalSigner m_signer;
    

    ---

    ---

    

    hebasto commented at 4:29 pm on October 23, 2021:

    Why m_signer is not a private: member?

    ---

    

    ryanofsky commented at 4:57 pm on October 25, 2021:

    re: #23004 (review)

    Why m_signer is not a private: member?

    Added. No real reason but the whole class is effectively private.

17. hebasto approved
18. 
    hebasto commented at 4:35 pm on October 23, 2021: member

    ACK e3afe660a44aec9cc6ce6a2e04461fdf5ade2483, I have reviewed the code and it looks OK. Also compiled with both configure options: --enable-external-signer and --disable-external-signer.
19. ryanofsky force-pushed on Oct 25, 2021
20. 
    ryanofsky commented at 5:03 pm on October 25, 2021: member

    Thanks for review!

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

21. hebasto approved
22. 
    hebasto commented at 6:50 pm on October 25, 2021: member

    re-ACK a032fa30d282fa69c304e0afd1f95f67c55d22e3
23. 
    laanwj commented at 4:04 pm on November 15, 2021: member

    Concept and code review ACK a032fa30d282fa69c304e0afd1f95f67c55d22e3
24. laanwj merged this on Nov 15, 2021
25. laanwj closed this on Nov 15, 2021

    ---

26. sidhujag referenced this in commit c5c7df6f0f on Nov 16, 2021
27. ryanofsky moved this from the "In progress" to the "Done" column in a project

    ---

28. DrahtBot locked this on Nov 15, 2022

Contributors
ryanofsky benthecarman DrahtBot Sjors meshcollider hebasto laanwj

Labels
Wallet interfaces