Merge bitcoin-core/gui#555: Restore Send button when using external signer#7271
Conversation
…igner 2efdfb8 gui: restore Send for external signer (Sjors Provoost) 4b5a6cd refactor: helper function signWithExternalSigner() (Sjors Provoost) 026b5b4 move-only: helper function to present PSBT (Sjors Provoost) Pull request description: Fixes dashpay#551 For the simplest use case of a wallet with one external signer and "PSBT Controls" disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there's no "Create Unsigned" button. When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction. In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before dashpay#441). This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review. ACKs for top commit: jonatack: utACK 2efdfb8 diff review since my last review, code re-review, rebased to current master, verified clean debug build of each commit luke-jr: utACK 2efdfb8 Tree-SHA512: e8731a0ef9e87564b2676c7b022b742d9621bba964c19dba9fd9f6961eb608737a9e1a22c0a3c8b2f2f6d583bba067606ee8392422e82082deefb20ea7b88c7c
|
✅ Review complete (commit 3af06ee) |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes refactor PSBT handling in the send coins dialog by extracting two new private methods: Sequence DiagramsequenceDiagram
participant User
participant Dialog as SendCoinsDialog
participant Wallet
participant Signer as External Signer
participant File as File System
User->>Dialog: Click Send (with external signer)
Dialog->>Wallet: fillPSBT (without signing)
Wallet-->>Dialog: unsigned PSBT
Dialog->>Dialog: signWithExternalSigner(psbtx)
Dialog->>Signer: Sign PSBT
alt Signing Successful
Signer-->>Dialog: Signed PSBT
Dialog->>Wallet: FinalizeAndExtractPSBT
Wallet-->>Dialog: Complete transaction
Dialog->>Dialog: Broadcast transaction
else Signing Failed/Incomplete
Signer-->>Dialog: Error or incomplete
Dialog->>Dialog: presentPSBT(psbtx)
Dialog->>File: Save to .psbt file (optional)
Dialog-->>User: Prompt user to save or discard
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/sendcoinsdialog.cpp`:
- Around line 509-513: presentPSBT() shows hard-coded English and a misleading
title ("Unsigned Transaction"); replace the literals with neutral, translatable
strings using QObject::tr() (e.g., title like tr("PSBT") or tr("Partially Signed
Transaction") and informative text like tr("The PSBT has been copied to the
clipboard. You can also save it.")), and update the other occurrences (the block
around the 557-565 region and any external-signer error boxes) to use tr() as
well so translations can pick them up and the wording is accurate for both
unsigned and partially-signed fallbacks.
- Around line 607-612: Replace the assertions after calls to
model->wallet().fillPSBT(...) with explicit error handling: check the returned
TransactionError (err) and the completion flag (complete) for both fillPSBT
invocations, show an appropriate user-facing error/dialog message when err !=
TransactionError::OK or when complete is unexpectedly true/false (depending on
the call site), and return early instead of proceeding to presentPSBT(...) or
signWithExternalSigner(...) so a partially-filled PSBT is never used; reference
the model->wallet().fillPSBT call, the TransactionError enum, the psbtx/complete
variables, and the downstream presentPSBT and signWithExternalSigner calls to
locate where to add the checks and early returns.
- Around line 536-539: The code currently writes ssTx to an ofstream and
immediately emits a success message via Q_EMIT message(...) without checking the
stream result; update the sendcoinsdialog.cpp write block to validate that the
file was opened and the write succeeded by testing out.is_open()/out.good() (or
checking out.fail()/out) after writing ssTx.str(), and if the stream indicates
failure emit an error message via Q_EMIT message(tr("PSBT save error"), /*
descriptive message including filename */ , CClientUIInterface::MSG_ERROR)
instead of the success message; only emit the existing success Q_EMIT
message(tr("PSBT saved"), ...) when the write completed successfully, and keep
closing the stream as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5800b3a-6cdf-412c-b5d3-5e62f1d569d0
📒 Files selected for processing (2)
src/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.h
| QMessageBox msgBox; | ||
| msgBox.setText("Unsigned Transaction"); | ||
| msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it."); | ||
| msgBox.setStandardButtons(QMessageBox::Save | QMessageBox::Discard); | ||
| msgBox.setDefaultButton(QMessageBox::Discard); |
There was a problem hiding this comment.
Use neutral, translated copy in the new PSBT/external-signer messages.
presentPSBT() is also used for the partially signed fallback on Line 637, so "Unsigned Transaction" is misleading there. The new strings here and in the external-signer error boxes are also hard-coded English, so they will not be picked up by Qt translations.
Also applies to: 557-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/sendcoinsdialog.cpp` around lines 509 - 513, presentPSBT() shows
hard-coded English and a misleading title ("Unsigned Transaction"); replace the
literals with neutral, translatable strings using QObject::tr() (e.g., title
like tr("PSBT") or tr("Partially Signed Transaction") and informative text like
tr("The PSBT has been copied to the clipboard. You can also save it.")), and
update the other occurrences (the block around the 557-565 region and any
external-signer error boxes) to use tr() as well so translations can pick them
up and the wording is accurate for both unsigned and partially-signed fallbacks.
| std::ofstream out{filename.toLocal8Bit().data(), std::ofstream::out | std::ofstream::binary}; | ||
| out << ssTx.str(); | ||
| out.close(); | ||
| Q_EMIT message(tr("PSBT saved"), "PSBT saved to disk", CClientUIInterface::MSG_INFORMATION); |
There was a problem hiding this comment.
Check the file write before telling the user the PSBT was saved.
If the file cannot be opened or written, this still emits a success message and the dialog is closed afterward, leaving the user with only the clipboard copy. Please validate the stream state and surface an error instead.
Proposed fix
std::ofstream out{filename.toLocal8Bit().data(), std::ofstream::out | std::ofstream::binary};
- out << ssTx.str();
- out.close();
- Q_EMIT message(tr("PSBT saved"), "PSBT saved to disk", CClientUIInterface::MSG_INFORMATION);
+ const std::string psbt = ssTx.str();
+ out.write(psbt.data(), psbt.size());
+ out.close();
+ if (!out) {
+ Q_EMIT message(tr("PSBT save failed"),
+ tr("Could not write the PSBT to \"%1\".").arg(filename),
+ CClientUIInterface::MSG_ERROR);
+ return;
+ }
+ Q_EMIT message(tr("PSBT saved"), tr("PSBT saved to disk"), CClientUIInterface::MSG_INFORMATION);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/sendcoinsdialog.cpp` around lines 536 - 539, The code currently writes
ssTx to an ofstream and immediately emits a success message via Q_EMIT
message(...) without checking the stream result; update the sendcoinsdialog.cpp
write block to validate that the file was opened and the write succeeded by
testing out.is_open()/out.good() (or checking out.fail()/out) after writing
ssTx.str(), and if the stream indicates failure emit an error message via Q_EMIT
message(tr("PSBT save error"), /* descriptive message including filename */ ,
CClientUIInterface::MSG_ERROR) instead of the success message; only emit the
existing success Q_EMIT message(tr("PSBT saved"), ...) when the write completed
successfully, and keep closing the stream as needed.
| TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete); | ||
| assert(!complete); | ||
| assert(err == TransactionError::OK); | ||
|
|
||
| // Copy PSBT to clipboard and offer to save | ||
| presentPSBT(psbtx); |
There was a problem hiding this comment.
Handle fillPSBT() failures explicitly instead of asserting.
fillPSBT() is a runtime wallet call that already returns TransactionError, not an invariant check. In release builds these asserts disappear, so a failed prefill can still fall through to presentPSBT() or signWithExternalSigner() with a half-populated PSBT.
Also applies to: 623-626
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/sendcoinsdialog.cpp` around lines 607 - 612, Replace the assertions
after calls to model->wallet().fillPSBT(...) with explicit error handling: check
the returned TransactionError (err) and the completion flag (complete) for both
fillPSBT invocations, show an appropriate user-facing error/dialog message when
err != TransactionError::OK or when complete is unexpectedly true/false
(depending on the call site), and return early instead of proceeding to
presentPSBT(...) or signWithExternalSigner(...) so a partially-filled PSBT is
never used; reference the model->wallet().fillPSBT call, the TransactionError
enum, the psbtx/complete variables, and the downstream presentPSBT and
signWithExternalSigner calls to locate where to add the checks and early
returns.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the Qt external-signer backport and did not find a code-backed regression on this SHA. The Send/Create Unsigned split is restored as intended for external-signer wallets, the fallback to presenting a PSBT after incomplete external signing still works, and the backport-prerequisite check did not uncover a missing upstream dependency.
Reviewed commit: 3af06ee
5 participants
Issue being fixed or feature implemented
Fixes bitcoin-core/gui#551
For the simplest use case of a wallet with one external signer and "PSBT Controls" disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there's no "Create Unsigned" button.
When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction.
In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before #441).
This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review.
What was done?
Backport bitcoin-core/gui#555
How Has This Been Tested?
Breaking Changes
N/A
Checklist: