qt: move P2MR fee bump signing off the GUI thread

[kiwidream]

Summary

Fixes #142.

Fee-bump preparation, signing, and commit previously ran synchronously on the Qt GUI thread, while feebumper::SignTransaction() held cs_wallet across the complete signing operation. P2MR cryptographic signing could therefore freeze the GUI and prevent other wallet work from acquiring the wallet lock.

This change:

• runs fee-bump preparation and signing/commit on a lifetime-safe cloned wallet interface;
• removes the fee-bumper's outer wallet lock so internal signing uses the existing snapshot-under-lock, sign-without-lock lifecycle;
• reports queued preparation, counter reservation, input signing, finalization, and commit progress;
• permits cancellation only before the durable PQC counter-reservation boundary;
• revalidates inputs, competing wallet spends, fees, current relay/mempool policy, standard weight, and chain limits before commit;
• rejects signer changes outside input signature and witness fields; and
• adds deterministic wallet-lock, GUI responsiveness, cancellation, revalidation, dialog/model destruction, external-signer, wallet-unload, and shutdown coverage.

Structured PQC usage data is preserved through the worker result, but user-facing usage presentation remains out of scope for #141.

Testing

• Built locally.
• Ran focused unit or functional tests for the changed area.
• Ran lint or formatting checks relevant to this change.
• Not run. Reason: All planned checks were run.

Commands and results:

• cmake --build build -j6 --target bitcoin_wallet bitcoinqt test_bitcoin test_bitcoin-qt — passed.
• feebumper_tests/p2mr_signing_releases_wallet_lock — passed with a deterministic post-reservation/pre-crypto latch proving another thread can acquire cs_wallet.
• feebumper_tests/commit_revalidates_fee_after_signing — passed.
• feebumper_tests/commit_revalidates_competing_wallet_spend — passed.
• wallet_tests/sign_transaction_* — four signing progress/cancellation tests passed.
• wallet_p2mr_batch_reservation_tests — three tests passed.
• feebumper_tests/external_max_weight_test — passed.
• ulimit -n 1024 && build/test/functional/test_runner.py --jobs=2 wallet_bumpfee.py feature_rbf.py — both passed.
• ulimit -n 1024 && QT_QPA_PLATFORM=cocoa QTEST_FUNCTION_TIMEOUT=600000 build/bin/test_qbit-qt — all wallet and application lifecycle suites passed; the binary still reports the unrelated existing RPCNestedTests::rpcNestedTests expected-hash mismatch in unchanged code.
• Repository Docker lint workflow — exited 0 with all checks passed.
• git diff --check origin/1.x.x...HEAD — passed.

Target Branch

• This PR targets main or a maintainer-requested release branch such as 0.1.x.

Target: 1.x.x.

Risk / Review Notes

• Consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.
• No consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.

Notes:

• Internal P2MR crypto no longer runs with the fee-bumper's outer cs_wallet lock held.
• External signing is off the GUI thread, but its internal FillPSBT path may retain cs_wallet while an external command runs.
• RPC bumpfee retains its existing caller-side wallet lock; this PR targets the Qt lifecycle from Move P2MR fee-bump signing off the qbit-qt GUI thread #142.
• Mempool state can still change between final revalidation and broadcast, so normal submission-time validation remains authoritative.

Docs / Process Impact

Choose exactly one:

• I updated public docs because this PR changes user-visible behavior, integration guidance, release/process guidance, or expected validation.
• No public docs update needed. Reason: This changes internal Qt threading, locking, and progress handling without changing the documented fee-bump interface.

libbitcoinpqc Subtree Checklist (if src/libbitcoinpqc changed)

• Source commit is reachable from an immutable release tag in Qbit-Org/qbit-libbitcoinpqc.
• qbit imports the tagged upstream tree directly without pruning or a curated subtree branch.
• Subtree import/update was performed with contrib/devtools/update-libbitcoinpqc-subtree.sh.
• test/lint/libbitcoinpqc-subtree-check.sh passes locally.
• Any default tag change in contrib/devtools/update-libbitcoinpqc-subtree.sh is intentional and matches doc/subtrees/libbitcoinpqc.md.

---

Note

High Risk
Touches wallet fee-bump signing, PQC counter reservation cancellation, and Qt lifecycle/shutdown paths. Incorrect cancellation or revalidation could consume counters, commit stale replacements, or leave the GUI/wallet in a bad state.

Overview
Moves Qt fee-bump preparation, signing, and commit off the GUI thread onto a cloned wallet worker, with a progress dialog and a new feeBumped completion signal.

Cancellation is cooperative only until the durable PQC counter-reservation or external-signer command boundary; after that the operation becomes irreversible and still completes through shutdown/teardown. Commit now revalidates inputs, competing spends, fees, and current relay/mempool policy, and rejects signer changes outside input signature/witness fields.

Adds broad Qt and wallet coverage for responsiveness, cancel races, revalidation failures, external-signer mutation rejection, and model/dialog destruction during an in-flight bump.

^{Reviewed by Cursor Bugbot for commit f8989e3. Bugbot is set up for automated code reviews on this repo. Configure here.}

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9723919. Configure here.}

[kiwidream]

PR Review

Findings

• [medium] src/wallet/test/feebumper_tests.cpp:100 — The wallet-lock regression pauses at the first non-cancellable reservation notification, which src/script/sign.cpp emits before durable reservation and before cryptographic signing. It catches the old outer-lock bug, but does not prove that another wallet operation can proceed while cryptographic signing itself is paused, as requested by Move P2MR fee-bump signing off the qbit-qt GUI thread #142.

  • Potential resolution: Latch inside the raw PQC signer, or after confirmed reservation immediately before raw signing, then acquire cs_wallet or perform a wallet operation from another thread.

• [medium] test/functional/wallet_signer.py:211 — The external-signer integrity check only has positive-path coverage. The mock now signs the exact PSBT and proves that a legitimate response passes, but no test changes an output, locktime, prevout, or sequence and demonstrates that SigningOnlyModifiedInputSignatures() rejects it. Because this guard protects wallet funds from signer-side transaction substitution, its rejection path deserves direct coverage.

  • Potential resolution: Add a mock-signer mode that returns a validly structured PSBT with one protected transaction field changed, then assert bumpfee fails and no replacement is committed. Cover at least an output and one input/locktime field.

No blocking correctness findings from this review pass.

Consensus Impact

Consensus-adjacent

The PR does not change block or transaction validity, but it changes P2MR counter-reservation/signing flow in src/script/sign.cpp and src/script/signingprovider.cpp, plus wallet replacement-policy and commit behavior in src/wallet/feebumper.cpp. Issue #142 explicitly authorizes this wallet-signing risk. The required evidence is therefore cancellation-boundary, counter-persistence, external-signer, RBF-policy, and lifecycle coverage; most is present, subject to the findings above.

Review Scope

Reviewed issue #142 as the contract, all eight commits and 22 changed files against the actual 1.x.x base, surrounding signing/wallet/Qt call paths, PR description, current checks, and all nine visible review threads. The PR is open and mergeable with a blocked merge state; all visible threads are resolved.

Deep Checks Performed

Traced preparation, unlock, signing, counter reservation, commit, cancellation, shutdown, wallet unload, and QObject destruction paths. Checked serial and parallel P2MR signing, external-signer PSBT replacement, PQC usage propagation, replacement ancestry, external inputs, competing wallet spends, fee/policy revalidation, and the fixes for every earlier inline finding.

Issue Fit

The implementation substantially matches #142:

• Preparation and signing/commit move off the GUI thread.
• The fee-bumper’s outer cs_wallet lock is removed.
• Cancellation and durable counter reservation share an atomic boundary.
• Commit revalidates inputs, lineage, fees, weight, policy, and chain limits.
• Structured PQC usage remains available to the result.
• External-signer and normal RBF behavior retain functional coverage.

PQC usage presentation remains correctly outside scope. No unrelated consensus or public RPC behavior was added.

qbit-Specific Checks

Reviewed P2MR serial/parallel counter durability, cancellation after reservation, provider snapshot lifetime, wallet relocking, external-signer command boundaries, replacement-chain metadata, and worker teardown during shutdown/unload. AuxPoW, ASERT, PHOTON, and release-signing surfaces are untouched.

Validation Reviewed

Current CI is green at 7485c3f00688. Logs confirm passing feebumper_tests, wallet_p2mr_parallel_signing_tests, Qt tests on Windows and sanitizer Linux, and full functional coverage including wallet_bumpfee.py and wallet_signer.py. Lint, P2MR conformance, sanitizer, Windows, and required merge gates also passed.

The author additionally reports focused wallet/Qt tests, wallet_bumpfee.py, feature_rbf.py, and the Cocoa Qt lifecycle suite. I independently ran git diff --check; no local build tree was available for an additional compile or test run.

Residual Risk

• The two adversarial cases identified above remain unproven.
• External-signer FillPSBT may still hold cs_wallet while its command runs, as disclosed in the PR.
• Mempool state can still change between final revalidation and submission; submission-time validation remains authoritative.

[kiwidream]

Follow-up to the written review, with the independent review feedback addressed at 4c87fd0288:

• Wallet-lock proof: p2mr_parallel_signing_releases_wallet_lock_mid_batch pauses after one real PQC signature in a two-input batch with one signing worker. Both counters are durably reserved, the second signature is pending, and witnesses have not been published. Another thread acquires cs_wallet at that point. Temporarily restoring the old outer lock makes the assertion fail; assertion cleanup releases the worker before joining it.
• External-signer substitutions: The mock returns validly signed replacements with an output value or input sequence changed. Each case reaches the integrity guard and is rejected with unchanged mempool, wallet transaction count, and replacement metadata. The exact-request positive control passes. Temporarily removing each corresponding integrity check makes its negative test fail.
• Qt assertion cleanup: 4c87fd0288 adds scope guards in all ten fee-bump lifecycle blocks, releasing every synthetic latch before model destruction joins the worker. An injected assertion previously caused a 20-second watchdog abort. Injected assertions at the signing, counter-reservation, and external-command boundaries now exit cleanly with the intended test failure. The independent follow-up review confirmed the fix.
• Current base: Signed merge a742909c98 integrates 1.x.x at 467d95b430, including descriptor publication after commit and self-transfer presentation. The overlapping wallet paths were reviewed, and asynchronous PSBT signing is preserved.

Local validation passed: full native build, 81 distinct wallet/unit cases across focused and full-wallet selections, all 12 Qt suites (75 entries), wallet_bumpfee.py, feature_rbf.py, wallet_signer.py, wallet_listsinceblock.py, and Docker lint. All temporary fault injections were removed before the final passing tests.

Fresh Core Checks, Full Validation, and Required Merge Gate are still running on this head; earlier green CI does not validate these latest commits. TSan excludes Qt, and Cocoa wallet tests disable native dialogs. Shutdown after durable counter reservation intentionally waits for signing to finish. PQC usage presentation remains outside #142.

All ten existing inline review threads are already resolved.

[kiwidream]

Closing this as shipped rather than abandoned: the work in this PR is on 1.x.x.

#167 was stacked on this branch and was squash-merged as 4bb6462, which carried every commit from here with it. The squash body lists them — "qt: move P2MR fee bump signing off GUI thread", "qt: harden async fee bump cancellation", "wallet: harden fee bump progress and revalidation", and the rest. 1.x.x now has bumpFeePrepared, startBumpFeeSigning, BumpFeeCancellationState and the asynchronous fee-bump transport from this branch.

That also means this PR can no longer be merged. The tree difference between 1.x.x and this branch is now +58/−1825, and every one of those 58 lines is superseded code — the previous AppendPQCUsageStatus, the previous processSendCoinsReturn signature, the previous MessageBoxClicker constructor. Merging it would revert the work that landed on top.

For the record, since it explains the red CI here: the three failing checks on this branch were never a real defect. All three traced to one job failing AppTests::appTests() on 'm_shutdown_elapsed_ms < 5000' at apptests.cpp:250 — a wall-clock bound that sanitizer and debug-build overhead exceeds, not a shutdown regression and not a sanitizer finding. The two gates simply aggregated that job. 1.x.x has since replaced that bound with deterministic sequence markers recording the order of worker completion, dialog destruction and teardown return, which is why the same ASan job passed on #167.

A related use-after-free in bumpFeePrepared, raised in review on #167, was fixed before that merge: every nested event loop in it now re-checks the model and the attempt generation before touching a member or emitting a signal, with a regression test per loop. That shipped in the same squash.

The branch is left at its merge with 1.x.x (24617cb) and can be deleted.