Skip to content

wallet: batch P2MR counters across mixed descriptors - #163

Open
kiwidream wants to merge 8 commits into
1.x.xfrom
kiwidream/batch-mixed-descriptor-signing
Open

kiwidream wants to merge 8 commits into
1.x.xfrom
kiwidream/batch-mixed-descriptor-signing

Conversation

@kiwidream

@kiwidream kiwidream commented Sep 15, 2026

Copy link
Copy Markdown
Member

Summary

Preserve batched, parallel P2MR signing when a transaction spends from more than one P2MR descriptor (for example internal and external change). Closes #155. Depends on #146; merge this PR after #146. This branch includes the earlier #146 implementation merged with 1.x.x; the issue-specific review range starts at c72d548640. The current #146 tip also has a later Qt test cleanup, which is not included here.

  • The parallel planner now classifies inputs per signing provider. Inputs that are strictly foreign to the current provider (no complete plan, no invalid existing signature, and no leaf key the provider can sign) are skipped for that provider instead of forcing the whole provider into serial signing. Every other bailout (missing data, non-P2MR, SIGHASH_SINGLE without an output, owned but not completely plannable) keeps its serial fallback, so partial signatures and error text are unchanged.
  • Foreign inputs are never signed by that provider, keep their original index, and leave an error, so a provider-local batch can never report whole-transaction success. Each provider still makes exactly one durable counter reservation for its own inputs, at the existing reservation site where qt: move P2MR fee bump signing off the GUI thread #146's reservation guard runs; there is no second reservation site and no cross-manager atomic commit.
  • A provider whose own inputs are all complete and which sees only foreign inputs now returns false instead of falling back to serial signing. The serial path cannot recognise an already-complete P2MR witness as complete and would re-sign it, consuming fresh counters; the parallel path already avoids that. This makes a retry after a partial failure reserve counters only for the descriptor that still has work.
  • Progress accounting counts only complete inputs, so the completed count is monotonic across descriptors within one wallet operation.
  • Cancellation is scoped to the whole wallet operation: once the caller declines, later descriptors stop before their counter reservation, the dummy pass is skipped and the call returns false with "Signing cancelled"; counters an earlier descriptor already committed stay consumed, and a retry with the same transaction reuses the existing witnesses. The Qt fee-bump path already behaves this way through its own sticky state. A reservation or signing failure (not a cancellation) in one descriptor still lets the others continue, as before.
  • Five new wallet tests use one real wallet with both managers and interleaved inputs in both orders, observe durable commits at the database boundary by descriptor id, verify the final transaction independently against all real coins for several hash types, reload persisted counters, and exercise failure before and after each descriptor's commit, retries, and cancellation at each boundary including the serial fallback.

Testing

  • Built locally (Debug, tests and benchmarks enabled).
  • At e98c88fb4e, 207 cases pass across pqc_tests, script_p2mr_tests, wallet_p2mr_parallel_signing_tests, wallet_p2mr_batch_reservation_tests, wallet_tests, feebumper_tests, scriptpubkeyman_tests, and walletdb_tests, including qt: move P2MR fee bump signing off the GUI thread #146's reservation-boundary tests. The full 1,275-case suite passed before the observer follow-up.
  • Docker lint passes on a clean clone of e98c88fb4e.
  • Twelve coordinator mutation variants are caught, with every restored named test passing: foreign-input fallback/error/completion mistakes, sighash context/index errors, early signing, reservation retry, rolled-back memory counters, cancellation guard/latch/error loss, and signing outside the reserved range. Actual CPQCKey::Sign calls are ordered against durable database commits.
  • Not run: post-commit raw signer failure injection at the real wallet entry (accepted instrumentation gap) and real-storage timing. Independent adversarial review is pending and will follow PR creation.

Measurements

These measurements were collected at b7f0fcebf6, before the primitive-observer follow-up; its overhead has not been measured.

Three paired processes per arm (A/B, B/A, A/B), on an Apple M5 Max, 18 cores, macOS 26.4, Apple clang 17, Release. Each mixed transaction spends 25 internal and 25 external inputs with four configured workers per provider. Baseline compiles out the foreign-input skip; candidate retains it. All six processes exited 0 and independently verified every completed transaction.

Mixed case, hook detached Baseline Candidate
Raw elapsed times (s) 13.455341, 13.599356, 13.561552 6.860967, 6.456946, 6.889978
Median (range), s 13.561552 (13.455341–13.599356) 6.860967 (6.456946–6.889978)
Durable commits 26 2
Signatures / counter delta 50 / 50 50 / 50

