Repository navigation
backport: bitcoin#35984 - #7827
PastaPastaPasta wants to merge 1 commit into
Conversation
|
✅ Final review complete — no blockers (commit 59391d5) · triage: low |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. Walkthrough
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This backport makes PSBT and raw transaction signing consistently refuse SIGHASH_SINGLE signatures when the input has no matching output. No merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the complete two-file change at e5823ec against Bitcoin bitcoin#35984 and Dash's signing callers; no actionable defects were found. The omitted upstream UTXO-selection edits are explicitly excluded in the PR description and commit message, so they do not warrant a completeness finding. Validation was static only: the supplied CI snapshot shows successful container builds and formatting/title/merge checks, two queued manifest jobs, and no completed unit or functional test results.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff moves a small missing-output guard into MutableTransactionSignatureCreator::CreateSig and adds focused regression coverage, making it contained and straightforward despite touching signing. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 95% left, weekly 95% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
e5823ec to
4e6d495
Compare
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 4e6d495, the complete two-file diff correctly centralizes the missing-output SIGHASH_SINGLE refusal without changing consensus verification or ALL-based wallet and CoinJoin signing. The retained regression covers PSBT and raw signing, including SINGLE|ANYONECANPAY and ALL controls; the omitted upstream test-maintenance hunks are explicitly excluded in the PR description and do not warrant findings. Verification was static: the supplied CI snapshot reports successful completed builds, lint, and no-wallet tests, with wallet-enabled Linux tests, sanitizer tests, and the multiprocess build still pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The change is a small, well-contained relocation of an existing SIGHASH_SINGLE guard into the shared signature creator, with focused regression coverage, rather than a large or intricate signature change. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 74% left, 5h 36% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
4e6d495 to
b88580c
Compare
… corresponding output 8df006f wallet: skip signing SIGHASH_SINGLE inputs with no corresponding output (furszy) Pull request description: `SIGHASH_SINGLE` only commits to the output at the input's index. If the output at such position doesn't exist, it commits to no output at all (legacy uses a fixed sighash of 1, segwit v0 zeroes `hashOutputs`), which means the signature stays valid even when outputs are swapped, which is a footgun that lets funds be redirected without the owner's consent. `SignTransaction()` already skipped these inputs, but `SignPSBTInput()` did not, so `walletprocesspsbt` signed them. This moves the check into the `CreateSig` so both paths, and any future one, skip producing the detached signature. Note: if there is a valid use for the segwit v0 case, I would rather re-allow it through an explicit opt-in arg than by default, so it is always a deliberate choice. Fixes bitcoin#35977 ACKs for top commit: thomasbuilds: ACK 8df006f achow101: ACK 8df006f Tree-SHA512: ac1c9659910eae889475c65b8dde68e711c4d85d0d520af06411728591e573bf027b41a9ac6dd0f6e93de425841ac6c7d9e6c9f07936b9d8ae390941fbe6f1f1 BACKPORT NOTE: - src/script/sign.cpp: Dash has no SignOptions or SIGHASH_DEFAULT, so the check uses nHashType. Dash also has no SIGHASH_OUTPUT_MASK (added with Taproot), so it tests (nHashType & 0x1f), the same test Dash's legacy SignatureHash uses for SIGHASH_SINGLE. For every sighash the wallet can sign with (ALL/NONE/SINGLE, optionally |ANYONECANPAY) this matches upstream's (hashtype & SIGHASH_OUTPUT_MASK). - test/functional/rpc_psbt.py, test_sighash_single(): - No loop over legacy/bech32/bech32m address types; Dash only has P2PKH. Instead the test covers SINGLE, SINGLE|ANYONECANPAY and ALL (control), and also checks signrawtransactionwithwallet, because this change removes SignTransaction's own SIGHASH_SINGLE check and Dash had no test for that path. - It runs after test_input_confs_control; Dash has none of the sighash tests upstream places it next to. - Omitted: the test_psbt_roundtrip and test_psbt_version UTXO selection changes; those tests (PSBTv2) do not exist in Dash. Co-authored-by: Ava Chow <github@achow101.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b88580c to
59391d5
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 59391d5 against the upstream merge and surrounding signing paths; no actionable defects were found. The shared refusal matches Dash's legacy signature-hash semantics, and the omitted PSBTv2 test-fixture changes are explicitly documented intentional exclusions. This was a static review: the supplied CI snapshot shows successful lint and several platform builds, with three test jobs and additional builds still pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The change moves a small, straightforward missing-output guard into the shared signature creator and adds focused regression tests, without the size or intricacy required for critical. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 69% left, 5h 90% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Declared omission: PSBTv2-dependent upstream test transformations — INTENTIONAL_EXCLUSION: The supplied PR description explicitly states that upstream's test_psbt_roundtrip/test_psbt_version UTXO-selection hunks are omitted because their PSBTv2 tests are absent; HEAD's BACKPORT NOTE repeats this exclusion. Comparing upstream merge e19f83e with its first parent confirms that these two hunks replace listunspent()[0] with selection by amount == Decimal(50). The tests were introduced by 8838418 and bcc1dca, both verified ancestors of the bitcoin#21283 merge d7ed284. Neither test exists in Dash's base f35a24c or exact head, and base-history git log -S searches show no introduction. Their omission is therefore an expressly bounded backport exception, not an undisclosed prerequisite gap; requesting future PSBTv2 work is outside this fix's scope.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
2 participants
Issue being fixed or feature implemented
When a signer explicitly requests
SINGLEorSINGLE|ANYONECANPAYfor a PSBT input that has no output at the same index, the legacy signature hash is the constant one, so the signature does not bind the transaction as the signer expects.SignTransaction()already skipped such inputs butSignPSBTInput()did not. DefaultALLsigning and the wallet's requested-sighash agreement checks limit exposure to explicit nondefault signing.What was done?
Backport of bitcoin#35984.
The missing-output check moves into
MutableTransactionSignatureCreator::CreateSig, so PSBT and raw transaction signing share the same refusal, and the redundant raw-signing check is removed.Dash adaptations:
nHashType & 0x1fwith the legacySignatureHash, since Dash lacksSIGHASH_OUTPUT_MASKand the upstream signing-options interface.SINGLE|ANYONECANPAY,ALLand raw-signing controls.test_psbt_roundtrip/test_psbt_versionUTXO-selection hunks are omitted, because those tests arrive with PSBTv2 (Implement BIP 370 PSBTv2 bitcoin/bitcoin#21283), which Dash does not have.How Has This Been Tested?
On macOS arm64 with the existing depends prefix and
--enable-werror:make -j1passed before and after the change.SINGLE) and passes after the fix.rpc_psbt.pypassed in both descriptor and legacy variants, including theSINGLE|ANYONECANPAY,ALLand raw-signing controls.src/test/test_dash --run_test=sighash_tests,script_tests,psbt_wallet_testspassed.git diff --checkpassed.Breaking Changes
The shared signer refuses to create
SINGLEsignatures when the matching output is missing. No consensus or RPC schema changes.Checklist:
🤖 Generated with Claude Code