Move external signer out of wallet module #21467

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2021/03/signer_no_wallet changing 27 files +179 −150
  1. Sjors commented at 2:23 PM on March 18, 2021: member

    In addition, this PR enables external signer testing on CI.

    This PR moves the ExternalSigner class and RPC methods out of the wallet module.

    The enumeratesigners RPC can be used without a wallet since #21417. With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.

    The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context. A future displayaddress RPC call without wallet context could take a descriptor argument.

    This commit fixes a rpc_help.py failure when configured with --disable-wallet.

  2. DrahtBot added the label Build system on Mar 18, 2021
  3. DrahtBot added the label Docs on Mar 18, 2021
  4. DrahtBot added the label RPC/REST/ZMQ on Mar 18, 2021
  5. DrahtBot added the label Wallet on Mar 18, 2021
  6. Sjors force-pushed on Mar 18, 2021
  7. DrahtBot commented at 11:05 PM on March 18, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21538 (fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing ("syscall sanitizer") by practicalswift)
    • #21378 (Convert taproot to flag day activation by ajtowns)
    • #21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
    • #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
    • #20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
    • #20586 (Fix Windows build with --enable-werror by hebasto)
    • #20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
    • #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
    • #19438 (Introduce deploymentstatus by ajtowns)

    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.

  8. DrahtBot cross-referenced this on Mar 18, 2021 from issue Convert taproot to flag day activation by ajtowns
  9. DrahtBot cross-referenced this on Mar 19, 2021 from issue Use std::filesystem. Remove Boost Filesystem & System by fanquake
  10. DrahtBot cross-referenced this on Mar 19, 2021 from issue Fix Windows build with --enable-werror by hebasto
  11. DrahtBot cross-referenced this on Mar 19, 2021 from issue Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift
  12. DrahtBot cross-referenced this on Mar 19, 2021 from issue wallet: Migrate legacy wallets to descriptor wallets by achow101
  13. DrahtBot cross-referenced this on Mar 19, 2021 from issue Introduce deploymentstatus by ajtowns
  14. Sjors force-pushed on Mar 19, 2021
  15. fanquake requested review from achow101 on Mar 19, 2021
  16. fanquake requested review from instagibbs on Mar 19, 2021
  17. DrahtBot cross-referenced this on Mar 22, 2021 from issue MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky
  18. DrahtBot cross-referenced this on Mar 22, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
  19. in src/rpc/external_signer.cpp:7 in cafc7d210d outdated 
       2 | @@ -3,13 +3,12 @@
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 |  #include <chainparamsbase.h>
   6 | +#include <external_signer.h>
   7 |  #include <key_io.h>
    fanquake commented at 7:49 AM on March 22, 2021:

    I don't think you need key_io.h anymore?

  20. fanquake commented at 7:52 AM on March 22, 2021: member

    Concept ACK. While you're moving this around, could you also pull in a change similar to https://github.com/fanquake/bitcoin/commit/42228d716a263cb787cbe8125c855518127e9449. Note there are changes other than whitespace & formattting.

  21. Sjors force-pushed on Mar 22, 2021
  22. Sjors commented at 2:51 PM on March 22, 2021: member

    @fanquake done

  23. in src/wallet/rpcwallet.cpp:4531 in a1991f44fa outdated 
    4526 | @@ -4527,36 +4527,36 @@ static RPCHelpMan upgradewallet()
