feat(wallet): external signer (hardware signer) #6019
Conversation
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
❌ Backport Verification - CATASTROPHIC FAILUREOriginal Bitcoin commit: Critical violations detected:
This PR violates fundamental backporting principles:
Recommendation: Create focused, smaller PRs that backport specific Bitcoin commits for hardware wallet functionality, following the established backporting process with:
This PR has been automatically closed due to catastrophic validation failures. |
|
Automatically closed due to catastrophic validation failures. Please see the detailed analysis above and create focused PRs with proper Bitcoin commit references and proportional scope. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
knst
changed the title
WalkthroughReplaces Boost.Process wiring with an opt-in "external signer" feature (configure option ENABLE_EXTERNAL_SIGNER) and removes CI build flags for boost-process. Adds an ExternalSigner class and process call plumbing, wallet flags and ExternalSignerScriptPubKeyMan for PSBT signing/display, Node/Wallet interface extensions, RPCs (enumeratesigners, walletdisplayaddress), Qt UI changes (options, create-wallet, receive/send dialogs), test mocks and functional tests, and related build/config updates. RunCommandParseJSON is exposed unconditionally with runtime guards when external signer support is absent. New error codes and DB checks for external-signer-only wallets included. Sequence Diagram(s)sequenceDiagram
%% Styling
participant UI as Qt UI
participant Model as WalletModel
participant Wallet as CWallet
participant SPKM as ExternalSignerScriptPubKeyMan
participant ExtSigner as External signer process
participant Sys as RunCommandParseJSON
note over UI,Model: User initiates send / verify
UI->>Model: prepare/send or displayAddress request
Model->>Wallet: displayAddress / create PSBT (FillPSBT sign=false)
Wallet->>SPKM: FillPSBT / DisplayAddress
SPKM->>Wallet: return PSBT / confirmation
alt External signer present
Wallet->>ExtSigner: Enumerate / Sign / DisplayAddress (via Sys)
ExtSigner-->>Sys: parse JSON I/O
Sys-->>Wallet: parsed JSON (signer output / descriptors / errors)
Wallet->>Model: signed/final PSBT or error
Model->>UI: show result (broadcast, copy, or prompt)
else No external signer compiled or configured
Wallet-->>Model: return error/runtime_error
Model->>UI: show guidance/configure -signer
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus review on:
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/qt/forms/createwalletdialog.ui (1)
150-156: Include Descriptor checkbox in tab orderDescriptor wallet option is missing from , which hurts keyboard navigation.
<tabstops> <tabstop>wallet_name_line_edit</tabstop> <tabstop>encrypt_wallet_checkbox</tabstop> <tabstop>disable_privkeys_checkbox</tabstop> <tabstop>blank_wallet_checkbox</tabstop> + <tabstop>descriptor_checkbox</tabstop> <tabstop>external_signer_checkbox</tabstop> </tabstops>src/test/system_tests.cpp (1)
39-43: Fix POSIX quoting in RunCommandParseJSON testThe JSON argument isn’t properly escaped; the shell will break on inner quotes.
- const UniValue result = RunCommandParseJSON("echo \"{\"success\": true}\""); + const UniValue result = RunCommandParseJSON("echo \"{\\\"success\\\": true}\"");
🧹 Nitpick comments (36)
test/functional/test_framework/test_node.py (1)
884-889: LGTM! Consider adding documentation for developer experience.The parameter addition is implemented correctly—the signature and call order match the RPC definition in
src/wallet/rpc/wallet.cpp. The default value ofNoneappropriately maps to the RPC's default offalse.However, the method signature has grown to 8 parameters without documentation. Consider adding a docstring that explains:
- The purpose and type of each parameter
- That
external_signer=Truerequires bothdisable_private_keys=Trueanddescriptors=True(per the RPC implementation)This would improve the developer experience when writing tests that use external signers.
Example docstring addition:
+ def createwallet(self, wallet_name, disable_private_keys=None, blank=None, passphrase='', avoid_reuse=None, descriptors=None, load_on_startup=None, external_signer=None): + """Create a new wallet via the createwallet RPC. + + Args: + wallet_name: Name for the new wallet + disable_private_keys: Disable private keys (watchonly mode) + blank: Create a blank wallet (no keys or HD seed) + passphrase: Encryption passphrase + avoid_reuse: Enable coin reuse avoidance + descriptors: Create a descriptor wallet + load_on_startup: Add wallet to startup list + external_signer: Use external signer (requires disable_private_keys=True and descriptors=True) + """ if descriptors is None:Based on learnings: Good documentation is especially important in test framework code where multiple developers write tests.
test/functional/rpc_help.py (1)
114-115: Make the titles comparison fully order‑independentTo avoid brittle failures if RPC help ordering changes, sort both sides.
- assert_equal(titles, sorted(components)) + assert_equal(sorted(titles), sorted(components))src/qt/optionsmodel.cpp (1)
226-232: Plumbing looks right; consider hiding when not supportedPersisting external_signer_path and SoftSetArg("-signer") is fine. To avoid confusing users, consider gating this option in the UI when ENABLE_EXTERNAL_SIGNER is off (e.g., hide row or mark read-only).
test/functional/mocks/signer.py (2)
11-19: Handle empty mock_result safelyAvoid index errors when mock_result exists but is empty.
def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") if(os.path.isfile(mock_result_path)): with open(mock_result_path, "r", encoding="utf8") as f: - mock_result = f.read() - if mock_result[0]: + mock_result = f.read() + if mock_result: sys.stdout.write(mock_result[2:]) sys.exit(int(mock_result[0]))
20-22: Normalize indentation to 4 spacesUse 4-space indents to satisfy linters and maintain consistency.
-def enumerate(args): - sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) +def enumerate(args): + sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) @@ - sys.argv.extend(buffer.rstrip().split(" ")) + sys.argv.extend(buffer.rstrip().split(" "))Also applies to: 96-97
src/qt/sendcoinsdialog.cpp (1)
540-545: Avoid tfm::format to stderr hereThis may not compile without including tinyformat and won’t surface in the GUI. Prefer Q_EMIT message or qWarning().
- if (err != TransactionError::OK) { - tfm::format(std::cerr, "Failed to sign PSBT"); - processSendCoinsReturn(WalletModel::TransactionCreationFailed); + if (err != TransactionError::OK) { + processSendCoinsReturn(WalletModel::TransactionCreationFailed);src/qt/walletmodel.h (1)
144-144: Pass string parameter by const reference.The
sAddressparameter creates an unnecessary copy. For strings, pass byconst std::string&to avoid allocation overhead.Apply this diff:
- bool displayAddress(std::string sAddress); + bool displayAddress(const std::string& sAddress);Note: The corresponding implementation in src/qt/walletmodel.cpp (line 581) should also be updated.
src/qt/forms/optionsdialog.ui (1)
412-441: Add a “Browse…” control and minor UX polish for the signer path.
- Add a browse button to reduce path-typing errors.
- Enable the clear button and a placeholder for discoverability.
- Ensure the group is hidden/disabled at runtime when ENABLE_EXTERNAL_SIGNER is off (guard in optionsdialog.cpp).
Apply minimal UI tweaks:
<layout class="QVBoxLayout" name="verticalLayoutHww"> <item> <layout class="QHBoxLayout" name="horizontalLayoutHww"> <item> <widget class="QLabel" name="externalSignerPathLabel"> @@ </item> <item> <widget class="QLineEdit" name="externalSignerPath"> <property name="toolTip"> <string>Full path to a Dash Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string> </property> + <property name="placeholderText"> + <string>/path/to/hwi(.py|.exe)</string> + </property> + <property name="clearButtonEnabled"> + <bool>true</bool> + </property> </widget> </item> + <item> + <widget class="QToolButton" name="externalSignerBrowse"> + <property name="text"> + <string>…</string> + </property> + <property name="toolTip"> + <string>Browse for signer executable/script</string> + </property> + </widget> + </item> </layout> </item> </layout>Please confirm optionsdialog.cpp hides/disables groupBoxHww when external signer is not compiled. If not, I can draft the guard.
src/interfaces/node.h (1)
160-168: Const-correctness and naming clarity for interfaces::ExternalSigner.
- Make getName() const to allow querying name on const instances and align with typical interface contracts.
- Since there is a global ::ExternalSigner, consider explicitly documenting the distinction or renaming this interface (e.g., IExternalSigner) to avoid confusion in mixed scopes.
-class ExternalSigner +class ExternalSigner { public: virtual ~ExternalSigner() {}; - //! Get signer display name - virtual std::string getName() = 0; + //! Get signer display name + virtual std::string getName() const = 0; };src/qt/walletmodel.cpp (1)
582-592: Consider passing a parent widget to QMessageBox.The method correctly handles exceptions from the wallet layer and displays them to the user. However,
QMessageBox::critical(nullptr, ...)uses a null parent, which may result in the dialog appearing disconnected from the main window.Consider accepting an optional parent widget parameter to improve the UX.
Apply this diff if you want to improve dialog parenting:
-bool WalletModel::displayAddress(std::string sAddress) +bool WalletModel::displayAddress(std::string sAddress, QWidget* parent) { CTxDestination dest = DecodeDestination(sAddress); bool res = false; try { res = m_wallet->displayAddress(dest); } catch (const std::runtime_error& e) { - QMessageBox::critical(nullptr, tr("Can't display address"), e.what()); + QMessageBox::critical(parent, tr("Can't display address"), e.what()); } return res; }src/util/system.cpp (1)
1399-1434: Harden external command execution: catch spawn errors and read full streamsTwo improvements:
- Propagate spawn failures uniformly (e.g., missing binary) by catching boost::process errors.
- Read the entire stdout/stderr to avoid truncation on multi-line JSON and reduce deadlock risk if output exceeds a line.
Apply this diff:
#ifdef ENABLE_EXTERNAL_SIGNER namespace bp = boost::process; UniValue result_json; bp::opstream stdin_stream; bp::ipstream stdout_stream; bp::ipstream stderr_stream; if (str_command.empty()) return UniValue::VNULL; - bp::child c( - str_command, - bp::std_out > stdout_stream, - bp::std_err > stderr_stream, - bp::std_in < stdin_stream - ); - if (!str_std_in.empty()) { - stdin_stream << str_std_in << std::endl; - } - stdin_stream.pipe().close(); - - std::string result; - std::string error; - std::getline(stdout_stream, result); - std::getline(stderr_stream, error); - - c.wait(); - const int n_error = c.exit_code(); - if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, n_error, error)); - if (!result_json.read(result)) throw std::runtime_error("Unable to parse JSON: " + result); + try { + bp::child c( + str_command, + bp::std_out > stdout_stream, + bp::std_err > stderr_stream, + bp::std_in < stdin_stream + ); + if (!str_std_in.empty()) { + stdin_stream << str_std_in << std::endl; + } + stdin_stream.pipe().close(); + + std::string result{std::istreambuf_iterator<char>(stdout_stream), std::istreambuf_iterator<char>()}; + std::string error{std::istreambuf_iterator<char>(stderr_stream), std::istreambuf_iterator<char>()}; + + c.wait(); + const int n_error = c.exit_code(); + if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, n_error, error)); + if (!result_json.read(result)) throw std::runtime_error("Unable to parse JSON: " + result); + } catch (const std::exception& e) { + throw std::runtime_error(strprintf("RunCommandParseJSON spawn/IO failure for '%s': %s", str_command, e.what())); + }doc/external-signer.md (3)
13-15: Add fenced code languages and address markdownlint MD014/MD040Specify languages (bash/json) and either drop leading “$” or include sample outputs to satisfy MD014. Improves readability and CI lint.
Also applies to: 25-33, 39-41, 47-50, 59-61, 80-87, 98-101, 115-119, 122-136, 141-149
70-70: Tighten phrasingReplace “In order to be compatible…” with “To be compatible…”.
167-167: Hyphenation nitUse “BIP44/49/84-compatible” for correct hyphenation.
src/qt/optionsdialog.cpp (1)
141-145: UI polish when feature is unavailableBesides disabling the field, consider hiding the entire row (label + editor) to reduce clutter when external signer support is not compiled.
src/qt/walletcontroller.cpp (1)
296-303: List-and-pass signers flow is OK; handle exceptionsCatching std::runtime_error and surfacing to users is good. Consider also logging the exception in the node logger for diagnostics.
test/functional/rpc_signer.py (2)
52-54: Fix flake8 E128 indentation on continued linesAdjust hanging indent for 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: 57-58, 63-65, 69-70
45-47: Make cleanup resilientIgnore missing mock_result file during cleanup to avoid spurious failures.
- def clear_mock_result(self, node): - os.remove(os.path.join(node.cwd, "mock_result")) + def clear_mock_result(self, node): + try: + os.remove(os.path.join(node.cwd, "mock_result")) + except FileNotFoundError: + passtest/functional/mocks/invalid_signer.py (2)
23-39: Descriptor samples: fix duplicated variable and address type for 84'Rename the second xpub_pkh to xpub_wpkh and use wpkh() for 84'/… paths.
- xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6" - xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN" - xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp" + xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6" + xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN" + xpub_wpkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp" @@ - "pkh([b3c19bfc/84'/1'/" + args.account + "']" + xpub_pkh + "/0/*)#jftn8ppv" + "wpkh([b3c19bfc/84'/1'/" + args.account + "']" + xpub_wpkh + "/0/*)#jftn8ppv" @@ - "pkh([b3c19bfc/84'/1'/" + args.account + "']" + xpub_pkh + "/1/*)#rawj6535" + "wpkh([b3c19bfc/84'/1'/" + args.account + "']" + xpub_wpkh + "/1/*)#rawj6535"
20-22: Unused function argument is acceptable hereRuff’s ARG001 for enumerate(args) can be ignored since argparse mandates the signature; or prefix with
_args.src/wallet/external_signer_scriptpubkeyman.cpp (2)
42-50: Selecting the first signer is fragileWhen multiple devices are present, defaulting to index 0 is ambiguous. Accept a fingerprint preference (CLI/UI) and select by match; otherwise surface a clear error.
- if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found"); - // TODO: add fingerprint argument in case of multiple signers - return signers[0]; + if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found"); + // TODO: wire a preferred fingerprint from args/UI and select by it; for now pick the first as a fallback. + return signers[0];
78-85: OK to rely on prior PSBT fill; consider documentingRelying on a prior non-signing FillPSBT pass is fine; consider a comment that inputs must carry bip32 derivations before invoking the signer.
src/qt/createwalletdialog.cpp (3)
9-9: Remove unused include to speed up builds.
#include <interfaces/node.h>isn’t used in this TU; header already declaressetSigners. Please drop it.-#include <interfaces/node.h>
42-45: Tiny typo in comment.“immedidately” → “immediately”.
-// watch-only wallet, even though we immedidately fetch keys from the +// watch-only wallet, even though we immediately fetch keys from the
115-134: setSigners(): robust defaults; consider empty label guard.Prefilling the wallet name from the first signer is nice. Add a fallback if
getName()is empty to keep the OK button logic predictable.-const std::string label = signers[0]->getName(); -ui->wallet_name_line_edit->setText(QString::fromStdString(label)); +const std::string label = signers[0]->getName(); +ui->wallet_name_line_edit->setText(label.empty() ? tr("Hardware wallet") : QString::fromStdString(label));src/wallet/wallet.cpp (2)
2505-2518: DisplayAddress ignores device result; return actual outcome.Currently always returns true despite TODO. Consider checking the signer response and surfacing failure.
-ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); -return signer_spk_man->DisplayAddress(scriptPubKey, signer); +ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); +return signer_spk_man->DisplayAddress(scriptPubKey, signer);And inside
ExternalSignerScriptPubKeyMan::DisplayAddress(...), parse theUniValueand return false on error (see header/cpp comment below).
3818-3851: Descriptor setup paths look correct; two small improvements.
- Non‑signer branch: OK.
- Signer branch: consider validating
GetDescriptors()keys exist and logging unexpected shapes; also avoidcontinuesilently on!GetOutputType().- if (!descriptor_vals.isArray()) throw std::runtime_error(...); + if (!descriptor_vals.isArray()) { + throw std::runtime_error(std::string(__func__) + ": Unexpected result (no descriptor array)"); + } @@ - if (!desc->GetOutputType()) { - continue; - } + if (!desc->GetOutputType()) { + // Skip non-spendable descriptors but log once for visibility. + m_storage.WalletLogPrintf("Skipping descriptor without recognized output type: %s\n", desc_str); + continue; + }Also applies to: 3852-3881
src/wallet/external_signer_scriptpubkeyman.h (1)
30-33: API contract: return real device outcome.Please make
DisplayAddressreturn the parsed success/failure from the signer command instead of unconditionallytrue(see wallet.cpp comment).src/node/interfaces.cpp (1)
16-16: Include external_signer.h unconditionallyOptional: guard the include to reduce compile surface when feature is disabled and to keep headers tighter.
-#include <external_signer.h> +#ifdef ENABLE_EXTERNAL_SIGNER +#include <external_signer.h> +#endifsrc/wallet/wallet.h (2)
30-31: Avoid pulling external_signer.h into wallet.hHeader coupling. wallet.h doesn't need ExternalSigner types in its public interface. Include externally in wallet.cpp instead, or guard by ENABLE_EXTERNAL_SIGNER to keep builds without the feature leaner.
-#include <external_signer.h> +#ifdef ENABLE_EXTERNAL_SIGNER +#include <external_signer.h> +#endif
697-704: Preserve [[nodiscard]] and align n_signed typesConsider keeping [[nodiscard]] on FillPSBT to prevent silent error ignoring. Also, n_signed here is size_t*, but ScriptPubKeyMan::FillPSBT uses int*. Align types across APIs to avoid accidental narrowing.
-TransactionError FillPSBT(PartiallySignedTransaction& psbtx, +[[nodiscard]] TransactionError FillPSBT(PartiallySignedTransaction& psbtx, @@ - size_t* n_signed = nullptr, + int* n_signed = nullptr,If changing the type is disruptive now, at least document the mixed int/size_t and cast explicitly in the implementation.
src/external_signer.h (2)
24-35: Pass-by-reference and namingUse const std::string& for chain to avoid copying; update comment to say “Dash” rather than “Bitcoin”.
- 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); @@ - //! Bitcoin mainnet, testnet, etc + //! Network (mainnet, testnet, etc.)
49-53: DisplayAddress return type vs actual usageCallers ignore the UniValue and just rely on exceptions. Either parse and validate here and return bool, or document that this throws on errors and the value is unused. Prefer returning bool and inspecting result to make walletdisplayaddress more predictable.
test/functional/wallet_signer.py (2)
21-34: Use sys.executable and quote paths for -signerAvoid “py ” launcher assumptions and spaces-in-path issues. Use the current interpreter path and quote.
+import sys @@ - if platform.system() == "Windows": - return "py " + path - else: - return path + return f"\"{sys.executable}\" \"{path}\"" @@ - if platform.system() == "Windows": - return "py " + path - else: - return path + return f"\"{sys.executable}\" \"{path}\""
48-53: Robust mock_result cleanupGuard against missing file to avoid spurious test errors.
def clear_mock_result(self, node): - os.remove(os.path.join(node.cwd, "mock_result")) + p = os.path.join(node.cwd, "mock_result") + if os.path.exists(p): + os.remove(p)src/external_signer.cpp (1)
31-31: Avoid copying UniValue objects in the loop.The loop creates a copy of each
UniValueobject. Use a const reference for better performance.Apply this diff:
- for (UniValue signer : result.getValues()) { + for (const UniValue& signer : result.getValues()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_mac_native_x86_64.sh(1 hunks)ci/test/00_setup_env_native_asan.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(1 hunks)ci/test/00_setup_env_native_qt5.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(7 hunks)doc/Doxyfile.in(1 hunks)doc/external-signer.md(1 hunks)src/Makefile.am(5 hunks)src/Makefile.test.include(1 hunks)src/dummywallet.cpp(1 hunks)src/external_signer.cpp(1 hunks)src/external_signer.h(1 hunks)src/interfaces/node.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/interfaces.cpp(3 hunks)src/qt/createwalletdialog.cpp(4 hunks)src/qt/createwalletdialog.h(2 hunks)src/qt/forms/createwalletdialog.ui(2 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/forms/receiverequestdialog.ui(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(7 hunks)src/qt/walletcontroller.cpp(4 hunks)src/qt/walletmodel.cpp(2 hunks)src/qt/walletmodel.h(1 hunks)src/qt/walletmodeltransaction.cpp(1 hunks)src/qt/walletmodeltransaction.h(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/external_signer.cpp(1 hunks)src/rpc/register.h(2 hunks)src/test/system_tests.cpp(2 hunks)src/util/error.cpp(1 hunks)src/util/error.h(1 hunks)src/util/system.cpp(3 hunks)src/util/system.h(0 hunks)src/wallet/external_signer_scriptpubkeyman.cpp(1 hunks)src/wallet/external_signer_scriptpubkeyman.h(1 hunks)src/wallet/init.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/rpc/wallet.cpp(6 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(17 hunks)src/wallet/wallet.h(5 hunks)src/wallet/walletdb.cpp(1 hunks)src/wallet/walletdb.h(1 hunks)src/wallet/walletutil.h(1 hunks)test/config.ini.in(1 hunks)test/functional/mocks/invalid_signer.py(1 hunks)test/functional/mocks/signer.py(1 hunks)test/functional/rpc_help.py(1 hunks)test/functional/rpc_signer.py(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)test/functional/test_framework/test_node.py(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_signer.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/util/system.h
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/util/error.hsrc/qt/walletmodeltransaction.hsrc/qt/walletmodel.hsrc/rpc/register.hsrc/qt/receiverequestdialog.cppsrc/wallet/rpc/addresses.cppsrc/wallet/init.cppsrc/wallet/rpc/backup.cppsrc/wallet/walletdb.cppsrc/util/system.cppsrc/wallet/walletdb.hsrc/qt/walletmodeltransaction.cppsrc/wallet/scriptpubkeyman.hsrc/interfaces/node.hsrc/qt/optionsdialog.cppsrc/test/system_tests.cppsrc/util/error.cppsrc/wallet/external_signer_scriptpubkeyman.cppsrc/dummywallet.cppsrc/interfaces/wallet.hsrc/qt/createwalletdialog.cppsrc/rpc/external_signer.cppsrc/node/interfaces.cppsrc/qt/optionsmodel.hsrc/rpc/client.cppsrc/qt/walletcontroller.cppsrc/wallet/interfaces.cppsrc/wallet/walletutil.hsrc/qt/walletmodel.cppsrc/wallet/external_signer_scriptpubkeyman.hsrc/external_signer.cppsrc/qt/optionsmodel.cppsrc/qt/sendcoinsdialog.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.hsrc/wallet/wallet.cppsrc/external_signer.hsrc/qt/createwalletdialog.hsrc/wallet/rpc/spend.cpp
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_native_nowallet.shci/test/00_setup_env_s390x.shci/test/00_setup_env_mac_native_x86_64.shci/test/00_setup_env_mac.shci/test/00_setup_env_native_qt5.shci/test/00_setup_env_native_tsan.shci/test/00_setup_env_native_fuzz.shci/test/00_setup_env_arm.shci/test/00_setup_env_native_multiprocess.shci/test/00_setup_env_win64.shci/test/00_setup_env_native_asan.sh
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/test_framework.pytest/functional/test_runner.pytest/functional/wallet_signer.pytest/functional/rpc_help.pytest/functional/test_framework/test_node.pytest/functional/rpc_signer.pytest/functional/mocks/signer.pytest/functional/mocks/invalid_signer.py
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/system_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/Doxyfile.indoc/external-signer.md
🧬 Code graph analysis (28)
src/qt/walletmodeltransaction.h (1)
src/qt/walletmodeltransaction.cpp (2)
setWtx(29-32)setWtx(29-29)
src/qt/walletmodel.h (1)
src/qt/walletmodel.cpp (2)
displayAddress(582-592)displayAddress(582-582)
src/rpc/register.h (2)
src/rpc/external_signer.cpp (2)
RegisterSignerRPCCommands(61-73)RegisterSignerRPCCommands(61-61)src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/wallet/rpc/addresses.cpp (2)
src/wallet/rpc/util.cpp (2)
GetWalletForJSONRPCRequest(55-77)GetWalletForJSONRPCRequest(55-55)src/wallet/interfaces.cpp (18)
LOCK(250-259)LOCK(260-263)LOCK(291-295)LOCK(296-300)LOCK(353-361)LOCK(479-491)dest(210-214)dest(210-210)dest(223-226)dest(223-223)dest(227-230)dest(227-227)dest(231-249)dest(231-234)dest(264-268)dest(264-264)dest(269-273)dest(269-269)
src/wallet/init.cpp (2)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)src/dummywallet.cpp (1)
argsman(31-31)
src/wallet/walletdb.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/util/system.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/wallet/scriptpubkeyman.h (2)
src/wallet/walletutil.h (1)
WalletDescriptor(70-103)src/wallet/external_signer_scriptpubkeyman.cpp (2)
SetupDescriptor(17-40)SetupDescriptor(17-17)
src/interfaces/node.h (2)
src/external_signer.cpp (1)
ExternalSigner(17-17)src/external_signer.h (1)
ExternalSigner(18-64)
src/qt/optionsdialog.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/test/system_tests.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/util/error.cpp (1)
src/util/translation.h (1)
Untranslated(48-48)
src/wallet/external_signer_scriptpubkeyman.cpp (3)
src/wallet/wallet.cpp (24)
batch(550-550)batch(815-815)batch(938-938)batch(1178-1178)batch(1257-1257)batch(1313-1313)batch(1358-1358)batch(1370-1370)batch(1391-1391)batch(1515-1515)batch(1660-1660)batch(1744-1744)batch(1793-1793)batch(2239-2239)batch(2300-2300)batch(2307-2307)DisplayAddress(2505-2518)DisplayAddress(2505-2505)GetSolvingProvider(3734-3738)GetSolvingProvider(3734-3734)GetSolvingProvider(3740-3748)GetSolvingProvider(3740-3740)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/scriptpubkeyman.cpp (16)
GetID(801-804)GetID(801-801)GetID(2375-2379)GetID(2375-2375)TopUp(1410-1413)TopUp(1410-1410)TopUp(1943-2004)TopUp(1943-1943)GetSolvingProvider(678-681)GetSolvingProvider(678-678)GetSolvingProvider(2231-2234)GetSolvingProvider(2231-2231)FillPSBT(734-778)FillPSBT(734-734)FillPSBT(2288-2355)FillPSBT(2288-2288)src/external_signer.cpp (4)
Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)
test/functional/rpc_help.py (2)
test/functional/test_framework/test_framework.py (2)
is_external_signer_compiled(1115-1117)is_zmq_compiled(1135-1137)test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/interfaces/wallet.h (2)
src/qt/walletmodel.cpp (2)
displayAddress(582-592)displayAddress(582-582)src/wallet/interfaces.cpp (12)
dest(210-214)dest(210-210)dest(223-226)dest(223-223)dest(227-230)dest(227-227)dest(231-249)dest(231-234)dest(264-268)dest(264-264)dest(269-273)dest(269-269)
test/functional/test_framework/test_node.py (1)
src/wallet/rpc/wallet.cpp (2)
createwallet(615-713)createwallet(615-615)
src/qt/createwalletdialog.cpp (2)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)src/qt/createwalletdialog.h (1)
CreateWalletDialog(24-44)
test/functional/rpc_signer.py (3)
test/functional/test_framework/test_framework.py (1)
skip_if_no_external_signer(1106-1109)test/functional/test_framework/util.py (2)
assert_equal(69-74)assert_raises_rpc_error(132-148)test/functional/wallet_signer.py (6)
mock_signer_path(21-26)set_test_params(35-41)skip_test_if_missing_module(43-45)set_mock_result(47-49)clear_mock_result(51-52)run_test(54-57)
src/rpc/external_signer.cpp (2)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)src/external_signer.cpp (2)
Enumerate(24-60)Enumerate(24-24)
src/node/interfaces.cpp (1)
src/external_signer.cpp (2)
Enumerate(24-60)Enumerate(24-24)
src/qt/walletmodel.cpp (1)
src/wallet/interfaces.cpp (28)
dest(210-214)dest(210-210)dest(223-226)dest(223-223)dest(227-230)dest(227-227)dest(231-249)dest(231-234)dest(264-268)dest(264-264)dest(269-273)dest(269-269)m_wallet(158-161)m_wallet(166-166)m_wallet(175-175)m_wallet(176-176)m_wallet(182-182)m_wallet(183-183)m_wallet(443-446)m_wallet(447-450)m_wallet(522-522)m_wallet(523-523)m_wallet(524-524)m_wallet(525-525)m_wallet(526-526)m_wallet(527-527)m_wallet(532-532)m_wallet(592-592)
src/wallet/external_signer_scriptpubkeyman.h (5)
src/wallet/scriptpubkeyman.h (6)
wallet(28-241)DescriptorScriptPubKeyMan(512-634)WalletStorage(34-234)bool(179-179)bool(218-218)bool(224-224)src/wallet/walletutil.h (1)
WalletDescriptor(70-103)src/wallet/external_signer_scriptpubkeyman.cpp (8)
SetupDescriptor(17-40)SetupDescriptor(17-17)GetExternalSigner(42-50)GetExternalSigner(42-42)DisplayAddress(52-61)DisplayAddress(52-52)FillPSBT(64-85)FillPSBT(64-64)src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/wallet.cpp (4)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)FillPSBT(2032-2081)FillPSBT(2032-2032)
src/external_signer.cpp (2)
src/external_signer.h (1)
ExternalSigner(18-64)src/util/system.cpp (2)
RunCommandParseJSON(1397-1434)RunCommandParseJSON(1397-1397)
src/wallet/rpc/wallet.cpp (2)
src/wallet/rpc/addresses.cpp (2)
walletdisplayaddress(670-708)walletdisplayaddress(670-670)src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/wallet/wallet.h (6)
src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/wallet.cpp (4)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/external_signer_scriptpubkeyman.cpp (4)
DisplayAddress(52-61)DisplayAddress(52-52)FillPSBT(64-85)FillPSBT(64-64)src/wallet/interfaces.cpp (12)
dest(210-214)dest(210-210)dest(223-226)dest(223-223)dest(227-230)dest(227-227)dest(231-249)dest(231-234)dest(264-268)dest(264-264)dest(269-273)dest(269-269)src/wallet/scriptpubkeyman.cpp (4)
FillPSBT(734-778)FillPSBT(734-734)FillPSBT(2288-2355)FillPSBT(2288-2288)src/psbt.cpp (1)
PartiallySignedTransaction(16-20)
src/wallet/wallet.cpp (4)
src/util/translation.h (1)
Untranslated(48-48)src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/external_signer_scriptpubkeyman.cpp (4)
DisplayAddress(52-61)DisplayAddress(52-52)GetExternalSigner(42-50)GetExternalSigner(42-42)src/wallet/interfaces.cpp (12)
dest(210-214)dest(210-210)dest(223-226)dest(223-223)dest(227-230)dest(227-227)dest(231-249)dest(231-234)dest(264-268)dest(264-264)dest(269-273)dest(269-269)
src/external_signer.h (3)
src/external_signer.cpp (11)
ExternalSigner(17-17)NetworkArg(19-22)NetworkArg(19-19)Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)GetDescriptors(67-70)GetDescriptors(67-67)SignTransaction(72-117)SignTransaction(72-72)src/wallet/wallet.cpp (6)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)SignTransaction(1998-2015)SignTransaction(1998-1998)SignTransaction(2017-2030)SignTransaction(2017-2017)src/wallet/external_signer_scriptpubkeyman.cpp (2)
DisplayAddress(52-61)DisplayAddress(52-52)
src/qt/createwalletdialog.h (2)
src/interfaces/node.h (2)
interfaces(48-411)ExternalSigner(161-168)src/qt/createwalletdialog.cpp (4)
setSigners(115-134)setSigners(115-115)CreateWalletDialog(17-108)CreateWalletDialog(110-113)
🪛 Flake8 (7.3.0)
test/functional/wallet_signer.py
[error] 106-106: continuation line under-indented for visual indent
(E128)
[error] 107-107: closing bracket does not match visual indentation
(E124)
[error] 126-126: continuation line missing indentation or outdented
(E122)
test/functional/rpc_signer.py
[error] 52-52: continuation line under-indented for visual indent
(E128)
[error] 57-57: continuation line under-indented for visual indent
(E128)
[error] 63-63: continuation line under-indented for visual indent
(E128)
[error] 69-69: continuation line under-indented for visual indent
(E128)
test/functional/mocks/signer.py
[error] 21-21: indentation is not a multiple of 4
(E111)
[error] 96-96: indentation is not a multiple of 4
(E111)
🪛 LanguageTool
doc/external-signer.md
[style] ~70-~70: Consider a more concise word here.
Context: ...ue, "txid": } ``` ## Signer API In order to be compatible with Dash Core any signer...
(IN_ORDER_TO_PREMIUM)
[grammar] ~167-~167: Use a hyphen to join words.
Context: ... support address types, in a BIP44/49/84 compatible manner. The `walletdisplayad...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
doc/external-signer.md
14-14: Dollar signs used before commands without showing output
(MD014, commands-show-output)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Dollar signs used before commands without showing output
(MD014, commands-show-output)
48-48: Dollar signs used before commands without showing output
(MD014, commands-show-output)
49-49: Dollar signs used before commands without showing output
(MD014, commands-show-output)
59-59: Dollar signs used before commands without showing output
(MD014, commands-show-output)
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.0)
test/functional/test_framework/test_framework.py
1109-1109: Avoid specifying long messages outside the exception class
(TRY003)
test/functional/mocks/signer.py
20-20: Unused function argument: args
(ARG001)
test/functional/mocks/invalid_signer.py
20-20: Unused function argument: args
(ARG001)
| # -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1" | ||
| # This could be removed once the ABI change warning does not show up by default | ||
| export BITCOIN_CONFIG="--enable-reduce-exports CXXFLAGS=-Wno-psabi --with-boost-process" | ||
| export BITCOIN_CONFIG="--enable-reduce-exports CXXFLAGS=-Wno-psabi" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider the scope of CI changes in this PR.
This file removes the --with-boost-process flag from the build configuration. The coding guidelines state: "Unless specifically prompted, avoid making changes to the ci directory."
Given the PR's scope issues, consider isolating CI configuration changes into a separate, focused PR.
As per coding guidelines.
🤖 Prompt for AI Agents
ci/test/00_setup_env_arm.sh around line 28: this change removed the
--with-boost-process flag from the CI build config (export
BITCOIN_CONFIG="--enable-reduce-exports CXXFLAGS=-Wno-psabi"), which violates
the guideline to avoid unsolicited ci/ changes; either restore the original
BITCOIN_CONFIG to include --with-boost-process and keep CI untouched in this PR,
or revert this file and move the CI configuration change into a separate focused
PR that documents and justifies the change.
| export RUN_BENCH=true | ||
| export GOAL="install" | ||
| export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++ --with-boost-process" | ||
| export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider the scope of CI changes in this PR.
This file removes the --with-boost-process flag from the build configuration. While the change itself is straightforward, the coding guidelines state: "Unless specifically prompted, avoid making changes to the ci directory."
Given the PR's catastrophic validation failures (1,156 files changed, 32,244 insertions, 31,996 deletions), consider whether CI changes should be isolated into a separate, focused PR with clear justification and Bitcoin commit references.
As per coding guidelines.
🤖 Prompt for AI Agents
ci/test/00_setup_env_native_asan.sh lines 15-15: this PR removed the
--with-boost-process build flag from the CI script, which violates the guideline
to avoid changes in the ci directory unless explicitly prompted; restore the
original BITCOIN_CONFIG line (re-adding --with-boost-process) in this PR or else
revert this file here and create a separate, focused PR that only modifies CI
with a clear justification and references to the upstream Bitcoin commits that
mandate the change, and include in that PR a brief rationale and test plan so
reviewers can evaluate CI-only changes independently.
doc/external-signer.md
Outdated
|
|
||
| ## Example usage | ||
|
|
||
| The following example is based on the [HWI](https://github.com/bitcoin-core/HWI) tool. Version 2.0 or newer is required. Although this tool is hosted under the Dash Core GitHub organization and maintained by Dash Core developers, it should be used with caution. It is considered experimental and has far less review than Dash Core itself. Be particularly careful when running tools such as these on a computer with private keys on it. |
There was a problem hiding this comment.
Fix HWI org reference
HWI is hosted under the bitcoin-core organization, not Dash Core. Update the wording to avoid confusion.
🤖 Prompt for AI Agents
In doc/external-signer.md around line 7, the paragraph incorrectly states HWI is
hosted under the Dash Core GitHub organization; change the wording to say HWI is
hosted under the bitcoin-core organization (link remains the same) to avoid
confusion and clarify maintainership.
| #include <wallet/scriptpubkeyman.h> | ||
|
|
||
| #include <memory> | ||
|
|
||
| namespace wallet { | ||
| class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan |
There was a problem hiding this comment.
Missing declaration for ExternalSigner causes compile errors.
ExternalSigner is used in this header but not declared or included. Add a forward declaration (preferred) or include the header to fix ODR-safe compilation.
#include <wallet/scriptpubkeyman.h>
#include <memory>
+// Forward declaration to avoid heavy includes in the header.
+class ExternalSigner;
+
namespace wallet {Also, ensure external_signer_scriptpubkeyman.cpp includes <external_signer.h> (it already does based on the impl).
Also applies to: 28-33
🤖 Prompt for AI Agents
In src/wallet/external_signer_scriptpubkeyman.h around lines 8 to 13 (and
similarly at lines 28 to 33), the class ExternalSigner is referenced but not
declared or included causing compile errors; add a forward declaration "class
ExternalSigner;" near the top of the header (preferably inside namespace wallet)
so the header is ODR-safe, and do NOT include the full external_signer.h here;
additionally ensure src/wallet/external_signer_scriptpubkeyman.cpp includes
<external_signer.h> (the implementation file should already include it) so the
full definition is available where needed.
| // First fill transaction with our data without signing, | ||
| // so external signers are not asked sign more than once. | ||
| bool complete; | ||
| (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true); | ||
| const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false); |
There was a problem hiding this comment.
Initialize ‘complete’ before first FillPSBT and document the ignored error.
Initialize to a known value to avoid any potential read-before-write on error paths inside FillPSBT, and make the intentional error ignore explicit.
- bool complete;
- (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true);
+ bool complete = false;
+ // Intentionally pre-fill without signing so external signers are prompted only once.
+ // Ignore the return value here; the subsequent signing call will surface errors.
+ (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // First fill transaction with our data without signing, | |
| // so external signers are not asked sign more than once. | |
| bool complete; | |
| (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true); | |
| const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false); | |
| // First fill transaction with our data without signing, | |
| // so external signers are not asked sign more than once. | |
| bool complete = false; | |
| // Intentionally pre-fill without signing so external signers are prompted only once. | |
| // Ignore the return value here; the subsequent signing call will surface errors. | |
| (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true); | |
| const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false); |
🤖 Prompt for AI Agents
In src/wallet/rpc/spend.cpp around lines 907 to 911, initialize the local bool
complete to a known value (e.g., bool complete = false;) before the first call
to pwallet->FillPSBT to avoid any potential read-before-write inside FillPSBT,
and replace the current silent cast-to-void with an explicit comment documenting
that the return/error from the first FillPSBT is intentionally ignored (e.g.,
"/* intentionally ignore result; only used to populate PSBT for external signers
*/") so readers and static analyzers understand the intent.
| {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."}, | ||
| }, |
There was a problem hiding this comment.
New createwallet arg external_signer: tighten invariants
Enforce at RPC boundary: when external_signer=true require descriptors=true, disable_private_keys=true, and a non-empty -signer. Otherwise fail fast with a clear error.
Apply this minimal guard before wallet creation:
@@
- if (!request.params[7].isNull() && request.params[7].get_bool()) {
+ if (!request.params[7].isNull() && request.params[7].get_bool()) {
#ifdef ENABLE_EXTERNAL_SIGNER
- flags |= WALLET_FLAG_EXTERNAL_SIGNER;
+ if (!(flags & WALLET_FLAG_DESCRIPTORS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Descriptor support must be enabled when using an external signer");
+ }
+ if (!(flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+ throw JSONRPCError(RPC_WALLET_ERROR, "Private keys must be disabled when using an external signer");
+ }
+ if (gArgs.GetArg("-signer", "").empty()) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "External signer requires -signer to be configured");
+ }
+ flags |= WALLET_FLAG_EXTERNAL_SIGNER;
#else
throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)");
#endif
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/wallet/rpc/wallet.cpp around lines 628-629, enforce at the RPC boundary
that if the new createwallet argument external_signer is true then descriptors
must be true, disable_private_keys must be true, and the configured -signer
string must be non-empty; if any of these invariants do not hold, return an RPC
error immediately with a clear message explaining which requirement failed (e.g.
"external_signer requires descriptors=true, disable_private_keys=true, and
-signer to be set"). Add this minimal guard check immediately before wallet
creation so the RPC fails fast rather than proceeding to wallet creation.
| def perform_pre_checks(): | ||
| mock_result_path = os.path.join(os.getcwd(), "mock_result") | ||
| if(os.path.isfile(mock_result_path)): | ||
| with open(mock_result_path, "r", encoding="utf8") as f: | ||
| mock_result = f.read() | ||
| if mock_result[0]: | ||
| sys.stdout.write(mock_result[2:]) | ||
| sys.exit(int(mock_result[0])) | ||
|
|
There was a problem hiding this comment.
Guard empty mock_result and check exit code explicitly
Indexing mock_result[0] will crash on empty files; also make the exit test explicit.
def perform_pre_checks():
mock_result_path = os.path.join(os.getcwd(), "mock_result")
if(os.path.isfile(mock_result_path)):
with open(mock_result_path, "r", encoding="utf8") as f:
mock_result = f.read()
- if mock_result[0]:
- sys.stdout.write(mock_result[2:])
- sys.exit(int(mock_result[0]))
+ if mock_result:
+ code_char = mock_result[0]
+ if code_char != '0':
+ sys.stdout.write(mock_result[2:])
+ sys.exit(int(code_char))🤖 Prompt for AI Agents
In test/functional/mocks/invalid_signer.py around lines 11 to 19: after reading
mock_result, first guard for an empty file (if not mock_result: return or exit
early) to avoid indexing; then validate the first character is an explicit digit
(use .isdigit()) before converting to int; write the rest of the file to stdout
after consuming that digit (use mock_result[1:] or strip a known separator if
present) and call sys.exit(int(mock_result[0])) so the exit code conversion is
explicit and safe.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/system_tests.cpp (1)
39-46: Fix invalid string literal and JSON quoting on non‑Windows.The literal at Line 41 is unterminated and will not compile. Escape inner quotes.
-#else - const UniValue result = RunCommandParseJSON("echo \"{\"success\": true}\""); -#endif +#else + const UniValue result = RunCommandParseJSON("echo \"{\\\"success\\\": true}\""); +#endif
♻️ Duplicate comments (23)
src/Makefile.test.include (1)
42-49: Unconditional wallet linkage breaks --disable-wallet builds.Linking $(LIBBITCOIN_WALLET) in FUZZ_SUITE_LD_COMMON unconditionally causes linker errors when wallet is disabled. Move it under ENABLE_WALLET.
@@ FUZZ_SUITE_LD_COMMON = \ $(LIBTEST_FUZZ) \ $(LIBTEST_UTIL) \ $(LIBBITCOIN_NODE) \ - $(LIBBITCOIN_WALLET) \ $(LIBBITCOIN_COMMON) \ $(LIBBITCOIN_UTIL) \ @@ if ENABLE_WALLET @@ -FUZZ_SUITE_LD_COMMON +=\ +FUZZ_SUITE_LD_COMMON += \ + $(LIBBITCOIN_WALLET) \ $(SQLITE_LIBS) \ $(BDB_LIBS)Also applies to: 223-226
src/wallet/rpc/spend.cpp (1)
907-911: Initialize ‘complete’ before first FillPSBT.Avoid potential read-before-write on error paths; keep the explanatory comment.
- // First fill transaction with our data without signing, - // so external signers are not asked sign more than once. - bool complete; - (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true); + // First fill transaction with our data without signing, + // so external signers are not asked sign more than once. + bool complete = false; + (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true);configure.ac (1)
327-331: Make external‑signer truly ‘auto’ and degrade gracefully when Boost.Process is absent.Default is currently “yes” (contradicts help) and forces hard failure without Boost.Process. Switch to auto-detect and only error if explicitly requested. Also drop unnecessary AC_SUBST.
@@ -AC_ARG_ENABLE([external-signer], - [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is auto, requires Boost::Process)])], - [use_external_signer=$enableval], - [use_external_signer=yes]) +AC_ARG_ENABLE([external-signer], + [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default: auto; requires Boost::Process)])], + [use_external_signer=$enableval], + [use_external_signer=auto]) @@ - dnl Opt-in to Boost Process if external signer support is requested - if test "$use_external_signer" != no; then - AC_MSG_CHECKING(for Boost Process) - AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <boost/process.hpp>]], - [[ boost::process::child* child = new boost::process::child; delete child; ]])], - [ AC_MSG_RESULT(yes) - AC_DEFINE([ENABLE_EXTERNAL_SIGNER],,[define if external signer support is enabled]) - ], - [ AC_MSG_ERROR([Boost::Process is required for external signer support, but not available!])] - ) - fi + dnl Probe Boost.Process when enabled explicitly or in auto mode + if test "$use_external_signer" = yes || test "$use_external_signer" = auto; then + AC_MSG_CHECKING([for Boost.Process]) + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <boost/process.hpp>]], + [[ boost::process::child c((char*)""); ]])], + [ AC_MSG_RESULT([yes]) + use_external_signer=yes + AC_DEFINE([ENABLE_EXTERNAL_SIGNER],[1],[Define if external signer support is enabled]) ], + [ AC_MSG_RESULT([no]) + if test "$use_external_signer" = yes; then + AC_MSG_ERROR([--enable-external-signer requires Boost.Process]) + else + use_external_signer=no + fi ]) + fi @@ -AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = "yes"]) +AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = yes]) @@ -AC_SUBST(ENABLE_EXTERNAL_SIGNER) +# not needed; use AM_CONDITIONAL and C/C++ macroAlso applies to: 1556-1586, 1591-1592, 1978-1979
ci/test/00_setup_env_native_asan.sh (1)
15-15: Consider isolating CI configuration changes.The coding guidelines state: "Unless specifically prompted, avoid making changes to the ci directory." Given the PR's substantial scope, CI configuration changes may be better suited for a separate, focused PR.
As per coding guidelines.
ci/test/00_setup_env_native_nowallet.sh (1)
14-14: Consider isolating CI configuration changes.The coding guidelines state: "Unless specifically prompted, avoid making changes to the ci directory." Given the PR's substantial scope, CI configuration changes may be better suited for a separate, focused PR.
As per coding guidelines.
src/qt/receiverequestdialog.cpp (1)
99-101: Fix signal connection leak in setInfo.The
connect()call creates a new signal-slot connection each timesetInfo()is invoked. If called multiple times, clicking the Verify button will triggerdisplayAddress()multiple times.Apply one of these fixes:
Option 1 (preferred): Move connection to constructor
Move the signal connection to the constructor so it's only connected once:ReceiveRequestDialog::ReceiveRequestDialog(QWidget *parent) : QDialog(parent, GUIUtil::dialog_flags), ui(new Ui::ReceiveRequestDialog), model(nullptr) { ui->setupUi(this); GUIUtil::updateFonts(); GUIUtil::handleCloseWindowShortcut(this); + + connect(ui->btnVerify, &QPushButton::clicked, this, [this] { + if (model) { + model->displayAddress(info.address.toStdString()); + } + }); }Then remove the connect call from
setInfo():ui->btnVerify->setVisible(model->wallet().hasExternalSigner()); - - connect(ui->btnVerify, &QPushButton::clicked, [this] { - model->displayAddress(info.address.toStdString()); - }); }Option 2: Use Qt::UniqueConnection flag
- connect(ui->btnVerify, &QPushButton::clicked, [this] { + connect(ui->btnVerify, &QPushButton::clicked, this, [this] { model->displayAddress(info.address.toStdString()); - }); + }, Qt::UniqueConnection);src/wallet/rpc/addresses.cpp (1)
669-710: RPC handler implementation is correct, but underlying DisplayAddress has known issue.The RPC endpoint properly validates the address and checks the return value from
DisplayAddress. However, as noted in previous reviews, the underlying implementation insrc/wallet/external_signer_scriptpubkeyman.cppdoes not inspect the result ofsigner.DisplayAddress()and unconditionally returnstrue, which could mask hardware device failures.doc/external-signer.md (1)
7-7: Fix HWI org and wording.HWI is under the bitcoin-core org, not Dash. Also reword to avoid implying Dash maintainership.
-The following example is based on the [HWI](https://github.com/dashpay/HWI) tool. Version 2.0 or newer is required. Although this tool is hosted under the Dash Core GitHub organization and maintained by Dash Core developers, it should be used with caution. +The following example is based on the [HWI](https://github.com/bitcoin-core/HWI) tool. Version 2.0 or newer is required. Although this tool is hosted under the bitcoin-core GitHub organization, it should be used with caution.src/wallet/external_signer_scriptpubkeyman.cpp (1)
52-61: Guard null provider and handle DisplayAddress errors.Current code may dereference a null provider and ignores failures from the external command.
bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const { // TODO: avoid the need to infer a descriptor from inside a descriptor wallet - auto provider = GetSolvingProvider(scriptPubKey); - auto descriptor = InferDescriptor(scriptPubKey, *provider); - - signer.DisplayAddress(descriptor->ToString()); - // TODO inspect result - return true; + auto provider = GetSolvingProvider(scriptPubKey); + if (!provider) { + return false; + } + auto descriptor = InferDescriptor(scriptPubKey, *provider); + if (!descriptor) { + return false; + } + try { + (void)signer.DisplayAddress(descriptor->ToString()); + return true; + } catch (const std::exception&) { + return false; + } }test/functional/mocks/invalid_signer.py (1)
11-19: Harden mock pre-checks, fix duplicate var, and require --account
- Guard empty mock_result, validate exit digit, and only treat nonzero as error.
- Remove duplicate xpub assignment typo.
- Make --account required to avoid None concatenation errors.
- Silence ARG001 by marking enumerate’s arg unused.
def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") if(os.path.isfile(mock_result_path)): with open(mock_result_path, "r", encoding="utf8") as f: mock_result = f.read() - if mock_result[0]: - sys.stdout.write(mock_result[2:]) - sys.exit(int(mock_result[0])) + if mock_result: + code_char = mock_result[0] + payload = mock_result[1:] if len(mock_result) > 1 else "" + if code_char.isdigit(): + code = int(code_char) + if code != 0: + sys.stdout.write(payload.lstrip()) + sys.exit(code) - xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6" + # legacy pkh xpub (unused here, kept for reference if needed) + # xpub_pkh_legacy = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6" xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN" xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp" -parser_enumerate = subparsers.add_parser('enumerate', help='list available signers') -parser_enumerate.set_defaults(func=enumerate) +parser_enumerate = subparsers.add_parser('enumerate', help='list available signers') +parser_enumerate.set_defaults(func=enumerate) parser_getdescriptors = subparsers.add_parser('getdescriptors') parser_getdescriptors.set_defaults(func=getdescriptors) -parser_getdescriptors.add_argument('--account', metavar='account') +parser_getdescriptors.add_argument('--account', metavar='account', required=True) -def enumerate(args): +def enumerate(_args): sys.stdout.write(json.dumps([{"fingerprint": "b3c19bfc", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}]))Also applies to: 24-27, 52-55
src/qt/sendcoinsdialog.cpp (3)
901-913: Apply same watch‑only fix in updateCoinControlState()Keep behavior consistent across code paths.
- // Include watch-only for wallets without private key - m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); + // Include watch-only for wallets without private key or with external signer + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner();
769-773: Assign external balance when showing “External balance”Balance remains 0 in this branch. Use watch‑only balance for external-signer wallets.
- if (model->wallet().hasExternalSigner()) { - ui->labelBalanceName->setText(tr("External balance:")); + if (model->wallet().hasExternalSigner()) { + balance = balances.watch_only_balance; + ui->labelBalanceName->setText(tr("External balance:"));
855-859: Allow watch‑only for external signer walletsCoin control must include watch‑only UTXOs for external-signer wallets.
- m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner();src/node/interfaces.cpp (1)
530-551: Harden listExternalSigners: catch enumerate errors; fix typos; minor cleanupsWrap Enumerate() to avoid crashing GUI/RPC on malformed signer output; log and return empty. Also fix comment typos and use .empty().
#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) { + LogPrintf("External signer enumerate failed: %s\n", e.what()); + return {}; + } @@ #else - // This result is undistinguisable from a succesful call that returns + // This result is indistinguishable from a successful call that returns // no signers. For the current GUI this doesn't matter, because the wallet // creation dialog disables the external signer checkbox in both // cases. The return type could be changed to std::optional<std::vector> // (or something that also includes error messages) if this distinction // becomes important. return {}; #endif // ENABLE_EXTERNAL_SIGNERsrc/external_signer.h (3)
54-59: Make GetDescriptors constIt doesn’t mutate state; mark const to improve API clarity.
- UniValue GetDescriptors(const int account); + UniValue GetDescriptors(int account) const;
60-64: Make SignTransaction constMethod doesn’t modify members; declare const.
- bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error); + bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error) const;
42-53: Safer command construction (argv, not shell strings)Public API embeds user-derived strings (descriptor, fingerprint) into shell command lines downstream. Switch RunCommandParseJSON to an argv-based exec (no shell), or add a robust escaping layer before concatenation. This prevents injection if unexpected characters appear.
src/external_signer.cpp (5)
19-22: Quote the chain flag to avoid shell splittingWrap chain value in quotes when building the argument.
- return " --chain " + m_chain; + return " --chain \"" + m_chain + "\"";
47-51: Don’t abort enumeration on a duplicateUse continue, not break, to keep processing remaining signers. Also avoid shadowing the
signername in the inner loop.- bool duplicate = false; - for (const ExternalSigner& signer : signers) { - if (signer.m_fingerprint.compare(fingerprintStr) == 0) duplicate = true; - } - if (duplicate) break; + bool duplicate = false; + for (const ExternalSigner& existing : signers) { + if (existing.m_fingerprint == fingerprintStr) { duplicate = true; break; } + } + if (duplicate) continue;
62-65: Command construction uses raw concatenation (injection risk)
descriptorandm_fingerprintare interpolated into a shell string executed by RunCommandParseJSON. Migrate to an argv-basedRunCommandParseJSONoverload (no shell), or strictly escape inputs. Short-term, ensure inputs are validated/escaped.
67-70: Same injection concern in GetDescriptorsAvoid embedding
m_fingerprintdirectly into a shell string; use argv-based exec or strict escaping.
72-96: Same injection concern in SignTransaction command
m_fingerprintis embedded in the executed command. Use argv-based exec; keep PSBT only on stdin. Also consider quoting/escaping until argv refactor is done.test/functional/wallet_signer.py (1)
119-132: Fix flake8 indentation and whitespace in list literalThe suggested indentation changes address E122, but the diff is incomplete—it omits the E231 whitespace fix required at line 122. The list
[0,1]must be updated to[0, 1].- result = mock_wallet.importdescriptors([{ + result = mock_wallet.importdescriptors([ + { "desc": "pkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#6k3x80k9", "timestamp": 0, - "range": [0,1], + "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 - }]) + } + ])
🧹 Nitpick comments (15)
src/util/system.cpp (1)
1397-1434: More robust stdout/stderr reads to handle multi-line JSON.Current code reads only one line from each stream; prefer consuming entire streams to avoid truncation. Safe as a non-functional improvement.
- std::string result; - std::string error; - std::getline(stdout_stream, result); - std::getline(stderr_stream, error); + std::string result{std::istreambuf_iterator<char>(stdout_stream), std::istreambuf_iterator<char>()}; + std::string error{std::istreambuf_iterator<char>(stderr_stream), std::istreambuf_iterator<char>()};ci/test/00_setup_env_win64.sh (1)
19-19: Consider isolating CI configuration changes.The coding guidelines state: "Unless specifically prompted, avoid making changes to the ci directory." Given the PR's substantial scope (1,156 files changed), CI configuration changes may be better suited for a separate, focused PR with clear upstream Bitcoin commit references.
As per coding guidelines.
doc/external-signer.md (1)
141-145: Add languages to code fences and drop leading “$” prompts.This satisfies MD040/MD014 and improves readability.
-```sh -$ dashd -signer=../HWI/hwi.py +```sh +dashd -signer=../HWI/hwi.py-``` -$ dash-cli enumeratesigners +```sh +dash-cli enumeratesigners { "signers": [ { "fingerprint": "c8df832a" } ] } -``` +```-```sh -$ dash-cli createwallet "hww" true true "" true true true +```sh +dash-cli createwallet "hww" true true "" true true true-```sh -$ dash-cli -rpcwallet=<wallet> getnewaddress -$ dash-cli -rpcwallet=<wallet> walletdisplayaddress <address> +```sh +dash-cli -rpcwallet=<wallet> getnewaddress +dash-cli -rpcwallet=<wallet> walletdisplayaddress <address>-```sh -$ dash-cli -rpcwallet=<wallet> sendtoaddress <address> <amount> +```sh +dash-cli -rpcwallet=<wallet> sendtoaddress <address> <amount>-``` +```json {"complete": true, "txid": <txid>}Also applies to: 147-149, 13-15, 25-33, 59-61, 80-87, 98-101, 115-118, 122-136 </blockquote></details> <details> <summary>src/interfaces/node.h (1)</summary><blockquote> `166-168`: **Make ExternalSigner::getName() const noexcept.** This improves API usability and signals non-throwing behavior. ```diff - virtual std::string getName() = 0; + virtual std::string getName() const noexcept = 0;src/Makefile.am (1)
910-929: Gate external signer sources behind ENABLE_EXTERNAL_SIGNER.To keep build lean when the feature is disabled, compile these only if enabled.
- external_signer.cpp \ +if ENABLE_EXTERNAL_SIGNER + external_signer.cpp \ @@ - rpc/external_signer.cpp \ + rpc/external_signer.cpp \ +endif- wallet/external_signer_scriptpubkeyman.cpp \ +if ENABLE_EXTERNAL_SIGNER + wallet/external_signer_scriptpubkeyman.cpp \ +endifHeaders may remain listed in BITCOIN_CORE_H as they don’t affect linking.
Please ensure configure.ac defines AM_CONDITIONAL(ENABLE_EXTERNAL_SIGNER, ...).
Also applies to: 650-651, 224-224
test/functional/mocks/signer.py (2)
11-19: Harden perform_pre_checks() against empty/short files.def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") - if(os.path.isfile(mock_result_path)): - with open(mock_result_path, "r", encoding="utf8") as f: - mock_result = f.read() - if mock_result[0]: - sys.stdout.write(mock_result[2:]) - sys.exit(int(mock_result[0])) + if os.path.isfile(mock_result_path): + with open(mock_result_path, "r", encoding="utf8") as f: + first = f.readline().strip() + payload = f.read() + try: + code = int(first) + except Exception: + code = 1 + if payload: + sys.stdout.write(payload) + sys.exit(code)
69-76: Optional: silence unused-arg warning in enumerate().-parser_enumerate.set_defaults(func=enumerate) +parser_enumerate.set_defaults(func=lambda a: enumerate(a))Or prefix arg with “_args” to signal intentional unused.
src/wallet/external_signer_scriptpubkeyman.cpp (1)
42-50: Consider caching the selected signer.GetExternalSigner() enumerates on each call; caching the chosen device (fingerprint) would reduce latency and avoid device churn between calls.
Also applies to: 78-85
src/wallet/wallet.h (2)
30-31: Avoid heavy include from a public headerwallet.h doesn’t need <external_signer.h>; implementation includes it. Dropping it reduces compile times and coupling.
-#include <external_signer.h>
566-568: Make DisplayAddress const (pure query)The method doesn’t mutate state; making it const expands usability and matches other query methods.
- bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool DisplayAddress(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);Please confirm no callers rely on non-const; adjust the definition in wallet.cpp accordingly.
src/wallet/wallet.cpp (2)
2505-2518: Harden DisplayAddress against unexpected states and exceptionsAdd an early flag check and catch runtime errors from signer discovery/invocation to avoid unexpected crashes; return false and log instead.
bool CWallet::DisplayAddress(const CTxDestination& dest) { CScript scriptPubKey = GetScriptForDestination(dest); const auto spk_man = GetScriptPubKeyMan(scriptPubKey); if (spk_man == nullptr) { return false; } auto signer_spk_man = dynamic_cast<ExternalSignerScriptPubKeyMan*>(spk_man); if (signer_spk_man == nullptr) { return false; } - ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(scriptPubKey, signer); + if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) return false; + try { + ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); + return signer_spk_man->DisplayAddress(scriptPubKey, signer); + } catch (const std::exception& e) { + WalletLogPrintf("DisplayAddress failed: %s\n", e.what()); + return false; + } }
3852-3880: Descriptor parsing from external source: consider requiring checksumsParsers currently allow descriptors without checksum (last arg false). Given input is external, prefer strictness to avoid subtle mismatches.
- std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, desc_error, false); + std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, desc_error, /*require_checksum=*/true);If mocks don’t include checksums yet, gate this under a flag or update tests accordingly.
src/external_signer.h (1)
27-28: Minor: drop const on value return typeReturning
const std::stringadds no benefit and inhibits moves. Preferstd::string NetworkArg() const;.- const std::string NetworkArg() const; + std::string NetworkArg() const;test/functional/wallet_signer.py (1)
103-108: Fix flake8 indentation for multiline callAlign continuation lines and closing paren.
- assert_raises_rpc_error(-1, 'RunCommandParseJSON error', - hww.walletdisplayaddress, address1 - ) + assert_raises_rpc_error( + -1, + 'RunCommandParseJSON error', + hww.walletdisplayaddress, + address1, + )src/qt/sendcoinsdialog.cpp (1)
229-240: UX copy: refine tooltip vs enabled stateIf a signer path is configured, enabling the button while showing “Connect your hardware wallet first.” is contradictory. Consider enabling with a “Ready to sign on device” tooltip; when unset, disable with the current “Set external signer…” message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_mac_native_x86_64.sh(1 hunks)ci/test/00_setup_env_native_asan.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(1 hunks)ci/test/00_setup_env_native_qt5.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(7 hunks)doc/Doxyfile.in(1 hunks)doc/external-signer.md(1 hunks)src/Makefile.am(5 hunks)src/Makefile.test.include(1 hunks)src/dummywallet.cpp(1 hunks)src/external_signer.cpp(1 hunks)src/external_signer.h(1 hunks)src/interfaces/node.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/interfaces.cpp(3 hunks)src/qt/createwalletdialog.cpp(4 hunks)src/qt/createwalletdialog.h(2 hunks)src/qt/forms/createwalletdialog.ui(2 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/forms/receiverequestdialog.ui(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(7 hunks)src/qt/walletcontroller.cpp(4 hunks)src/qt/walletmodel.cpp(2 hunks)src/qt/walletmodel.h(1 hunks)src/qt/walletmodeltransaction.cpp(1 hunks)src/qt/walletmodeltransaction.h(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/external_signer.cpp(1 hunks)src/rpc/register.h(2 hunks)src/test/system_tests.cpp(2 hunks)src/util/error.cpp(1 hunks)src/util/error.h(1 hunks)src/util/system.cpp(3 hunks)src/util/system.h(0 hunks)src/wallet/external_signer_scriptpubkeyman.cpp(1 hunks)src/wallet/external_signer_scriptpubkeyman.h(1 hunks)src/wallet/init.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/rpc/wallet.cpp(6 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(9 hunks)src/wallet/wallet.h(5 hunks)src/wallet/walletdb.cpp(1 hunks)src/wallet/walletdb.h(1 hunks)src/wallet/walletutil.h(1 hunks)test/config.ini.in(1 hunks)test/functional/mocks/invalid_signer.py(1 hunks)test/functional/mocks/signer.py(1 hunks)test/functional/rpc_help.py(1 hunks)test/functional/rpc_signer.py(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)test/functional/test_framework/test_node.py(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_signer.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/util/system.h
🚧 Files skipped from review as they are similar to previous changes (27)
- src/wallet/init.cpp
- src/rpc/external_signer.cpp
- src/qt/createwalletdialog.cpp
- src/dummywallet.cpp
- src/wallet/rpc/backup.cpp
- src/qt/optionsmodel.cpp
- src/wallet/walletutil.h
- ci/test/00_setup_env_arm.sh
- ci/test/00_setup_env_native_tsan.sh
- src/rpc/register.h
- src/rpc/client.cpp
- doc/Doxyfile.in
- ci/test/00_setup_env_native_qt5.sh
- src/qt/forms/receiverequestdialog.ui
- src/wallet/walletdb.h
- src/wallet/walletdb.cpp
- src/qt/walletmodeltransaction.cpp
- ci/test/00_setup_env_s390x.sh
- test/functional/rpc_help.py
- src/qt/forms/optionsdialog.ui
- test/functional/test_runner.py
- src/qt/createwalletdialog.h
- test/functional/test_framework/test_node.py
- src/qt/walletmodel.h
- src/wallet/scriptpubkeyman.h
- src/wallet/rpc/wallet.cpp
- src/wallet/interfaces.cpp
🧰 Additional context used
📓 Path-based instructions (5)
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_native_nowallet.shci/test/00_setup_env_mac.shci/test/00_setup_env_native_multiprocess.shci/test/00_setup_env_mac_native_x86_64.shci/test/00_setup_env_native_asan.shci/test/00_setup_env_win64.shci/test/00_setup_env_native_fuzz.sh
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/interfaces/node.hsrc/interfaces/wallet.hsrc/util/error.hsrc/qt/walletmodel.cppsrc/wallet/wallet.hsrc/external_signer.cppsrc/qt/walletcontroller.cppsrc/util/system.cppsrc/wallet/rpc/spend.cppsrc/util/error.cppsrc/qt/receiverequestdialog.cppsrc/wallet/external_signer_scriptpubkeyman.cppsrc/qt/optionsdialog.cppsrc/external_signer.hsrc/wallet/wallet.cppsrc/qt/sendcoinsdialog.cppsrc/qt/walletmodeltransaction.hsrc/wallet/external_signer_scriptpubkeyman.hsrc/qt/optionsmodel.hsrc/wallet/rpc/addresses.cppsrc/test/system_tests.cppsrc/node/interfaces.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/mocks/invalid_signer.pytest/functional/wallet_signer.pytest/functional/mocks/signer.pytest/functional/rpc_signer.pytest/functional/test_framework/test_framework.py
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/system_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/external-signer.md
🧬 Code graph analysis (18)
src/interfaces/node.h (2)
src/external_signer.cpp (1)
ExternalSigner(17-17)src/external_signer.h (1)
ExternalSigner(18-64)
src/interfaces/wallet.h (2)
src/qt/walletmodel.cpp (2)
displayAddress(582-592)displayAddress(582-582)src/wallet/interfaces.cpp (12)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/qt/walletmodel.cpp (1)
src/wallet/interfaces.cpp (28)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)m_wallet(157-160)m_wallet(165-165)m_wallet(174-174)m_wallet(175-175)m_wallet(181-181)m_wallet(182-182)m_wallet(442-445)m_wallet(446-449)m_wallet(521-521)m_wallet(522-522)m_wallet(523-523)m_wallet(524-524)m_wallet(525-525)m_wallet(526-526)m_wallet(531-531)m_wallet(591-591)
src/wallet/wallet.h (5)
src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/wallet.cpp (4)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/external_signer_scriptpubkeyman.cpp (4)
DisplayAddress(52-61)DisplayAddress(52-52)FillPSBT(64-85)FillPSBT(64-64)src/wallet/interfaces.cpp (12)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)src/wallet/scriptpubkeyman.cpp (4)
FillPSBT(734-778)FillPSBT(734-734)FillPSBT(2288-2355)FillPSBT(2288-2288)
src/external_signer.cpp (3)
src/external_signer.h (1)
ExternalSigner(18-64)src/util/system.cpp (2)
RunCommandParseJSON(1397-1434)RunCommandParseJSON(1397-1397)src/wallet/external_signer_scriptpubkeyman.cpp (2)
DisplayAddress(52-61)DisplayAddress(52-52)
src/util/system.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/util/error.cpp (1)
src/util/translation.h (1)
Untranslated(48-48)
src/wallet/external_signer_scriptpubkeyman.cpp (3)
src/wallet/wallet.cpp (24)
batch(550-550)batch(815-815)batch(938-938)batch(1178-1178)batch(1257-1257)batch(1313-1313)batch(1358-1358)batch(1370-1370)batch(1391-1391)batch(1515-1515)batch(1660-1660)batch(1744-1744)batch(1793-1793)batch(2239-2239)batch(2300-2300)batch(2307-2307)DisplayAddress(2505-2518)DisplayAddress(2505-2505)GetSolvingProvider(3734-3738)GetSolvingProvider(3734-3734)GetSolvingProvider(3740-3748)GetSolvingProvider(3740-3740)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/scriptpubkeyman.cpp (16)
GetID(801-804)GetID(801-801)GetID(2375-2379)GetID(2375-2375)TopUp(1410-1413)TopUp(1410-1410)TopUp(1943-2004)TopUp(1943-1943)GetSolvingProvider(678-681)GetSolvingProvider(678-678)GetSolvingProvider(2231-2234)GetSolvingProvider(2231-2231)FillPSBT(734-778)FillPSBT(734-734)FillPSBT(2288-2355)FillPSBT(2288-2288)src/external_signer.cpp (4)
Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)
test/functional/wallet_signer.py (6)
test/functional/test_framework/test_framework.py (6)
skip_test_if_missing_module(423-425)skip_test_if_missing_module(1477-1478)skip_if_no_external_signer(1106-1109)skip_if_no_wallet(1063-1071)restart_node(694-697)is_bdb_compiled(1147-1149)test/functional/rpc_signer.py (4)
mock_signer_path(21-26)skip_test_if_missing_module(38-39)set_mock_result(41-43)clear_mock_result(45-46)src/wallet/rpc/wallet.cpp (16)
res(591-591)createwallet(615-713)createwallet(615-615)getwalletinfo(152-276)getwalletinfo(152-152)setwalletflag(547-613)setwalletflag(547-547)getnewaddress(1098-1098)getaddressinfo(1097-1097)result(308-308)result(766-766)result(1088-1088)walletdisplayaddress(1108-1108)walletcreatefundedpsbt(1151-1151)walletprocesspsbt(1150-1150)send(1149-1149)test/functional/test_framework/test_node.py (2)
createwallet(884-889)get_wallet_rpc(363-369)src/wallet/rpc/addresses.cpp (10)
getnewaddress(17-56)getnewaddress(17-17)getaddressinfo(418-559)getaddressinfo(418-418)result(254-254)result(703-703)walletdisplayaddress(670-708)walletdisplayaddress(670-670)dest(382-382)dest(382-382)src/wallet/rpc/spend.cpp (11)
result(631-631)result(725-725)result(919-919)result(1014-1014)result(1164-1164)walletcreatefundedpsbt(1025-1171)walletcreatefundedpsbt(1025-1025)walletprocesspsbt(946-1023)walletprocesspsbt(946-946)send(732-944)send(732-732)
src/qt/optionsdialog.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/external_signer.h (3)
src/external_signer.cpp (11)
ExternalSigner(17-17)NetworkArg(19-22)NetworkArg(19-19)Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)GetDescriptors(67-70)GetDescriptors(67-67)SignTransaction(72-117)SignTransaction(72-72)src/wallet/wallet.cpp (6)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)SignTransaction(1998-2015)SignTransaction(1998-1998)SignTransaction(2017-2030)SignTransaction(2017-2017)src/wallet/external_signer_scriptpubkeyman.cpp (2)
DisplayAddress(52-61)DisplayAddress(52-52)
src/wallet/wallet.cpp (4)
src/util/translation.h (1)
Untranslated(48-48)src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/external_signer_scriptpubkeyman.cpp (4)
DisplayAddress(52-61)DisplayAddress(52-52)GetExternalSigner(42-50)GetExternalSigner(42-42)src/wallet/interfaces.cpp (12)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/qt/walletmodeltransaction.h (1)
src/qt/walletmodeltransaction.cpp (2)
setWtx(29-32)setWtx(29-29)
src/wallet/external_signer_scriptpubkeyman.h (4)
src/wallet/scriptpubkeyman.h (6)
wallet(28-241)DescriptorScriptPubKeyMan(512-634)WalletStorage(34-234)bool(179-179)bool(218-218)bool(224-224)src/wallet/walletutil.h (1)
WalletDescriptor(70-103)src/wallet/external_signer_scriptpubkeyman.cpp (8)
SetupDescriptor(17-40)SetupDescriptor(17-17)GetExternalSigner(42-50)GetExternalSigner(42-42)DisplayAddress(52-61)DisplayAddress(52-52)FillPSBT(64-85)FillPSBT(64-64)src/wallet/wallet.cpp (4)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)FillPSBT(2032-2081)FillPSBT(2032-2032)
test/functional/rpc_signer.py (4)
test/functional/test_framework/test_framework.py (1)
skip_if_no_external_signer(1106-1109)test/functional/test_framework/util.py (2)
assert_equal(69-74)assert_raises_rpc_error(132-148)test/functional/wallet_signer.py (5)
mock_signer_path(21-26)set_test_params(35-41)skip_test_if_missing_module(43-45)set_mock_result(47-49)clear_mock_result(51-52)src/rpc/external_signer.cpp (3)
enumeratesigners(16-59)enumeratesigners(16-16)result(54-54)
src/wallet/rpc/addresses.cpp (2)
src/wallet/rpc/util.cpp (2)
GetWalletForJSONRPCRequest(55-77)GetWalletForJSONRPCRequest(55-55)src/wallet/interfaces.cpp (18)
LOCK(249-258)LOCK(259-262)LOCK(290-294)LOCK(295-299)LOCK(352-360)LOCK(478-490)dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/test/system_tests.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/node/interfaces.cpp (1)
src/external_signer.cpp (2)
Enumerate(24-60)Enumerate(24-24)
🪛 Flake8 (7.3.0)
test/functional/wallet_signer.py
[error] 106-106: continuation line under-indented for visual indent
(E128)
[error] 107-107: closing bracket does not match visual indentation
(E124)
[error] 126-126: continuation line missing indentation or outdented
(E122)
test/functional/mocks/signer.py
[error] 21-21: indentation is not a multiple of 4
(E111)
[error] 96-96: indentation is not a multiple of 4
(E111)
test/functional/rpc_signer.py
[error] 52-52: continuation line under-indented for visual indent
(E128)
[error] 57-57: continuation line under-indented for visual indent
(E128)
[error] 63-63: continuation line under-indented for visual indent
(E128)
[error] 69-69: continuation line under-indented for visual indent
(E128)
🪛 LanguageTool
doc/external-signer.md
[style] ~70-~70: Consider a more concise word here.
Context: ...ue, "txid": } ``` ## Signer API In order to be compatible with Dash Core any signer...
(IN_ORDER_TO_PREMIUM)
[grammar] ~167-~167: Use a hyphen to join words.
Context: ... support address types, in a BIP44/49/84 compatible manner. The `walletdisplayad...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
doc/external-signer.md
14-14: Dollar signs used before commands without showing output
(MD014, commands-show-output)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Dollar signs used before commands without showing output
(MD014, commands-show-output)
48-48: Dollar signs used before commands without showing output
(MD014, commands-show-output)
49-49: Dollar signs used before commands without showing output
(MD014, commands-show-output)
59-59: Dollar signs used before commands without showing output
(MD014, commands-show-output)
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
test/functional/mocks/invalid_signer.py
20-20: Unused function argument: args
(ARG001)
test/functional/mocks/signer.py
20-20: Unused function argument: args
(ARG001)
test/functional/test_framework/test_framework.py
1109-1109: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (23)
src/qt/optionsmodel.h (1)
76-76: LGTM!The addition of
ExternalSignerPathto theOptionIDenum is consistent with the external signer feature integration and properly typed as QString.src/util/error.h (1)
33-34: LGTM!The addition of
EXTERNAL_SIGNER_NOT_FOUNDandEXTERNAL_SIGNER_FAILEDto theTransactionErrorenum appropriately extends error handling for the external signer feature.test/config.ini.in (1)
26-26: LGTM!The addition of
ENABLE_EXTERNAL_SIGNERto the components section follows the established pattern for feature flags and appropriately enables external signer support in the test environment.src/qt/walletmodeltransaction.h (1)
30-31: LGTM!The addition of the
setWtx()method complements the existinggetWtx()accessor and follows standard setter patterns. The implementation is straightforward and correct.src/qt/forms/createwalletdialog.ui (1)
112-121: LGTM!The external signer checkbox is properly integrated into the Advanced Options section with clear tooltip guidance for users.
src/util/error.cpp (1)
36-39: LGTM!The new external signer error strings are clear and follow the established pattern in this function.
ci/test/00_setup_env_mac_native_x86_64.sh (1)
13-13: LGTM!Removal of
--with-boost-processaligns with the architectural shift to external signer support.ci/test/00_setup_env_mac.sh (1)
17-17: LGTM!Consistent with the removal of Boost.Process dependency across CI configurations.
src/interfaces/wallet.h (1)
154-155: LGTM!The new interface methods are well-positioned and follow the established pattern for wallet interface extensions. The placement near related functionality (address operations and key management) enhances code organization.
Also applies to: 294-295
test/functional/test_framework/test_framework.py (1)
1106-1117: LGTM!The new test framework methods follow the established pattern for feature detection and test skipping. The static analysis hint about message length is a false positive—the message is appropriately concise for a SkipTest exception.
src/qt/walletmodel.cpp (1)
582-592: LGTM!The
displayAddressimplementation properly handles errors with appropriate user feedback via QMessageBox. The try-catch block ensures graceful handling of runtime errors from the underlying wallet interface.doc/external-signer.md (2)
66-67: Double-check sendtoaddress output shape.Example shows {"complete": true, "txid": ...}. dash-cli sendtoaddress typically returns a txid string. Please confirm actual output and adjust the example accordingly.
27-36: Fix descriptor types in getdescriptors example.Wrapped SegWit must be sh(wpkh(...)), and native SegWit must be wpkh(...). Current text shows pkh in those slots.
- "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", - "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", - "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" + "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", + "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", + "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" @@ - "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", - "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", - "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc" + "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", + "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", + "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc"Likely an incorrect or invalid review comment.
src/interfaces/node.h (1)
234-236: API surface change: ensure all Node impls override listExternalSigners().Header adds a new pure virtual. Confirm all Node implementations are updated (node/interfaces.cpp shows an override).
src/qt/optionsdialog.cpp (1)
141-145: UI wiring looks good; verify OptionsModel plumbed.
- Gating and tooltip under !ENABLE_EXTERNAL_SIGNER are correct.
- Restart warning hook on externalSignerPath is consistent.
- Data mapper adds ExternalSignerPath.
Please confirm OptionsModel::ExternalSignerPath exists, persists, and is included in save/load.
Also applies to: 277-277, 344-345
ci/test/00_setup_env_native_fuzz.sh (1)
18-18: Confirm intent after dropping Boost.Process in fuzz config.Ensure this job doesn’t rely on Boost.Process-dependent bits and that sanitizer/fuzz targets compile without external signer support.
ci/test/00_setup_env_native_multiprocess.sh (1)
16-16: Verify external signer build behavior or explicitly disable it.The analysis confirms your concern is valid. When
--with-boost-processis removed:
- External signer defaults to enabled (line 330 of configure.ac:
use_external_signer=yes)- Build will check for
boost/process.hpp(line 1579:AC_COMPILE_IFELSEtest)- Build fails if Boost::Process is unavailable (line 1584:
AC_MSG_ERRORif missing)The CI job will either:
- Succeed if the CI environment has Boost::Process headers available, OR
- Fail at configure time if not
To clarify intent, either:
- Confirm the CI environment has
boost/process.hppand document this dependency, OR- Add
--disable-external-signerto explicitly skip this feature for this jobsrc/wallet/wallet.cpp (2)
315-335: Good preflight validation for external signer walletsEnforcing “disable private keys” and “descriptors” when WALLET_FLAG_EXTERNAL_SIGNER is set prevents invalid configurations. LGTM.
3805-3811: SPKM type selection via wallet flag is correctSelecting ExternalSignerScriptPubKeyMan when WALLET_FLAG_EXTERNAL_SIGNER is set is the right split. LGTM.
src/node/interfaces.cpp (2)
17-17: LGTM includeHeader include is fine; guarded usage below prevents ODR/link issues when feature is disabled.
402-411: LGTM: thin wrapperWrapper forwards name and moves the underlying signer; OK.
src/qt/sendcoinsdialog.cpp (2)
383-389: LGTM: clearer PSBT-only promptConditionally clarifies draft/PSBT flow when no external signer.
515-603: LGTM: safe PSBT flowFill without signing first, then sign if external signer, finalize/broadcast if complete; good error surfacing.
Please verify a tx-complete path also emits coinsSent or equivalent UI feedback when broadcast occurs via this branch.
| Follow the hardware manufacturers instructions for the initial device setup, as well as their instructions for creating a backup. Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/dashpay/HWI). | ||
|
|
There was a problem hiding this comment.
Update second HWI link to bitcoin-core.
-Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/dashpay/HWI).
+Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/bitcoin-core/HWI).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Follow the hardware manufacturers instructions for the initial device setup, as well as their instructions for creating a backup. Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/dashpay/HWI). | |
| Follow the hardware manufacturers instructions for the initial device setup, as well as their instructions for creating a backup. Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/bitcoin-core/HWI). |
🤖 Prompt for AI Agents
In doc/external-signer.md around lines 19 to 20, the second HWI link points to
the dashpay/HWI repo; update that link to the bitcoin-core HWI repository by
replacing the URL https://github.com/dashpay/HWI with
https://github.com/bitcoin-core/HWI so the documentation references the official
Bitcoin Core HWI project.
|
|
||
| In order to be compatible with Dash Core any signer command should conform to the specification below. This specification is subject to change. Ideally a BIP should propose a standard so that other wallets can also make use of it. |
There was a problem hiding this comment.
Tighten wording.
-In order to be compatible with Dash Core any signer command should conform to the specification below.
+To be compatible with Dash Core, any signer command must conform to the specification below.🧰 Tools
🪛 LanguageTool
[style] ~70-~70: Consider a more concise word here.
Context: ...ue, "txid": } ``` ## Signer API In order to be compatible with Dash Core any signer...
(IN_ORDER_TO_PREMIUM)
🤖 Prompt for AI Agents
In doc/external-signer.md around lines 70 to 71, tighten the wording of the
paragraph to be more precise and directive: replace the tentative phrasing with
an active, concise sentence such as "To be compatible with Dash Core, signer
commands must conform to the specification below (subject to change);" and
follow with a separate clear sentence like "Ideally, a BIP should propose this
as a standard so other wallets can implement it." Ensure use of "must" for
compatibility requirement, remove vague words like "any" and "should" in the
compatibility clause, and split into two sentences for clarity.
| It then imports descriptors for all support address types, in a BIP44/49/84 compatible manner. | ||
|
|
||
| The `walletdisplayaddress` RPC reuses some code from `getaddressinfo` on the provided address and obtains the inferred descriptor. It then calls `<cmd> --fingerprint=00000000 displayaddress --desc=<descriptor>`. |
There was a problem hiding this comment.
Hyphenate compound modifier.
-It then imports descriptors for all support address types, in a BIP44/49/84 compatible manner.
+It then imports descriptors for all supported address types in a BIP44/49/84-compatible manner.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| It then imports descriptors for all support address types, in a BIP44/49/84 compatible manner. | |
| The `walletdisplayaddress` RPC reuses some code from `getaddressinfo` on the provided address and obtains the inferred descriptor. It then calls `<cmd> --fingerprint=00000000 displayaddress --desc=<descriptor>`. | |
| It then imports descriptors for all supported address types in a BIP44/49/84-compatible manner. | |
| The `walletdisplayaddress` RPC reuses some code from `getaddressinfo` on the provided address and obtains the inferred descriptor. It then calls `<cmd> --fingerprint=00000000 displayaddress --desc=<descriptor>`. |
🧰 Tools
🪛 LanguageTool
[grammar] ~167-~167: Use a hyphen to join words.
Context: ... support address types, in a BIP44/49/84 compatible manner. The `walletdisplayad...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In doc/external-signer.md around lines 167 to 169, hyphenate the compound
modifier "BIP44/49/84 compatible manner" to "BIP44/49/84-compatible manner" so
the adjective correctly modifies "manner"; update that phrase in the sentence
describing imported descriptors and leave the rest of the wording unchanged.
| #include <string> | ||
| #include <vector> | ||
|
|
||
| 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) {} |
There was a problem hiding this comment.
Validate inputs early (fingerprint/chain)
Reject invalid m_fingerprint (non-hex or wrong length) and unknown m_chain in ctor to reduce downstream risk.
-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) {}
+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)
+{
+ if (!(m_chain == "main" || m_chain == "test" || m_chain == "regtest" || m_chain == "signet")) {
+ throw std::invalid_argument("ExternalSigner: invalid chain");
+ }
+ if (!IsHex(m_fingerprint) || m_fingerprint.size() != 8) {
+ throw std::invalid_argument("ExternalSigner: invalid fingerprint (expected 8 hex chars)");
+ }
+}🤖 Prompt for AI Agents
In src/external_signer.cpp around line 17, the constructor currently accepts any
fingerprint and chain; add early validation to reject invalid inputs by
verifying the fingerprint is hex and the expected length (e.g., 64 characters
for a 32-byte fingerprint or the project-specific length) and that the chain
string is one of the known/allowed values (e.g., "mainnet", "testnet", etc.). If
validation fails, throw a std::invalid_argument with a clear message indicating
whether the fingerprint format/length is wrong or the chain is unknown; perform
these checks before initializing member fields so invalid objects are never
created.
| std::vector<std::unique_ptr<interfaces::ExternalSigner>> signers; | ||
| try { | ||
| signers = node().listExternalSigners(); | ||
| } catch (const std::runtime_error& e) { | ||
| QMessageBox::critical(nullptr, tr("Can't list signers"), e.what()); | ||
| } | ||
| m_create_wallet_dialog->setSigners(signers); | ||
|
|
There was a problem hiding this comment.
Move unique_ptr signers into the dialog to fix compilation
std::vector<std::unique_ptr<...>> is move-only. Passing by copy won’t compile. Move it.
- m_create_wallet_dialog->setSigners(signers);
+ m_create_wallet_dialog->setSigners(std::move(signers));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::vector<std::unique_ptr<interfaces::ExternalSigner>> signers; | |
| try { | |
| signers = node().listExternalSigners(); | |
| } catch (const std::runtime_error& e) { | |
| QMessageBox::critical(nullptr, tr("Can't list signers"), e.what()); | |
| } | |
| m_create_wallet_dialog->setSigners(signers); | |
| std::vector<std::unique_ptr<interfaces::ExternalSigner>> signers; | |
| try { | |
| signers = node().listExternalSigners(); | |
| } catch (const std::runtime_error& e) { | |
| QMessageBox::critical(nullptr, tr("Can't list signers"), e.what()); | |
| } | |
| m_create_wallet_dialog->setSigners(std::move(signers)); |
🤖 Prompt for AI Agents
In src/qt/walletcontroller.cpp around lines 296 to 303, the local variable
signers is a std::vector of move-only std::unique_ptrs and cannot be copied into
the dialog; change the call to transfer ownership by moving the vector into the
dialog (use std::move on signers when calling setSigners) so the dialog receives
the vector by move rather than by copy.
| sys.exit(int(mock_result[0])) | ||
|
|
||
| def enumerate(args): | ||
| sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) |
There was a problem hiding this comment.
Fix indentation to satisfy flake8 (E111).
-def enumerate(args):
- sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}]))
+def enumerate(args):
+ sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}]))
@@
- sys.argv.extend(buffer.rstrip().split(" "))
+ sys.argv.extend(buffer.rstrip().split(" "))Also applies to: 96-96
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 21-21: indentation is not a multiple of 4
(E111)
🤖 Prompt for AI Agents
In test/functional/mocks/signer.py around lines 21 and 96, two lines with
sys.stdout.write are mis-indented causing flake8 E111; adjust their indentation
to match the surrounding block (remove the extra leading spaces so
sys.stdout.write is aligned with the other top-level/expected indentation in
that scope) and run flake8 to confirm the E111 errors are resolved.
| "receive": [ | ||
| "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", | ||
| "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", | ||
| "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" | ||
| ], | ||
| "internal": [ | ||
| "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", | ||
| "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", | ||
| "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc" | ||
| ] |
There was a problem hiding this comment.
Correct descriptor types in getdescriptors mock response.
Align with wrapped/native SegWit.
- "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n",
- "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq"
+ "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n",
+ "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq"
@@
- "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968",
- "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc"
+ "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968",
+ "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "receive": [ | |
| "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", | |
| "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", | |
| "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" | |
| ], | |
| "internal": [ | |
| "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", | |
| "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", | |
| "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc" | |
| ] | |
| "receive": [ | |
| "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", | |
| "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", | |
| "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" | |
| ], | |
| "internal": [ | |
| "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", | |
| "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", | |
| "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc" | |
| ] |
🤖 Prompt for AI Agents
In test/functional/mocks/signer.py around lines 27 to 36, the mock
getdescriptors response is using incorrect descriptor types (e.g., sh(pkh(...))
and pkh(...) for SegWit xpubs); update the descriptors to use correct wrapped
and native SegWit forms: keep legacy 44' as pkh(...), change 49' to
sh(wpkh(...)) for P2SH-P2WPKH (wrapped SegWit), and change 84' to wpkh(...) for
native SegWit, applying the same replacements for both the "receive" (/0/*) and
"internal" (/1/*) entries so the mock matches real descriptor types.
| expected_desc = [ | ||
| "pkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r" | ||
| ] | ||
| 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"})) | ||
|
|
There was a problem hiding this comment.
Make expected descriptor consistent with native SegWit.
- expected_desc = [
- "pkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r"
- ]
+ expected_desc = [
+ "wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r"
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expected_desc = [ | |
| "pkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r" | |
| ] | |
| 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"})) | |
| expected_desc = [ | |
| "wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r" | |
| ] | |
| 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"})) |
🤖 Prompt for AI Agents
In test/functional/mocks/signer.py around lines 46 to 53, the test expects a
legacy P2PKH descriptor ("pkh(...)") but returns a native SegWit address
(bech32), so update the expected_desc to the native SegWit descriptor form:
replace
"pkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r"
with the corresponding
"wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#<checksum>"
descriptor (or otherwise match the wpkh-prefixed descriptor expected by the
code); ensure the descriptor prefix is "wpkh" and the checksum matches the
descriptor string used by the signer.
| parser_signtx = subparsers.add_parser('signtx') | ||
| parser_signtx.add_argument('psbt', metavar='psbt') | ||
|
|
||
| parser_signtx.set_defaults(func=signtx) | ||
|
|
There was a problem hiding this comment.
Command mismatch: use “signtransaction” (and keep “signtx” as alias).
Core calls signtransaction; the mock uses signtx. This will break tests/integration.
-def signtx(args):
+def signtransaction(args):
@@
-parser_signtx = subparsers.add_parser('signtx')
-parser_signtx.add_argument('psbt', metavar='psbt')
-
-parser_signtx.set_defaults(func=signtx)
+parser_signtransaction = subparsers.add_parser('signtransaction')
+parser_signtransaction.add_argument('psbt', metavar='psbt')
+parser_signtransaction.set_defaults(func=signtransaction)
+
+# Back-compat alias
+parser_signtx = subparsers.add_parser('signtx')
+parser_signtx.add_argument('psbt', metavar='psbt')
+parser_signtx.set_defaults(func=signtransaction)Also applies to: 54-68
🤖 Prompt for AI Agents
In test/functional/mocks/signer.py around lines 88-92 (and similarly lines
54-68), the mock CLI command is registered as 'signtx' while Core expects
'signtransaction'; update the parser to register the primary command name as
"signtransaction" and add "signtx" as an alias or alternative name via
add_parser/aliases so both names map to the same handler, and ensure
set_defaults(func=signtx) remains so the existing handler is used for both
names.
| assert_raises_rpc_error(-1, 'Error: restart dashd with -signer=<cmd>', | ||
| self.nodes[0].enumeratesigners | ||
| ) | ||
|
|
||
| # Handle script missing: | ||
| assert_raises_rpc_error(-1, 'execve failed: No such file or directory', | ||
| self.nodes[3].enumeratesigners | ||
| ) |
There was a problem hiding this comment.
Fix E128 under-indented continuations and make error assertion OS-portable
- Flake8 E128: align wrapped arguments under the opening parenthesis.
- The missing-signer error message is OS-specific (“execve failed” on POSIX vs CreateProcess/The system cannot find the file on Windows). Relax the assertion to a portable substring.
Apply:
- 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
- )
+ err_substr = 'No such file or directory' if platform.system() != 'Windows' else 'find the file'
+ assert_raises_rpc_error(
+ -1,
+ err_substr,
+ 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 consider quoting the Windows mock path to survive spaces:
- return "py " + path
+ return f'py "{path}"'Also applies to: 60-64, 67-70
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 52-52: continuation line under-indented for visual indent
(E128)
[error] 57-57: continuation line under-indented for visual indent
(E128)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (27)
src/wallet/rpc/spend.cpp (1)
907-911: Initializecompleteand document why the first call's error is intentionally ignored.The
completevariable is declared uninitialized on line 909, which can lead to undefined behavior ifFillPSBTreads it before writing. Initialize it tofalse. Additionally, the comment should clarify that the first call's return value is safely ignored because any errors will be surfaced by the second call.Apply this diff:
- // First fill transaction with our data without signing, - // so external signers are not asked sign more than once. - bool complete; - (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true); + // First fill transaction with our data without signing, + // so external signers are not asked to sign more than once. + // Ignore errors here; the subsequent signing call will surface them. + bool complete = false; + (void)pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true); const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false);src/Makefile.test.include (1)
46-46: Unconditional wallet library linkage breaks--disable-walletbuilds.Adding
$(LIBBITCOIN_WALLET)unconditionally toFUZZ_SUITE_LD_COMMONwill cause linker errors when the project is configured with--disable-wallet, as the wallet library won't be built. The fuzz binary (line 271) depends on this variable for linking.The proper fix is to move this library into the
if ENABLE_WALLETconditional block (similar to howtest_test_dash_LDADDis guarded at line 252-254):FUZZ_SUITE_LD_COMMON = \ $(LIBTEST_FUZZ) \ $(LIBTEST_UTIL) \ $(LIBBITCOIN_NODE) \ - $(LIBBITCOIN_WALLET) \ $(LIBBITCOIN_COMMON) \ $(LIBBITCOIN_UTIL) \Then add it conditionally within the
if ENABLE_WALLETblock after line 221:if ENABLE_WALLET BITCOIN_TESTS += \ wallet/test/bip39_tests.cpp \ ... FUZZ_SUITE_LD_COMMON += \ $(LIBBITCOIN_WALLET) \ $(SQLITE_LIBS) \ $(BDB_LIBS)src/qt/sendcoinsdialog.cpp (3)
769-773: External balance not assigned (shows 0)When hasExternalSigner() is true, you set the label but never set balance. Use the watch-only balance to display available funds for external signer wallets.
Apply:
- if (model->wallet().hasExternalSigner()) { - ui->labelBalanceName->setText(tr("External balance:")); + if (model->wallet().hasExternalSigner()) { + balance = balances.watch_only_balance; + ui->labelBalanceName->setText(tr("External balance:"));Also applies to: 781-782
858-859: Allow watch‑only coins for external signer walletsExternal signer wallets are watch‑only; coin control must include watch‑only UTXOs.
Apply:
- m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner();
912-913: Apply same watch‑only fix in updateCoinControlState()Mirror the OR condition here to ensure consistent UTXO selection and fee estimation.
Apply:
- m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner();ci/test/00_setup_env_arm.sh (1)
28-28: CI configuration changes should be isolated.The removal of
--with-boost-processfrom the build configuration violates the coding guideline: "Unless specifically prompted, avoid making changes to the ci directory." This change, along with similar modifications in other CI setup scripts, should be split into a separate, focused PR with clear justification for the configuration update.As per coding guidelines.
ci/test/00_setup_env_s390x.sh (1)
25-25: CI configuration changes should be isolated.Same issue as noted for
ci/test/00_setup_env_arm.sh: this removal of--with-boost-processviolates the coding guideline to avoid unsolicited ci/ directory changes. Consolidate all CI configuration updates into a separate PR.As per coding guidelines.
ci/test/00_setup_env_native_nowallet.sh (1)
14-14: CI configuration changes should be isolated.Despite being marked as addressed in previous commits, this CI configuration change removing
--with-boost-processremains and violates the coding guideline. Move all CI modifications to a separate, focused PR.As per coding guidelines.
ci/test/00_setup_env_mac.sh (1)
17-17: CI configuration changes should be isolated.This removal of
--with-boost-processfrom the macOS build configuration should be part of a separate CI-focused PR, per coding guidelines.As per coding guidelines.
ci/test/00_setup_env_native_multiprocess.sh (1)
16-16: CI configuration changes should be isolated.This is the sixth CI setup script removing
--with-boost-process. All these configuration changes should be consolidated into a separate PR with clear justification, per coding guidelines.As per coding guidelines.
doc/external-signer.md (4)
70-71: Tighten specification wording.-In order to be compatible with Dash Core any signer command should conform to the specification below. This specification is subject to change. Ideally a BIP should propose a standard so that other wallets can also make use of it. +To be compatible with Dash Core, signer commands must conform to the specification below (subject to change). Ideally, a BIP should propose this as a standard so other wallets can implement it.
7-7: Clarify HWI maintainership.The documentation incorrectly states that HWI is "hosted under the Dash Core GitHub organization." HWI is actually hosted under the bitcoin-core organization at https://github.com/bitcoin-core/HWI.
19-19: Update HWI link to bitcoin-core repository.-Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/dashpay/HWI). +Alternatively, for some devices, you can use the `setup`, `restore` and `backup` commands provided by [HWI](https://github.com/bitcoin-core/HWI).
167-167: Hyphenate compound modifier.-It then imports descriptors for all support address types, in a BIP44/49/84 compatible manner. +It then imports descriptors for all supported address types in a BIP44/49/84-compatible manner.configure.ac (1)
327-330: Duplicate concern: Default should be "auto" with graceful degradation.A previous review comment already identified this issue: the help text says "default is auto" but the code defaults to "yes", and the Boost.Process probe at lines 1577-1585 will error out instead of gracefully degrading when Boost is absent. This will break builds on systems without Boost.Process unnecessarily.
src/qt/receiverequestdialog.cpp (1)
96-101: Fix null-deref and prevent duplicate signal connections for Verify button.
- model is dereferenced without a null check (Line 97).
- connect inside setInfo() will add multiple connections if setInfo() is called repeatedly.
Apply this refactor: connect once in the constructor with UniqueConnection, guard model usage, and keep visibility logic null-safe in setInfo().
@@ ReceiveRequestDialog::ReceiveRequestDialog(QWidget *parent) GUIUtil::handleCloseWindowShortcut(this); + // Connect once; read current address from member 'info' at click time. + connect(ui->btnVerify, &QPushButton::clicked, this, [this] { + if (!model) return; + model->displayAddress(info.address.toStdString()); + }, Qt::UniqueConnection); } @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) - ui->btnVerify->setVisible(model->wallet().hasExternalSigner()); - - connect(ui->btnVerify, &QPushButton::clicked, [this] { - model->displayAddress(info.address.toStdString()); - }); + ui->btnVerify->setVisible(model && model->wallet().hasExternalSigner());src/qt/createwalletdialog.h (1)
10-15: Include for the public API that uses std::vector.Make the header self-contained and resilient to include order.
#include <QDialog> #include <memory> +#include <vector> namespace interfaces { class ExternalSigner; } // namespace interfacestest/functional/rpc_signer.py (1)
21-26: Fix E128 indent, make missing-signer assertion portable, and quote Windows path
- Align continuation lines to satisfy flake8 E128.
- Use an OS-portable substring for the missing-executable error.
- Quote the Windows mock path to handle spaces.
@@ def mock_signer_path(self): path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py') if platform.system() == "Windows": - return "py " + path + return f'py "{path}"' else: return path @@ - 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, + ) @@ - # Handle script missing: - assert_raises_rpc_error(-1, 'execve failed: No such file or directory', - self.nodes[3].enumeratesigners - ) + # Handle script missing: + err_substr = 'No such file or directory' if platform.system() != 'Windows' else 'find the file' + assert_raises_rpc_error( + -1, + err_substr, + 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: 51-54, 55-58, 60-66, 67-70
test/functional/mocks/signer.py (1)
11-19: Lint fixes, correct descriptor types, and align command name with Core
- Fix E111/E201/E203/E231/E241 style issues and indentation.
- Use sh(wpkh(...)) for 49' and wpkh(...) for 84'.
- Expect wpkh in displayaddress test vector.
- Rename signtx to signtransaction; keep 'signtx' as alias.
@@ -def perform_pre_checks(): +def perform_pre_checks(): @@ -def enumerate(args): - sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) +def enumerate(_args): + sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) @@ - "receive": [ - "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", - "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", - "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" - ], + "receive": [ + "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", + "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#az7uyg3n", + "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#58ssc2nq" + ], @@ - "internal": [ - "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", - "sh(pkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", - "pkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc" - ] + "internal": [ + "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", + "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#mpkel968", + "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#9n439lrc" + ] @@ - expected_desc = [ - "pkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r" - ] + expected_desc = [ + "wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r" + ] @@ - if args.fingerprint == "00000001" : + if args.fingerprint == "00000001": @@ -parser_signtx = subparsers.add_parser('signtx') -parser_signtx.add_argument('psbt', metavar='psbt') - -parser_signtx.set_defaults(func=signtx) +parser_signtransaction = subparsers.add_parser('signtransaction') +parser_signtransaction.add_argument('psbt', metavar='psbt') +parser_signtransaction.set_defaults(func=signtx) + +# Back-compat alias +parser_signtx = subparsers.add_parser('signtx') +parser_signtx.add_argument('psbt', metavar='psbt') +parser_signtx.set_defaults(func=signtx) @@ - sys.argv.extend(buffer.rstrip().split(" ")) + sys.argv.extend(buffer.rstrip().split(" "))Also applies to: 20-22, 26-37, 39-53, 54-66, 69-72, 80-92, 93-97
test/functional/mocks/invalid_signer.py (1)
11-19: Guard empty mock_result and validate exit codeAvoid indexing empty strings; only exit on non-zero digit codes.
def perform_pre_checks(): mock_result_path = os.path.join(os.getcwd(), "mock_result") - if(os.path.isfile(mock_result_path)): + if os.path.isfile(mock_result_path): with open(mock_result_path, "r", encoding="utf8") as f: mock_result = f.read() - if mock_result[0]: - sys.stdout.write(mock_result[2:]) - sys.exit(int(mock_result[0])) + if mock_result: + code_char = mock_result[0] + if code_char.isdigit() and code_char != '0': + sys.stdout.write(mock_result[2:]) + sys.exit(int(code_char))src/wallet/rpc/wallet.cpp (1)
678-684: Enforce external_signer invariants at RPC boundaryIf external_signer=true, require descriptors=true, disable_private_keys=true, and -signer is set. Fail fast with clear errors.
if (!request.params[7].isNull() && request.params[7].get_bool()) { #ifdef ENABLE_EXTERNAL_SIGNER - flags |= WALLET_FLAG_EXTERNAL_SIGNER; + if (!(flags & WALLET_FLAG_DESCRIPTORS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Descriptor support must be enabled when using an external signer"); + } + if (!(flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Private keys must be disabled when using an external signer"); + } + if (gArgs.GetArg("-signer", "").empty()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "External signer requires -signer to be configured"); + } + flags |= WALLET_FLAG_EXTERNAL_SIGNER; #else throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)"); #endif }src/node/interfaces.cpp (1)
530-551: Harden listExternalSigners: catch enumerate failures and fix typos.Wrap ExternalSigner::Enumerate in try/catch to avoid crashing callers on malformed signer output; log and return empty on failure. Also fix “undistinguisable/succesful” typos.
Apply:
#ifdef ENABLE_EXTERNAL_SIGNER - std::vector<ExternalSigner> signers = {}; + std::vector<ExternalSigner> signers; const std::string command = gArgs.GetArg("-signer", ""); if (command == "") return {}; - ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); + try { + ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); + } catch (const std::exception& e) { + LogPrintf("External signer enumerate failed: %s\n", e.what()); + return {}; + } @@ -#else - // This result is undistinguisable from a succesful call that returns +#else + // This result is indistinguishable from a successful call that returns // no signers. For the current GUI this doesn't matter, because the walletsrc/wallet/external_signer_scriptpubkeyman.cpp (1)
52-61: Avoid NPE and handle display failures in DisplayAddress.Guard null provider and catch signer errors; only return true on success.
bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const { - // TODO: avoid the need to infer a descriptor from inside a descriptor wallet - auto provider = GetSolvingProvider(scriptPubKey); - auto descriptor = InferDescriptor(scriptPubKey, *provider); - - signer.DisplayAddress(descriptor->ToString()); - // TODO inspect result - return true; + // TODO: avoid the need to infer a descriptor from inside a descriptor wallet + auto provider = GetSolvingProvider(scriptPubKey); + if (!provider) return false; + auto descriptor = InferDescriptor(scriptPubKey, *provider); + try { + (void)signer.DisplayAddress(descriptor->ToString()); + return true; + } catch (const std::exception&) { + return false; + } }src/external_signer.cpp (4)
19-22: Quote chain argument to avoid shell injection via m_chain.Return a quoted --chain value.
- return " --chain " + m_chain; + return " --chain \"" + m_chain + "\"";
24-60: Do not abort enumeration on first duplicate; use continue.Break skips remaining devices; continue only skips the duplicate.
- if (duplicate) break; + if (duplicate) continue;
62-65: Avoid shell-constructing commands with unescaped data (injection risk).m_fingerprint/descriptors are concatenated into a single command string. Switch RunCommandParseJSON to accept argv vectors (no shell), or strictly escape arguments. At minimum, ensure descriptor quotes are escaped and fingerprint validated (see ctor fix).
Also applies to: 67-70, 92-96
17-17: Validate inputs early (fingerprint/chain) to reduce attack surface.Reject invalid chain and non-hex/incorrect-length fingerprints in ctor.
-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) {} +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) +{ + if (!(m_chain == "main" || m_chain == "test" || m_chain == "regtest" || m_chain == "signet")) { + throw std::invalid_argument("ExternalSigner: invalid chain"); + } + if (!IsHex(m_fingerprint) || m_fingerprint.size() != 8) { + throw std::invalid_argument("ExternalSigner: invalid fingerprint (expected 8 hex chars)"); + } +}
🧹 Nitpick comments (12)
src/Makefile.test.include (1)
53-54: Consider deduplicating database library linkage.
$(BDB_LIBS)and$(SQLITE_LIBS)are added toFUZZ_SUITE_LD_COMMONin the base section (lines 53-54) and again within theif ENABLE_WALLETblock (lines 223-225). While duplicate library references don't typically break builds, this redundancy suggests incomplete refactoring.If these libraries are wallet-specific, consider moving them entirely into the conditional block alongside the wallet library. If they're used by non-wallet code, the duplication can be removed from lines 223-225.
Also applies to: 223-225
src/qt/sendcoinsdialog.cpp (1)
229-240: Minor: clarify tooltip and include util/system.h for gArgs
- The enabled state looks fine, but the tooltip shown when -signer is set reads like a precondition. Consider making it action-oriented.
- Accessing gArgs without an explicit include relies on transitive headers. Include util/system.h here to be safe.
Apply:
+#include <util/system.h>And adjust the tooltip:
- ui->sendButton->setToolTip(tr("Connect your hardware wallet first.")); + ui->sendButton->setToolTip(tr("Create and sign using your external signer. Connect the device if prompted."));src/wallet/interfaces.cpp (1)
268-272: Avoid holding cs_wallet across external-signer I/O.
DisplayAddressmay spawn/await an external process. Holdingcs_walletcan block other wallet ops. Prefer calling without the lock (or acquire only for quick prechecks).- { - LOCK(m_wallet->cs_wallet); - return m_wallet->DisplayAddress(dest); - } + { + // Do not hold cs_wallet while interacting with external signer. + return m_wallet->DisplayAddress(dest); + }Please also confirm
CWallet::DisplayAddressis available (stubbed) when external-signer support is disabled, to keep this translation unit buildable across configs.src/interfaces/node.h (1)
160-169: MakegetName()const for proper const-correctness.The override of
listExternalSigners()is already properly implemented inNodeImplwith appropriate handling for both enabled and disabled external signer build configurations. However,getName()should be markedconstsince it is a query method that does not modify object state.src/util/system.cpp (2)
11-18: Gating Boost.Process under ENABLE_EXTERNAL_SIGNER is correct.Optional: undefine the temporary __kernel_entry macro after including Boost headers to avoid namespace pollution.
#include <boost/process.hpp> -#endif // ENABLE_EXTERNAL_SIGNER +#ifdef __kernel_entry +#undef __kernel_entry +#endif +#endif // ENABLE_EXTERNAL_SIGNER
1399-1434: Handle full stdout/stderr to avoid truncation; keep behavior identical otherwise.Current code reads only a single line, which can drop data from some signers. Read until EOF before wait/parse.
- std::string result; - std::string error; - std::getline(stdout_stream, result); - std::getline(stderr_stream, error); + std::string result{std::istreambuf_iterator<char>(stdout_stream), std::istreambuf_iterator<char>()}; + std::string error{std::istreambuf_iterator<char>(stderr_stream), std::istreambuf_iterator<char>()};src/qt/walletmodel.cpp (1)
582-593: Validate address and show a clear error before calling the wallet.Avoid silent failures on invalid input.
bool WalletModel::displayAddress(std::string sAddress) { - CTxDestination dest = DecodeDestination(sAddress); + if (!IsValidDestinationString(sAddress)) { + QMessageBox::critical(nullptr, tr("Can't display address"), tr("Invalid Dash address.")); + return false; + } + CTxDestination dest = DecodeDestination(sAddress); bool res = false; try { res = m_wallet->displayAddress(dest); } catch (const std::runtime_error& e) { QMessageBox::critical(nullptr, tr("Can't display address"), e.what()); } return res; }test/functional/mocks/invalid_signer.py (1)
20-22: Silence unused args and require --accountMinor polish: underscore unused parameter; make --account mandatory for getdescriptors.
-def enumerate(args): +def enumerate(_args): @@ -parser_getdescriptors.add_argument('--account', metavar='account') +parser_getdescriptors.add_argument('--account', metavar='account', required=True)Also applies to: 52-55
src/qt/createwalletdialog.cpp (1)
61-65: Fix comment typos“immedidately” → “immediately”.
- // watch-only wallet, even though we immedidately fetch keys from the + // watch-only wallet, even though we immediately fetch keys from thesrc/wallet/wallet.h (1)
30-31: Avoid including external_signer.h in wallet.hHeader doesn’t use ExternalSigner types. Remove the include to reduce compile-time coupling.
-#include <external_signer.h>src/wallet/external_signer_scriptpubkeyman.cpp (1)
78-85: Prefer wallet logging over std::cerr for errors.Use LogPrintf/WalletLogPrintf instead of writing to std::cerr in daemon context.
- tfm::format(std::cerr, "Failed to sign: %s\n", strFailReason); + LogPrintf("External signer failed to sign: %s\n", strFailReason);src/wallet/wallet.cpp (1)
2505-2518: Make DisplayAddress robust to signer exceptions.ExternalSignerScriptPubKeyMan::GetExternalSigner() can throw; wrap in try/catch and return false on failure to avoid bubbling exceptions through RPC/UI.
- ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(scriptPubKey, signer); + try { + ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); + return signer_spk_man->DisplayAddress(scriptPubKey, signer); + } catch (const std::exception& ) { + return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_mac_native_x86_64.sh(1 hunks)ci/test/00_setup_env_native_asan.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(1 hunks)ci/test/00_setup_env_native_qt5.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(7 hunks)doc/Doxyfile.in(1 hunks)doc/external-signer.md(1 hunks)src/Makefile.am(5 hunks)src/Makefile.test.include(1 hunks)src/dummywallet.cpp(1 hunks)src/external_signer.cpp(1 hunks)src/external_signer.h(1 hunks)src/interfaces/node.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/interfaces.cpp(3 hunks)src/qt/createwalletdialog.cpp(4 hunks)src/qt/createwalletdialog.h(2 hunks)src/qt/forms/createwalletdialog.ui(2 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/forms/receiverequestdialog.ui(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(7 hunks)src/qt/walletcontroller.cpp(4 hunks)src/qt/walletmodel.cpp(2 hunks)src/qt/walletmodel.h(1 hunks)src/qt/walletmodeltransaction.cpp(1 hunks)src/qt/walletmodeltransaction.h(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/external_signer.cpp(1 hunks)src/rpc/register.h(2 hunks)src/test/system_tests.cpp(2 hunks)src/util/error.cpp(1 hunks)src/util/error.h(1 hunks)src/util/system.cpp(3 hunks)src/util/system.h(0 hunks)src/wallet/external_signer_scriptpubkeyman.cpp(1 hunks)src/wallet/external_signer_scriptpubkeyman.h(1 hunks)src/wallet/init.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/rpc/wallet.cpp(6 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(9 hunks)src/wallet/wallet.h(5 hunks)src/wallet/walletdb.cpp(1 hunks)src/wallet/walletdb.h(1 hunks)src/wallet/walletutil.h(1 hunks)test/config.ini.in(1 hunks)test/functional/mocks/invalid_signer.py(1 hunks)test/functional/mocks/signer.py(1 hunks)test/functional/rpc_help.py(1 hunks)test/functional/rpc_signer.py(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)test/functional/test_framework/test_node.py(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_signer.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/util/system.h
🚧 Files skipped from review as they are similar to previous changes (23)
- src/interfaces/wallet.h
- ci/test/00_setup_env_native_tsan.sh
- test/config.ini.in
- src/qt/forms/createwalletdialog.ui
- src/wallet/external_signer_scriptpubkeyman.h
- src/qt/forms/optionsdialog.ui
- src/test/system_tests.cpp
- src/rpc/external_signer.cpp
- ci/test/00_setup_env_native_qt5.sh
- src/qt/walletmodeltransaction.cpp
- ci/test/00_setup_env_mac_native_x86_64.sh
- src/wallet/walletdb.cpp
- test/functional/test_runner.py
- doc/Doxyfile.in
- ci/test/00_setup_env_native_asan.sh
- src/external_signer.h
- src/rpc/client.cpp
- src/wallet/init.cpp
- src/qt/forms/receiverequestdialog.ui
- src/qt/optionsdialog.cpp
- src/wallet/walletdb.h
- src/wallet/walletutil.h
- src/Makefile.am
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/register.hsrc/qt/createwalletdialog.hsrc/qt/createwalletdialog.cppsrc/qt/walletmodeltransaction.hsrc/dummywallet.cppsrc/wallet/rpc/addresses.cppsrc/util/system.cppsrc/qt/optionsmodel.cppsrc/wallet/scriptpubkeyman.hsrc/node/interfaces.cppsrc/qt/optionsmodel.hsrc/util/error.hsrc/qt/sendcoinsdialog.cppsrc/wallet/wallet.cppsrc/qt/receiverequestdialog.cppsrc/interfaces/node.hsrc/wallet/rpc/wallet.cppsrc/qt/walletmodel.cppsrc/util/error.cppsrc/qt/walletcontroller.cppsrc/external_signer.cppsrc/wallet/rpc/spend.cppsrc/wallet/external_signer_scriptpubkeyman.cppsrc/wallet/wallet.hsrc/wallet/interfaces.cppsrc/qt/walletmodel.h
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_native_multiprocess.shci/test/00_setup_env_win64.shci/test/00_setup_env_arm.shci/test/00_setup_env_native_nowallet.shci/test/00_setup_env_s390x.shci/test/00_setup_env_mac.shci/test/00_setup_env_native_fuzz.sh
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/mocks/signer.pytest/functional/mocks/invalid_signer.pytest/functional/wallet_signer.pytest/functional/rpc_help.pytest/functional/test_framework/test_node.pytest/functional/rpc_signer.pytest/functional/test_framework/test_framework.py
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/external-signer.md
🧬 Code graph analysis (21)
src/rpc/register.h (2)
src/rpc/external_signer.cpp (2)
RegisterSignerRPCCommands(61-69)RegisterSignerRPCCommands(61-61)src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/qt/createwalletdialog.h (2)
src/interfaces/node.h (2)
interfaces(48-411)ExternalSigner(161-168)src/qt/createwalletdialog.cpp (16)
setSigners(115-134)setSigners(115-115)walletName(136-139)walletName(136-136)isEncryptWalletChecked(141-144)isEncryptWalletChecked(141-141)isDisablePrivateKeysChecked(146-149)isDisablePrivateKeysChecked(146-146)isMakeBlankWalletChecked(151-154)isMakeBlankWalletChecked(151-151)isDescriptorWalletChecked(156-159)isDescriptorWalletChecked(156-156)isExternalSignerChecked(161-164)isExternalSignerChecked(161-161)CreateWalletDialog(17-108)CreateWalletDialog(110-113)
src/qt/createwalletdialog.cpp (2)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)src/qt/createwalletdialog.h (1)
CreateWalletDialog(24-44)
src/qt/walletmodeltransaction.h (1)
src/qt/walletmodeltransaction.cpp (2)
setWtx(29-32)setWtx(29-29)
src/wallet/rpc/addresses.cpp (2)
src/wallet/rpc/util.cpp (2)
GetWalletForJSONRPCRequest(55-77)GetWalletForJSONRPCRequest(55-55)src/wallet/interfaces.cpp (18)
LOCK(249-258)LOCK(259-262)LOCK(290-294)LOCK(295-299)LOCK(352-360)LOCK(478-490)dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/util/system.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/wallet/scriptpubkeyman.h (2)
src/wallet/walletutil.h (1)
WalletDescriptor(70-103)src/wallet/external_signer_scriptpubkeyman.cpp (2)
SetupDescriptor(17-40)SetupDescriptor(17-17)
src/node/interfaces.cpp (1)
src/external_signer.cpp (2)
Enumerate(24-60)Enumerate(24-24)
src/wallet/wallet.cpp (4)
src/util/translation.h (1)
Untranslated(48-48)src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/external_signer_scriptpubkeyman.cpp (4)
DisplayAddress(52-61)DisplayAddress(52-52)GetExternalSigner(42-50)GetExternalSigner(42-42)src/wallet/interfaces.cpp (12)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/interfaces/node.h (3)
src/external_signer.cpp (1)
ExternalSigner(17-17)src/external_signer.h (1)
ExternalSigner(18-64)src/interfaces/wallet.h (1)
std(146-352)
test/functional/wallet_signer.py (3)
test/functional/test_framework/test_framework.py (9)
set_test_params(415-417)set_test_params(1473-1475)skip_test_if_missing_module(423-425)skip_test_if_missing_module(1477-1478)skip_if_no_external_signer(1106-1109)run_test(494-496)run_test(1480-1482)restart_node(694-697)is_bdb_compiled(1147-1149)test/functional/rpc_signer.py (6)
mock_signer_path(21-26)set_test_params(28-36)skip_test_if_missing_module(38-39)set_mock_result(41-43)clear_mock_result(45-46)run_test(48-76)test/functional/test_framework/test_node.py (2)
createwallet(884-889)get_wallet_rpc(363-369)
src/wallet/rpc/wallet.cpp (2)
src/wallet/rpc/addresses.cpp (2)
walletdisplayaddress(670-708)walletdisplayaddress(670-670)src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/qt/walletmodel.cpp (1)
src/wallet/interfaces.cpp (28)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)m_wallet(157-160)m_wallet(165-165)m_wallet(174-174)m_wallet(175-175)m_wallet(181-181)m_wallet(182-182)m_wallet(442-445)m_wallet(446-449)m_wallet(521-521)m_wallet(522-522)m_wallet(523-523)m_wallet(524-524)m_wallet(525-525)m_wallet(526-526)m_wallet(531-531)m_wallet(591-591)
src/util/error.cpp (1)
src/util/translation.h (1)
Untranslated(48-48)
test/functional/rpc_help.py (2)
test/functional/test_framework/test_framework.py (2)
is_external_signer_compiled(1115-1117)is_zmq_compiled(1135-1137)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/test_framework/test_node.py (1)
src/wallet/rpc/wallet.cpp (2)
createwallet(615-713)createwallet(615-615)
src/external_signer.cpp (2)
src/external_signer.h (1)
ExternalSigner(18-64)src/util/system.cpp (2)
RunCommandParseJSON(1397-1434)RunCommandParseJSON(1397-1397)
src/wallet/external_signer_scriptpubkeyman.cpp (3)
src/wallet/wallet.cpp (24)
batch(550-550)batch(815-815)batch(938-938)batch(1178-1178)batch(1257-1257)batch(1313-1313)batch(1358-1358)batch(1370-1370)batch(1391-1391)batch(1515-1515)batch(1660-1660)batch(1744-1744)batch(1793-1793)batch(2239-2239)batch(2300-2300)batch(2307-2307)DisplayAddress(2505-2518)DisplayAddress(2505-2505)GetSolvingProvider(3734-3738)GetSolvingProvider(3734-3734)GetSolvingProvider(3740-3748)GetSolvingProvider(3740-3740)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/scriptpubkeyman.cpp (16)
GetID(801-804)GetID(801-801)GetID(2375-2379)GetID(2375-2375)TopUp(1410-1413)TopUp(1410-1410)TopUp(1943-2004)TopUp(1943-1943)GetSolvingProvider(678-681)GetSolvingProvider(678-678)GetSolvingProvider(2231-2234)GetSolvingProvider(2231-2231)FillPSBT(734-778)FillPSBT(734-734)FillPSBT(2288-2355)FillPSBT(2288-2288)src/external_signer.cpp (4)
Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)
src/wallet/wallet.h (4)
src/external_signer.cpp (2)
DisplayAddress(62-65)DisplayAddress(62-62)src/wallet/wallet.cpp (4)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/external_signer_scriptpubkeyman.cpp (4)
DisplayAddress(52-61)DisplayAddress(52-52)FillPSBT(64-85)FillPSBT(64-64)src/wallet/interfaces.cpp (12)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/qt/walletmodel.h (1)
src/qt/walletmodel.cpp (2)
displayAddress(582-592)displayAddress(582-582)
test/functional/rpc_signer.py (4)
test/functional/test_framework/test_framework.py (1)
skip_if_no_external_signer(1106-1109)test/functional/test_framework/util.py (2)
assert_equal(69-74)assert_raises_rpc_error(132-148)test/functional/wallet_signer.py (6)
mock_signer_path(21-26)set_test_params(35-41)skip_test_if_missing_module(43-45)set_mock_result(47-49)clear_mock_result(51-52)run_test(54-57)src/rpc/external_signer.cpp (3)
enumeratesigners(16-59)enumeratesigners(16-16)result(54-54)
🪛 Flake8 (7.3.0)
test/functional/mocks/signer.py
[error] 21-21: indentation is not a multiple of 4
(E111)
[error] 96-96: indentation is not a multiple of 4
(E111)
test/functional/wallet_signer.py
[error] 106-106: continuation line under-indented for visual indent
(E128)
[error] 107-107: closing bracket does not match visual indentation
(E124)
[error] 126-126: continuation line missing indentation or outdented
(E122)
test/functional/rpc_signer.py
[error] 52-52: continuation line under-indented for visual indent
(E128)
[error] 57-57: continuation line under-indented for visual indent
(E128)
[error] 63-63: continuation line under-indented for visual indent
(E128)
[error] 69-69: continuation line under-indented for visual indent
(E128)
🪛 LanguageTool
doc/external-signer.md
[style] ~70-~70: Consider a more concise word here.
Context: ...ue, "txid": } ``` ## Signer API In order to be compatible with Dash Core any signer...
(IN_ORDER_TO_PREMIUM)
[grammar] ~167-~167: Use a hyphen to join words.
Context: ... support address types, in a BIP44/49/84 compatible manner. The `walletdisplayad...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
doc/external-signer.md
14-14: Dollar signs used before commands without showing output
(MD014, commands-show-output)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Dollar signs used before commands without showing output
(MD014, commands-show-output)
48-48: Dollar signs used before commands without showing output
(MD014, commands-show-output)
49-49: Dollar signs used before commands without showing output
(MD014, commands-show-output)
59-59: Dollar signs used before commands without showing output
(MD014, commands-show-output)
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
test/functional/mocks/signer.py
20-20: Unused function argument: args
(ARG001)
test/functional/mocks/invalid_signer.py
20-20: Unused function argument: args
(ARG001)
test/functional/test_framework/test_framework.py
1109-1109: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (29)
src/qt/sendcoinsdialog.cpp (1)
383-389: LGTM: clearer guidance when no external signerThe revised copy correctly distinguishes “draft PSBT” flow when private keys are disabled and no signer is configured.
src/dummywallet.cpp (1)
54-54: LGTM! External signer option properly added.The addition of the
-signer=<cmd>hidden argument is consistent with the external signer feature and correctly placed in the dummy wallet initialization.src/rpc/register.h (1)
53-55: LGTM! Signer RPC registration properly gated.The conditional compilation guard ensures signer RPCs are only registered when external signer support is enabled.
ci/test/00_setup_env_native_fuzz.sh (1)
18-18: LGTM!The removal of
--with-boost-processaligns with the PR's shift to the new external signer configuration model.ci/test/00_setup_env_win64.sh (1)
19-19: LGTM!The change from
--without-boost-processto--disable-external-signercorrectly reflects the new configuration option introduced in configure.ac.test/functional/test_framework/test_node.py (1)
884-889: LGTM!The addition of the
external_signerparameter correctly extends thecreatewalletRPC wrapper to match the updated RPC interface shown in src/wallet/rpc/wallet.cpp. The parameter is properly defaulted toNoneand forwarded through the call chain.configure.ac (4)
1414-1414: Appropriate gating for fuzzing builds.Disabling external signer support when fuzzing is enabled is reasonable, as the fuzzing environment doesn't require or support external hardware interactions.
1548-1552: Correct conditional Boost requirement logic.The logic appropriately determines whether Boost is needed based on which components are being built. Setting
use_boost=nowhen only libdashconsensus is being built is correct.
1591-1591: LGTM!The
AM_CONDITIONALandAC_SUBSTdeclarations forENABLE_EXTERNAL_SIGNERfollow standard autoconf patterns and correctly propagate the external signer configuration to the build system.Also applies to: 1978-1978
2040-2080: LGTM!The output formatting changes correctly display the external signer configuration status and align with the existing options summary format.
test/functional/rpc_help.py (2)
108-110: LGTM!The conditional addition of the 'Signer' category follows the established pattern for wallet and ZMQ components and correctly gates on the external signer compilation flag.
114-114: Good defensive programming with sorted comparison.Wrapping
componentsinsorted()ensures a deterministic comparison regardless of the order in which categories are appended. This prevents intermittent test failures due to category ordering variations.src/qt/walletmodeltransaction.h (1)
30-30: LGTM!The
setWtxsetter complements the existinggetWtx()getter and enables updating the transaction reference after construction, which is useful for external signer workflows where the PSBT may be modified externally before finalization.src/qt/optionsmodel.h (1)
76-76: LGTM!The
ExternalSignerPathenum addition correctly extends the options model to support persistence of the external signer path configuration. The placement and type (QString) are appropriate for a path setting.src/qt/walletmodel.h (1)
144-144: LGTM!The
displayAddressmethod declaration correctly extends WalletModel to support triggering address display on external hardware devices. The implementation appropriately handles errors with user-facing error dialogs.src/util/error.h (1)
33-35: LGTM: new external-signer error codes are well-scoped.src/util/error.cpp (1)
36-39: LGTM: error strings added for new external-signer cases.src/wallet/interfaces.cpp (1)
524-524: LGTM: hasExternalSigner correctly checks WALLET_FLAG_EXTERNAL_SIGNER.test/functional/test_framework/test_framework.py (2)
1106-1110: Good test gating for external signer.Matches existing skip_* helpers and keeps tests self-skipping when not compiled.
1115-1118: ENABLE_EXTERNAL_SIGNER check looks consistent.Aligned with other is_*_compiled helpers reading test config.
src/wallet/scriptpubkeyman.h (1)
551-553: Access change is appropriate.Exposing m_wallet_descriptor as protected enables derived managers to initialize descriptors.
src/qt/optionsmodel.cpp (1)
226-232: External signer path wiring looks good.Init, data(), and setData() correctly persist and map to -signer, with restart gating.
Also applies to: 503-508, 683-688
src/qt/createwalletdialog.h (1)
32-33: API additions LGTM.The signer plumbing methods and state are minimal and clear.
Also applies to: 39-44
src/qt/walletcontroller.cpp (1)
296-303: Passing signers by const-ref is correct; no move neededsetSigners takes a const std::vector<std::unique_ptr<...>>&. Forwarding the local vector without std::move avoids copying unique_ptrs and compiles cleanly. LGTM.
src/wallet/rpc/wallet.cpp (2)
192-195: Expose external_signer in getwalletinfo: LGTMThe new field mirrors the wallet flag. No issues.
1107-1109: Conditional walletdisplayaddress registration: LGTMGuarded by ENABLE_EXTERNAL_SIGNER; consistent with implementation.
Also applies to: 1233-1235
src/wallet/wallet.cpp (3)
315-335: Creation checks for external-signer wallets look correct.Validates “no privkeys” and “descriptors required” before create. LGTM.
3805-3811: LoadDescriptorScriptPubKeyMan: correct manager selection.Switching to ExternalSignerScriptPubKeyMan when WALLET_FLAG_EXTERNAL_SIGNER is set is appropriate.
2849-2852: Good UX: explicit error when support not compiled.Clear message for EXTERNAL_SIGNER_SUPPORT_REQUIRED. LGTM.
| // Always fill without signing first. This prevents an external signer | ||
| // from being called prematurely and is not expensive. | ||
| TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete); | ||
| assert(!complete); | ||
| assert(err == TransactionError::OK); | ||
| if (model->wallet().hasExternalSigner()) { | ||
| try { | ||
| err = model->wallet().fillPSBT(SIGHASH_ALL, true /* sign */, true /* bip32derivs */, nullptr, psbtx, complete); | ||
| } catch (const std::runtime_error& e) { | ||
| QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); | ||
| send_failure = true; | ||
| return; | ||
| } | ||
| if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { | ||
| //: "External signer" means using devices such as hardware wallets. | ||
| QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found"); | ||
| send_failure = true; | ||
| return; | ||
| } | ||
| if (err == TransactionError::EXTERNAL_SIGNER_FAILED) { | ||
| //: "External signer" means using devices such as hardware wallets. | ||
| QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure"); | ||
| send_failure = true; | ||
| return; | ||
| } | ||
| if (err != TransactionError::OK) { | ||
| tfm::format(std::cerr, "Failed to sign PSBT"); | ||
| processSendCoinsReturn(WalletModel::TransactionCreationFailed); | ||
| send_failure = true; | ||
| return; | ||
| } | ||
| // fillPSBT does not always properly finalize | ||
| complete = FinalizeAndExtractPSBT(psbtx, mtx); | ||
| } | ||
|
|
||
| // Broadcast transaction if complete (even with an external signer this | ||
| // is not always the case, e.g. in a multisig wallet). | ||
| if (complete) { | ||
| const CTransactionRef tx = MakeTransactionRef(mtx); | ||
| m_current_transaction->setWtx(tx); | ||
| model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin()); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Broadcast path skips cleanup/signals; improve i18n and PSBT wording
- When signing completes (Lines 553-557), you return immediately after sendCoins. This skips coinsSent emission and dialog cleanup (accept(), UnSelectAll(), reset, etc.), diverging from the normal-send path.
- The “Unsigned Transaction” text (Line 566) is inaccurate if the PSBT is partially signed and should be translatable. Same for the informative text (Line 567) and error messages (Lines 530, 536).
Apply:
if (complete) {
const CTransactionRef tx = MakeTransactionRef(mtx);
m_current_transaction->setWtx(tx);
- model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin());
- return;
+ const auto sent_hash = m_current_transaction->getWtx()->GetHash();
+ model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin());
+ Q_EMIT coinsSent(sent_hash);
+ // mirror non-PSBT send cleanup
+ accept();
+ m_coin_control->UnSelectAll();
+ coinControlUpdateLabels();
+ fNewRecipientAllowed = true;
+ m_current_transaction.reset();
+ return;
}- msgBox.setText("Unsigned Transaction");
- msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it.");
+ msgBox.setText(tr("Partially Signed Transaction"));
+ msgBox.setInformativeText(tr("The PSBT has been copied to the clipboard. You can also save it."));- QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found");
+ QMessageBox::critical(nullptr, tr("External signer not found"), tr("External signer not found"));
...
- QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure");
+ QMessageBox::critical(nullptr, tr("External signer failure"), tr("External signer failure"));Also applies to: 559-603
🤖 Prompt for AI Agents
In src/qt/sendcoinsdialog.cpp around lines 515-603, the broadcast path returns
immediately after model->sendCoins which skips the normal post-send cleanup and
signals (coinsSent, accept(), UnSelectAll(), reset UI), and several user-visible
strings are not translatable or misworded for partially-signed PSBTs; modify the
flow so after a successful sendCoins call you run the same cleanup/signal
sequence used by the normal-send path instead of returning early, ensure all
user-facing strings use tr(...) and change "Unsigned Transaction"/informative
texts to accurately reflect partially-signed PSBTs (e.g. "Incomplete
transaction" / "Partially signed PSBT") and update the error dialog messages
(EXTERNAL_SIGNER_NOT_FOUND / EXTERNAL_SIGNER_FAILED) to use tr(...) as well;
keep the same error-handling behavior but ensure FinalizeAndExtractPSBT and
subsequent messaging reflect the correct PSBT state and that cleanup always runs
on success or failure.
| /** Register raw transaction RPC commands */ | ||
| void RegisterSignerRPCCommands(CRPCTable &tableRPC); |
There was a problem hiding this comment.
Fix incorrect comment for RegisterSignerRPCCommands.
The comment describes "raw transaction RPC commands" but this function registers signer RPC commands.
-/** Register raw transaction RPC commands */
+/** Register signer RPC commands */
void RegisterSignerRPCCommands(CRPCTable &tableRPC);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Register raw transaction RPC commands */ | |
| void RegisterSignerRPCCommands(CRPCTable &tableRPC); | |
| /** Register signer RPC commands */ | |
| void RegisterSignerRPCCommands(CRPCTable &tableRPC); |
🤖 Prompt for AI Agents
In src/rpc/register.h around lines 25 to 26, the Doxygen comment above
RegisterSignerRPCCommands incorrectly says "Register raw transaction RPC
commands"; change the comment to accurately describe what the function does
(e.g., "Register signer RPC commands" or "Register RPC commands for signer
operations") and keep the existing comment style and formatting (/** ... */) so
the header correctly documents the function.
| #ifdef ENABLE_EXTERNAL_SIGNER | ||
| RPCHelpMan walletdisplayaddress() | ||
| { | ||
| return RPCHelpMan{"walletdisplayaddress", | ||
| "Display address on an external signer for verification.", | ||
| { | ||
| {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "dash address to display"}, | ||
| }, | ||
| RPCResult{ | ||
| RPCResult::Type::OBJ,"","", | ||
| { | ||
| {RPCResult::Type::STR, "address", "The address as confirmed by the signer"}, | ||
| } | ||
| }, | ||
| RPCExamples{""}, | ||
| [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | ||
| { | ||
| std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request); | ||
| if (!wallet) return NullUniValue; | ||
| CWallet* const pwallet = wallet.get(); | ||
|
|
||
| LOCK(pwallet->cs_wallet); | ||
|
|
||
| CTxDestination dest = DecodeDestination(request.params[0].get_str()); | ||
|
|
||
| // Make sure the destination is valid | ||
| if (!IsValidDestination(dest)) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); | ||
| } | ||
|
|
||
| if (!pwallet->DisplayAddress(dest)) { | ||
| throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address"); | ||
| } | ||
|
|
||
| UniValue result(UniValue::VOBJ); | ||
| result.pushKV("address", request.params[0].get_str()); | ||
| return result; | ||
| } | ||
| }; | ||
| } | ||
| #endif // ENABLE_EXTERNAL_SIGNER | ||
|
|
There was a problem hiding this comment.
Return the normalized address; align result with help text.
Help says the address is “confirmed by the signer”, but the code returns the raw input. Return EncodeDestination(dest) to avoid mismatch and normalize formatting.
- UniValue result(UniValue::VOBJ);
- result.pushKV("address", request.params[0].get_str());
+ UniValue result(UniValue::VOBJ);
+ result.pushKV("address", EncodeDestination(dest));
return result;Optionally, propagate a more specific error (e.g., no external signer configured) if DisplayAddress fails.
🤖 Prompt for AI Agents
In src/wallet/rpc/addresses.cpp around lines 669 to 710, the RPC currently
returns the raw input string but the help text promises the address "as
confirmed by the signer"; change the return to the normalized address by
encoding the decoded destination (use EncodeDestination(dest)) when pushing
"address" into the result so the RPC returns the canonical form the signer
confirmed. Also, when DisplayAddress fails, replace the generic RPC_MISC_ERROR
with a more specific JSONRPCError (e.g., RPC_INTERNAL_ERROR or a new message
like "External signer not configured or failed to display address") by using the
failure reason if available from DisplayAddress or by adding a clearer error
message so callers get actionable feedback.
| /** Provide a descriptor at setup time | ||
| * Returns false if already setup or setup fails, true if setup is successful | ||
| */ | ||
| bool SetupDescriptor(std::unique_ptr<Descriptor>desc); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make SetupDescriptor virtual to allow correct overriding.
ExternalSignerScriptPubKeyMan defines SetupDescriptor; mark the base as virtual to avoid accidental static binding via base pointers.
- bool SetupDescriptor(std::unique_ptr<Descriptor>desc);
+ virtual bool SetupDescriptor(std::unique_ptr<Descriptor> desc);Note: Add override to the derived declaration to enforce signature matching.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Provide a descriptor at setup time | |
| * Returns false if already setup or setup fails, true if setup is successful | |
| */ | |
| bool SetupDescriptor(std::unique_ptr<Descriptor>desc); | |
| /** Provide a descriptor at setup time | |
| * Returns false if already setup or setup fails, true if setup is successful | |
| */ | |
| virtual bool SetupDescriptor(std::unique_ptr<Descriptor> desc); |
🤖 Prompt for AI Agents
In src/wallet/scriptpubkeyman.h around lines 588 to 592, the SetupDescriptor
method is not declared virtual which prevents correct overriding through base
pointers; change the base declaration to virtual bool
SetupDescriptor(std::unique_ptr<Descriptor> desc); and update the derived
ExternalSignerScriptPubKeyMan declaration to use the override specifier (bool
SetupDescriptor(std::unique_ptr<Descriptor> desc) override;) to enforce
signature matching and prevent accidental static binding.
| xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6" | ||
| xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN" | ||
| xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp" | ||
|
|
There was a problem hiding this comment.
Remove accidental double assignment of xpub_pkh
The second assignment overwrites the first; keep a single, intentional value.
- xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6"
- xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN"
- xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp"
+ xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN"
+ xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6" | |
| xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN" | |
| xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp" | |
| xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN" | |
| xpub_pkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp" |
🤖 Prompt for AI Agents
In test/functional/mocks/invalid_signer.py around lines 24 to 27 there are two
consecutive assignments to xpub_pkh which causes the first value to be
unintentionally overwritten; remove the accidental duplicate so only one
intentional xpub_pkh assignment remains (confirm and keep the correct value for
tests), leaving xpub_sh unchanged.
| # Handle error thrown by script | ||
| self.set_mock_result(self.nodes[1], "2") | ||
| assert_raises_rpc_error(-1, 'RunCommandParseJSON error', | ||
| hww.walletdisplayaddress, address1 | ||
| ) | ||
| self.clear_mock_result(self.nodes[1]) | ||
|
|
There was a problem hiding this comment.
Fix flake8 E124/E128/E122 indentation in multiline calls.
Indent continuation lines and closing brackets consistently.
- assert_raises_rpc_error(-1, 'RunCommandParseJSON error',
- hww.walletdisplayaddress, address1
- )
+ assert_raises_rpc_error(
+ -1,
+ 'RunCommandParseJSON error',
+ hww.walletdisplayaddress,
+ address1,
+ )
@@
- 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
- }])
+ },
+ ])Also applies to: 126-132
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 106-106: continuation line under-indented for visual indent
(E128)
[error] 107-107: closing bracket does not match visual indentation
(E124)
🤖 Prompt for AI Agents
In test/functional/wallet_signer.py around lines 103-109 and also 126-132, the
multiline function calls violate flake8 E124/E128/E122; re-indent continuation
lines so arguments align under the first argument (or use a hanging indent) and
place the closing parenthesis on its own line aligned with the start of the call
statement. Update both blocks to use consistent indentation for the wrapped
assert_raises_rpc_error call and its arguments so continuation lines and the
closing bracket are vertically aligned to satisfy flake8.
|
This pull request has conflicts, please rebase. |
f75e0c1 doc: add external-signer.md (Sjors Provoost) d4b0107 rpc: send: support external signer (Sjors Provoost) 245b445 rpc: signerdisplayaddress (Sjors Provoost) 7ebc7c0 wallet: ExternalSigner: add GetDescriptors method (Sjors Provoost) fc5da52 wallet: add GetExternalSigner() (Sjors Provoost) 259f52c test: external_signer wallet flag is immutable (Sjors Provoost) 2655197 rpc: add external_signer option to createwallet (Sjors Provoost) 2700f09 rpc: signer: add enumeratesigners to list external signers (Sjors Provoost) 07b7c94 rpc: add external signer RPC files (Sjors Provoost) 8ce7767 wallet: add ExternalSignerScriptPubKeyMan (Sjors Provoost) 157ea7c wallet: add external_signer flag (Sjors Provoost) f3e6ce7 test: add external signer test (Sjors Provoost) 8cf543f wallet: add -signer argument for external signer command (Sjors Provoost) f7eb7ec test: framework: add skip_if_no_external_signer (Sjors Provoost) 87a9794 configure: add --enable-external-signer (Sjors Provoost) Pull request description: Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d). This PR lets `bitcoind` call an arbitrary command `-signer=<cmd>`, e.g. a hardware wallet driver, where it can fetch public keys, ask to display an address, and sign a transaction (using PSBT under the hood). It's design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described [here](https://github.com/bitcoin-core/HWI/blob/master/docs/bitcoin-core-usage.md). Usage is documented in [doc/external-signer.md]( https://github.com/Sjors/bitcoin/blob/2019/08/hww-box2/doc/external-signer.md), which also describes what protocol a different signer binary should conform to. Use `--enable-external-signer` to opt in, requires Boost::Process: ``` Options used to compile and link: with wallet = yes with gui / qt = no external signer = yes ``` It adds the following RPC methods: * `enumeratesigners`: asks <cmd> for a list of signers (e.g. devices) and their master key fingerprint * `signerdisplayaddress <address>`: asks <cmd> to display an address It enhances the following RPC methods: * `createwallet`: takes an additional `external_signer` argument and fetches keys from device * `send`: automatically sends transaction to device and waits Usage TL&DR: * clone HWI repo somewhere and launch `bitcoind -signer=../HWI/hwi.py` * check if you can see your hardware device: `bitcoin-cli enumeratesigners` * create wallet and auto import keys `bitcoin-cli createwallet "hww" true true "" true true true` * display address on device: `bitcoin-cli signerdisplayaddress ...` * to spend, use `send` RPC and approve transaction on device Prerequisites: - [x] bitcoin#21127 load wallet flags before everything else - [x] bitcoin#21182 remove mostly pointless BOOST_PROCESS macro Potentially useful followups: - GUI support: bitcoin-core/gui#4 - bumpfee support - (automatically) verify (a subset of) keys on the device after import, through message signing ACKs for top commit: laanwj: re-ACK f75e0c1 Tree-SHA512: 7db8afd54762295c1424c3f01d8c587ec256a72f34bd5256e04b21832dabd5dc212be8ab975ae3b67de75259fd569a561491945750492f417111dc7b6641e77f
…ion and typos 8b08d0f build, doc: Fix configure script output indentation and typos (Hennadii Stepanov) Pull request description: This PR is follow up of bitcoin#16546, that breaks the `configure` script output indentation for gui/qt/qr lines: ``` Options used to compile and link: external signer = no multiprocess = no with libs = yes with wallet = yes with sqlite = yes with bdb = yes with gui / qt = yes with qr = yes with zmq = yes with test = yes ... ``` With this PR: ``` Options used to compile and link: external signer = no multiprocess = no with libs = yes with wallet = yes with sqlite = yes with bdb = yes with gui / qt = yes with qr = yes with zmq = yes with test = yes ... ``` Also typos are fixed. ACKs for top commit: Sjors: utACK 8b08d0f vasild: ACK 8b08d0f Tree-SHA512: 46dfcfb754192dbcc080348781327d1714e2f9a696f3ed9252746b426e3afc628d84adb197ba3b8080eacaa6053ccac07e670998930ae92cef8ed713dd457c10
57ff5a4 doc: specify minimum HWI version (Sjors Provoost) 03308b2 rpc: don't require wallet for enumeratesigners (Sjors Provoost) Pull request description: HWI just released 2.0. See https://github.com/bitcoin-core/HWI/releases/tag/2.0.0 As of bitcoin#16546 we already rely on features that are in 2.0 and not in the previous 1.* releases: * `--chain` param This shouldn't be a problem, because HWI 2.0 has been released before we release v22. Misc improvements: * document that HWI 2.0 is required * drop wallet requirement for `enumeratesigners` ACKs for top commit: achow101: Code Review ACK 57ff5a4 Tree-SHA512: 3fb6ba20894e52a116f89525a5f5a1f61d363ccd904e1cffd0e6d095640fc6d2edf0388cd6ae20f83bbc31e5f458255ec090b6e823798d426eba3e45b4336bf9
88d4d5f rpc: add help for enumeratesigners and walletdisplayaddress (Sjors Provoost) b0db187 ci: use --enable-external-signer instead of --with-boost-process (Sjors Provoost) b54b2e7 Move external signer out of wallet module (Sjors Provoost) Pull request description: In addition, this PR enables external signer testing on CI. This PR moves the ExternalSigner class and RPC methods out of the wallet module. The `enumeratesigners` RPC can be used without a wallet since bitcoin#21417. With additional modifications external signers could be used without a wallet in general, e.g. via `signrawtransaction`. The `signerdisplayaddress` RPC is ranamed to `walletdisplayaddress` because it requires wallet context. A future `displayaddress` RPC call without wallet context could take a descriptor argument. This commit fixes a `rpc_help.py` failure when configured with `--disable-wallet`. ACKs for top commit: ryanofsky: Code review ACK 88d4d5f fanquake: ACK 88d4d5f Tree-SHA512: 3242a24e22313aed97eee32a520bfcb1c17495ba32a2b8e06a5e151e2611320e2da5ef35b572d84623af0a49a210d2f9377a2531250868d1a0ccf3e144352a97
c8f469c external_signer: remove ExternalSignerException (fanquake) 9e0b199 external_signer: use const where appropriate (fanquake) aaa4e5a wallet: remove CWallet::GetExternalSigner() (fanquake) 06a0673 external_signer: remove ignore_errors from Enumerate() (fanquake) 8fdbb89 refactor: unify external wallet runtime errors (fanquake) f4652bf refactor: add missing includes to external signer code (fanquake) 54569cc refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs (fanquake) Pull request description: These are a few followups after bitcoin#21467. ACKs for top commit: Sjors: tACK c8f469c instagibbs: utACK bitcoin@c8f469c Tree-SHA512: 3d5ac5df81680075e71e0e4a7595c520d746c3e37f016cf168c1e10da15541ebb1595aecaf2c08575636e9ff77d499644cae53180232b7049cfae0b923106e4e
…allet) 1c4b456 gui: send using external signer (Sjors Provoost) 24815c6 gui: wallet creation detects external signer (Sjors Provoost) 3f845ea node: add externalSigners to interface (Sjors Provoost) 62ac119 gui: display address on external signer (Sjors Provoost) 450cb40 wallet: add displayAddress to interface (Sjors Provoost) eef8d64 gui: create wallet with external signer (Sjors Provoost) 6cdbc83 gui: add external signer path to options dialog (Sjors Provoost) Pull request description: Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d). This PR adds GUI support for external signers, based on the since merged bitcoin#16546 (RPC). The UX isn't amazing - especially the blocking calls - but it works. First we adds a GUI setting for the signer script (e.g. path to HWI): <img width="625" alt="Schermafbeelding 2019-08-05 om 19 32 59" src="https://user-images.githubusercontent.com/10217/62483415-e1ff1680-b7b7-11e9-97ca-8d2ce54ca1cb.png"> Then we add an external signer checkbox to the wallet creation dialog: <img width="374" alt="Schermafbeelding 2019-11-07 om 19 17 23" src="https://user-images.githubusercontent.com/10217/68416387-b57ee000-0194-11ea-9730-127d60273008.png"> It's checked by default if HWI detects a device. It also grabs the name. It then creates a fresh wallet and imports the keys. You can verify an address on the device (blocking...): <img width="673" alt="Schermafbeelding 2019-08-05 om 19 29 22" src="https://user-images.githubusercontent.com/10217/62483560-43bf8080-b7b8-11e9-9902-8a036116dc4b.png"> Sending, including coin selection, Just Works(tm) as long the device is present. ~External signer support is enabled by default when the GUI is configured and Boost::Process is present.~ External signer support remains disabled by default, see bitcoin#21935. ACKs for top commit: achow101: Code Review ACK 1c4b456 hebasto: ACK 1c4b456, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW `2.0.2-rc.1`. promag: Tested ACK 1c4b456 but rebased with e033ca1, with HWI 2.0.2, with Nano S and Nano X. meshcollider: re-code-review ACK 1c4b456 Tree-SHA512: 3503113c5c69d40adb6ce364d8e7cae23ce82d032a00474ba9aeb6202eb70f496ef4a6bf2e623e5171e524ad31ade7941a4e0e89539c64518aaec74f4562d86b
…n unsupported e60cd26 Do not load external signers wallets when unsupported (Andrew Chow) Pull request description: When external signer support is not compiled, do not load external signer wallets. Alternative to bitcoin#22168. ACKs for top commit: promag: Tested ACK e60cd26. meshcollider: Code review ACK e60cd26 Tree-SHA512: aed2d0038f448c2f89c6b48f412b106e63c9ed20e748e69aae21fb58c33fc7e4fa73375a52372c73788669eb2b968a8da6b022c65658fa4484f5bbcf205b1b15
…e #ifdef 2f5bdcb gui: misc external signer fixes and translation hints (Sjors Provoost) d672404 refactor: make ExternalSigner NetworkArg() and m_chain private (Sjors Provoost) 4455145 refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage (Sjors Provoost) 5be90c9 build: enable external signer by default (Sjors Provoost) 7d94530 refactor: clean up external_signer.h includes (Sjors Provoost) fc0eca3 fuzz: fix fuzz binary linking order (Sjors Provoost) Pull request description: This follows the introduction of GUI support in bitcoin-core/gui#4 I don't think we should expect GUI users to self compile. This also enables external signer support by default for RPC users. In addition this PR reduces the number of `#ifdef ENABLE_EXTERNAL_SIGNER`, which also fixes bitcoin#21919. When compiled with `--disable-external-signer` such wallets can't be created in RPC or GUI, but they can be loaded. Attempting any action that calls HWI will trigger an error. Side-note: this PR may or may not (currently) break CI for the GUI repository, as explained here: bitcoin-core/gui#4 (comment) ACKs for top commit: achow101: ACK 2f5bdcb hebasto: re-ACK 2f5bdcb Tree-SHA512: 1b71c5a8bea2be077ee9fa33a01130c957a0cf90951d4b7b04d3d0ef826bb77e474c3963abddfef2e2c1ea99d9c72cd2302d1eb9b5fcb7ba0bd2a625f006aa05
…ocess 67669ab build: Fix Boost Process compatibility with mingw-w64 compiler (Hennadii Stepanov) Pull request description: On master (9c3751a) the cross build for Win64 is broken if configured with `--enable-external-signer`: ``` ... CXX crypto/libbitcoin_crypto_base_a-chacha_poly_aead.o In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23, from util/system.cpp:9: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:208:51: error: expected ‘)’ before ‘*’ token 208 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_system_query_information_p )( | ~ ^~ | ) /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:223:51: error: expected ‘)’ before ‘*’ token 223 | typedef ::boost::winapi::NTSTATUS_ (__kernel_entry *nt_query_object_p )( | ~ ^~ | ) /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::NTSTATUS_ boost::process::detail::windows::workaround::nt_system_query_information(boost::process::detail::windows::workaround::SYSTEM_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:239:12: error: ‘nt_system_query_information_p’ does not name a type; did you mean ‘nt_system_query_information’? 239 | static nt_system_query_information_p f = reinterpret_cast<nt_system_query_information_p>(::boost::winapi::get_proc_address(h, "NtQuerySystemInformation")); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | nt_system_query_information In file included from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handles.hpp:11, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/used_handles.hpp:17, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/async_in.hpp:20, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/async.hpp:49, from /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process.hpp:23, from util/system.cpp:9: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:241:14: error: ‘f’ was not declared in this scope 241 | return (*f)(SystemInformationClass, SystemInformation, SystemInformationLength, ReturnLength); | ^ /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp: In function ‘boost::winapi::BOOL_ boost::process::detail::windows::workaround::nt_query_object(boost::winapi::HANDLE_, boost::process::detail::windows::workaround::OBJECT_INFORMATION_CLASS_, void*, boost::winapi::ULONG_, boost::winapi::PULONG_)’: /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:253:12: error: ‘nt_query_object_p’ does not name a type; did you mean ‘nt_query_object’? 253 | static nt_query_object_p f = reinterpret_cast<nt_query_object_p>(::boost::winapi::get_proc_address(h, "NtQueryObject")); | ^~~~~~~~~~~~~~~~~ | nt_query_object /home/hebasto/GitHub/bitcoin/depends/x86_64-w64-mingw32/include/boost/process/detail/windows/handle_workaround.hpp:255:14: error: ‘f’ was not declared in this scope 255 | return (*f)(Handle, ObjectInformationClass, ObjectInformation, ObjectInformationLength, ReturnLength); | ^ make[2]: *** [Makefile:9906: util/libbitcoin_util_a-system.o] Error 1 make[2]: *** Waiting for unfinished jobs.... CXX crypto/libbitcoin_crypto_base_a-chacha20.o make[2]: Leaving directory '/home/hebasto/GitHub/bitcoin/src' make[1]: *** [Makefile:16141: all-recursive] Error 1 make[1]: Leaving directory '/home/hebasto/GitHub/bitcoin/src' make: *** [Makefile:820: all-recursive] Error 1 ``` The upstream bug: boostorg/process#96 Also see: https://stackoverflow.com/a/59338759 bitcoin#22348 (comment): > [This commit](boostorg/process@7fc41b2), containing the `__kernel_entry` [SAL annotations](https://docs.microsoft.com/en-us/cpp/code-quality/using-sal-annotations-to-reduce-c-cpp-code-defects?view=msvc-160) was included in Boost Process as part of the `1.71.0` release, which broke support for compiling with mingw-w64 because it doesn't define the `__kernel_entry` SAL annotation (but it does define some others, i.e see [`sal.h`](https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/include/sal.h)). > > A [commit was made](boostorg/process@d7a721e) to remove the annotations, however, it hasn't made it into either of the two Boost releases that have happened since (1.75.0 & 1.76.0). Meaning that this is currently needed for all versions of Boost process from 1.71.0 onwards. ACKs for top commit: fanquake: ACK 67669ab - thanks for updating this. Tree-SHA512: 5931ca1fb77ce38c042cf5a7556add024ea2386c208bf26c792a8ca4a771d97fac9802c32fa8aa2e3de1ad35f3362d8c066f0a83ee675859d226c602fd0bcf93
…abled without signers a9b9ca8 gui: ensure external signer option remains disabled without signers (Andrew Chow) Pull request description: When no external signers are available, the option to enable external signers should always be disabled. However the encrypt wallet checkbox can erroneously re-enable the external signer checkbox. To avoid this, CreateWalletDialog now stores whether signers were available during setSigners so that future calls to external_signer_checkbox->setEnabled can account for whether signers are available. Fixes dashpay#395 ACKs for top commit: hebasto: ACK a9b9ca8, tested on Linux Mint 20.2 (Qt 5.12.8). Sjors: tACK a9b9ca8 jarolrod: ACK a9b9ca8 Tree-SHA512: 98951bcadc23fce99a66ea2d367c44360989e888c253845a767e1f7085c594562d0f099de4130f4a078c5072aa7806294097d976ee6407291f3d3c5a4a608b44
…gic (stop on first match) d047ed7 external_signer: improve fingerprint matching logic (stop on first match) (Sebastian Falbesoner) Pull request description: The fingerprint matching logic in `ExternalSigner::SignTransaction` currently always iterates all inputs of a PSBT, even after a match has already been found. I guess the reason for that is not that it was not thought of, but rather the fact that breaking out of a nested loop is simply not possible (at least not without adding ugly constructs like gotos or extra state variables). This PR fixes this by using `std::any_of` from C++'s standard library, see http://www.cplusplus.com/reference/algorithm/any_of/ ACKs for top commit: lsilva01: Code Review ACK bitcoin@d047ed7 Sjors: utACK d047ed7 Zero-1729: crACK d047ed7 mjdietzx: Code review ACK d047ed7 hebasto: ACK d047ed7, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 447e7c0c6a5b5549a2c09d52e55ba4146302c1a06e4d96de11f6945d09f98c89129cba221202dff7e0718e01a83dd173b9f19b1f02b6be228978f3f6e35d8096
a032fa3 multiprocess: add interfaces::ExternalSigner class (Russell Yanofsky) Pull request description: Add `interfaces::ExternalSigner` class to let signer objects be passed between processes and let signer code run in the original process where the object was created. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). ACKs for top commit: laanwj: Concept and code review ACK a032fa3 hebasto: re-ACK a032fa3 Tree-SHA512: 99a729fb3a64d010e142cc778a9f1f358e58345b77faaf2664de7d2277715d59df3352326e8f0f2a6628038670eaa4556310a549079fb28af6d2eeb05aea1460
5493e92 Check descriptors returned by external signers (sstone) Pull request description: Check that descriptors returned by external signers have been parsed properly when creating a new wallet. See bitcoin#23627 for context. The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`. I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`. ACKs for top commit: jamesob: Code review ACK bitcoin@5493e92 achow101: ACK 5493e92 Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
b75f4c8 RPC: Return external_signer in getwalletinfo (Kristaps Kaupe) Pull request description: Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet. ACKs for top commit: S3RK: utACK b75f4c8 achow101: ACK b75f4c8 prayank23: utACK bitcoin@b75f4c8 brunoerg: utACK b75f4c8 Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
…or message 7f3a6a9 wallet: Add external-signer-support specific error message (Hennadii Stepanov) Pull request description: On master (5f44c5c) an attempt to load an external signer wallet using Bitcoin Core compiled without external signer support fails with the following log messages: ``` 2022-02-20T19:01:11Z [qt-walletctrl] Using SQLite Version 3.31.1 2022-02-20T19:01:11Z [qt-walletctrl] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/coldcard-0220 2022-02-20T19:01:11Z [qt-walletctrl] init message: Loading wallet… 2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Error: External signer wallet being loaded without external signer support compiled 2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Releasing wallet ``` While log messages are good, a message in the GUI window is completely misleading:  This PR fixes this issue:  ACKs for top commit: achow101: ACK 7f3a6a9 kristapsk: ACK 7f3a6a9 brunoerg: crACK 7f3a6a9 Tree-SHA512: a4842751c0ca8a37ccc3ea00503678f6b712a7f53d6cbdc07ce02dcb85ca8a94890d1c2da20307be043faa347747abeba29185c88ba12edd5253bfca56531585
…rnalSigner::SignTransaction 2a22f03 parsing external signer master fingerprint string as bytes instead of caring for lower/upper case in ExternalSigner::SignTransaction (avirgovi) Pull request description: Some external signers scripts may provide master fingerprint in uppercase format. In that case core will fail with `Signer fingerprint 00000000 does not match any of the inputs` as it only works with lowercase format. Even if the fingerprints match, yet one is lowercase the other uppercase. ExternalSigner::SignTransaction is the only place where it is needed IMO, as changing it in other places may break the communication with the external signer (i.e. enumerating with lowercase may not find the device). ACKs for top commit: achow101: ACK 2a22f03 theStack: Code-review ACK 2a22f03 Sjors: utACK 2a22f03 Tree-SHA512: f3d84b83fb0b5e06c405eaf9bf20a2fa864bf4172fd4de113b80b9b9a525a76f2f8cf63031b480358b3a7666023a2aef131fb89ff50448c66df3ed541da10f99
…gner` configure option 8df063e build: Fix help string for `--enable-external-signer` configure option (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin#24065 and fixes the help string according to the actual default value https://github.com/bitcoin/bitcoin/blob/816ca01650f4cc66a61ac2f9b0f8b74cd9cd0cf8/configure.ac#L324-L327 ACKs for top commit: kristapsk: cr utACK 8df063e jarolrod: ACK 8df063e Tree-SHA512: ad3f457a53c9238ddd8ded9efd1224e564e6cb9da8b7ff7733a11e32a7daad5c0f6c6223509218f44944a874470cb0d2447897662eaf4e78c763b30785717c50
…ationDialog 742918c qt: hide Create Unsigned button behind an expert mode option (Andrew Chow) 5c3b800 qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow) Pull request description: Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled. Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets. Moved from bitcoin#18789 ACKs for top commit: jarolrod: ACK 742918c ryanofsky: Code review ACK 742918c. Just suggested changes since last review. Looks great! hebasto: ACK 742918c, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (14)
src/wallet/rpc/spend.cpp (1)
907-911: Initializecompletebefore first use and document the ignored error.The local variable
completeshould be initialized to a known value (e.g.,bool complete = false;) before the first call toFillPSBTto avoid potential undefined behavior ifFillPSBTerrors out early without setting it.Additionally, the cast-to-void on line 910 should be accompanied by an explicit comment explaining that the return value is intentionally ignored because the first call is only used to populate PSBT data for external signers, and any errors will be surfaced in the subsequent signing call.
Based on learnings
src/wallet/rpc/addresses.cpp (1)
669-706: Return normalized address and surface signer errors.
- Result should return the canonical address string, not the raw input. Use EncodeDestination(dest) to match help text “as confirmed by the signer.”
- If DisplayAddress fails due to missing/disabled external signer, return a more specific error to aid callers.
@@ - if (!pwallet->DisplayAddress(dest)) { - throw JSONRPCError(RPC_MISC_ERROR, "Failed to display address"); - } + if (!pwallet->DisplayAddress(dest)) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "External signer not configured or failed to display address"); + } @@ - UniValue result(UniValue::VOBJ); - result.pushKV("address", request.params[0].get_str()); + UniValue result(UniValue::VOBJ); + result.pushKV("address", EncodeDestination(dest)); return result;Upstream path may be swallowing signer errors. Please ensure wallet->DisplayAddress(dest) actually reflects device/display failures (ExternalSignerScriptPubKeyMan::DisplayAddress currently returns true unconditionally per snippet). Consider propagating the RunCommandParseJSON result. Based on learnings.
#!/bin/bash # Inspect implementation to confirm return value propagation: rg -n -C2 'DisplayAddress\(' src/wallet | sed -n '1,120p'src/qt/createwalletdialog.h (1)
10-14: Include in the header.Header exposes std::vector in a public signature but doesn’t include . Include what you use to keep it self‑contained.
#include <QDialog> #include <memory> +#include <vector> namespace interfaces { class ExternalSigner; } // namespace interfacesdoc/external-signer.md (1)
7-9: Point HWI links to bitcoin-core/HWI.Use the official repository to avoid confusion.
-The following example is based on the [HWI](https://github.com/dashpay/HWI) tool. +The following example is based on the [HWI](https://github.com/bitcoin-core/HWI) tool. @@ -... provided by [HWI](https://github.com/dashpay/HWI). +... provided by [HWI](https://github.com/bitcoin-core/HWI).Based on learnings.
Also applies to: 19-20
configure.ac (1)
328-331: Make ‘external‑signer’ truly auto and probe Boost.Process gracefully.Current default “yes” plus hard error breaks builds without Boost.Process and contradicts help text.
-AC_ARG_ENABLE([external-signer], - [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is auto, requires Boost::Process)])], - [use_external_signer=$enableval], - [use_external_signer=yes]) +AC_ARG_ENABLE([external-signer], + [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default: auto; requires Boost::Process)])], + [use_external_signer=$enableval], + [use_external_signer=auto]) @@ - if test "$use_external_signer" != no; then + if test "$use_external_signer" = yes || test "$use_external_signer" = auto; then AC_MSG_CHECKING(for Boost Process) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <boost/process.hpp>]], - [[ boost::process::child* child = new boost::process::child; delete child; ]])], - [ AC_MSG_RESULT(yes) - AC_DEFINE([ENABLE_EXTERNAL_SIGNER],,[define if external signer support is enabled]) - ], - [ AC_MSG_ERROR([Boost::Process is required for external signer support, but not available!])] + [[ boost::process::child c((char*)""); ]])], + [ AC_MSG_RESULT(yes) + use_external_signer=yes + AC_DEFINE([ENABLE_EXTERNAL_SIGNER],[1],[Define if external signer support is enabled]) ], + [ AC_MSG_RESULT(no) + if test "$use_external_signer" = yes; then + AC_MSG_ERROR([--enable-external-signer requires Boost.Process]) + else + use_external_signer=no + fi ]) fi @@ -AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = "yes"]) +AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = yes]) @@ -AC_SUBST(ENABLE_EXTERNAL_SIGNER) +# No AC_SUBST needed for ENABLE_EXTERNAL_SIGNER; use AM_CONDITIONAL and C/C++ macro.Also keep summary printouts consistent with final resolved value of use_external_signer. Based on learnings.
Also applies to: 1557-1587, 1592-1593, 1980-1980, 2045-2065, 2076-2085
src/qt/sendcoinsdialog.cpp (4)
553-606: Don’t return before the normal post-send cleanup
Whencompleteis true youreturnimmediately aftermodel->sendCoins(...), so coinsSent is never emitted and the dialog never runs the cleanup/reset block at the end of the method. This leaves the UI in an inconsistent state (entries stay frozen, coin control not cleared,fNewRecipientAllowedremains false, etc.). Instead, mirror the non-PSBT send path: emitcoinsSent, fall through to the shared cleanup, and only exit after the state has been reset.- if (complete) { - const CTransactionRef tx = MakeTransactionRef(mtx); - m_current_transaction->setWtx(tx); - model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin()); - return; - } + if (complete) { + const CTransactionRef tx = MakeTransactionRef(mtx); + m_current_transaction->setWtx(tx); + model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin()); + Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash()); + } else { // Copy PSBT to clipboard and offer to save assert(!complete); ... - switch (msgBox.exec()) { + switch (msgBox.exec()) { ... } }Ensure every exit route (including error handling above) reaches the final block that resets
fNewRecipientAllowed, clearsm_current_transaction, and refreshes coin control.
770-781: Assign the external signer balance before displaying it
In thehasExternalSigner()branch you change the label but leavebalanceat 0, so external signer wallets always show zero funds. Load the appropriate watch-only/external balance (same value used for privateKeysDisabled wallets) before formatting the display string.- if (model->wallet().hasExternalSigner()) { - ui->labelBalanceName->setText(tr("External balance:")); - } else if (model->wallet().privateKeysDisabled()) { - balance = balances.watch_only_balance; + if (model->wallet().hasExternalSigner()) { + balance = balances.watch_only_balance; + ui->labelBalanceName->setText(tr("External balance:")); + } else if (model->wallet().privateKeysDisabled()) { + balance = balances.watch_only_balance;
858-913: Allow watch-only coin control for external-signer wallets
External signer wallets are watch-only from the node’s perspective. Settingm_coin_control->fAllowWatchOnly = privateKeysDisabled() && !hasExternalSigner()disables watch-only inputs precisely when they are required, making it impossible to gather UTXOs or build a PSBT. Flip the logic so watch-only inputs remain enabled whenever private keys are disabled or we rely on an external signer. Apply the same fix inupdateCoinControlState().- m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner();
520-545: Reset dialog state on signer failures instead of returning early
Each error branch (fillPSBTruntime error,EXTERNAL_SIGNER_NOT_FOUND,EXTERNAL_SIGNER_FAILED, generic error) setssend_failure = trueand thenreturns. Those early returns skip the final block that restoresfNewRecipientAllowed, resetsm_current_transaction, and clears coin control, leaving the dialog stuck in a “sending” state until the window is reopened. Please funnel these failures through the shared cleanup (e.g., set a flag andbreak/gototo the end) or explicitly reset the state before returning.- } catch (const std::runtime_error& e) { - QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); - send_failure = true; - return; - } + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); + send_failure = true; + goto finalize; + } ... - if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { - QMessageBox::critical(nullptr, tr("External signer not found"), tr("External signer not found")); - send_failure = true; - return; - } + if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { + QMessageBox::critical(nullptr, tr("External signer not found"), tr("External signer not found")); + send_failure = true; + goto finalize; + }Add a
finalize:label just before the existing cleanup block (and ensure every return path jumps there).src/external_signer.cpp (4)
17-18: Validate chain name and fingerprint at construction
ExternalSignercurrently accepts anychain/fingerprintstrings and stores them verbatim. With no whitelist or format checks, a malformed fingerprint (non-hex, wrong length) or unexpected chain value flows straight into every command invocation, opening the door to command injection later. Validate here (e.g., chain in {"main","test","regtest","signet"}; fingerprint is 8 hex chars) and reject invalid inputs up front.-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) {} +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) +{ + if (m_chain != "main" && m_chain != "test" && m_chain != "regtest" && m_chain != "signet") { + throw std::invalid_argument("ExternalSigner: invalid chain"); + } + if (!IsHex(m_fingerprint) || m_fingerprint.size() != 8) { + throw std::invalid_argument("ExternalSigner: invalid fingerprint (expected 8 hex chars)"); + } +}
21-22: Quote the chain argument to avoid shell injection
NetworkArg()concatenatesm_chainwithout quoting, so any unexpected character (whether malicious or just whitespace) escapes into the shell command built later. Wrap the chain value in quotes (and ensure validation as above) so the generated command is safe.- return " --chain " + m_chain; + return " --chain \"" + m_chain + "\"";
47-58: Skip duplicate signer entries without aborting enumeration
When a duplicate fingerprint is encountered youbreak, which stops processing the remaining results entirely. This drops subsequent legitimate signers. Replacebreakwithcontinueso only the duplicate entry is skipped.- if (duplicate) break; + if (duplicate) continue;
62-99: Sanitize command arguments for descriptor/fingerprint
DisplayAddress,GetDescriptors, andSignTransactionall concatenatem_fingerprintand, in the first case, a wallet-provided descriptor directly into a shell command. Even with constructor validation, the descriptor can contain characters that break quoting or inject shell tokens, resulting in command execution on the host. Refactor these calls to pass arguments as discrete elements (e.g., updateRunCommandParseJSONto accept an argument vector) or at minimum perform strict escaping/validation before concatenation so descriptors cannot inject shell metacharacters.src/wallet/external_signer_scriptpubkeyman.cpp (1)
55-59: Guard provider/descriptor before callingDisplayAddress
GetSolvingProvider(scriptPubKey)can legitimately returnnullptr, andInferDescriptor(...)may also fail. Dereferencing either without checking will crash (null dereference) before we even reach the signer. Wrap these calls with null checks, bail out (and ideally log) when they fail, and only callsigner.DisplayAddresswhen a descriptor is available. Capture/inspect the return or exception fromDisplayAddressand propagate failure to the caller instead of always returning true.// TODO: avoid the need to infer a descriptor from inside a descriptor wallet - auto provider = GetSolvingProvider(scriptPubKey); - auto descriptor = InferDescriptor(scriptPubKey, *provider); - - signer.DisplayAddress(descriptor->ToString()); - // TODO inspect result - return true; + auto provider = GetSolvingProvider(scriptPubKey); + if (!provider) { + return false; + } + auto descriptor = InferDescriptor(scriptPubKey, *provider); + if (!descriptor) { + return false; + } + try { + (void)signer.DisplayAddress(descriptor->ToString()); + return true; + } catch (const std::exception&) { + return false; + }
🧹 Nitpick comments (6)
src/qt/walletmodel.cpp (1)
582-591: Defensively bail on invalid addresses.
DecodeDestination()collapses malformed strings toCNoDestination, which means we only discover bad input after acquiring the wallet lock and letting the backend throw. A quickIsValidDestinationString(sAddress)check here would let the GUI fail fast and surface the problem without relying on backend exceptions.doc/external-signer.md (2)
70-71: Tighten wording and hyphenate compound modifier.
- Prefer “To be compatible … must …” over “In order to … should …”.
- Use “BIP44/49/84‑compatible” as a compound modifier.
-In order to be compatible with Dash Core any signer command should conform to the specification below. +To be compatible with Dash Core, any signer command must conform to the specification below. @@ -It then imports descriptors for all support address types, in a BIP44/49/84 compatible manner. +It then imports descriptors for all supported address types in a BIP44/49/84‑compatible manner.Based on learnings.
Also applies to: 167-169
13-16: Minor Markdown hygiene (optional).
- Add language hints to fenced blocks (sh, json).
- Prefer command blocks without leading “$” prompts to satisfy MD014.
Example:
-```sh -$ dashd -signer=../HWI/hwi.py +```sh +dashd -signer=../HWI/hwi.py…and similarly for other code fences.
Also applies to: 25-33, 39-41, 48-50, 59-61, 80-87, 98-104, 115-136, 141-149
src/qt/forms/optionsdialog.ui (1)
412-441: Add a “Browse…” button and basic validation.A QFileDialog next to externalSignerPath improves UX and reduces typo risk; consider existence/execute checks and warning if path is empty.
src/qt/createwalletdialog.cpp (1)
30-47: External‑signer flow wiring looks solid; fix small nits.
- Spelling: “immedidately” → “immediately”; “checkbox box” → “checkbox”.
- Remove unused include <interfaces/node.h> if not needed.
- // watch-only wallet, even though we immedidately fetch keys from the + // watch-only wallet, even though we immediately fetch keys from the @@ - // When the external_signer_checkbox box is disabled, uncheck it. + // When the external_signer_checkbox is disabled, uncheck it.Also applies to: 49-65, 115-134, 161-164
src/external_signer.h (1)
54-64: Make API const‑correct.Neither method mutates the signer; mark them const.
- UniValue GetDescriptors(const int account); + UniValue GetDescriptors(int account) const; @@ - bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error); + bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error) const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_mac_native_x86_64.sh(1 hunks)ci/test/00_setup_env_native_asan.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(1 hunks)ci/test/00_setup_env_native_qt5.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(7 hunks)doc/Doxyfile.in(1 hunks)doc/external-signer.md(1 hunks)src/Makefile.am(5 hunks)src/Makefile.test.include(1 hunks)src/dummywallet.cpp(1 hunks)src/external_signer.cpp(1 hunks)src/external_signer.h(1 hunks)src/interfaces/node.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/interfaces.cpp(3 hunks)src/qt/createwalletdialog.cpp(4 hunks)src/qt/createwalletdialog.h(2 hunks)src/qt/forms/createwalletdialog.ui(2 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/forms/receiverequestdialog.ui(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(7 hunks)src/qt/walletcontroller.cpp(4 hunks)src/qt/walletmodel.cpp(2 hunks)src/qt/walletmodel.h(1 hunks)src/qt/walletmodeltransaction.cpp(1 hunks)src/qt/walletmodeltransaction.h(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/external_signer.cpp(1 hunks)src/rpc/register.h(2 hunks)src/test/system_tests.cpp(2 hunks)src/util/error.cpp(1 hunks)src/util/error.h(1 hunks)src/util/system.cpp(3 hunks)src/util/system.h(0 hunks)src/wallet/external_signer_scriptpubkeyman.cpp(1 hunks)src/wallet/external_signer_scriptpubkeyman.h(1 hunks)src/wallet/init.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/spend.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/util/system.h
🚧 Files skipped from review as they are similar to previous changes (24)
- src/dummywallet.cpp
- src/rpc/external_signer.cpp
- ci/test/00_setup_env_native_tsan.sh
- ci/test/00_setup_env_native_asan.sh
- src/wallet/interfaces.cpp
- src/qt/receiverequestdialog.cpp
- src/wallet/external_signer_scriptpubkeyman.h
- ci/test/00_setup_env_win64.sh
- src/Makefile.test.include
- ci/test/00_setup_env_arm.sh
- ci/test/00_setup_env_native_fuzz.sh
- src/qt/walletmodeltransaction.h
- src/qt/forms/receiverequestdialog.ui
- ci/test/00_setup_env_native_multiprocess.sh
- ci/test/00_setup_env_native_qt5.sh
- src/util/error.h
- src/wallet/init.cpp
- src/qt/walletcontroller.cpp
- src/test/system_tests.cpp
- src/rpc/register.h
- src/node/interfaces.cpp
- src/Makefile.am
- src/qt/forms/createwalletdialog.ui
- src/qt/optionsmodel.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/optionsmodel.hsrc/qt/walletmodeltransaction.cppsrc/interfaces/wallet.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/spend.cppsrc/rpc/client.cppsrc/util/error.cppsrc/qt/createwalletdialog.cppsrc/qt/optionsdialog.cppsrc/wallet/rpc/addresses.cppsrc/qt/sendcoinsdialog.cppsrc/qt/walletmodel.hsrc/util/system.cppsrc/qt/createwalletdialog.hsrc/external_signer.cppsrc/qt/walletmodel.cppsrc/interfaces/node.hsrc/wallet/external_signer_scriptpubkeyman.cppsrc/external_signer.h
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_native_nowallet.shci/test/00_setup_env_mac_native_x86_64.shci/test/00_setup_env_mac.shci/test/00_setup_env_s390x.sh
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/Doxyfile.indoc/external-signer.md
🧠 Learnings (14)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/interfaces/wallet.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/spend.cppsrc/rpc/client.cppsrc/wallet/rpc/addresses.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/wallet/rpc/backup.cppdoc/external-signer.md
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/wallet/rpc/backup.cppconfigure.ac
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/wallet/rpc/spend.cppci/test/00_setup_env_native_nowallet.shci/test/00_setup_env_mac_native_x86_64.sh
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/wallet/rpc/spend.cppsrc/qt/sendcoinsdialog.cppsrc/external_signer.cppsrc/wallet/external_signer_scriptpubkeyman.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/rpc/client.cppsrc/wallet/rpc/addresses.cppsrc/wallet/external_signer_scriptpubkeyman.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/qt/sendcoinsdialog.cpp
📚 Learning: 2025-01-06T09:51:03.167Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
Applied to files:
configure.ac
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
ci/test/00_setup_env_native_nowallet.sh
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
ci/test/00_setup_env_native_nowallet.sh
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
ci/test/00_setup_env_native_nowallet.sh
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
ci/test/00_setup_env_native_nowallet.sh
📚 Learning: 2025-01-02T08:33:26.751Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6516
File: ci/test/00_setup_env_mac.sh:11-11
Timestamp: 2025-01-02T08:33:26.751Z
Learning: The removal of DMG support in the macOS packaging process eliminates the need for python-based scripts or python3-setuptools in the build environment. The PACKAGES variable in ci/test/00_setup_env_mac.sh is vestigial due to the use of a general-purpose Docker container defined in contrib/containers/ci/Dockerfile.
Applied to files:
ci/test/00_setup_env_mac_native_x86_64.shci/test/00_setup_env_mac.sh
🧬 Code graph analysis (13)
src/interfaces/wallet.h (2)
src/qt/walletmodel.cpp (2)
displayAddress(582-592)displayAddress(582-582)src/wallet/interfaces.cpp (12)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/util/error.cpp (1)
src/util/translation.h (1)
Untranslated(48-48)
src/qt/createwalletdialog.cpp (2)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)src/qt/createwalletdialog.h (1)
CreateWalletDialog(24-44)
src/qt/optionsdialog.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/wallet/rpc/addresses.cpp (2)
src/wallet/rpc/util.cpp (2)
GetWalletForJSONRPCRequest(55-77)GetWalletForJSONRPCRequest(55-55)src/wallet/interfaces.cpp (18)
LOCK(249-258)LOCK(259-262)LOCK(290-294)LOCK(295-299)LOCK(352-360)LOCK(478-490)dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)
src/qt/walletmodel.h (1)
src/qt/walletmodel.cpp (2)
displayAddress(582-592)displayAddress(582-582)
src/util/system.cpp (1)
src/node/interfaces.cpp (1)
ENABLE_EXTERNAL_SIGNER(530-551)
src/qt/createwalletdialog.h (2)
src/interfaces/node.h (2)
interfaces(48-411)ExternalSigner(161-168)src/qt/createwalletdialog.cpp (6)
setSigners(115-134)setSigners(115-115)isExternalSignerChecked(161-164)isExternalSignerChecked(161-161)CreateWalletDialog(17-108)CreateWalletDialog(110-113)
src/external_signer.cpp (4)
src/external_signer.h (1)
ExternalSigner(18-64)src/util/system.cpp (2)
RunCommandParseJSON(1397-1434)RunCommandParseJSON(1397-1397)src/wallet/external_signer_scriptpubkeyman.cpp (2)
DisplayAddress(52-61)DisplayAddress(52-52)src/wallet/wallet.cpp (6)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)SignTransaction(1998-2015)SignTransaction(1998-1998)SignTransaction(2017-2030)SignTransaction(2017-2017)
src/qt/walletmodel.cpp (1)
src/wallet/interfaces.cpp (28)
dest(209-213)dest(209-209)dest(222-225)dest(222-222)dest(226-229)dest(226-226)dest(230-248)dest(230-233)dest(263-267)dest(263-263)dest(268-272)dest(268-268)m_wallet(157-160)m_wallet(165-165)m_wallet(174-174)m_wallet(175-175)m_wallet(181-181)m_wallet(182-182)m_wallet(442-445)m_wallet(446-449)m_wallet(521-521)m_wallet(522-522)m_wallet(523-523)m_wallet(524-524)m_wallet(525-525)m_wallet(526-526)m_wallet(531-531)m_wallet(591-591)
src/interfaces/node.h (2)
src/external_signer.cpp (1)
ExternalSigner(17-17)src/external_signer.h (1)
ExternalSigner(18-64)
src/wallet/external_signer_scriptpubkeyman.cpp (3)
src/wallet/wallet.cpp (24)
batch(550-550)batch(815-815)batch(938-938)batch(1178-1178)batch(1257-1257)batch(1313-1313)batch(1358-1358)batch(1370-1370)batch(1391-1391)batch(1515-1515)batch(1660-1660)batch(1744-1744)batch(1793-1793)batch(2239-2239)batch(2300-2300)batch(2307-2307)DisplayAddress(2505-2518)DisplayAddress(2505-2505)GetSolvingProvider(3734-3738)GetSolvingProvider(3734-3734)GetSolvingProvider(3740-3748)GetSolvingProvider(3740-3740)FillPSBT(2032-2081)FillPSBT(2032-2032)src/wallet/scriptpubkeyman.cpp (16)
GetID(801-804)GetID(801-801)GetID(2375-2379)GetID(2375-2375)TopUp(1410-1413)TopUp(1410-1410)TopUp(1943-2004)TopUp(1943-1943)GetSolvingProvider(678-681)GetSolvingProvider(678-678)GetSolvingProvider(2231-2234)GetSolvingProvider(2231-2231)FillPSBT(734-778)FillPSBT(734-734)FillPSBT(2288-2355)FillPSBT(2288-2288)src/external_signer.cpp (4)
Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)
src/external_signer.h (3)
src/external_signer.cpp (11)
ExternalSigner(17-17)NetworkArg(19-22)NetworkArg(19-19)Enumerate(24-60)Enumerate(24-24)DisplayAddress(62-65)DisplayAddress(62-62)GetDescriptors(67-70)GetDescriptors(67-67)SignTransaction(72-117)SignTransaction(72-72)src/wallet/external_signer_scriptpubkeyman.cpp (2)
DisplayAddress(52-61)DisplayAddress(52-52)src/wallet/wallet.cpp (6)
DisplayAddress(2505-2518)DisplayAddress(2505-2505)SignTransaction(1998-2015)SignTransaction(1998-1998)SignTransaction(2017-2030)SignTransaction(2017-2017)
🪛 LanguageTool
doc/external-signer.md
[style] ~70-~70: Consider a more concise word here.
Context: ...ue, "txid": } ``` ## Signer API In order to be compatible with Dash Core any signer...
(IN_ORDER_TO_PREMIUM)
[grammar] ~167-~167: Use a hyphen to join words.
Context: ... support address types, in a BIP44/49/84 compatible manner. The `walletdisplayad...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
doc/external-signer.md
14-14: Dollar signs used before commands without showing output
(MD014, commands-show-output)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Dollar signs used before commands without showing output
(MD014, commands-show-output)
48-48: Dollar signs used before commands without showing output
(MD014, commands-show-output)
49-49: Dollar signs used before commands without showing output
(MD014, commands-show-output)
59-59: Dollar signs used before commands without showing output
(MD014, commands-show-output)
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: main
🔇 Additional comments (12)
src/wallet/rpc/backup.cpp (1)
1957-1957: LGTM! Correct branding update.The change from "bitcoind" to "dashd" is appropriate for Dash Core and maintains consistency with the similar error message at line 1659.
src/qt/walletmodeltransaction.cpp (1)
29-32: LGTM! Simple setter complements the existing getter.The implementation is straightforward and correct. The method provides the ability to update the transaction reference after construction, which is necessary for the external signer workflow where transactions may be modified externally (e.g., signed by hardware wallet).
src/rpc/client.cpp (1)
241-241: LGTM – RPC parameter conversion entry added correctly.The new
external_signerparameter at index 7 forcreatewalletfollows the existing pattern and integrates cleanly with the external signer feature.src/util/error.cpp (1)
36-39: LGTM – External signer error cases added correctly.The new error strings are clear, appropriately untranslated (consistent with other error messages in this function), and correctly placed before the final assert.
doc/Doxyfile.in (1)
2096-2096: LGTM – Doxygen predefined macro updated appropriately.The change from
HAVE_BOOST_PROCESStoENABLE_EXTERNAL_SIGNERaligns with the broader architectural shift in this PR and ensures Doxygen correctly processes external signer code.src/qt/optionsmodel.h (1)
76-76: LGTM – New option enum member added correctly.The
ExternalSignerPathenum member is properly typed asQStringand follows the existing pattern. The placement betweenPruneSizeandSpendZeroConfChangeis reasonable for a new feature.ci/test/00_setup_env_mac_native_x86_64.sh (1)
13-13: LGTM – Boost process configuration removed consistently.The removal of
--with-boost-processaligns with the broader architectural shift away from boost process in favor of the external signer implementation.src/interfaces/node.h (2)
160-168: LGTM – External signer interface defined cleanly.The
ExternalSignerinterface is minimal and well-designed with a proper virtual destructor and a single pure virtual method for retrieving the signer name.
234-236: LGTM – List external signers method added appropriately.The
listExternalSigners()method correctly returns a vector ofunique_ptrfor proper ownership semantics and is appropriately declared as pure virtual.src/qt/walletmodel.h (1)
144-144: LGTM – Display address method added for external signer support.The
displayAddressmethod is properly exposed in the public API to support address verification on external signing devices.src/qt/optionsdialog.cpp (2)
142-146: Good compile-time gating of UI when external signing is disabled.Clear tooltip + disabled control prevents confusing UX. LGTM.
278-279: All ExternalSignerPath implementation verified—no changes needed.The enum, getter/setter, settings key persistence, signal connections, and mapper registrations are all correctly implemented and wired. Specifically:
ExternalSignerPathenum declared in OptionsModel (optionsmodel.h:76)- Getter returns
settings.value("external_signer_path")(optionsmodel.cpp:506-507)- Setter validates and persists with restart flag (optionsmodel.cpp:683-686)
- Settings key initialized with default empty string (optionsmodel.cpp:226-230)
- Signal connection properly invokes
showRestartWarning()(line 278)- Mapper registration correctly links UI widget to enum (line 345)
| public: | ||
| //! @param[in] command the command which handles interaction with the external signer | ||
| //! @param[in] fingerprint master key fingerprint of the signer | ||
| //! @param[in] chain "main", "test", "regtest" or "signet" | ||
| //! @param[in] name device name | ||
| ExternalSigner(const std::string& command, const std::string chain, const std::string& fingerprint, const std::string name); | ||
|
|
There was a problem hiding this comment.
Avoid shell injection; use argv‑style exec, not string concatenation.
Descriptor, fingerprint, and account values are concatenated into a shell command. If RunCommandParseJSON invokes a shell, this is injection‑prone.
- Prefer an exec API that takes a vector argv (Boost.Process child with args), no shell.
- If unavailable, introduce a robust ShellEscape and pass PSBT via stdin only (already done for SignTransaction).
Follow‑ups needed in external_signer.cpp for DisplayAddress/GetDescriptors. Based on learnings.
Also applies to: 49-58, 60-64
🤖 Prompt for AI Agents
In src/external_signer.h around lines 29 to 35, the constructor currently
accepts a single command string which leads to later string concatenation and
shell-injection risk; change the interface and implementation to accept an
argv-style command (e.g. vector<string> or initializer_list<string>) or
otherwise store command + args separately so callers can run the signer with an
exec API that takes a string vector (Boost.Process child with args) instead of
invoking a shell. Update external_signer.cpp call sites
(DisplayAddress/GetDescriptors and the other spots noted at lines 49-58 and
60-64) to build and pass argument vectors rather than concatenating into one
shell string; if an argv-style exec is not available, introduce a robust
ShellEscape utility and ensure sensitive data (descriptor/fingerprint/account)
is never injected via the shell and PSBTs are passed only via stdin. Ensure
constructor and member types reflect the new command-args representation and
adapt all RunCommandParseJSON invocations to use the arg vector exec path.
7 participants
Issue being fixed or feature implemented
This PR introduce support of external signers (for example, hardware signers such as Ledger) which could be used to sign transactions, using existing feature PSBT.
What was done?
Bitcoin backports from Bitcoin Core v22, v23, v24 to support hardware wallets in Dash Core.
This PR is not the smallest possible set of backports, but it is set that provide working external signer in both CLI and GUI versions of Dash Core. Extra backports to improve user experience are to be done in the next PRs, see #6913
How Has This Been Tested?
Got successfully signed transaction using a repo https://github.com/knst/hwi/ (WIP, see also knst/HWI#1)
Breaking Changes
It introduces multiple new RPC, doesn't seems as any really breaking change
Checklist: