qt: preserve and display consumed PQC usage in transaction signing flows - #167
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kiwidream
changed the base branch from
kiwidream/integration-base-146-161
to
1.x.x
September 16, 2026 14:40
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83ceebb1ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/qt/test/apptests.cpp
A transaction-signing attempt can consume PQC signature capacity and then fail, or succeed through a path that discarded its usage report. The Qt dialogs dropped that report, so the wallet silently lost track of capacity that had already been spent. Add a shared transaction formatter, qt/pqcusageformat, that renders a usage report as the overall state, every affected key with its counter, limit, remaining capacity and limit state, and every usage warning. An empty report formats to nothing, so an absent report is never presented as zero consumption, and an outcome sentence distinguishes a consuming success from a failure after consumption. Send preparation failures now keep their original reason and severity and append the attempt's usage before the prepared transaction is discarded. The PSBT dialog uses the shared formatter and reports failure outcomes as failures. Fee-bump signing failure, commit failure and success all present the attempt's usage; the error boxes are plain text, so backend error strings are shown literally, and the success message precedes the state reset. The presentation paths re-check the model and the attempt generation after every modal, so a model destroyed during presentation changes no further state. Assign the usage report in the wallet interface's createTransaction before its error return, so a normal signing failure still reports the capacity it reserved. Portable transactions and PSBTs are unchanged: usage stays wallet-local. Refs #141.
kiwidream
force-pushed
the
kiwidream/preserve-usage-in-qt-transactions
branch
from
September 16, 2026 20:23
83ceebb to
89bb2f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89bb2f0568
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
WalletModel::bumpFeePrepared presents several modal dialogs, each of which runs a nested event loop. Unloading the wallet during one of those loops destroys the model, and every path then returned into the destroyed object: the prepare-failure and draft-failure boxes and the copied-PSBT message called resetBumpFeeState(), the fee bump confirmation read m_bump_fee_generation and the cancellation state, and the unlock path wrote m_bump_fee_unlock_context. Each one is a use-after-free. Apply the guard bumpFeeFinished already uses: hold a QPointer to the model, and check it together with the attempt generation after every nested loop before touching a member or emitting a signal. The short-circuit keeps the member read from happening once the model is gone. The unlock context is acquired across such a loop, so hold it in a local until the model is known to have survived and only then hand it over. A destroyed model cannot take ownership, and dropping the context releases the unlock rather than leaking it into freed memory. requestUnlock() itself runs the passphrase dialog in a nested loop and then reads the encryption status, so it reports an invalid context when the model did not survive. Its other callers already treat an invalid context as a failed unlock, so they stop reading freed memory too. Add a regression test per guarded loop, destroying the model from inside the dialog. Under an address sanitizer build, removing any one guard reports a heap-use-after-free at that site and the tests fail; with the guards in place the suite passes.
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A transaction-signing attempt can consume PQC signature capacity and then fail, or succeed through a path that discarded its usage report. The Qt dialogs dropped that report, so the wallet silently lost track of capacity that had already been spent.
This adds a shared transaction formatter,
qt/pqcusageformat, that renders a usage report as the overall state, every affected key with its counter, limit, remaining capacity and limit state, and every usage warning. An empty report formats to nothing, so an absent report is never presented as zero consumption. An outcome sentence distinguishes a consuming success from a failure after consumption.createTransactioninwallet/interfaces.cppassigns the usage report before its error return, so a normal signing failure still reports the capacity it reserved.Portable transactions and PSBTs are unchanged: usage stays wallet-local.
Refs #141.
Testing
Linux (Qt 6.4.2, GCC 13.3) before the rebase, and re-run on macOS after it.
RelWithDebInfoin both cases.On macOS the GUI suites must be run with
QT_QPA_PLATFORM=cocoa; under the defaultminimalplatformAppTestsandWalletTestsreport as passing without executing (0 ms). The results below are from the cocoa run.cmake --build build --target test_bitcoin-qtand the fulltest_qbit-qtsuite under Xvfb/xcb: all suites pass, includingWalletTests 3/0andPSBTOperationsDialogTests 13/0with the newusageStatesOnSuccessAndFailureslot. The same base is all-pass without this change.test_qbit -t 'wallet_tests,feebumper_tests,wallet_p2mr_parallel_signing_tests,spend_tests': 62 cases, 1214 assertions, pass.feeBumped, forcing the informational severity, rendering the error box as auto-detected rich text, and adding usage metadata to a portable PSBT or transaction or replacing a failed PSBT. Reverting only theinterfaces.cppreorder fails the new real-wallet regression withusage.key_states.size() == 1U ... [0 != 1], after a check proves the durable counter was already reserved.ci/lint_imagefile):Success: no issues found in 356 source files.Target Branch
mainor a maintainer-requested release branch such as0.1.x.Targets
1.x.x.#161 has landed, so #146 is the only remaining dependency. The branch is rebased onto current
1.x.xwith #146 merged in, so until #146 lands, "Files changed" against1.x.xincludes its content as well.For review, use the #141-only diff:
831e01e6...89bb2f05— 14 files, one commit. That base is also pushed askiwidream/integration-base-146if a branch ref is easier to work with. Once #146 lands, the1.x.xdiff collapses to exactly those 14 files on its own.Risk / Review Notes
Notes:
Wallet-facing presentation plus one interface-transport reorder. This is stacked on #146's asynchronous fee-bump transport. The review base is
831e01e615, which is current1.x.xwith #146 merged.A follow-up for cross-dialog consistency is tracked in #166: the sign/verify message dialog still formats limit states with its own local helper and untranslatable enum names. That dialog is out of scope here.
Operation generations, cancellation boundaries, unlock lifetime and worker-thread cleanup are preserved. Portable transactions and PSBTs keep their exact bytes; the new tests assert that directly. Exception paths below Qt still lose usage, as documented previously; this change does not widen them. The external-signer route reports nothing, so no hardware usage is ever claimed.
Docs / Process Impact
libbitcoinpqc Subtree Checklist (if
src/libbitcoinpqcchanged)Not applicable; the subtree is untouched.