4527 |  #ifdef ENABLE_EXTERNAL_SIGNER
4528 |  static RPCHelpMan walletdisplayaddress()
4529 |  {
4530 | -    return RPCHelpMan{
4531 | -        "walletdisplayaddress",
4532 | -        "Display address on an external signer for verification.\n",
4533 | +    return RPCHelpMan{"walletdisplayaddress",
4534 | +        "\nDisplay address on an external signer for verification.\n",
    MarcoFalke commented at 6:45 PM on March 22, 2021:

    nit: I think the leading and trailing newlines should be the responsibility of RPCHelpMan. Also, they wouldn't make sense if the help is exported as json.

  24. in src/wallet/rpcwallet.cpp:4538 in a1991f44fa outdated 
    4540 | -        [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
4541 | -            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
4542 | -            if (!wallet) return NullUniValue;
4543 | -            CWallet* const pwallet = wallet.get();
4544 | +    [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
4545 | +{
    MarcoFalke commented at 6:46 PM on March 22, 2021:

    nit: For new code it would be good to use properly clang-formatted code. Any reason to make a change to move away from clang-formatted code?

  25. fanquake commented at 5:26 AM on March 23, 2021: member

    @Sjors I discussed with @MarcoFalke, and given there isn't currently a correct RPCHelpMan formatting style, can you drop the formatting/whitespace changes? We'll just add the [&]s, the examples etc. Sorry for the bum steer.

  26. Sjors force-pushed on Mar 26, 2021
  27. Sjors commented at 9:28 AM on March 26, 2021: member

    @fanquake done (I think) @MarcoFalke wrote:

    I think the leading and trailing newlines should be the responsibility of RPCHelpMan. Also, they wouldn't make sense if the help is exported as json.

    I left out the newline characters.

  28. DrahtBot cross-referenced this on Mar 28, 2021 from issue fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing ("syscall sanitizer") by practicalswift
  29. Sjors cross-referenced this on Mar 28, 2021 from issue UI external signer support (e.g. hardware wallet) by Sjors
  30. Sjors force-pushed on Apr 1, 2021
  31. Sjors force-pushed on Apr 2, 2021
  32. Sjors cross-referenced this on Apr 2, 2021 from issue rpc, gui: bumpfee signer support by Sjors
  33. ryanofsky cross-referenced this on Apr 2, 2021 from issue Drop JSONRPCRequest constructors after #21366 by ryanofsky
  34. DrahtBot cross-referenced this on Apr 3, 2021 from issue Fix wrong wallet RPC context set after #21366 by ryanofsky
  35. DrahtBot cross-referenced this on Apr 4, 2021 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  36. fanquake added this to the "PRs" column in a project
  37. DrahtBot added the label Needs rebase on Apr 7, 2021
  38. Sjors force-pushed on Apr 7, 2021
  39. Sjors commented at 6:57 PM on April 7, 2021: member

    After rebase the command walletdisplayaddress(address1) in test/function/wallet_signer.py line 99 fails with

    test_framework.authproxy.JSONRPCException: rpc/util.cpp:539 (HandleRequest)
Internal bug detected: 'std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [ret](const RPCResult& res) { return res.MatchesType(ret); })'

    Not sure what I'm doing wrong here... @MarcoFalke?

  40. DrahtBot removed the label Needs rebase on Apr 7, 2021
  41. in src/wallet/rpcwallet.cpp:4537 in fdab2d042f outdated 
    4532 | +    return RPCHelpMan{"walletdisplayaddress",
4533 | +        "Display address on an external signer for verification.",
4534 | +        {
4535 | +            {"address",     RPCArg::Type::STR, RPCArg::Optional::NO, /* default_val */ "", "bitcoin address to display"},
4536 | +        },
4537 | +        RPCResult{RPCResult::Type::NONE,"",""},
    MarcoFalke commented at 7:08 AM on April 8, 2021:

    This is wrong. It is an obj

  42. DrahtBot added the label Needs rebase on Apr 8, 2021
  43. Sjors force-pushed on Apr 8, 2021
  44. DrahtBot removed the label Needs rebase on Apr 8, 2021
  45. Move external signer out of wallet module
    This commit moves the ExternalSigner class and RPC methods out of the wallet module.

The enumeratesigners RPC can be used without a wallet since #21417.
With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.

The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context.
A future displayaddress RPC call without wallet context could take a descriptor argument.

This commit fixes a rpc_help.py failure when configured with --disable-wallet.
    b54b2e7b1a
  46. ci: use --enable-external-signer instead of --with-boost-process
    An earlier version of #16546 used both --with-boost-process and --enable-external-signer, which was simplified to only use the latter. However I forgot to update CI, so the external signer tests were not run.
    b0db187e5b
  47. rpc: add help for enumeratesigners and walletdisplayaddress 88d4d5ff2f
  48. Sjors force-pushed on Apr 8, 2021
  49. bitcoin deleted a comment on Apr 9, 2021
  50. ryanofsky approved
  51. ryanofsky commented at 6:18 PM on April 9, 2021: contributor

    Code review ACK 88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee

  52. RonSherfey changes_requested
  53. RonSherfey commented at 3:17 PM on April 11, 2021: none

    requested changes

  54. fanquake approved
  55. fanquake commented at 6:34 AM on April 13, 2021: member

    ACK 88d4d5ff2f5c71a9a2f4c78c2b2e2fd00568cfee

  56. fanquake merged this on Apr 13, 2021
  57. fanquake closed this on Apr 13, 2021
  58. Sjors deleted the branch on Apr 13, 2021
  59. fanquake cross-referenced this on Apr 13, 2021 from issue Miscellaneous external signer changes by fanquake
  60. sidhujag referenced this in commit ad75fd3f1b on Apr 13, 2021
  61. fanquake moved this from the "PRs" to the "Done" column in a project
  62. fanquake referenced this in commit e7af2f35af on Apr 14, 2021
  63. Sjors cross-referenced this on Jun 15, 2021 from issue Enable external signer support by default, reduce #ifdef by Sjors
  64. bitcoin locked this on Aug 18, 2022
Contributors
SjorsDrahtBotfanquakeMarcoFalkeryanofskyRonSherfey


achow101instagibbs

Labels
DocsWalletBuild systemRPC/REST/ZMQ
Linked (view graph)
#21417 Misc external signer improvement and HWI 2 support #21574 Drop JSONRPCRequest constructors after #21366#21576 rpc, gui: bumpfee signer support#21666 Miscellaneous external signer changes#21935 Enable external signer support by default, reduce #ifdef
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC