test/functional/wallet_listtransactions.py (5)

21-24: num_nodes=3: OK, but keep restart args consistent.

Bumping to 3 is justified for cross‑node wallet checks. When you restart later, pass self.extra_args to preserve initial node config.

Apply this small change at the later restart:

-        self.start_nodes()
+        self.start_nodes(extra_args=self.extra_args)

---

113-118: Avoid hard‑coding wallet.dat paths; use RPC‑based cloning.

Directly copying wallet.dat is brittle (backend differences, walletdir layout like “wallets/”, multiwallet). Prefer RPCs:

• dumpwallet on node 0 and importwallet on node 2 (no stop/start needed), or
• backupwallet while running, then copy the backup file.

This removes assumptions about self.chain/default_wallet_name and survives backend changes.

Example replacement:

-        self.nodes[0].keypoolrefill(1000)
-        self.stop_nodes()
-        wallet0 = os.path.join(self.nodes[0].datadir, self.chain, self.default_wallet_name, "wallet.dat")
-        wallet2 = os.path.join(self.nodes[2].datadir, self.chain, self.default_wallet_name, "wallet.dat")
-        shutil.copyfile(wallet0, wallet2)
-        self.start_nodes()
+        self.nodes[0].keypoolrefill(1000)
+        dump_file = os.path.join(self.nodes[0].datadir, "w0_dump.txt")
+        self.nodes[0].dumpwallet(dump_file)
+        # Ensure wallet 2 is fresh and imports all keys/labels from node 0
+        self.nodes[2].importwallet(dump_file)

If you must keep file‑copy semantics, derive the wallet path via RPC or node attributes (e.g., walletdir) rather than constructing it manually.

---

118-123: Reconnect is fine; add sync for determinism.

After restarting/reconnecting, call sync_* to avoid races before subsequent sends/mines and comparisons.

         self.connect_nodes(0, 1)
         self.connect_nodes(1, 2)
         self.connect_nodes(2, 0)
+        self.sync_all()

---

128-140: Add chain syncs after mining to avoid flakiness across nodes.

generate() mines on a single node; peers may lag briefly. Sync before comparing listtransactions to ensure both nodes see the same chain.

         self.nodes[1].sendtoaddress(addr3, "0.001")
         self.generate(self.nodes[1], 1)
+        self.sync_blocks()
@@
         self.nodes[1].sendtoaddress(addr2, "0.001")
         self.generate(self.nodes[1], 1)
+        self.sync_blocks()
@@
         self.nodes[0].sendtoaddress(addr1, "0.001")
         self.generate(self.nodes[0], 1)
+        self.sync_blocks()

---

145-152: Strengthen normalization sort key to avoid duplicate‑txid ordering differences.

listtransactions can emit multiple entries with the same txid (e.g., send‑to‑self). Sorting only by txid may keep per‑txid order different across nodes, causing spurious mismatches. Include category and vout in the sort key.

-        def normalize_list(txs):
+        def normalize_list(txs):
             for tx in txs:
                 tx.pop('label', None)
                 tx.pop('time', None)
                 tx.pop('timereceived', None)
-            txs.sort(key=lambda x: x['txid'])
+            txs.sort(key=lambda x: (x['txid'], x.get('category', ''), x.get('vout', -1)))

src/wallet/init.cpp (1)

70-72: Hide -signer when feature is not compiled for consistent CLI UX.

Mirror existing patterns (e.g., -unsafesqlitesync) to avoid unknown-option noise.

 #ifdef ENABLE_EXTERNAL_SIGNER
-    argsman.AddArg("-signer=<cmd>", "External signing tool, see doc/external-signer.md", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
+    argsman.AddArg("-signer=<cmd>", "External signing tool, see doc/external-signer.md", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
+#else
+    argsman.AddHiddenArgs({"-signer"});
 #endif

src/qt/forms/createwalletdialog.ui (2)

112-121: Text/style polish and availability gating.

• Use title case to match help/category naming: “External Signer”.
• Ensure the checkbox is hidden/disabled when external signer is not compiled or no signers are detected.

-         <string>External signer</string>
+         <string>External Signer</string>

If not already handled in CreateWalletDialog code, I can add the visibility/enablement toggle based on signer enumeration.

---

155-155: Tab order looks fine; consider adding descriptor checkbox too (if missing).

If descriptor_checkbox isn’t already in tabstops elsewhere, add it for keyboard navigation consistency.

src/qt/walletmodel.h (1)

144-144: Consider passing string parameter by const reference.

The method signature takes std::string sAddress by value. For efficiency, consider using const std::string& sAddress to avoid unnecessary string copies, especially since the implementation only needs to read the value.

Apply this diff if you'd like to optimize the parameter passing:

-    bool displayAddress(std::string sAddress);
+    bool displayAddress(const std::string& sAddress);

Also update the implementation in src/qt/walletmodel.cpp accordingly.

src/qt/walletmodel.cpp (2)

582-592: Consider using this as the parent widget for the error dialog.

Passing nullptr as the parent to QMessageBox::critical can lead to suboptimal behavior: the dialog may not be modal to the wallet window, may appear on the wrong screen in multi-monitor setups, and lacks proper window ownership. Consider passing this or another appropriate parent widget instead.

Apply this diff:

-        QMessageBox::critical(nullptr, tr("Can't display address"), e.what());
+        QMessageBox::critical(/* appropriate parent widget */, tr("Can't display address"), e.what());

Note: You may need to refactor this method to accept a parent widget parameter, or use a pattern that ensures proper parent widget handling.

---

582-582: Parameter efficiency: pass by const reference.

The parameter sAddress is passed by value, which creates an unnecessary copy. Consider passing by const std::string& instead.

Apply this diff:

-bool WalletModel::displayAddress(std::string sAddress)
+bool WalletModel::displayAddress(const std::string& sAddress)

src/qt/walletcontroller.cpp (1)

296-306: Consider using a translatable string for the multi-signer error message.

The error message on line 303 uses QString::fromStdString with a hardcoded English string, while other error messages in the same flow (like line 300) use Qt's tr() for translation.

Apply this diff to make the error message translatable and consistent with the rest of the codebase:

     if (signers.size() > 1) {
-        QMessageBox::critical(nullptr, tr("Too many external signers found"), QString::fromStdString("More than one external signer found. Please connect only one at a time."));
+        QMessageBox::critical(nullptr, tr("Too many external signers found"), tr("More than one external signer found. Please connect only one at a time."));
         signers.clear();
     }

src/wallet/rpc/backup.cpp (1)

2041-2047: CoinJoin detection via string search is brittle

Parsing the descriptor string to find "/'/'/4'/0'" is fragile. Prefer a structured check (e.g., store derivation purpose/path metadata in WalletDescriptor or a helper that decodes derivation steps) to avoid false positives if formatting changes or keys are reordered.

src/interfaces/node.h (1)

160-169: Make ExternalSigner interface const-correct and defaulted

getName is an accessor; mark it const. Also default the virtual destructor for clarity.

Apply:

-class ExternalSigner
+class ExternalSigner
 {
 public:
-    virtual ~ExternalSigner() {};
+    virtual ~ExternalSigner() = default;
 
     //! Get signer display name
-    virtual std::string getName() = 0;
+    virtual std::string getName() const = 0;
 };

doc/external-signer.md (1)

1-171: Tighten Markdown (fences, prompts, style) and mention build flag

• Add language identifiers (sh, json).
• Drop leading “$ ” prompts or include sample outputs per MD014.
• Minor grammar: “In order to be” → “To be”, hyphenate “BIP44/49/84-compatible”.
• Document that builds must enable external signer support (configure with --enable-external-signer) if that’s the gating.

Apply sample edits:

-```sh
-$ dashd -signer=../HWI/hwi.py
-```
+```sh
+dashd -signer=../HWI/hwi.py
+```

-```
-$ dash-cli enumeratesigners
+```json
+{
+  "signers": [
+    { "fingerprint": "c8df832a" }
+  ]
+}
+```

-```sh
-$ dash-cli createwallet "hww" true true "" true true true
-```
+```sh
+dash-cli createwallet "hww" true true "" true true true
+```

-``` 
-$ <cmd> enumerate
+```sh
+<cmd> enumerate
 [
   { "fingerprint": "00000000" }
 ]

+Note: build with --enable-external-signer to enable this feature.

Also fix “immedidately” → “immediately” if you keep that note in code comments elsewhere (see createwalletdialog.cpp).

</blockquote></details>
<details>
<summary>src/qt/createwalletdialog.cpp (2)</summary><blockquote>

`49-65`: **External signer toggle: align blank-wallet semantics and fix typo**

- You set blank_wallet_checkbox to checked when toggled on (Line 64), implying watch-only wallets should be “blank”. In setSigners() you later force it unchecked (see below), which is inconsistent. Decide the intended default and make both paths consistent.
- Typo in comment: “immedidately” → “immediately”.



Would you like me to update both spots to a single policy (e.g., blank=false because descriptors are imported immediately)?

---

`115-134`: **setSigners(): minor polish and consistency**

- Consistency: As noted above, this forces blank_wallet_checkbox unchecked even though the “toggled” path sets it checked. Pick one behavior.
- UX: When no signers are found (else branch), consider re-enabling the other checkboxes back to their defaults to avoid a partially locked UI if setSigners() is called after prior toggling.
- API: You call signers[0]->getName(); with the proposed const qualifier, this still works.


```diff
-        ui->blank_wallet_checkbox->setEnabled(false);
-        ui->blank_wallet_checkbox->setChecked(false);
+        ui->blank_wallet_checkbox->setEnabled(false);
+        ui->blank_wallet_checkbox->setChecked(true); // or false, but keep consistent with the toggle handler
@@
     } else {
         ui->external_signer_checkbox->setEnabled(false);
+        // Optionally restore defaults:
+        ui->encrypt_wallet_checkbox->setEnabled(true);
+        ui->blank_wallet_checkbox->setEnabled(true);
+        ui->disable_privkeys_checkbox->setEnabled(true);
+        ui->descriptor_checkbox->setEnabled(true);
     }

If you prefer blank=false, update the toggle handler accordingly.

src/node/interfaces.cpp (3)

530-551: Make listExternalSigners resilient (catch enumerate errors, guard empty cmd).

Avoid throwing across the Node interface. Catch exceptions from ExternalSigner::Enumerate and return an empty list; also use command.empty() for clarity.

Apply:

     std::vector<std::unique_ptr<interfaces::ExternalSigner>> listExternalSigners() override
     {
 #ifdef ENABLE_EXTERNAL_SIGNER
-        std::vector<ExternalSigner> signers = {};
-        const std::string command = gArgs.GetArg("-signer", "");
-        if (command == "") return {};
-        ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
+        std::vector<ExternalSigner> signers;
+        const std::string command = gArgs.GetArg("-signer", "");
+        if (command.empty()) return {};
+        try {
+            ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
+        } catch (const std::exception& e) {
+            // Log and degrade gracefully
+            LogPrintf("listExternalSigners: enumerate failed: %s\n", e.what());
+            return {};
+        }
         std::vector<std::unique_ptr<interfaces::ExternalSigner>> result;
-        for (auto& signer : signers) {
+        result.reserve(signers.size());
+        for (auto& signer : signers) {
             result.emplace_back(std::make_unique<ExternalSignerImpl>(std::move(signer)));
         }
         return result;
 #else

---

17-17: Optional: include only when enabled.

Wrap the header include with ENABLE_EXTERNAL_SIGNER to avoid unnecessary coupling when the feature is off.

-#include <external_signer.h>
+#ifdef ENABLE_EXTERNAL_SIGNER
+#include <external_signer.h>
+#endif

---

402-411: Minor: keep adaptor minimal.

Consider storing just the name instead of copying the whole ::ExternalSigner, unless future methods are planned.

-class ExternalSignerImpl : public interfaces::ExternalSigner
-{
-public:
-    ExternalSignerImpl(::ExternalSigner signer) : m_signer(std::move(signer)) {}
-    std::string getName() override { return m_signer.m_name; }
-private:
-    ::ExternalSigner m_signer;
-};
+class ExternalSignerImpl : public interfaces::ExternalSigner {
+public:
+    explicit ExternalSignerImpl(std::string name) : m_name(std::move(name)) {}
+    std::string getName() override { return m_name; }
+private:
+    std::string m_name;
+};

And in the loop:

- result.emplace_back(std::make_unique<ExternalSignerImpl>(std::move(signer)));
+ result.emplace_back(std::make_unique<ExternalSignerImpl>(std::move(signer.m_name)));

src/external_signer.h (1)

27-35: API polish for performance and const-correctness.

• Remove top-level const on by-value return.
• Pass chain by const reference in ctor.
• Make GetDescriptors const.

-    const std::string NetworkArg() const;
+    std::string NetworkArg() const;
@@
-    ExternalSigner(const std::string& command, const std::string chain, const std::string& fingerprint, const std::string name);
+    ExternalSigner(const std::string& command, const std::string& chain, const std::string& fingerprint, const std::string& name);
@@
-    UniValue GetDescriptors(const int account);
+    UniValue GetDescriptors(int account) const;

Also update implementations:

-const std::string ExternalSigner::NetworkArg() const
+std::string ExternalSigner::NetworkArg() const
 {
     return " --chain " + m_chain;
 }
@@
-ExternalSigner::ExternalSigner(const std::string& command, const std::string chain, const std::string& fingerprint, const std::string name)
+ExternalSigner::ExternalSigner(const std::string& command, const std::string& chain, const std::string& fingerprint, const std::string& name)
   : m_command(command), m_chain(chain), m_fingerprint(fingerprint), m_name(name) {}
@@
-UniValue ExternalSigner::GetDescriptors(const int account)
+UniValue ExternalSigner::GetDescriptors(int account) const

Also applies to: 54-59

src/wallet/external_signer_scriptpubkeyman.cpp (1)

64-86: Use wallet logging instead of stderr on signing failure.

Direct writes to stderr bypass logging. Prefer LogPrintf/WalletLogPrintf.

-    if(!GetExternalSigner().SignTransaction(psbt, strFailReason)) {
-        tfm::format(std::cerr, "Failed to sign: %s\n", strFailReason);
+    if (!GetExternalSigner().SignTransaction(psbt, strFailReason)) {
+        LogPrintf("%s: external signer failed: %s\n", __func__, strFailReason);
         return TransactionError::EXTERNAL_SIGNER_FAILED;
     }

Add:

+#include <logging.h>

near the includes.

test/functional/rpc_signer.py (2)

51-54: Fix Flake8 E128 under-indented continuations.

Reformat the four multi-line assert_raises_rpc_error calls.

-        assert_raises_rpc_error(-1, 'Error: restart dashd with -signer=<cmd>',
-            self.nodes[0].enumeratesigners
-        )
+        assert_raises_rpc_error(
+            -1,
+            'Error: restart dashd with -signer=<cmd>',
+            self.nodes[0].enumeratesigners,
+        )

-        assert_raises_rpc_error(-1, 'execve failed: No such file or directory',
-            self.nodes[3].enumeratesigners
-        )
+        assert_raises_rpc_error(
+            -1,
+            'execve failed: No such file or directory',
+            self.nodes[3].enumeratesigners,
+        )

-        assert_raises_rpc_error(-1, 'RunCommandParseJSON error',
-            self.nodes[1].enumeratesigners
-        )
+        assert_raises_rpc_error(
+            -1,
+            'RunCommandParseJSON error',
+            self.nodes[1].enumeratesigners,
+        )

-        assert_raises_rpc_error(-1, 'fingerprint not found',
-            self.nodes[1].enumeratesigners
-        )
+        assert_raises_rpc_error(
+            -1,
+            'fingerprint not found',
+            self.nodes[1].enumeratesigners,
+        )

Also applies to: 56-58, 62-64, 68-70

---

55-58: Harden missing-signer assertion across OSes.

The exact message “execve failed: No such file or directory” is Linux-specific and will differ on Windows/macOS. Match a less brittle substring or accept multiple options (e.g., “No such file or directory” or “The system cannot find the file specified.”).

src/qt/sendcoinsdialog.cpp (1)

503-518: Optional: sanitize suggested filename for PSBT saves.

Labels/addresses may contain characters invalid for filenames. Consider replacing forbidden characters (/:*?"<>| etc.) or falling back to a safe default.

src/wallet/wallet.h (1)

30-31: Guard external_signer include for non-signer builds.

Protect include behind ENABLE_EXTERNAL_SIGNER to avoid unnecessary coupling.

-#include <external_signer.h>
+#ifdef ENABLE_EXTERNAL_SIGNER
+#include <external_signer.h>
+#endif

test/functional/wallet_signer.py (5)

113-116: Fix flake8 E128/E124: align continuation and closing paren.

-        assert_raises_rpc_error(-1, 'RunCommandParseJSON error',
-            hww.walletdisplayaddress, address1
-        )
+        assert_raises_rpc_error(
+            -1,
+            'RunCommandParseJSON error',
+            hww.walletdisplayaddress,
+            address1,
+        )

---

128-141: Fix flake8 E122: indent the second descriptor dict.

-        result = mock_wallet.importdescriptors([{
+        result = mock_wallet.importdescriptors([
+            {
             "desc": "pkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#6k3x80k9",
             "timestamp": 0,
             "range": [0,1],
             "internal": False,
             "active": True
-        },
-        {
+            },
+            {
             "desc": "pkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/*)#tz5866xa",
             "timestamp": 0,
             "range": [0, 0],
             "internal": True,
             "active": True
-        }])
+            },
+        ])

---

179-180: Prefer assert_equal for clearer failures.

-        assert(hww.testmempoolaccept([mock_tx])[0]["allowed"])
+        assert_equal(hww.testmempoolaccept([mock_tx])[0]["allowed"], True)

---

21-26: Make signer path robust on Windows and paths with spaces.
Use sys.executable and quoting to avoid shell issues.

+import sys
+import shlex
...
-        if platform.system() == "Windows":
-            return "py " + path
-        else:
-            return path
+        if platform.system() == "Windows":
+            return f'"{sys.executable}" {shlex.quote(path)}'
+        return path

Please confirm CI on Windows still finds Python via sys.executable.

Also applies to: 29-34, 36-40

---

100-106: Reduce brittleness of exact address assertion.
Hardcoding the full address risks future derivation/encoding changes. Consider asserting validity and keypath properties instead.

-        assert_equal(address3, "yg8pCsowDbY7PbJMthTi4xkkmhNfcNNdHV")
+        assert_equal(address_info['hdkeypath'].startswith("m/44'/1'/0'/0/"), True)
+        assert_equal(address_info['iswitness'], False)

If the exact address is intentionally fixed by the mock, feel free to keep it.

src/wallet/scriptpubkeyman.h (1)

604-608: SetupDescriptor public hook — consider nodiscard.
Return value indicates success; marking [[nodiscard]] would prevent accidental ignore.

-    bool SetupDescriptor(std::unique_ptr<Descriptor>desc);
+    [[nodiscard]] bool SetupDescriptor(std::unique_ptr<Descriptor> desc);

src/wallet/wallet.cpp (3)

3731-3741: Avoid reusing SignatureData across SPKMs.
Stateful CanProvide() may mutate sigdata; reinitialize per iteration.

 std::set<ScriptPubKeyMan*> CWallet::GetScriptPubKeyMans(const CScript& script) const
 {
     std::set<ScriptPubKeyMan*> spk_mans;
-    SignatureData sigdata;
     for (const auto& spk_man_pair : m_spk_managers) {
-        if (spk_man_pair.second->CanProvide(script, sigdata)) {
+        SignatureData sigdata;
+        if (spk_man_pair.second->CanProvide(script, sigdata)) {
             spk_mans.insert(spk_man_pair.second.get());
         }
     }
     return spk_mans;
 }

---

3869-3897: External signer descriptor ingestion — minor hardening.
Consider logging/skipping descriptors with missing/unsupported output types; current continue silently drops them.

-                if (!desc->GetOutputType()) {
-                    continue;
-                }
+                if (!desc->GetOutputType()) {
+                    WalletLogPrintf("%s: skipping descriptor without output type: %s\n", __func__, desc_str);
+                    continue;
+                }

---

3965-3987: Remove unused variable and assert in IsInternalScriptPubKeyMan.
The output type is fetched but not used; trim dead code.

-    const auto& type = desc_spk_man->GetWalletDescriptor().descriptor->GetOutputType();
-    assert(type.has_value());
-
     return GetScriptPubKeyMan(/*internal=*/true) == desc_spk_man;

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro