Improve display address handling for external signer #24313

Sjors · 2022-02-10T16:22:39Z

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 · 2022-02-11T10:51:17Z

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.

jonatack

Approach ACK

test/functional/wallet_signer.py

src/wallet/external_signer_scriptpubkeyman.h

src/wallet/external_signer_scriptpubkeyman.cpp

jonatack · 2022-02-11T13:34:19Z

src/wallet/external_signer_scriptpubkeyman.cpp

+
+    if (ret_address.getValStr() != EncodeDestination(dest)) {
+        return strprintf(_("Signer echoed unexpected address %s"), ret_address.getValStr());
+    }


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

This particular one should be doable.

luke-jr · 2022-02-13T01:21:35Z

src/wallet/external_signer_scriptpubkeyman.cpp

+    const UniValue& ret_address = find_value(result, "address");
+    if (!ret_address.isStr()) return _("Signer did not echo address");


Won't this break older external signers?

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

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

src/qt/walletmodel.cpp

Sjors · 2022-06-23T14:08:31Z

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 (comment).

Sjors · 2022-10-26T14:15:10Z

Rebased after #23578.

Sjors · 2023-05-08T13:28:49Z

Rebased and tested that it still works.

brunoerg · 2023-12-18T14:41:42Z

Concept ACK

brunoerg · 2023-12-18T15:51:34Z

src/wallet/external_signer_scriptpubkeyman.cpp

@@ -52,15 +53,19 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
    return signers[0];
 }

-bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const
+bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const
 {
    // TODO: avoid the need to infer a descriptor from inside a descriptor wallet


Do you intend to address this TODO?

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

brunoerg · 2023-12-18T22:12:30Z

src/wallet/wallet.cpp

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


In 5536ace: 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".

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

brunoerg · 2023-12-22T13:30:34Z

I tested it with HWI and Ledger Nano S Plus.

I created a wallet:

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

Got a new address:

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

Ran walletdisplayaddress:

$ ./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:

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

This PR:

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

Sjors · 2023-12-22T14:25:44Z

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 · 2023-12-22T14:27:24Z

@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 · 2023-12-22T14:53:54Z

@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 · 2023-12-22T15:38:35Z

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

brunoerg · 2023-12-22T16:10:44Z

@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:

class DeviceException(Exception):  # pylint: disable=too-few-public-methods
    exc: Dict[int, Any] = {
        0x6985: DenyError,
        0x6982: SecurityStatusNotSatisfiedError,
        0x6A80: IncorrectDataError,
        0x6A82: NotSupportedError,
        0x6A86: WrongP1P2Error,
        0x6A87: WrongDataLengthError,
        0x6D00: InsNotSupportedError,
        0x6E00: ClaNotSupportedError,
        0xB000: WrongResponseLengthError,
        0xB007: BadStateError,
        0xB008: SignatureFailError,
        0xE000: InterruptedExecution,  # not an error
    }

0d5412a Improve error handle for `Ledger` devices (Bruno Garcia) Pull request description: Discovered it while reviewing [#bitcoin/24313](bitcoin/bitcoin#24313). The `ledger_exception` function was not dealing with `ApduException` exceptions. It's basically similar to `BTChipException` and we can use its "error code" to return friendly error messages. e.g for `displayaddress`: **before**: `{"error": "Exception: invalid status 0x6985", "code": -13}` **after**: `{"error": "display_singlesig_address canceled", "code": -14}` ACKs for top commit: achow101: ACK 0d5412a Tree-SHA512: f4dbe57e5f41c67a19ce6e9c0f4b4af8184ff360cfc3709f4fb15f55014906c0a6c04a869a9315083aac18175dd8b921c022d18826d3e1f31d4bdb4a736b6171

Sjors · 2024-02-13T12:35:13Z

Rebased to give CI another try.

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

Sjors · 2024-02-16T10:05:41Z

Tested with HWI 2.4.0

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

'...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:

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

Consistent with bitcoin#26076

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

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.

Sjors · 2024-04-16T16:01:01Z

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

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

brunoerg · 2024-04-17T18:05:30Z

ACK 4357158

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

./src/bitcoin-cli -rpcwallet=ledger7 walletdisplayaddress "bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m"
error code: -1
error message:
'/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

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

achow101 · 2024-04-23T21:13:01Z

ACK 4357158

DrahtBot added GUI interfaces RPC/REST/ZMQ Wallet labels Feb 10, 2022

jonatack reviewed Feb 11, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 12, 2022

Add external signer taproot support #23578

Merged

luke-jr reviewed Feb 13, 2022

View reviewed changes

luke-jr reviewed Feb 20, 2022

View reviewed changes

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved

Sjors mentioned this pull request Feb 22, 2022

ledger: Support Bitcoin App 2.0 bitcoin-core/HWI#550

Merged

DrahtBot mentioned this pull request Mar 29, 2022

wallet: Improve AvailableCoins performance by reducing duplicated operations #24699

Merged

Sjors mentioned this pull request Apr 15, 2022

Awesome multisig PR labyrinth guide #24861

Open

16 tasks

Sjors force-pushed the 2022/02/displayaddress branch from 803387f to c16d7db Compare June 23, 2022 14:08

DrahtBot mentioned this pull request Jul 27, 2022

refactor: make member functions const when applicable #25673

Closed

Sjors force-pushed the 2022/02/displayaddress branch from c16d7db to d92cae0 Compare September 6, 2022 13:50

DrahtBot mentioned this pull request Sep 14, 2022

RPC/Wallet: Access wallets via interfaces::Wallet #26082

Closed

DrahtBot added the Needs rebase label Oct 26, 2022

Sjors force-pushed the 2022/02/displayaddress branch from d92cae0 to af0a585 Compare October 26, 2022 14:14

DrahtBot removed the Needs rebase label Oct 26, 2022

DrahtBot mentioned this pull request Nov 9, 2022

bumpfee: Allow the user to choose which output is change #26467

Merged

DrahtBot mentioned this pull request Jan 24, 2023

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

Open

DrahtBot mentioned this pull request Apr 4, 2023

move-only: Extract common/args from util/system #27419

Merged

DrahtBot added the Needs rebase label Apr 21, 2023

achow101 self-requested a review April 25, 2023 15:21

Sjors force-pushed the 2022/02/displayaddress branch from af0a585 to dfc2767 Compare May 8, 2023 13:28

DrahtBot removed the Needs rebase label May 8, 2023

DrahtBot mentioned this pull request May 12, 2023

kernel: Remove util/system from kernel library, interface_ui from validation. #27636

Merged

DrahtBot removed the CI failed label Dec 15, 2023

DrahtBot requested a review from jonatack December 18, 2023 14:41

brunoerg reviewed Dec 18, 2023

View reviewed changes

brunoerg mentioned this pull request Dec 26, 2023

Improve error handle for Ledger devices bitcoin-core/HWI#718

Merged

DrahtBot added the CI failed label Jan 14, 2024

Sjors force-pushed the 2022/02/displayaddress branch from 5536ace to d38ddc1 Compare February 13, 2024 12:35

DrahtBot removed the CI failed label Feb 13, 2024

Sjors added 3 commits April 16, 2024 17:47

test: use h marker for external signer mock

6c1a2cc

Consistent with bitcoin#26076

wallet: compare address returned by displayaddress

dc55531

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

wallet: return and display signer error

4357158

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.

Sjors force-pushed the 2022/02/displayaddress branch from d38ddc1 to 4357158 Compare April 16, 2024 16:00

Sjors requested a review from brunoerg April 16, 2024 16:01

DrahtBot requested a review from achow101 April 17, 2024 18:06

achow101 merged commit a7129f8 into bitcoin:master Apr 23, 2024

Sjors deleted the 2022/02/displayaddress branch May 6, 2024 12:47

bitcoin locked and limited conversation to collaborators May 6, 2025

		const UniValue& ret_address = find_value(result, "address");
		if (!ret_address.isStr()) return _("Signer did not echo address");

Improve display address handling for external signer #24313

Improve display address handling for external signer #24313

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack Feb 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 23, 2022

Uh oh!

Sjors commented Oct 26, 2022

Uh oh!

Sjors commented May 8, 2023

Uh oh!

brunoerg commented Dec 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 22, 2023

Uh oh!

brunoerg commented Dec 22, 2023

Uh oh!

Sjors commented Dec 22, 2023

Uh oh!

brunoerg commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 13, 2024

Uh oh!

Sjors commented Feb 16, 2024

Uh oh!

Sjors commented Apr 16, 2024

Uh oh!

brunoerg commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Apr 23, 2024

Uh oh!

Uh oh!

Sjors commented Feb 10, 2022 •

edited

Loading

DrahtBot commented Feb 11, 2022 •

edited

Loading

jonatack Feb 11, 2022 •

edited

Loading

brunoerg commented Dec 22, 2023 •

edited

Loading

Sjors commented Dec 22, 2023 •

edited

Loading

brunoerg commented Dec 22, 2023 •

edited

Loading

brunoerg commented Apr 17, 2024 •

edited

Loading