Improve display address handling for external signer

Sjors commented at 4:22 pm on February 10, 2022: member

HWI returns the requested address: as a sanity check, we now compare that to what we expected
- external signer documentation now reflects that HWI alternatives must implement this check
both RPC and GUI will now return an error text, rather than just fail (the GUI even failed silently in some cases)

DrahtBot added the label GUI on Feb 10, 2022

DrahtBot added the label interfaces on Feb 10, 2022

DrahtBot added the label RPC/REST/ZMQ on Feb 10, 2022

DrahtBot added the label Wallet on Feb 10, 2022

DrahtBot commented at 10:51 am on February 11, 2022: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	brunoerg, achow101
Approach ACK	jonatack

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:

#bitcoin-core/gui/700 (Dialog for allowing the user to choose the change output when bumping a tx by achow101)

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.

in test/functional/wallet_signer.py:115 in 803387f054 outdated

110@@ -111,8 +111,10 @@ def test_valid_signer(self):
111         assert_equal(address_info['hdkeypath'], "m/44'/1'/0'/0/0")
112 
113         self.log.info('Test walletdisplayaddress')
114-        result = hww.walletdisplayaddress(address1)
115-        assert_equal(result, {"address": address1})
116+        for address in [address1, address2, address3]:
117+            print(address)

jonatack commented at 1:24 pm on February 11, 2022:

c83ab3059 The printed addresses correspond to the updated mocks. I think you intended to remove this line, though.

in src/wallet/external_signer_scriptpubkeyman.h:36 in 803387f054 outdated

28@@ -27,7 +29,11 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
29 
30   static ExternalSigner GetExternalSigner();
31 
32-  bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const;
33+  /**
34+  * Display address on the device and verify that the returned value matches.
35+  * @returns an error message or nullopt
36+  */
37+  std::optional<bilingual_str> DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const;

jonatack commented at 1:26 pm on February 11, 2022:

c83ab3059 since you’re touching this line

0  std::optional<bilingual_str> DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const;

in src/wallet/external_signer_scriptpubkeyman.cpp:61 in 803387f054 outdated

60     auto descriptor = InferDescriptor(scriptPubKey, *provider);
61 
62-    signer.DisplayAddress(descriptor->ToString());
63-    // TODO inspect result
64-    return true;
65+    UniValue result = signer.DisplayAddress(descriptor->ToString());

jonatack commented at 1:28 pm on February 11, 2022:

c83ab3059

 0@@ -54,11 +54,11 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
 1 std::optional<bilingual_str> ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const
 2 {
 3     // TODO: avoid the need to infer a descriptor from inside a descriptor wallet
 4-    CScript scriptPubKey = GetScriptForDestination(dest);
 5+    const CScript& scriptPubKey = GetScriptForDestination(dest);
 6     auto provider = GetSolvingProvider(scriptPubKey);
 7     auto descriptor = InferDescriptor(scriptPubKey, *provider);
 8 
 9-    UniValue result = signer.DisplayAddress(descriptor->ToString());
10+    const UniValue& result = signer.DisplayAddress(descriptor->ToString());

in src/wallet/external_signer_scriptpubkeyman.cpp:73 in 803387f054 outdated

70+    const UniValue& ret_address = find_value(result, "address");
71+    if (!ret_address.isStr()) return _("Signer did not echo address");
72+
73+    if (ret_address.getValStr() != EncodeDestination(dest)) {
74+        return strprintf(_("Signer echoed unexpected address %s"), ret_address.getValStr());
75+    }

jonatack commented at 1:34 pm on February 11, 2022:

Tests of these new errors would be nice, not sure how feasible this would be.

Sjors commented at 10:48 am on February 17, 2022:

This particular one should be doable.

Sjors commented at 2:08 pm on June 23, 2022:

Added.

jonatack commented at 1:34 pm on February 11, 2022: contributor

Approach ACK

in src/wallet/external_signer_scriptpubkeyman.cpp:70 in 803387f054 outdated

66+
67+    const UniValue& error = find_value(result, "error");
68+    if (error.isStr()) return strprintf(_("Signer returned error: %s"), error.getValStr());
69+
70+    const UniValue& ret_address = find_value(result, "address");
71+    if (!ret_address.isStr()) return _("Signer did not echo address");

luke-jr commented at 1:21 am on February 13, 2022:

Won’t this break older external signers?

Sjors commented at 10:47 am on February 17, 2022:

HWI has always done this, we just didn’t check. That said, it’s not (yet) in the spec (which may be a bit outdated) so other signer software might not do this. Do you know of any? cc @achow101

https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md#displayaddress-optional

achow101 commented at 5:02 pm on May 24, 2023:

AFAIK there aren’t any other external signer software yet.

in src/qt/walletmodel.cpp:572 in 803387f054 outdated

570     }
571-    return res;
572+    if (error) {
573+        QMessageBox::warning(nullptr, tr("Signer error"), QString::fromStdString(error->translated));
574+    }
575+    return true;

