From 6c1a2cc09a00baa6ff3ff34455c2243b43067fb5 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 19 May 2023 14:57:22 +0200 Subject: [PATCH 1/3] test: use h marker for external signer mock Consistent with #26076 --- test/functional/mocks/signer.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index 5f4fad63808..64572ddda5b 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -25,16 +25,16 @@ def getdescriptors(args): sys.stdout.write(json.dumps({ "receive": [ - "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", - "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#r0grqw5x", - "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#x30uthjs", - "tr([00000001/86'/1'/" + args.account + "']" + xpub + "/0/*)#sng9rd4t" + "pkh([00000001/44h/1h/" + args.account + "']" + xpub + "/0/*)#aqllu46s", + "sh(wpkh([00000001/49h/1h/" + args.account + "']" + xpub + "/0/*))#5dh56mgg", + "wpkh([00000001/84h/1h/" + args.account + "']" + xpub + "/0/*)#h62dxaej", + "tr([00000001/86h/1h/" + args.account + "']" + xpub + "/0/*)#pcd5w87f" ], "internal": [ - "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", - "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#kwx4c3pe", - "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#h92akzzg", - "tr([00000001/86'/1'/" + args.account + "']" + xpub + "/1/*)#p8dy7c9n" + "pkh([00000001/44h/1h/" + args.account + "']" + xpub + "/1/*)#v567pq2g", + "sh(wpkh([00000001/49h/1h/" + args.account + "']" + xpub + "/1/*))#pvezzyah", + "wpkh([00000001/84h/1h/" + args.account + "']" + xpub + "/1/*)#xw0vmgf2", + "tr([00000001/86h/1h/" + args.account + "']" + xpub + "/1/*)#svg4njw3" ] })) @@ -47,8 +47,8 @@ def displayaddress(args): return sys.stdout.write(json.dumps({"error": "Unexpected fingerprint", "fingerprint": args.fingerprint})) expected_desc = [ - "wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r", - "tr([00000001/86'/1'/0'/0/0]c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#4vdj9jqk", + "wpkh([00000001/84h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#3te6hhy7", + "tr([00000001/86h/1h/0h/0/0]c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#puqqa90m", ] if args.desc not in expected_desc: return sys.stdout.write(json.dumps({"error": "Unexpected descriptor", "desc": args.desc})) From dc55531087478d01fbde4f5fbb75375b672960c3 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 23 Nov 2021 16:12:55 +0100 Subject: [PATCH 2/3] wallet: compare address returned by displayaddress Update external signer documentation to reflect this requirement, which HWI already implements. --- doc/external-signer.md | 3 +++ src/wallet/external_signer_scriptpubkeyman.cpp | 13 +++++++++---- src/wallet/external_signer_scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 5 ++++- test/functional/mocks/signer.py | 14 +++++++------- test/functional/wallet_signer.py | 5 +++-- 7 files changed, 28 insertions(+), 16 deletions(-) diff --git a/doc/external-signer.md b/doc/external-signer.md index de44cdd8801..1468e852ef6 100644 --- a/doc/external-signer.md +++ b/doc/external-signer.md @@ -150,6 +150,9 @@ Example, display the first native SegWit receive address on Testnet: The command MUST be able to figure out the address type from the descriptor. +The command MUST return an object containing `{"address": "[the address]"}`. +As a sanity check, for devices that support this, it SHOULD ask the device to derive the address. + If contains a master key fingerprint, the command MUST fail if it does not match the fingerprint known by the device. If contains an xpub, the command MUST fail if it does not match the xpub known by the device. diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index a71f8f9fbc0..ce668539e61 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -51,15 +52,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 + const CScript& scriptPubKey = GetScriptForDestination(dest); auto provider = GetSolvingProvider(scriptPubKey); auto descriptor = InferDescriptor(scriptPubKey, *provider); - signer.DisplayAddress(descriptor->ToString()); - // TODO inspect result - return true; + const UniValue& result = signer.DisplayAddress(descriptor->ToString()); + + const UniValue& ret_address = result.find_value("address"); + if (!ret_address.isStr()) return false; + + return ret_address.getValStr() == EncodeDestination(dest); } // If sign is true, transaction must previously have been filled diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index c052ce61298..b2498332711 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -27,7 +27,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan static ExternalSigner GetExternalSigner(); - bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const; + bool DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const; TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 96c43975049..675d8f3985d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2676,7 +2676,7 @@ bool CWallet::DisplayAddress(const CTxDestination& dest) continue; } ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(scriptPubKey, signer); + return signer_spk_man->DisplayAddress(dest, signer); } return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b49b5a7d0d8..74c0c5ed087 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -537,7 +537,10 @@ public: bool IsSpentKey(const CScript& scriptPubKey) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** Display address on an external signer. Returns false if external signer support is not compiled */ + /** Display address on an external signer. + * Returns false if the signer does not respond with a matching address. + * Returns false if external signer support is not compiled. + */ bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index 64572ddda5b..dc3701fae2e 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -41,19 +41,19 @@ def getdescriptors(args): def displayaddress(args): - # Several descriptor formats are acceptable, so allowing for potential - # changes to InferDescriptor: if args.fingerprint != "00000001": return sys.stdout.write(json.dumps({"error": "Unexpected fingerprint", "fingerprint": args.fingerprint})) - expected_desc = [ - "wpkh([00000001/84h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#3te6hhy7", - "tr([00000001/86h/1h/0h/0/0]c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#puqqa90m", - ] + expected_desc = { + "wpkh([00000001/84h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#3te6hhy7": "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g", + "sh(wpkh([00000001/49h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7))#kz9y5w82": "2N2gQKzjUe47gM8p1JZxaAkTcoHPXV6YyVp", + "pkh([00000001/44h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#q3pqd8wh": "n1LKejAadN6hg2FrBXoU1KrwX4uK16mco9", + "tr([00000001/86h/1h/0h/0/0]c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#puqqa90m": "tb1phw4cgpt6cd30kz9k4wkpwm872cdvhss29jga2xpmftelhqll62mscq0k4g", + } if args.desc not in expected_desc: return sys.stdout.write(json.dumps({"error": "Unexpected descriptor", "desc": args.desc})) - return sys.stdout.write(json.dumps({"address": "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g"})) + return sys.stdout.write(json.dumps({"address": expected_desc[args.desc]})) def signtx(args): if args.fingerprint != "00000001": diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 32a18871537..425f535b512 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -130,8 +130,9 @@ class WalletSignerTest(BitcoinTestFramework): assert_equal(address_info['hdkeypath'], "m/86h/1h/0h/0/0") self.log.info('Test walletdisplayaddress') - result = hww.walletdisplayaddress(address1) - assert_equal(result, {"address": address1}) + for address in [address1, address2, address3]: + result = hww.walletdisplayaddress(address) + assert_equal(result, {"address": address}) # Handle error thrown by script self.set_mock_result(self.nodes[1], "2") From 4357158c4712d479522d5cd441ad4dd1693fdd05 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 13 Feb 2024 13:25:59 +0100 Subject: [PATCH 3/3] 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 to clarify that this either succeeds or returns an error message. --- src/interfaces/wallet.h | 2 +- src/qt/walletmodel.cpp | 9 +++++---- src/qt/walletmodel.h | 2 +- src/wallet/external_signer_scriptpubkeyman.cpp | 16 ++++++++++++---- src/wallet/external_signer_scriptpubkeyman.h | 8 +++++++- src/wallet/interfaces.cpp | 2 +- src/wallet/rpc/addresses.cpp | 5 ++--- src/wallet/scriptpubkeyman.h | 1 - src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 7 ++----- test/functional/mocks/signer.py | 1 + test/functional/wallet_signer.py | 7 +++++++ 12 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 61142366234..c41f35829d7 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -127,7 +127,7 @@ public: virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; //! Display address on external signer - virtual bool displayAddress(const CTxDestination& dest) = 0; + virtual util::Result displayAddress(const CTxDestination& dest) = 0; //! Lock coin. virtual bool lockCoin(const COutPoint& output, const bool write_to_db) = 0; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 1bdf94d3b57..fe000bcbb8e 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -569,16 +569,17 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash) return true; } -bool WalletModel::displayAddress(std::string sAddress) const +void WalletModel::displayAddress(std::string sAddress) const { CTxDestination dest = DecodeDestination(sAddress); - bool res = false; try { - res = m_wallet->displayAddress(dest); + util::Result result = m_wallet->displayAddress(dest); + if (!result) { + QMessageBox::warning(nullptr, tr("Signer error"), QString::fromStdString(util::ErrorString(result).translated)); + } } catch (const std::runtime_error& e) { QMessageBox::critical(nullptr, tr("Can't display address"), e.what()); } - return res; } bool WalletModel::isWalletEnabled() diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 503ee16823c..ab2096c1fef 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -130,7 +130,7 @@ public: UnlockContext requestUnlock(); bool bumpFee(uint256 hash, uint256& new_hash); - bool displayAddress(std::string sAddress) const; + void displayAddress(std::string sAddress) const; static bool isWalletEnabled(); diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index ce668539e61..b5703fa54ae 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -52,7 +53,7 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { return signers[0]; } -bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const +util::Result ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const { // TODO: avoid the need to infer a descriptor from inside a descriptor wallet const CScript& scriptPubKey = GetScriptForDestination(dest); @@ -61,10 +62,17 @@ bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CTxDestination& dest, c const UniValue& result = signer.DisplayAddress(descriptor->ToString()); - const UniValue& ret_address = result.find_value("address"); - if (!ret_address.isStr()) return false; + const UniValue& error = result.find_value("error"); + if (error.isStr()) return util::Error{strprintf(_("Signer returned error: %s"), error.getValStr())}; - return ret_address.getValStr() == EncodeDestination(dest); + const UniValue& ret_address = result.find_value("address"); + if (!ret_address.isStr()) return util::Error{_("Signer did not echo address")}; + + if (ret_address.getValStr() != EncodeDestination(dest)) { + return util::Error{strprintf(_("Signer echoed unexpected address %s"), ret_address.getValStr())}; + } + + return util::Result(); } // If sign is true, transaction must previously have been filled diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index b2498332711..44286456b61 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -9,6 +9,8 @@ #include +struct bilingual_str; + namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { @@ -27,7 +29,11 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan static ExternalSigner GetExternalSigner(); - bool DisplayAddress(const CTxDestination& dest, const ExternalSigner &signer) const; + /** + * Display address on the device and verify that the returned value matches. + * @returns nothing or an error message + */ + util::Result DisplayAddress(const CTxDestination& dest, const ExternalSigner& signer) const; TransactionError FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; }; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d33e6f3873b..e17e6c332d9 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -247,7 +247,7 @@ public: return value.empty() ? m_wallet->EraseAddressReceiveRequest(batch, dest, id) : m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } - bool displayAddress(const CTxDestination& dest) override + util::Result displayAddress(const CTxDestination& dest) override { LOCK(m_wallet->cs_wallet); return m_wallet->DisplayAddress(dest); diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 7f068c05ef2..bed9ec029aa 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -789,9 +789,8 @@ RPCHelpMan walletdisplayaddress() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); } - if (!pwallet->DisplayAddress(dest)) { - throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address"); - } + util::Result res = pwallet->DisplayAddress(dest); + if (!res) throw JSONRPCError(RPC_MISC_ERROR, util::ErrorString(res).original); UniValue result(UniValue::VOBJ); result.pushKV("address", request.params[0].get_str()); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 4575881d96a..2c1ab8d44ae 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -27,7 +27,6 @@ #include enum class OutputType; -struct bilingual_str; namespace wallet { struct MigrationData; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 675d8f3985d..8f4171eb15d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2667,7 +2667,7 @@ void ReserveDestination::ReturnDestination() address = CNoDestination(); } -bool CWallet::DisplayAddress(const CTxDestination& dest) +util::Result CWallet::DisplayAddress(const CTxDestination& dest) { CScript scriptPubKey = GetScriptForDestination(dest); for (const auto& spk_man : GetScriptPubKeyMans(scriptPubKey)) { @@ -2678,7 +2678,7 @@ bool CWallet::DisplayAddress(const CTxDestination& dest) ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); return signer_spk_man->DisplayAddress(dest, signer); } - return false; + return util::Error{_("There is no ScriptPubKeyManager for this address")}; } bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 74c0c5ed087..6a998fa3983 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -537,11 +537,8 @@ public: bool IsSpentKey(const CScript& scriptPubKey) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** Display address on an external signer. - * Returns false if the signer does not respond with a matching address. - * Returns false if external signer support is not compiled. - */ - bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Display address on an external signer. */ + util::Result DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index dc3701fae2e..23d163aac33 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -49,6 +49,7 @@ def displayaddress(args): "sh(wpkh([00000001/49h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7))#kz9y5w82": "2N2gQKzjUe47gM8p1JZxaAkTcoHPXV6YyVp", "pkh([00000001/44h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#q3pqd8wh": "n1LKejAadN6hg2FrBXoU1KrwX4uK16mco9", "tr([00000001/86h/1h/0h/0/0]c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#puqqa90m": "tb1phw4cgpt6cd30kz9k4wkpwm872cdvhss29jga2xpmftelhqll62mscq0k4g", + "wpkh([00000001/84h/1h/0h/0/1]03a20a46308be0b8ded6dff0a22b10b4245c587ccf23f3b4a303885be3a524f172)#aqpjv5xr": "wrong_address", } if args.desc not in expected_desc: return sys.stdout.write(json.dumps({"error": "Unexpected descriptor", "desc": args.desc})) diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 425f535b512..abfc3c1ba1b 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -141,6 +141,13 @@ class WalletSignerTest(BitcoinTestFramework): ) self.clear_mock_result(self.nodes[1]) + # Returned address MUST match: + address_fail = hww.getnewaddress(address_type="bech32") + assert_equal(address_fail, "bcrt1ql7zg7ukh3dwr25ex2zn9jse926f27xy2jz58tm") + assert_raises_rpc_error(-1, 'Signer echoed unexpected address wrong_address', + hww.walletdisplayaddress, address_fail + ) + self.log.info('Prepare mock PSBT') self.nodes[0].sendtoaddress(address4, 1) self.generate(self.nodes[0], 1)