wallet: allow toggling external_signer flag #21928

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2021/05/hww-toggle changing 4 files +23 −2
  1. Sjors commented at 9:48 AM on May 12, 2021: member

    Currently WALLET_FLAG_EXTERNAL_SIGNER is just a precaution. This PR makes the flag mutable so it can be toggled with setwalletflag. There's a warning that in the future it may no longer be possible to toggle. This might be the case if we need to store additional device specific data in the wallet payload.

    Rationale for this PR is to make it a bit easier to switch between tooling, e.g. to "upgrade" a regular watch-only wallet created by Specter to work with Core hardware wallet support directly.

  2. Sjors force-pushed on May 12, 2021
  3. DrahtBot added the label RPC/REST/ZMQ on May 12, 2021
  4. DrahtBot added the label Wallet on May 12, 2021
  5. Sjors force-pushed on May 12, 2021
  6. Sjors cross-referenced this on May 12, 2021 from issue wallet: bitcoind fails to auto-load a non-empty external-signer wallet by hebasto
  7. Rspigler commented at 8:50 PM on May 12, 2021: contributor

    What would the upgrade process be from Spectre to Core (for example) with/without this PR? I'm confused how the toggling would work to make this easier

  8. Sjors commented at 10:58 AM on May 13, 2021: member

    Before this PR if you opened a Specter wallet in Bitcoin Core you can see the transaction history, create receive addresses and create a PSBT. But you can't use the send RPC (or GUI in https://github.com/bitcoin-core/gui/pull/4) to have it sign on a device and you can't verify an address on the device.

    The WALLET_FLAG_EXTERNAL_SIGNER must be set in order for Bitcoin Core to make calls to HWI. Without this PR you'd have to manually edit the wallet file to set that flag. Or export the descriptors and then re-import them in a fresh wallet that has the flag enabled.

    With this PR you just call bitcoin-cli setwalletflag external_signer true.

  9. Rspigler commented at 4:07 PM on May 13, 2021: contributor

    Great, thanks!

  10. laanwj commented at 6:53 AM on May 18, 2021: member

    Rationale for this PR is to make it a bit easier to switch between tooling, e.g. to "upgrade" a regular watch-only wallet created by Specter to work with Core hardware wallet support directly.

    I guess it should only be possible to do so for watch-only wallets, not for wallets that already have their own private keys?

  11. Sjors force-pushed on May 20, 2021
  12. Sjors commented at 7:12 PM on May 20, 2021: member

    Rebased and added check that it's a watch-only descriptor wallet. I don't mention this in caveats, because those are only shown after you set a flag.

  13. jonatack commented at 7:16 PM on May 20, 2021: contributor

    Concept ACK

  14. Rspigler commented at 6:50 PM on May 21, 2021: contributor

    Concept ACK

  15. Sjors force-pushed on Aug 5, 2021
  16. in src/wallet/rpcwallet.cpp:2444 in 7f6947dd70 outdated 
    2439 | @@ -2440,6 +2440,9 @@ static RPCHelpMan getwalletinfo()
2440 |                              {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"},
2441 |                          }},
2442 |                          {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"},
2443 | +#ifdef ENABLE_EXTERNAL_SIGNER
2444 | +                        {RPCResult::Type::BOOL, "external_signer", "whether this wallet uses an external signer such as a hardware wallet"},
    luke-jr commented at 2:08 AM on September 1, 2021:

    Not sure about having this as a compile-time conditional. Maybe document it and mention that omission means the build doesn't support it?

    Sjors commented at 1:40 PM on September 1, 2021:

    I'm dropping the #ifdef here and below. The WALLET_FLAG_EXTERNAL_SIGNER is always defined anyway.

  17. luke-jr changes_requested
  18. Sjors force-pushed on Sep 1, 2021
  19. luke-jr referenced this in commit b8978899d2 on Oct 10, 2021
  20. luke-jr referenced this in commit b875569916 on Oct 10, 2021
  21. Sjors force-pushed on Nov 2, 2021
  22. Sjors force-pushed on Nov 18, 2021
  23. DrahtBot cross-referenced this on Nov 23, 2021 from issue Add external signer taproot support by Sjors
  24. DrahtBot commented at 3:42 AM on November 24, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK w0xlt, Rspigler
    Concept ACK jonatack
    Approach NACK achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24897 ([Draft / POC] Silent Payments by w0xlt)

    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.

  25. DrahtBot cross-referenced this on Nov 29, 2021 from issue Check descriptors returned by external signers by sstone
  26. DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider
  27. DrahtBot cross-referenced this on Dec 6, 2021 from issue Split up rpcwallet by meshcollider
  28. DrahtBot added the label Needs rebase on Dec 8, 2021
  29. Sjors force-pushed on Dec 8, 2021
  30. Sjors commented at 6:03 AM on December 8, 2021: member

    Rebased after #23667 (manually re-applied changes from wallet/rpcwallet.cpp to wallet/rpc/wallet.cpp)

  31. DrahtBot removed the label Needs rebase on Dec 8, 2021
  32. Sjors force-pushed on Dec 9, 2021
  33. Sjors force-pushed on Jan 7, 2022
  34. Sjors cross-referenced this on Feb 10, 2022 from issue RPC: Return external_signer in getwalletinfo by kristapsk
  35. DrahtBot added the label Needs rebase on Feb 11, 2022
  36. wallet: allow external_signer flag toggle 1af2083180
  37. Sjors force-pushed on Feb 15, 2022
  38. Sjors commented at 7:24 PM on February 15, 2022: member

    Rebased since #24307 covered the first commit.

  39. DrahtBot removed the label Needs rebase on Feb 15, 2022
  40. in src/wallet/wallet.cpp:59 in 1af2083180 
      54 | @@ -55,6 +55,9 @@ const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
  55 |          "destinations in the past. Until this is done, some destinations may "
  56 |          "be considered unused, even if the opposite is the case."
  57 |      },
  58 | +    {WALLET_FLAG_EXTERNAL_SIGNER,
  59 | +        "The ability to toggle this flag may be removed in a future update."
    unknown commented at 11:49 PM on February 15, 2022:

    Why do you expect this to be removed in future?

    Sjors commented at 10:42 AM on February 17, 2022:

    If we ever need to make a backwards incompatible change to these types of wallets.

    I don't have anything specific in mind. Right now it's perfectly safe to treat such a wallet as a regular watch-only wallet, create a PSBT and call HWI yourself. All unknown future fields and flags are simply ignored (and not deleted).

    But perhaps there's some future fancy multisig setup with radioactive nonces and hash pre-images, that would make any naive use of the wallet keys dangerous.

    luke-jr commented at 10:46 PM on April 23, 2022:

    Likely possibility: In the future, setting it may require providing a specific path to the signer program

  41. DrahtBot cross-referenced this on Feb 17, 2022 from issue RPC: Return external_signer command in getwalletinfo by kristapsk
  42. unknown approved
  44. Sjors cross-referenced this on Apr 15, 2022 from issue Awesome multisig PR labyrinth guide by Sjors
  45. DrahtBot cross-referenced this on Apr 17, 2022 from issue [Draft / POC] Silent Payments by w0xlt
  46. in src/wallet/rpc/wallet.cpp:287 in 1af2083180 
     282 | +             throw JSONRPCError(RPC_WALLET_ERROR, "This flag can only be set on a watch-only descriptor wallet");
 283 | +         }
 284 | + #else
 285 | +         throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)");
 286 | + #endif
 287 | +     }
    luke-jr commented at 10:08 PM on April 23, 2022:

    nit: extra leading space for all this

  47. in src/wallet/wallet.h:132 in 1af2083180 
     128 | @@ -129,7 +129,8 @@ static constexpr uint64_t KNOWN_WALLET_FLAGS =
 129 |      |   WALLET_FLAG_EXTERNAL_SIGNER;
 130 |  
 131 |  static constexpr uint64_t MUTABLE_WALLET_FLAGS =
 132 | -        WALLET_FLAG_AVOID_REUSE;
 133 | +        WALLET_FLAG_EXTERNAL_SIGNER
    luke-jr commented at 10:57 PM on April 23, 2022:

    Alphabetical order?

  48. Sjors commented at 5:46 PM on April 25, 2022: member

    @luke-jr I'll implement your nits if the PR needs a rebase or bigger changes.

    OTOH not sure how to weigh an anonymous ACK, and I forgot who it was from: <img width="440" alt="Schermafbeelding 2022-04-25 om 19 45 45" src="https://user-images.githubusercontent.com/10217/165144632-74a4a09e-17b8-4fb7-9680-c980c854839e.png">

  49. w0xlt approved
  50. w0xlt commented at 9:13 AM on April 26, 2022: contributor

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/21928/commits/1af208318067843cc1adbc352c3332ed68ebf391

  51. luke-jr referenced this in commit 439d08bdcc on May 21, 2022
  52. Rspigler commented at 8:38 PM on July 10, 2022: contributor

    tACK commit 1af208318067843cc1adbc352c3332ed68ebf391

    All tests pass

    I did not test with HWI, but I was able to toggle external_signer with setwalletflag (console says that this can only be set on a watch only descriptor wallet).

    This can only be done on a wallet that has private keys disabled, not on a blank wallet - I don't know if that should be made more clear? setwalletflag external_signer

    { "flag_name": "external_signer", "flag_state": true, "warnings": "The ability to toggle this flag may be removed in a future update." }

    I did get this error in the terminal when starting bitcoin-qt:

    QVariant::load: unknown user type with name BitcoinUnits::Unit.

  53. Sjors commented at 1:16 PM on July 13, 2022: member

    This can only be done on a wallet that has private keys disabled, not on a blank wallet

    It should be same constraint as with createwallet.

    I did get this error in the terminal

    Only for this PR or also on the commit it builds on (git checkout HEAD~1)?

  54. Rspigler commented at 3:16 PM on July 13, 2022: contributor

    If I do it on a blank wallet without private keys disabled, I get this error:

    This flag can only be set on a watch-only descriptor wallet (code -4)

    Hm I apologize, I didn't get the error this time - not sure what the issue was

  55. achow101 commented at 8:42 PM on December 14, 2022: member

    Approach NACK

    This is incomplete. A wallet that has the flag set when it was loaded will behave differently from one that has the flag set after it's been loaded.The new expected behavior (either enable or disable external signer) will only come into effect after the wallet has been reloaded. This is unintuitive and potentially dangerous.

    The flag is only checked during wallet creation and loading. It isn't checked during normal operations. We instead use ExternalSignerScriptPubKeyMan to do all of the external signer things. When this flag is toggled, it does not replace the ExternalSignerSPKMs with normal DescriptorSPKMs or vice versa. So the previous behavior will still be in effect post toggling.

  56. Sjors commented at 6:34 PM on December 15, 2022: member

    @achow101 in that case perhaps it's safer to implement this in bitcoin-wallet?

  57. achow101 commented at 12:42 AM on January 4, 2023: member

    @achow101 in that case perhaps it's safer to implement this in bitcoin-wallet?

    That would be better.

  58. Sjors commented at 11:06 AM on January 10, 2023: member

    Leaving it up for grabs to implement this in bitcoin-wallet.

  59. Sjors closed this on Jan 10, 2023
  60. luke-jr referenced this in commit dc97030bfc on Aug 16, 2023
  61. bitcoin locked this on Jan 10, 2024
Contributors
SjorsRspiglerlaanwjjonatackluke-jrDrahtBotw0xltachow101
Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#21919 wallet: bitcoind fails to auto-load a non-empty external-signer wallet#24307 RPC: Return external_signer in getwalletinfo#24861 Awesome multisig PR labyrinth guide
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