luke-jr commented at 1:55 am on February 20, 2022:

0    return !error;

Sjors commented at 2:08 pm on June 23, 2022: member

Rebased and addressed comments.

I could change “Signer did not echo address” from an error to a warning. However I’d rather only do this is we know there is an HWI clone out there that relies on the (less safe) earlier behavior. I updated the documentation. See also #24313 (review).

Sjors force-pushed on Jun 23, 2022

Sjors force-pushed on Sep 6, 2022

DrahtBot added the label Needs rebase on Oct 26, 2022

Sjors force-pushed on Oct 26, 2022

Sjors commented at 2:15 pm on October 26, 2022: member

Rebased after #23578.

DrahtBot removed the label Needs rebase on Oct 26, 2022

DrahtBot added the label Needs rebase on Apr 21, 2023

achow101 requested review from achow101 on Apr 25, 2023

Sjors commented at 1:28 pm on May 8, 2023: member

Rebased and tested that it still works.

Sjors force-pushed on May 8, 2023

DrahtBot removed the label Needs rebase on May 8, 2023

DrahtBot added the label CI failed on May 13, 2023

Sjors force-pushed on May 19, 2023

Sjors commented at 8:46 am on May 19, 2023: member

Rebased because <key_io.h> moved (Kernel related?) ~~and to add an explicit include <univalue.h> (macOS / clang doesn’t care, but CI does and it makes sense anyway)~~.

Sjors force-pushed on May 19, 2023

Sjors commented at 9:03 am on May 19, 2023: member

Oops, I didn’t rebase all the way. Specifically not past #27605. Will fix.

Sjors force-pushed on May 19, 2023

maflcko commented at 10:24 am on May 19, 2023: member

Looks like CI failed?

Sjors commented at 12:46 pm on May 19, 2023: member

Ah yes, broken by my own #26076.

Sjors force-pushed on May 19, 2023

DrahtBot removed the label CI failed on May 19, 2023

Sjors commented at 7:31 am on May 20, 2023: member

CI is happy now

in src/interfaces/wallet.h:128 in 8218ce6b02 outdated

124@@ -125,7 +125,7 @@ class Wallet
125     virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0;
126 
127     //! Display address on external signer
128-    virtual bool displayAddress(const CTxDestination& dest) = 0;
129+    virtual std::optional<bilingual_str> displayAddress(const CTxDestination& dest) = 0;

achow101 commented at 9:27 pm on May 23, 2023:

In 8218ce6b02b24eeef31f4e343ea777b7844907fc “wallet: return and display signer error”

Returning the error in an optional feels a bit wrong since it inverts the typical pattern. Rather than checking that the result is equivalent to true, we have to check that it’s equivalent to false.

I think it would be better to use util::Result<bool> instead of an optional in order to preserve our typical error checking pattern.

Sjors commented at 9:26 am on May 24, 2023:

Done

ryanofsky commented at 12:35 pm on May 24, 2023:

re: #24313 (review)

Returning the error in an optional feels a bit wrong since it inverts the typical pattern. Rather than checking that the result is equivalent to true, we have to check that it’s equivalent to false.

Returning optional<bilingual_str> or Result<bool> both seem ok here, but the ideal thing would be to return Result<void> so there are just 2 cases for calling code to handle 1-success, and 2-failure with an error message. Not 3 cases to handle: 1-success returning true, 2-success returning false, and 3-failure with an error message.

Complaining about the optional<bilingual_str> pattern is a recurring topic, see #27632 (review), #27632 (review), #25648 (review) and I have a PR to replace uses of optional<bilingual_str> with util::Result in #25977, but it’s blocked waiting on a complicated base PR which adds support for Result<void>

Since this keeps coming up, though I think I will just add simple support for Result<void> to #25977 and decouple it from the base PR so it can get unstuck.

Just thinking out loud here, though (and advertising #25977)

Sjors commented at 2:21 pm on August 1, 2023:

I switched to using util::Result in my last rebase a few months ago.

Sjors force-pushed on May 24, 2023

DrahtBot added the label CI failed on May 24, 2023

DrahtBot removed the label CI failed on May 24, 2023

achow101 commented at 5:02 pm on May 24, 2023: member

ACK e3d02f8ef8cfe1d3c502ede863d14f82212a0ce6

fanquake referenced this in commit 4d13fe47be on May 26, 2023

sidhujag referenced this in commit 20272f34e0 on May 26, 2023

DrahtBot added the label CI failed on May 31, 2023

DrahtBot removed the label CI failed on May 31, 2023

in src/wallet/rpc/addresses.cpp:794 in e3d02f8ef8 outdated

779@@ -780,9 +780,8 @@ RPCHelpMan walletdisplayaddress()
780                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
781             }
782 
783-            if (!pwallet->DisplayAddress(dest)) {
784-                throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address");
785-            }
786+            util::Result<bool> res = pwallet->DisplayAddress(dest);
787+            if (!res) throw JSONRPCError(RPC_MISC_ERROR, util::ErrorString(res).translated);
788

maflcko commented at 2:47 pm on August 1, 2023:

Pretty sure this is wrong, as you are not checking !res.value(). (Previously it threw, now it silently continues. Now it only throws when no ExternalSignerScriptPubKeyMan is available.)

edit: nvm. The res.value() is just unused.

This can be fixed non-confusingly by using Result<void>

Sjors commented at 3:49 pm on August 1, 2023: member

Rebased and switched to Result<void>. (need to push a fix)

Sjors force-pushed on Aug 1, 2023

DrahtBot added the label CI failed on Aug 1, 2023

in src/wallet/rpc/addresses.cpp:784 in 0e0163a062 outdated

779@@ -780,9 +780,8 @@ RPCHelpMan walletdisplayaddress()
780                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
781             }
782 
783-            if (!pwallet->DisplayAddress(dest)) {
784-                throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address");
785-            }
786+            util::Result<void> res = pwallet->DisplayAddress(dest);
787+            if (!res) throw JSONRPCError(RPC_MISC_ERROR, util::ErrorString(res).translated);

ryanofsky commented at 5:35 pm on August 1, 2023:

In commit “wallet: return and display signer error” (0e0163a062b09a3f762d7dd0af3d6e78e675aae1)

s/translated/original/ here since RPC methods return untranslated error strings

Sjors commented at 4:21 pm on August 2, 2023:

Fixed

DrahtBot removed the label CI failed on Aug 1, 2023

Sjors force-pushed on Aug 2, 2023

achow101 commented at 7:26 pm on August 24, 2023: member

re-ACK 5536ace894e67322161a649f998265f461fbc8ad

DrahtBot added the label CI failed on Dec 10, 2023

DrahtBot removed the label CI failed on Dec 15, 2023

brunoerg commented at 2:41 pm on December 18, 2023: contributor

Concept ACK

DrahtBot requested review from jonatack on Dec 18, 2023

in src/wallet/external_signer_scriptpubkeyman.cpp:58 in 35daa69980 outdated

52@@ -52,15 +53,19 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
53     return signers[0];
54 }
55 
56-bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const
57+bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const
58 {
59     // TODO: avoid the need to infer a descriptor from inside a descriptor wallet

brunoerg commented at 3:51 pm on December 18, 2023:

Do you intend to address this TODO?

Sjors commented at 10:57 am on December 22, 2023:

Not in this PR, unless someone else gives me the code for it :-)

in src/wallet/wallet.cpp:2647 in 5536ace894 outdated

2643@@ -2644,7 +2644,7 @@ bool CWallet::DisplayAddress(const CTxDestination& dest)
2644         ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner();
2645         return signer_spk_man->DisplayAddress(dest, signer);
2646     }
2647-    return false;
2648+    return util::Error{_("No external signer ScriptPubKeyManager")};

brunoerg commented at 10:12 pm on December 18, 2023:

In 5536ace894e67322161a649f998265f461fbc8ad: I think we can improve the “No external signer ScriptPubKeyManager” message, it doesn’t sound intuitive. First time I tested it prior reading the code, I got this error and I thought I didn’t have any external signer, so I checked it with enumerate.

Perhaps: “There is no ScriptPubKeyManager for this address”.

Sjors commented at 10:58 am on December 22, 2023:

Will consider new wording if I need to touch / rebase.

Sjors commented at 12:24 pm on February 13, 2024:

Done

brunoerg commented at 1:30 pm on December 22, 2023: contributor

I tested it with HWI and Ledger Nano S Plus.

I created a wallet:

0./src/bitcoin-cli -named createwallet wallet_name=ledger3 disable_private_keys=true descriptors=true external_signer=true`

Got a new address:

0$ ./src/bitcoin-cli -rpcwallet=ledger3 getnewaddress
1bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m

Ran walletdisplayaddress:

0$ ./src/bitcoin-cli -rpcwallet=ledger3 walletdisplayaddress "bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m"

Checked it in the Ledger device:
I locked the Ledger device and tried walletdisplayaddress to check the error message:

This PR: '/Users/bg/projects/HWI/hwi.py' error: Could not open client or get fingerprint information: Exception: invalid status 0x6982

Master: '/Users/brunogarcia/projects/HWI/hwi.py' error: Could not open client or get fingerprint information: Exception: invalid status 0x6982

Ran walletdisplayaddress with a random address:

On master:

0error code: -1
1error message:
2Failed to display address

This PR:

0error code: -1
1error message:
2No external signer ScriptPubKeyManager

Sjors commented at 2:25 pm on December 22, 2023: member

Mmm, on the bright side, both the master and this PR have a really unclear message when the device is not connected or locked :-)

I’ll try to improve that.

Sjors commented at 2:27 pm on December 22, 2023: member

@achow101 any thoughts on having HWI catch this cryptic 0x6982 status code and throw a more clear error (that we can then just pass on to the user)?

brunoerg commented at 2:53 pm on December 22, 2023: contributor

@achow101 any thoughts on having HWI catch this cryptic 0x6982 status code and throw a more clear error (that we can then just pass on to the user)?

Not sure, but this status code is specific from Ledger?

Sjors commented at 3:38 pm on December 22, 2023: member

@brunoerg most likely yes, but I can’t find a full list anywhere.

brunoerg commented at 4:10 pm on December 22, 2023: contributor

@brunoerg most likely yes, but I can’t find a full list anywhere.

Me either. I’ve some of them but I manually mapped.

edit: I found them in HWI:

 0class DeviceException(Exception):  # pylint: disable=too-few-public-methods
 1    exc: Dict[int, Any] = {
 2        0x6985: DenyError,
 3        0x6982: SecurityStatusNotSatisfiedError,
 4        0x6A80: IncorrectDataError,
 5        0x6A82: NotSupportedError,
 6        0x6A86: WrongP1P2Error,
 7        0x6A87: WrongDataLengthError,
 8        0x6D00: InsNotSupportedError,
 9        0x6E00: ClaNotSupportedError,
10        0xB000: WrongResponseLengthError,
11        0xB007: BadStateError,
12        0xB008: SignatureFailError,
13        0xE000: InterruptedExecution,  # not an error
14    }

DrahtBot added the label CI failed on Jan 14, 2024

achow101 referenced this in commit 59eeb858b1 on Jan 17, 2024

Sjors force-pushed on Feb 13, 2024

Sjors commented at 12:35 pm on February 13, 2024: member

Rebased to give CI another try.

HWI now returns better errors, though I have not yet tested it here.

DrahtBot removed the label CI failed on Feb 13, 2024

Sjors commented at 10:05 am on February 16, 2024: member

Tested with HWI 2.4.0

When a Ledger Nano X is in screen saver mode, I now get the following error:

0'...hwi' error: Could not open client or get fingerprint information: ('0x5515', 'Error in <DefaultInsType.GET_VERSION: 1> command', '')

That’s what HWI returns, so can’t make it much better…

When the Bitcoin app is not opened on the device the error is more clear:

0'...hwi' error: Could not open client or get fingerprint information: Ledger is not in either the Bitcoin or Bitcoin Testnet app

test: use h marker for external signer mock

Consistent with #26076

6c1a2cc09a

wallet: compare address returned by displayaddress

Update external signer documentation to reflect this requirement, which HWI already implements.

dc55531087

wallet: return and display signer error

Both RPC and GUI now render a useful error message instead of (silently) failing.

Replace bool with util::Result<void> to clarify that this either succeeds or returns an error message.

4357158c47

Sjors force-pushed on Apr 16, 2024

Sjors commented at 4:01 pm on April 16, 2024: member

Rebased after #28981 (I previously already tested on top of that).

Briefly tested with HWI 3.0 as well on a Ledger Nano X.

Sjors requested review from brunoerg on Apr 16, 2024

brunoerg commented at 6:05 pm on April 17, 2024: contributor

ACK 4357158c4712d479522d5cd441ad4dd1693fdd05

I did same test as #24313 (comment) but with the latest version of HWI.

0./src/bitcoin-cli -rpcwallet=ledger7 walletdisplayaddress "bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m"
1error code: -1
2error message:
3'/Users/brunogarcia/projects/HWI/hwi.py' error: Could not open client or get fingerprint information: Ledger is not in either the Bitcoin or Bitcoin Testnet app

0./src/bitcoin-cli -rpcwallet=ledger7 walletdisplayaddress "randomaddresshere"
1error code: -1
2error message:
3There is no ScriptPubKeyManager for this address

DrahtBot requested review from achow101 on Apr 17, 2024

achow101 commented at 9:13 pm on April 23, 2024: member

ACK 4357158c4712d479522d5cd441ad4dd1693fdd05

achow101 merged this on Apr 23, 2024

achow101 closed this on Apr 23, 2024

Sjors deleted the branch on May 6, 2024

Improve display address handling for external signer #24313

Code Coverage

Reviews

Conflicts