From 4357158c4712d479522d5cd441ad4dd1693fdd05 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 13 Feb 2024 13:25:59 +0100 Subject: 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 | 14 +++++++++++--- 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, 40 insertions(+), 22 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 6114236623..c41f35829d 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 1bdf94d3b5..fe000bcbb8 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 503ee16823..ab2096c1fe 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 ce668539e6..b5703fa54a 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& error = result.find_value("error"); + if (error.isStr()) return util::Error{strprintf(_("Signer returned error: %s"), error.getValStr())}; + const UniValue& ret_address = result.find_value("address"); - if (!ret_address.isStr()) return false; + 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 ret_address.getValStr() == EncodeDestination(dest); + 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 b249833271..44286456b6 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 d33e6f3873..e17e6c332d 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 7f068c05ef..bed9ec029a 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 4575881d96..2c1ab8d44a 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 675d8f3985..8f4171eb15 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 74c0c5ed08..6a998fa398 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 dc3701fae2..23d163aac3 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 425f535b51..abfc3c1ba1 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) -- cgit v1.2.3