wallet: allow toggling external_signer flag #21928

Sjors · 2021-05-12T09:48:43Z

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.

Rspigler · 2021-05-12T20:50:37Z

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 · 2021-05-13T10:58:19Z

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 bitcoin-core/gui#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 · 2021-05-13T16:07:20Z

Great, thanks!

laanwj · 2021-05-18T06:53:18Z

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 · 2021-05-20T19:12:23Z

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 · 2021-05-20T19:16:09Z

Concept ACK

Rspigler · 2021-05-21T18:50:39Z

Concept ACK

luke-jr · 2021-09-01T02:08:54Z

src/wallet/rpcwallet.cpp

@@ -2440,6 +2440,9 @@ static RPCHelpMan getwalletinfo()
                            {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"},
                        }},
                        {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"},
+#ifdef ENABLE_EXTERNAL_SIGNER
+                        {RPCResult::Type::BOOL, "external_signer", "whether this wallet uses an external signer such as a hardware wallet"},


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

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

Github-Pull: bitcoin#21928 Rebased-From: 737373f

Github-Pull: bitcoin#21928 Rebased-From: 8a4eaaf

DrahtBot · 2021-11-24T03:42:20Z

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.

Sjors · 2021-12-08T06:03:14Z

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

Sjors · 2022-02-15T19:24:59Z

Rebased since #24307 covered the first commit.

ghost · 2022-02-15T23:49:48Z

src/wallet/wallet.cpp

@@ -55,6 +55,9 @@ const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
        "destinations in the past. Until this is done, some destinations may "
        "be considered unused, even if the opposite is the case."
    },
+    {WALLET_FLAG_EXTERNAL_SIGNER,
+        "The ability to toggle this flag may be removed in a future update."


Why do you expect this to be removed in future?

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.

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

ghost

utACK 1af2083

luke-jr · 2022-04-23T22:08:18Z

src/wallet/rpc/wallet.cpp

+    if (flag == WALLET_FLAG_EXTERNAL_SIGNER) {
+ #ifdef ENABLE_EXTERNAL_SIGNER
+         if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) || !pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
+             throw JSONRPCError(RPC_WALLET_ERROR, "This flag can only be set on a watch-only descriptor wallet");
+         }
+ #else
+         throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)");
+ #endif
+     }


nit: extra leading space for all this

luke-jr · 2022-04-23T22:57:48Z

src/wallet/wallet.h

@@ -129,7 +129,8 @@ static constexpr uint64_t KNOWN_WALLET_FLAGS =
    |   WALLET_FLAG_EXTERNAL_SIGNER;

 static constexpr uint64_t MUTABLE_WALLET_FLAGS =
-        WALLET_FLAG_AVOID_REUSE;
+        WALLET_FLAG_EXTERNAL_SIGNER


Alphabetical order?

Sjors · 2022-04-25T17:46:19Z

@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

Code Review ACK 1af2083

Github-Pull: bitcoin#21928 Rebased-From: 1af2083

Rspigler · 2022-07-10T20:38:51Z

tACK commit 1af2083

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 · 2022-07-13T13:16:33Z

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 · 2022-07-13T15:16:08Z

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 · 2022-12-14T20:42:16Z

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 · 2022-12-15T18:34:11Z

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

achow101 · 2023-01-04T00:42:20Z

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

That would be better.

Sjors · 2023-01-10T11:06:35Z

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

Github-Pull: bitcoin#21928 Rebased-From: 1af2083

Sjors force-pushed the 2021/05/hww-toggle branch from b536f77 to a7db1d3 Compare May 12, 2021 09:50

DrahtBot added RPC/REST/ZMQ Wallet labels May 12, 2021

Sjors force-pushed the 2021/05/hww-toggle branch from a7db1d3 to f6b76c3 Compare May 12, 2021 19:29

Sjors mentioned this pull request May 12, 2021

wallet: bitcoind fails to auto-load a non-empty external-signer wallet #21919

Closed

Sjors force-pushed the 2021/05/hww-toggle branch from f6b76c3 to 8671a2e Compare May 20, 2021 19:11

Sjors force-pushed the 2021/05/hww-toggle branch from 8671a2e to 7f6947d Compare August 5, 2021 14:31

luke-jr suggested changes Sep 1, 2021

View reviewed changes

Sjors force-pushed the 2021/05/hww-toggle branch from 7f6947d to 8a4eaaf Compare September 1, 2021 13:45

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

rpc: getwalletinfo shows external_signer flag

b897889

Github-Pull: bitcoin#21928 Rebased-From: 737373f

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

wallet: allow external_signer flag toggle

b875569

Github-Pull: bitcoin#21928 Rebased-From: 8a4eaaf

Sjors force-pushed the 2021/05/hww-toggle branch from 8a4eaaf to 9fcf302 Compare November 2, 2021 14:51

Sjors force-pushed the 2021/05/hww-toggle branch from 9fcf302 to 1770fb4 Compare November 18, 2021 12:09

DrahtBot mentioned this pull request Nov 23, 2021

Add external signer taproot support #23578

Merged

This was referenced Nov 29, 2021

Check descriptors returned by external signers #23628

Merged

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

Split up rpcwallet #23667

Merged

DrahtBot added the Needs rebase label Dec 8, 2021

Sjors force-pushed the 2021/05/hww-toggle branch from 1770fb4 to bd932e8 Compare December 8, 2021 06:02

DrahtBot removed the Needs rebase label Dec 8, 2021

Sjors force-pushed the 2021/05/hww-toggle branch from bd932e8 to 6cff15b Compare December 9, 2021 08:57

Sjors force-pushed the 2021/05/hww-toggle branch from 6cff15b to 2ed95d8 Compare January 7, 2022 10:31

Sjors mentioned this pull request Feb 10, 2022

RPC: Return external_signer in getwalletinfo #24307

Merged

DrahtBot added the Needs rebase label Feb 11, 2022

wallet: allow external_signer flag toggle

1af2083

Sjors force-pushed the 2021/05/hww-toggle branch from 2ed95d8 to 1af2083 Compare February 15, 2022 19:24

DrahtBot removed the Needs rebase label Feb 15, 2022

ghost reviewed Feb 15, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 17, 2022

RPC: Return external_signer command in getwalletinfo #24353

Closed

ghost approved these changes Feb 17, 2022

View reviewed changes

Sjors mentioned this pull request Apr 15, 2022

Awesome multisig PR labyrinth guide #24861

Open

16 tasks

DrahtBot mentioned this pull request Apr 17, 2022

[Draft / POC] Silent Payments #24897

Closed

luke-jr reviewed Apr 23, 2022

View reviewed changes

w0xlt approved these changes Apr 26, 2022

View reviewed changes

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

wallet: allow external_signer flag toggle

439d08b

Github-Pull: bitcoin#21928 Rebased-From: 1af2083

Sjors closed this Jan 10, 2023

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

wallet: allow external_signer flag toggle

dc97030

Github-Pull: bitcoin#21928 Rebased-From: 1af2083

bitcoin locked and limited conversation to collaborators Jan 10, 2024

wallet: allow toggling external_signer flag #21928

wallet: allow toggling external_signer flag #21928

Uh oh!

Conversation

Sjors commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rspigler commented May 12, 2021

Uh oh!

Sjors commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rspigler commented May 13, 2021

Uh oh!

laanwj commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented May 20, 2021

Uh oh!

Rspigler commented May 21, 2021

Uh oh!

luke-jr Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Sjors commented Dec 8, 2021

Uh oh!

Sjors commented Feb 15, 2022

Uh oh!

ghost Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors commented Apr 25, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Rspigler commented Jul 10, 2022

Uh oh!

Sjors commented Jul 13, 2022

Uh oh!

Rspigler commented Jul 13, 2022

Uh oh!

achow101 commented Dec 14, 2022

Uh oh!

Sjors commented Dec 15, 2022

Uh oh!

achow101 commented Jan 4, 2023

Uh oh!

Sjors commented Jan 10, 2023

Uh oh!

Uh oh!

Sjors commented May 12, 2021 •

edited

Loading

Sjors commented May 13, 2021 •

edited

Loading

laanwj commented May 18, 2021 •

edited

Loading

Sjors commented May 20, 2021 •

edited

Loading

DrahtBot commented Nov 24, 2021 •

edited

Loading

Sjors Feb 17, 2022 •

edited

Loading