With the recorder attached, the candidate made exactly one durable commit per descriptor, reserving 25 counters each; baseline made 25 serial commits for one descriptor plus one batch for the other. The external-only control used one commit in both arms. These are shared-host measurements using an in-memory test database; real-storage cost and production speedup remain unmeasured. Hook-on and hook-off samples do not isolate instrumentation overhead.

Target Branch

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: wallet signing. Required invariants: no signature before durable reservation and no counter reuse. A scoped, null-by-default observer now checks actual signing calls against earlier successful reservations and rejects repeated counter use. Installation and removal require quiescent signing threads; callbacks are synchronized in wallet tests. The cryptographic backend is unchanged. Whole-transaction completion and refusal of serial retries after reservation failure were verified by coordinator mutation checks. Known pre-existing limitation, unchanged: when a provider does fall back to serial signing, the serial loop re-signs already-complete P2MR inputs it owns and consumes fresh counters. Transactions mixing P2MR and non-P2MR inputs still sign serially.

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: no RPC, option or default change; cancellation semantics for the GUI are unchanged.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Review follow-up

  • Initialize all fixture fields for compilers that treat missing aggregate initializers as errors.
  • Recognize already-satisfied foreign P2MR leaves using the full spent-output context, including no-work retries.
  • Revalidate every replacement input against live coins and mempool spenders from the same snapshot. This also addresses the inherited fee-bump review finding: an unseen conflict cannot be hidden by a cached wallet parent. The original replacement lineage remains allowed.
  • Regression tests reproduced both unseen mempool conflicts and spent wallet outputs before the fix, plus the false provider error on a valid foreign witness.
  • Validation: build with warnings treated as errors, 210 targeted regression tests, wallet_bumpfee.py, and Docker lint all passed. The dependency on qt: move P2MR fee bump signing off the GUI thread #146 remains.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-16T20:09:03.922932Z 4a475ae New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e98c88fb4e

ℹ️ 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".

Comment thread src/wallet/feebumper.cpp Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c8e658b80

ℹ️ 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".

Comment thread src/wallet/feebumper.cpp

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b45d02848

ℹ️ 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".

Comment thread src/qt/walletmodel.cpp
kiwidream and others added 8 commits September 18, 2026 12:03
A parallel signing provider used to fall back to serial signing for its
whole descriptor as soon as it met an input it could not plan, such as
an input owned by another descriptor. A transaction spending from the
external and internal P2MR descriptors therefore made one durable
counter commit per signature for the first descriptor.

Plan strictly foreign inputs (no complete plan, no invalid existing
P2MR signature, and no signable key in any leaf the provider sees) as
skipped: they get no jobs, stay untouched and keep an error at their
original index, so a provider never reports the transaction complete
while one exists. Every other planning bailout keeps its serial
fallback. A provider whose own inputs are all complete and that only
sees foreign inputs reports failure instead of falling back to serial
signing, which would re-sign the complete inputs and reserve fresh
counters. Signing progress seeds only complete inputs so the completed
count stays monotonic across descriptors.

Cancellation now applies to the whole wallet signing operation: once
the caller declines, later callbacks decline too, so the next
descriptor stops before its reservation boundary and "Signing
cancelled" is preserved. Counters an earlier descriptor committed stay
consumed; a retry of the same transaction reuses its witnesses.

Refs #155

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MixedManagersPreserveSighashContext completes its unowned input with a
constructed provider before the wallet signs. Doing that through the
parallel planner made the fixture depend on the code under test, so a
planner regression failed the fixture instead of the wallet signing
assertions. Sign the fixture input serially.

Refs #155

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verify unknown foreign leaves with the complete spent-output context before classifying them as incomplete. Cover a valid nonstandard leaf during a batch and a no-work retry.
Read live coins and mempool spenders from the same snapshot before committing a replacement. Reject conflicts whose wallet notifications are pending while allowing the original replacement lineage. Cover unseen mempool conflicts, spent wallet coins, and normal replacement.
Hoist the fee-bump commit results into named locals so the assertion
macro's repeated expansion does not read a moved-from transaction.
Reserve the spent-output vectors before the loops that fill them.
A backward reorg during asynchronous signing can leave the replacement's
anti-fee-sniping nLockTime ahead of the tip. Revalidation covered coins,
fees, size, and chain limits, so the wallet recorded the replacement and
marked the original replaced while the broadcast failed. Check finality
against the current tip using the rule the mempool applies on accept.
@kiwidream
kiwidream force-pushed the kiwidream/batch-mixed-descriptor-signing branch from 4a475ae to f542422 Compare September 18, 2026 16:04
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant