wallet: allow toggling external

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.

Sjors force-pushed on May 12, 2021

DrahtBot added the label RPC/REST/ZMQ on May 12, 2021

DrahtBot added the label Wallet on May 12, 2021

Sjors force-pushed on May 12, 2021

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

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.

Rspigler commented at 4:07 pm on May 13, 2021: contributor

Great, thanks!

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?

Sjors force-pushed on May 20, 2021

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.

jonatack commented at 7:16 pm on May 20, 2021: contributor

Concept ACK

Rspigler commented at 6:50 pm on May 21, 2021: contributor

Concept ACK

Sjors force-pushed on Aug 5, 2021

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.

luke-jr changes_requested

Sjors force-pushed on Sep 1, 2021

luke-jr referenced this in commit b8978899d2 on Oct 10, 2021

luke-jr referenced this in commit b875569916 on Oct 10, 2021

Sjors force-pushed on Nov 2, 2021

Sjors force-pushed on Nov 18, 2021

DrahtBot commented at 3:42 am on November 24, 2021: contributor

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

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.

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.

DrahtBot added the label Needs rebase on Dec 8, 2021

Sjors force-pushed on Dec 8, 2021

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)

DrahtBot removed the label Needs rebase on Dec 8, 2021

Sjors force-pushed on Dec 9, 2021

Sjors force-pushed on Jan 7, 2022

DrahtBot added the label Needs rebase on Feb 11, 2022

wallet: allow external_signer flag toggle 1af2083180

Sjors force-pushed on Feb 15, 2022

Sjors commented at 7:24 pm on February 15, 2022: member

Rebased since #24307 covered the first commit.

DrahtBot removed the label Needs rebase on Feb 15, 2022

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

unknown approved

unknown commented at 11:17 am on February 17, 2022: none

utACK https://github.com/bitcoin/bitcoin/pull/21928/commits/1af208318067843cc1adbc352c3332ed68ebf391

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

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?

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:

w0xlt approved

w0xlt commented at 9:13 am on April 26, 2022: contributor

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

luke-jr referenced this in commit 439d08bdcc on May 21, 2022

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.

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)?

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

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.

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?

achow101 commented at 0: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.

Sjors commented at 11:06 am on January 10, 2023: member

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

Sjors closed this on Jan 10, 2023

luke-jr referenced this in commit dc97030bfc on Aug 16, 2023

bitcoin locked this on Jan 10, 2024

wallet: allow toggling external_signer flag #21928

Reviews

Conflicts