wallet: compare address returned by displayaddress

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

Author: Sjors

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 <descriptor> contains a master key fingerprint, the command MUST fail if it does not match the fingerprint known by the device.
 
 If <descriptor> contains an xpub, the command MUST fail if it does not match the xpub known by the device.

src/wallet/external_signer_scriptpubkeyman.cpp

@@ -9,6 +9,7 @@
 #include <wallet/external_signer_scriptpubkeyman.h>
 
 #include <iostream>
+#include <key_io.h>
 #include <memory>
 #include <stdexcept>
 #include <string>
@@ -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

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;
 };

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;
 }

src/wallet/wallet.h

@@ -537,7 +537,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     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<CTxDestination>& 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);

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

test/functional/wallet_signer.py

@@ -130,8 +130,9 @@ def test_valid_signer(self):
         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")