Merge bitcoin/bitcoin#24313: Improve display address handling for external signer

4357158c47 wallet: return and display signer error (Sjors Provoost)
dc55531087 wallet: compare address returned by displayaddress (Sjors Provoost)
6c1a2cc09a test: use h marker for external signer mock (Sjors Provoost)

Pull request description:

  * 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)

ACKs for top commit:
  brunoerg:
    ACK 4357158c47
  achow101:
    ACK 4357158c47

Tree-SHA512: 4f56edf3846745c8e7d08ef55cf29e8bb468256457149377c5f02da097931f9ca0c06bdbd856dc2385cde4fd11e4dc3b634c5a48814ff27f5562c8a25d43da93

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.

doc/managing-wallets.md

@@ -122,6 +122,22 @@ $ bitcoin-cli -rpcwallet="restored-wallet" getwalletinfo
 
 The restored wallet can also be loaded in the GUI via `File` ->`Open wallet`.
 
+## Wallet Passphrase
+
+Understanding wallet security is crucial for safely storing your Bitcoin. A key aspect is the wallet passphrase, used for encryption. Let's explore its nuances, role, encryption process, and limitations.
+
+- **Not the Seed:**
+The wallet passphrase and the seed are two separate components in wallet security. The seed, or HD seed, functions as a master key for deriving private and public keys in a hierarchical deterministic (HD) wallet. In contrast, the passphrase serves as an additional layer of security specifically designed to secure the private keys within the wallet. The passphrase serves as a safeguard, demanding an additional layer of authentication to access funds in the wallet.
+
+- **Protection Against Unauthorized Access:**
+The passphrase serves as a protective measure, securing your funds in situations where an unauthorized user gains access to your unlocked computer or device while your wallet application is active. Without the passphrase, they would be unable to access your wallet's funds or execute transactions. However, it's essential to be aware that someone with access can potentially compromise the security of your passphrase by installing a keylogger.
+
+- **Doesn't Encrypt Metadata or Public Keys:**
+It's important to note that the passphrase primarily secures the private keys and access to funds within the wallet. It does not encrypt metadata associated with transactions or public keys. Information about your transaction history and the public keys involved may still be visible.
+
+- **Risk of Fund Loss if Forgotten or Lost:**
+If the wallet passphrase is too complex and is subsequently forgotten or lost, there is a risk of losing access to the funds permanently. A forgotten passphrase will result in the inability to unlock the wallet and access the funds.
+
 ## Migrating Legacy Wallets to Descriptor Wallets
 
 Legacy wallets (traditional non-descriptor wallets) can be migrated to become Descriptor wallets

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<void> displayAddress(const CTxDestination& dest) = 0;
 
     //! Lock coin.
     virtual bool lockCoin(const COutPoint& output, const bool write_to_db) = 0;

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<void> 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()

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();
 

src/util/subprocess.hpp

@@ -154,19 +154,6 @@ public:
   {}
 };
 
-//--------------------------------------------------------------------
-
-//Environment Variable types
-#ifndef _MSC_VER
-	using env_string_t = std::string;
-	using env_char_t = char;
-#else
-	using env_string_t = std::wstring;
-	using env_char_t = wchar_t;
-#endif
-using env_map_t = std::map<env_string_t, env_string_t>;
-using env_vector_t = std::vector<env_char_t>;
-
 //--------------------------------------------------------------------
 namespace util
 {
@@ -305,100 +292,6 @@ namespace util
     if (!SetHandleInformation(*child_handle, HANDLE_FLAG_INHERIT, 0))
       throw OSError("SetHandleInformation", 0);
   }
-
-  // env_map_t MapFromWindowsEnvironment()
-  // * Imports current Environment in a C-string table
-  // * Parses the strings by splitting on the first "=" per line
-  // * Creates a map of the variables
-  // * Returns the map
-  inline env_map_t MapFromWindowsEnvironment(){
-      wchar_t *variable_strings_ptr;
-      wchar_t *environment_strings_ptr;
-      std::wstring delimiter(L"=");
-      int del_len = delimiter.length();
-      env_map_t mapped_environment;
-
-      // Get a pointer to the environment block.
-      environment_strings_ptr = GetEnvironmentStringsW();
-      // If the returned pointer is NULL, exit.
-      if (environment_strings_ptr == NULL)
-      {
-          throw OSError("GetEnvironmentStringsW", 0);
-      }
-
-      // Variable strings are separated by NULL byte, and the block is
-      // terminated by a NULL byte.
-
-      variable_strings_ptr = (wchar_t *) environment_strings_ptr;
-
-      //Since the environment map ends with a null, we can loop until we find it.
-      while (*variable_strings_ptr)
-      {
-          // Create a string from Variable String
-          env_string_t current_line(variable_strings_ptr);
-          // Find the first "equals" sign.
-          auto pos = current_line.find(delimiter);
-          // Assuming it's not missing ...
-          if(pos!=std::wstring::npos){
-              // ... parse the key and value.
-              env_string_t key = current_line.substr(0, pos);
-              env_string_t value = current_line.substr(pos + del_len);
-              // Map the entry.
-              mapped_environment[key] = value;
-          }
-          // Jump to next line in the environment map.
-          variable_strings_ptr += std::wcslen(variable_strings_ptr) + 1;
-      }
-      // We're done with the old environment map buffer.
-      FreeEnvironmentStringsW(environment_strings_ptr);
-
-      // Return the map.
-      return mapped_environment;
-  }
-
-  // env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map)
-  // * Creates a vector buffer for the new environment string table
-  // * Copies in the mapped variables
-  // * Returns the vector
-  inline env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map)
-  {
-	// Make a new environment map buffer.
-	env_vector_t environment_map_buffer;
-	// Give it some space.
-	environment_map_buffer.reserve(4096);
-
-	// And fill'er up.
-	for(auto kv: source_map){
-	  // Create the line
-	  env_string_t current_line(kv.first); current_line += L"="; current_line += kv.second;
-	  // Add the line to the buffer.
-	  std::copy(current_line.begin(), current_line.end(), std::back_inserter(environment_map_buffer));
-	  // Append a null
-	  environment_map_buffer.push_back(0);
-	}
-	// Append one last null because of how Windows does it's environment maps.
-	environment_map_buffer.push_back(0);
-
-	return environment_map_buffer;
-  }
-
-  // env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map)
-  // * Merges host environment with new mapped variables
-  // * Creates and returns string vector based on map
-  inline env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map){
-	// Import the environment map
-	env_map_t environment_map = MapFromWindowsEnvironment();
-    // Merge in the changes with overwrite
-	for(auto& it: changes_map)
-	{
-		environment_map[it.first] = it.second;
-	}
-    // Create a Windows-usable Environment Map Buffer
-    env_vector_t environment_map_strings_vector = WindowsEnvironmentVectorFromMap(environment_map);
-
-	return environment_map_strings_vector;
-  }
-
 #endif
 
   /*!
@@ -431,26 +324,6 @@ namespace util
   }
 
 
-  /*!
-   * Function: join
-   * Parameters:
-   * [in] vec : Vector of strings which needs to be joined to form
-   *            a single string with words separated by a separator char.
-   *  [in] sep : String used to separate 2 words in the joined string.
-   *             Default constructed to ' ' (space).
-   *  [out] string: Joined string.
-   */
-  static inline
-  std::string join(const std::vector<std::string>& vec,
-                   const std::string& sep = " ")
-  {
-    std::string res;
-    for (auto& elem : vec) res.append(elem + sep);
-    res.erase(--res.end());
-    return res;
-  }
-
-
 #ifndef __USING_WINDOWS__
   /*!
    * Function: set_clo_on_exec
@@ -651,56 +524,6 @@ namespace util
  * -------------------------------
  */
 
-/*!
- * The buffer size of the stdin/stdout/stderr
- * streams of the child process.
- * Default value is 0.
- */
-struct bufsize {
-  explicit bufsize(int sz): bufsiz(sz) {}
-  int  bufsiz = 0;
-};
-
-/*!
- * Option to defer spawning of the child process
- * till `Popen::start_process` API is called.
- * Default value is false.
- */
-struct defer_spawn {
-  explicit defer_spawn(bool d): defer(d) {}
-  bool defer  = false;
-};
-
-/*!
- * Option to close all file descriptors
- * when the child process is spawned.
- * The close fd list does not include
- * input/output/error if they are explicitly
- * set as part of the Popen arguments.
- *
- * Default value is false.
- */
-struct close_fds {
-  explicit close_fds(bool c): close_all(c) {}
-  bool close_all = false;
-};
-
-/*!
- * Option to make the child process as the
- * session leader and thus the process
- * group leader.
- * Default value is false.
- */
-struct session_leader {
-  explicit session_leader(bool sl): leader_(sl) {}
-  bool leader_ = false;
-};
-
-struct shell {
-  explicit shell(bool s): shell_(s) {}
-  bool shell_ = false;
-};
-
 /*!
  * Base class for all arguments involving string value.
  */
@@ -726,34 +549,6 @@ struct executable: string_arg
   executable(T&& arg): string_arg(std::forward<T>(arg)) {}
 };
 
-/*!
- * Option to set the current working directory
- * of the spawned process.
- *
- * Eg: cwd{"/some/path/x"}
- */
-struct cwd: string_arg
-{
-  template <typename T>
-  cwd(T&& arg): string_arg(std::forward<T>(arg)) {}
-};
-
-/*!
- * Option to specify environment variables required by
- * the spawned process.
- *
- * Eg: environment{{ {"K1", "V1"}, {"K2", "V2"},... }}
- */
-struct environment
-{
-  environment(env_map_t&& env):
-    env_(std::move(env)) {}
-  explicit environment(const env_map_t& env):
-    env_(env) {}
-  env_map_t env_;
-};
-
-
 /*!
  * Used for redirecting input/output/error
  */
@@ -870,44 +665,6 @@ struct error
   int wr_ch_ = -1;
 };
 
-// Impoverished, meager, needy, truly needy
-// version of type erasure to store function pointers
-// needed to provide the functionality of preexec_func
-// ATTN: Can be used only to execute functions with no
-// arguments and returning void.
-// Could have used more efficient methods, ofcourse, but
-// that won't yield me the consistent syntax which I am
-// aiming for. If you know, then please do let me know.
-
-class preexec_func
-{
-public:
-  preexec_func() {}
-
-  template <typename Func>
-  explicit preexec_func(Func f): holder_(new FuncHolder<Func>(std::move(f)))
-  {}
-
-  void operator()() {
-    (*holder_)();
-  }
-
-private:
-  struct HolderBase {
-    virtual void operator()() const = 0;
-    virtual ~HolderBase(){};
-  };
-  template <typename T>
-  struct FuncHolder: HolderBase {
-    FuncHolder(T func): func_(std::move(func)) {}
-    void operator()() const override { func_(); }
-    // The function pointer/reference
-    T func_;
-  };
-
-  std::unique_ptr<HolderBase> holder_ = nullptr;
-};
-
 // ~~~~ End Popen Args ~~~~
 
 
@@ -1007,17 +764,9 @@ struct ArgumentDeducer
   ArgumentDeducer(Popen* p): popen_(p) {}
 
   void set_option(executable&& exe);
-  void set_option(cwd&& cwdir);
-  void set_option(bufsize&& bsiz);
-  void set_option(environment&& env);
-  void set_option(defer_spawn&& defer);
-  void set_option(shell&& sh);
   void set_option(input&& inp);
   void set_option(output&& out);
   void set_option(error&& err);
-  void set_option(close_fds&& cfds);
-  void set_option(preexec_func&& prefunc);
-  void set_option(session_leader&& sleader);
 
 private:
   Popen* popen_ = nullptr;
@@ -1168,9 +917,6 @@ public:// Yes they are public
   HANDLE g_hChildStd_ERR_Wr = nullptr;
 #endif
 
-  // Buffer size for the IO streams
-  int bufsiz_ = 0;
-
   // Pipes for communicating with child
 
   // Emulates stdin
@@ -1200,9 +946,9 @@ private:
  * interface to the client.
  *
  * API's provided by the class:
- * 1. Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....)
+ * 1. Popen({"cmd"}, output{..}, error{..}, ....)
  *    Command provided as a sequence.
- * 2. Popen("cmd arg1"m output{..}, error{..}, cwd{..}, ....)
+ * 2. Popen("cmd arg1"m output{..}, error{..}, ....)
  *    Command provided in a single string.
  * 3. wait()             - Wait for the child to exit.
  * 4. retcode()          - The return code of the exited child.
@@ -1218,8 +964,6 @@ private:
                            in case of redirection. See piping examples.
  *12. error()            - Get the error channel/File pointer. Usually used
                            in case of redirection.
- *13. start_process()    - Start the child process. Only to be used when
- *                         `defer_spawn` option was provided in Popen constructor.
  */
 class Popen
 {
@@ -1237,7 +981,7 @@ public:
     // Setup the communication channels of the Popen class
     stream_.setup_comm_channels();
 
-    if (!defer_process_start_) execute_process();
+    execute_process();
   }
 
   template <typename... Args>
@@ -1249,7 +993,7 @@ public:
     // Setup the communication channels of the Popen class
     stream_.setup_comm_channels();
 
-    if (!defer_process_start_) execute_process();
+    execute_process();
   }
 
   template <typename... Args>
@@ -1260,7 +1004,7 @@ public:
     // Setup the communication channels of the Popen class
     stream_.setup_comm_channels();
 
-    if (!defer_process_start_) execute_process();
+    execute_process();
   }
 
 /*
@@ -1272,8 +1016,6 @@ public:
   }
 */
 
-  void start_process() noexcept(false);
-
   int pid() const noexcept { return child_pid_; }
 
   int retcode() const noexcept { return retcode_; }
@@ -1347,16 +1089,7 @@ private:
   std::future<void> cleanup_future_;
 #endif
 
-  bool defer_process_start_ = false;
-  bool close_fds_ = false;
-  bool has_preexec_fn_ = false;
-  bool shell_ = false;
-  bool session_leader_ = false;
-
   std::string exe_name_;
-  std::string cwd_;
-  env_map_t env_;
-  preexec_func preexec_fn_;
 
   // Command in string format
   std::string args_;
@@ -1391,20 +1124,6 @@ inline void Popen::populate_c_argv()
   cargv_.push_back(nullptr);
 }
 
-inline void Popen::start_process() noexcept(false)
-{
-  // The process was started/tried to be started
-  // in the constructor itself.
-  // For explicitly calling this API to start the
-  // process, 'defer_spawn' argument must be set to
-  // true in the constructor.
-  if (!defer_process_start_) {
-    assert (0);
-    return;
-  }
-  execute_process();
-}
-
 inline int Popen::wait() noexcept(false)
 {
 #ifdef __USING_WINDOWS__
@@ -1483,8 +1202,7 @@ inline void Popen::kill(int sig_num)
     throw OSError("TerminateProcess", 0);
   }
 #else
-  if (session_leader_) killpg(child_pid_, sig_num);
-  else ::kill(child_pid_, sig_num);
+  ::kill(child_pid_, sig_num);
 #endif
 }
 
@@ -1492,17 +1210,6 @@ inline void Popen::kill(int sig_num)
 inline void Popen::execute_process() noexcept(false)
 {
 #ifdef __USING_WINDOWS__
-  if (this->shell_) {
-    throw OSError("shell not currently supported on windows", 0);
-  }
-
-  void* environment_string_table_ptr = nullptr;
-  env_vector_t environment_string_vector;
-  if(this->env_.size()){
-	  environment_string_vector = util::CreateUpdatedWindowsEnvironmentVector(env_);
-	  environment_string_table_ptr = (void*)environment_string_vector.data();
-  }
-
   if (exe_name_.length()) {
     this->vargs_.insert(this->vargs_.begin(), this->exe_name_);
     this->populate_c_argv();
@@ -1549,7 +1256,7 @@ inline void Popen::execute_process() noexcept(false)
                             NULL,         // primary thread security attributes
                             TRUE,         // handles are inherited
                             creation_flags,	// creation flags
-                            environment_string_table_ptr,  // use provided environment
+                            NULL,         // use parent's environment
                             NULL,         // use parent's current directory
                             &siStartInfo, // STARTUPINFOW pointer
                             &piProcInfo); // receives PROCESS_INFORMATION
@@ -1588,14 +1295,6 @@ inline void Popen::execute_process() noexcept(false)
   int err_rd_pipe, err_wr_pipe;
   std::tie(err_rd_pipe, err_wr_pipe) = util::pipe_cloexec();
 
-  if (shell_) {
-    auto new_cmd = util::join(vargs_);
-    vargs_.clear();
-    vargs_.insert(vargs_.begin(), {"/bin/sh", "-c"});
-    vargs_.push_back(new_cmd);
-    populate_c_argv();
-  }
-
   if (exe_name_.length()) {
     vargs_.insert(vargs_.begin(), exe_name_);
     populate_c_argv();
@@ -1662,30 +1361,6 @@ namespace detail {
     popen_->exe_name_ = std::move(exe.arg_value);
   }
 
-  inline void ArgumentDeducer::set_option(cwd&& cwdir) {
-    popen_->cwd_ = std::move(cwdir.arg_value);
-  }
-
-  inline void ArgumentDeducer::set_option(bufsize&& bsiz) {
-    popen_->stream_.bufsiz_ = bsiz.bufsiz;
-  }
-
-  inline void ArgumentDeducer::set_option(environment&& env) {
-    popen_->env_ = std::move(env.env_);
-  }
-
-  inline void ArgumentDeducer::set_option(defer_spawn&& defer) {
-    popen_->defer_process_start_ = defer.defer;
-  }
-
-  inline void ArgumentDeducer::set_option(shell&& sh) {
-    popen_->shell_ = sh.shell_;
-  }
-
-  inline void ArgumentDeducer::set_option(session_leader&& sleader) {
-    popen_->session_leader_ = sleader.leader_;
-  }
-
   inline void ArgumentDeducer::set_option(input&& inp) {
     if (inp.rd_ch_ != -1) popen_->stream_.read_from_parent_ = inp.rd_ch_;
     if (inp.wr_ch_ != -1) popen_->stream_.write_to_child_ = inp.wr_ch_;
@@ -1708,15 +1383,6 @@ namespace detail {
     if (err.rd_ch_ != -1) popen_->stream_.err_read_ = err.rd_ch_;
   }
 
-  inline void ArgumentDeducer::set_option(close_fds&& cfds) {
-    popen_->close_fds_ = cfds.close_all;
-  }
-
-  inline void ArgumentDeducer::set_option(preexec_func&& prefunc) {
-    popen_->preexec_fn_ = std::move(prefunc);
-    popen_->has_preexec_fn_ = true;
-  }
-
 
   inline void Child::execute_child() {
 #ifndef __USING_WINDOWS__
@@ -1763,41 +1429,8 @@ namespace detail {
       if (stream.err_write_ != -1 && stream.err_write_ > 2)
         close(stream.err_write_);
 
-      // Close all the inherited fd's except the error write pipe
-      if (parent_->close_fds_) {
-        int max_fd = sysconf(_SC_OPEN_MAX);
-        if (max_fd == -1) throw OSError("sysconf failed", errno);
-
-        for (int i = 3; i < max_fd; i++) {
-          if (i == err_wr_pipe_) continue;
-          close(i);
-        }
-      }
-
-      // Change the working directory if provided
-      if (parent_->cwd_.length()) {
-        sys_ret = chdir(parent_->cwd_.c_str());
-        if (sys_ret == -1) throw OSError("chdir failed", errno);
-      }
-
-      if (parent_->has_preexec_fn_) {
-        parent_->preexec_fn_();
-      }
-
-      if (parent_->session_leader_) {
-        sys_ret = setsid();
-        if (sys_ret == -1) throw OSError("setsid failed", errno);
-      }
-
       // Replace the current image with the executable
-      if (parent_->env_.size()) {
-        for (auto& kv : parent_->env_) {
-          setenv(kv.first.c_str(), kv.second.c_str(), 1);
-        }
-        sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());
-      } else {
-        sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());
-      }
+      sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());
 
       if (sys_ret == -1) throw OSError("execve failed", errno);
 
@@ -1840,16 +1473,7 @@ namespace detail {
 
     for (auto& h : handles) {
       if (h == nullptr) continue;
-      switch (bufsiz_) {
-      case 0:
-        setvbuf(h, nullptr, _IONBF, BUFSIZ);
-        break;
-      case 1:
-        setvbuf(h, nullptr, _IONBF, BUFSIZ);
-        break;
-      default:
-        setvbuf(h, nullptr, _IOFBF, bufsiz_);
-      };
+      setvbuf(h, nullptr, _IONBF, BUFSIZ);
     }
   #endif
   }

src/wallet/external_signer_scriptpubkeyman.cpp

@@ -9,9 +9,11 @@
 #include <wallet/external_signer_scriptpubkeyman.h>
 
 #include <iostream>
+#include <key_io.h>
 #include <memory>
 #include <stdexcept>
 #include <string>
+#include <univalue.h>
 #include <utility>
 #include <vector>
 
@@ -51,15 +53,26 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
     return signers[0];
 }
 
-bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const
+util::Result<void> 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& 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 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<void>();
 }
 
 // If sign is true, transaction must previously have been filled

src/wallet/external_signer_scriptpubkeyman.h

@@ -9,6 +9,8 @@
 
 #include <memory>
 
+struct bilingual_str;
+
 namespace wallet {
 class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
 {
@@ -27,7 +29,11 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
 
   static ExternalSigner GetExternalSigner();
 
-  bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const;
+  /**
+  * Display address on the device and verify that the returned value matches.
+  * @returns nothing or an error message
+  */
+ util::Result<void> 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/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<void> displayAddress(const CTxDestination& dest) override
     {
         LOCK(m_wallet->cs_wallet);
         return m_wallet->DisplayAddress(dest);

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<void> 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());

src/wallet/scriptpubkeyman.h

@@ -27,7 +27,6 @@
 #include <unordered_map>
 
 enum class OutputType;
-struct bilingual_str;
 
 namespace wallet {
 struct MigrationData;

src/wallet/wallet.cpp

@@ -2667,7 +2667,7 @@ void ReserveDestination::ReturnDestination()
     address = CNoDestination();
 }
 
-bool CWallet::DisplayAddress(const CTxDestination& dest)
+util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
 {
     CScript scriptPubKey = GetScriptForDestination(dest);
     for (const auto& spk_man : GetScriptPubKeyMans(scriptPubKey)) {
@@ -2676,9 +2676,9 @@ 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;
+    return util::Error{_("There is no ScriptPubKeyManager for this address")};
 }
 
 bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch)

src/wallet/wallet.h

@@ -537,8 +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<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    /** Display address on an external signer. 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<void> 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);

test/functional/mocks/signer.py

@@ -25,35 +25,36 @@ 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"
 
         ]
     }))
 
 
 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/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r",
-        "tr([00000001/86'/1'/0'/0/0]c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#4vdj9jqk",
-    ]
+    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",
+        "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}))
 
-    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/p2p_disconnect_ban.py

@@ -77,6 +77,7 @@ class DisconnectBanTest(BitcoinTestFramework):
         self.nodes[1].setmocktime(old_time)
         self.nodes[1].setban("127.0.0.0/32", "add")
         self.nodes[1].setban("127.0.0.0/24", "add")
+        self.nodes[1].setban("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", "add")
         self.nodes[1].setban("192.168.0.1", "add", 1)  # ban for 1 seconds
         self.nodes[1].setban("2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19", "add", 1000)  # ban for 1000 seconds
         listBeforeShutdown = self.nodes[1].listbanned()
@@ -85,13 +86,13 @@ class DisconnectBanTest(BitcoinTestFramework):
         self.log.info("setban: test banning with absolute timestamp")
         self.nodes[1].setban("192.168.0.2", "add", old_time + 120, True)
 
-        # Move time forward by 3 seconds so the third ban has expired
+        # Move time forward by 3 seconds so the fourth ban has expired
         self.nodes[1].setmocktime(old_time + 3)
-        assert_equal(len(self.nodes[1].listbanned()), 4)
+        assert_equal(len(self.nodes[1].listbanned()), 5)
 
         self.log.info("Test ban_duration and time_remaining")
         for ban in self.nodes[1].listbanned():
-            if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24"]:
+            if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24", "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"]:
                 assert_equal(ban["ban_duration"], 86400)
                 assert_equal(ban["time_remaining"], 86397)
             elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19":
@@ -108,6 +109,7 @@ class DisconnectBanTest(BitcoinTestFramework):
         assert_equal("127.0.0.0/32", listAfterShutdown[1]['address'])
         assert_equal("192.168.0.2/32", listAfterShutdown[2]['address'])
         assert_equal("/19" in listAfterShutdown[3]['address'], True)
+        assert_equal("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", listAfterShutdown[4]['address'])
 
         # Clear ban lists
         self.nodes[1].clearbanned()

test/functional/rpc_setban.py

@@ -64,20 +64,10 @@ class SetBanTests(BitcoinTestFramework):
         assert self.is_banned(node, tor_addr)
         assert not self.is_banned(node, ip_addr)
 
-        self.log.info("Test the ban list is preserved through restart")
-
-        self.restart_node(1)
-        assert self.is_banned(node, tor_addr)
-        assert not self.is_banned(node, ip_addr)
-
         node.setban(tor_addr, "remove")
         assert not self.is_banned(self.nodes[1], tor_addr)
         assert not self.is_banned(node, ip_addr)
 
-        self.restart_node(1)
-        assert not self.is_banned(node, tor_addr)
-        assert not self.is_banned(node, ip_addr)
-
         self.log.info("Test -bantime")
         self.restart_node(1, ["-bantime=1234"])
         self.nodes[1].setban("127.0.0.1", "add")

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")
@@ -140,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)