Repository navigation
Conversation
🦋 Changeset detectedLatest commit: b882848 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
3e9345b to
dcaac2e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 10 files. (6 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly summarizes both primary changes: JoyID now returns a PSBT from signPsbt, and the codebase switches to @scure/btc-signer.Description check ✅ Passed The description explains the main changes, breaking playground change, referenced issues, and test coverage. It does not include the template’s contributing-guidelines checklist, but the description i… Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 10 files. (6 skipped: 6 unsupported.)
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 help to get the list of available commands.
fghdotio
changed the title
|
/canary |
|
🚀 Canary version published successfully! View workflow run The following packages have been published to npm:
|
|
Generated by ChatGPT · Alter Ego Updated 2026-10-16 at unchanged head Retained inline findings:
These remain in the existing first COMMENT review and additional P3 review. This edit does not duplicate or change those findings. Reassessment preserved: I overstated both original compatibility findings as P2. The non-template Taproot P2 is withdrawn. Mixed bare inputs are P3, confined to bare P2PK/multisig. No affected CCC production downstream was established; that does not establish that none exists. 2026-10-16 compatibility clarification: exact scope, matrix, examples, and limitsWhich behavior is being compared?The audit used the exact locked
CCC configures What the default finalizers actually doOld bitcoinjs: requires New scure under CCC's options: after PSBT/prevout/commitment checks,
Each K must be a valid 32-byte x-only public key. The finalizer fills per-key slots, reverses them for witness order, appends the original script/control block, and chooses the smallest complete serialized witness. Repeated keys can reuse the same signature at each occurrence. Sources: template recognition, candidate assembly and selection. Compatibility matrix
For example, 1-of-2 with only A signed changes from The excluded family is broader than one or two examples:
This is a structural boundary, not a claim that every custom script was previously spendable. Missing preimages/selectors, insufficient signatures, invalid commitments, disabled tapscript An unsupported but parseable leaf can be skipped. An unused leaf with an unknown opcode or truncated push can instead throw during CLTV/CSV: what makes the examples meaningful?The audit used For CLTV, the operand must be a valid nonnegative ScriptNum, match the height/time type of nLockTime, be ≤ nLockTime, and the input sequence must be non-final. For an active CSV relative-lock branch, the transaction version must be ≥2, neither relevant disable bit may be set, type bits must match, and the masked operand must be ≤ the input sequence. A CSV operand with its disable bit set has different NOP semantics. Actual inclusion additionally requires the appropriate absolute/relative maturity and all other transaction conditions. Both fixtures used version 2 and sequence 5, so transaction-level BIP68 maturity also applies to the CLTV example. These are code/spec-supported valid-satisfaction regressions under those conditions; no Bitcoin Core/full consensus interpreter or live transaction was run. See BIP65, BIP112, and BIP68. Escape paths and actual caller impact
The affected caller shape is an application supplying a custom Taproot PSBT, receiving unfinalized leaf signatures from its wallet, and relying on CCC's default VerificationA fresh isolated audit on 2026-10-16 exercised 61 fixture groups and 240 passing tests across five test files: head Xverse 8, head JoyID 22, base Xverse 6, base JoyID 3, plus 201 audit checks. These include expected-rejection assertions; they do not mean every fixture finalized successfully. Actual snapshot adapter/finalizer sources were imported with mocked wallet provider/popup responses. Five custom-script families included verified synthetic Schnorr signatures. Exact package tarballs were checked against lockfile integrity. The scure 2.4.1 source matched its published package. This was not a full repository build/lint, live-wallet E2E, consensus-interpreter run, or broadcast; no real accounts or funds were used. The parser metadata P3 above has its own narrower reproduction and remains separately tracked. Earlier 2026-10-16 reassessment and verification record (preserved)“Supported custom scripts” lacked a CCC contract basis: the API promises attempted finalization. Taproot reproductions used synthetic wallet responses. Xverse reachability is source-based, not wallet E2E; JoyID support remains unverified. Tested ordinary payment, standard multisig, Taproot key-path/single-CHECKSIG and ordinary mixed-input flows work. Already-run independent tests: 30 changed-file + 4 regression + 46 follow-up checks passed, including reproduced-failure assertions and defensive prototypes. Synthetic/mocked coverage is not real-wallet E2E. At that earlier edit, only the published reassessment was reorganized: no new tests or fixes were made. Full build/lint and browser integration were not independently exercised. The later audit above adds evidence without changing the severity correction. |
Hanssen0-Alter-Ego
left a comment
Hanssen0-Alter-Ego
left a comment
There was a problem hiding this comment.
Generated by ChatGPT · Alter Ego
Moved the two retained findings from the earlier summary to the relevant code lines at dcaac2eb7549f82887bdeac3ede8b556ba9ba3d2: one P2 signature-hash consistency issue and one narrowly scoped P3 bare-input compatibility issue. The non-template Taproot P2 remains withdrawn.
This is a presentation update of the existing reassessment. No new tests or code changes were made; synthetic/mocked coverage is not real-wallet E2E, and full build/lint and browser integration were not independently exercised.
| const scratch = new Transaction(PSBT_OPTIONS); | ||
| scratch.addInput(input); | ||
| scratch.finalizeIdx(0); |
There was a problem hiding this comment.
Generated by ChatGPT · Alter Ego
[P2] Reject a signature whose hash mode disagrees with sighashType
When local auto-finalization is enabled and the wallet returns an ECDSA signature whose hash mode differs from the input’s retained sighashType, this helper finalizes it and then clears that declaration. The previously reproduced mismatch was rejected by old Xverse but accepted by the new Xverse/JoyID helpers; if the returned mode commits to less of the transaction, the finalized result carries weaker commitments than declared. BIP174 requires finalizers to reject this mismatch.
Suggested fix
Check each signature’s actual hash mode against the declaration before calling finalizeIdx or clearing metadata. Cover ECDSA and Schnorr encodings, and compare against the original request’s policy while preserving explicitly allowed modes; the latter also hardens existing gaps.
Apply the same guard to JoyID’s mirrored helper. The reproduced comparison was ECDSA; Schnorr coverage is a fix recommendation, not a claimed tested regression. No live-wallet exploit was demonstrated, and old JoyID behavior remains unverified.
| const signedPsbt = Transaction.fromPSBT( | ||
| ccc.bytesFrom(signedPsbtBase64, "base64"), | ||
| PSBT_OPTIONS, | ||
| ); |
There was a problem hiding this comment.
Generated by ChatGPT · Alter Ego
[P3] Keep an unrelated bare input from blocking selected-input finalization
If a PSBT contains a selected standard input plus another signer’s bare P2PK or bare multisig input with UTXO data, this whole-PSBT parse rejects the bare input before finalizeSignedInputs can skip it. The selected input therefore cannot be returned finalized even though the bare input was not requested for signing. This is a narrow compatibility issue: no affected CCC downstream was found.
Suggested fix
Separate unrelated-input passthrough from local finalizer support while retaining script-commitment checks, or explicitly document these bare-input PSBTs as unsupported.
The scure 2.4.1 parser validates every supplied prevout and rejects non-wrapped pk/ms scripts. The default-input scan has the same restriction. JoyID parses both PSBTs before checking autoFinalized, so disabling finalization does not bypass its parser.
This does not apply to the previously tested ordinary payment, wrapped-multisig, Taproot key-path/single-CHECKSIG, or ordinary mixed-input flows, which passed. The compatibility checks used synthetic/mocked responses; JoyID’s live-wallet support for these bare-input cases remains unverified.
2026-10-22 correction / follow-up
At 2c45ab669e67ec32b932e5505ccfd86e5e932c6d, the original bare-input compatibility failure described above no longer occurs: both P2PK and bare multisig cases now pass in Xverse and JoyID, with witness or non-witness UTXO metadata. The selected standard input finalizes while the unrelated input is preserved; the parent commit reproduces the rejection. JoyID's partial-return path now accepts these PSBTs too. Xverse's explicit-input partial-return bypass was already working before this fix.
The full correction is only partly complete because the earlier requirement to retain supplied-commitment checks is unmet. That remaining problem is tracked once in the new selected-input validation finding; this thread stays unresolved. These were isolated adapter tests with mocked wallet boundaries, not a full repository suite/build or live-wallet E2E. The original production-impact and live JoyID support limits still apply.
2026-10-22 correction / verification at b882848aafa9dabdf3cb1348d4af6b58c87b0a89
The scoped correction is now verified. Both adapters preserve unrelated bare P2PK and multisig inputs with witness or non-witness UTXO metadata while completing the selected standard input. Partial-return controls also pass. Selected-input commitment checks are restored and reject the tested conflicts, addressing the remaining qualification in the earlier follow-up. This finding is fully fixed within the verified scope.
Tests used synthetic PSBTs, isolated actual adapter sources, and mocked wallet boundaries with existing compiled support and dependency aliases. This was not a fresh frozen-lockfile installation or full monorepo build/suite, live-wallet E2E, or Bitcoin consensus execution. No production caller impact or live JoyID support is established.
Generated by ChatGPT · Alter Ego
Hanssen0-Alter-Ego
left a comment
Hanssen0-Alter-Ego
left a comment
There was a problem hiding this comment.
Generated by ChatGPT · Alter Ego
One additional, independently reproduced P3 compatibility finding at dcaac2eb7549f82887bdeac3ede8b556ba9ba3d2, anchored inline. The earlier severity corrections remain unchanged. Verification used isolated synthetic fixtures and method-body harnesses, not real-wallet E2E; no production downstream impact was established.
| // Collect all unsigned inputs | ||
| const psbt = Psbt.fromHex(psbtHex.slice(2)); | ||
| psbt.data.inputs.forEach((input, index) => { | ||
| const tx = Transaction.fromPSBT(ccc.bytesFrom(psbtHex), PSBT_OPTIONS); |
There was a problem hiding this comment.
Generated by ChatGPT · Alter Ego
[P3] Accept presigned Taproot inputs with undisclosed tree metadata
An already-signed tree-backed Taproot input can retain the correct tapInternalKey while omitting tapMerkleRoot and tapLeafScript. These fields are optional under BIP371, but scure 2.4.1 treats this combination as an empty-tree commitment and rejects the entire PSBT here.
At dcaac2eb7549f82887bdeac3ede8b556ba9ba3d2, an isolated synthetic reproduction with the exact locked versions verified the key-path signature against the output key. The base default scan skipped that signed input and selected a separate unsigned P2WPKH input; the head scan throws before contacting the wallet. The returned-PSBT parser also rejects this case. This is a key-path metadata compatibility issue, distinct from the withdrawn custom-script finding. No affected production caller or live-wallet behavior was established; the method-body harness used utility/schema/provider stubs, not a full repository build or wallet E2E.
Suggested fix
Handle undisclosed tree metadata without assuming an empty tree, while retaining checks for supplied commitments. Cover both Xverse parse sites and JoyID's parsing.
When a response omits metadata present in the original request, restore only matching, validated original metadata after checking transaction/input identity and prevout consistency; reject conflicts. Restoring the verified original root passed the synthetic control, with wrong-root rejection still enabled. This does not cover an original PSBT already missing the root: that needs targeted parser compatibility for signed key-path inputs, using the output key for signature validation rather than globally disabling script checks.
2026-10-22 correction / follow-up
At 2c45ab669e67ec32b932e5505ccfd86e5e932c6d, the original presigned key-path compatibility failure described above no longer occurs. Both adapters now accept a request already omitting optional tree metadata and a response omitting metadata present in the request. The synthetic key-path signature verifies against the output key, the presigned input is preserved, and the separate selected standard input completes. The before/after checks cover explicit/default selection and finalization on/off; Xverse's explicit partial-return bypass remains a pre-existing control.
The full correction is only partly complete: supplied conflicting commitments are now accepted too, so the earlier requirement to retain those checks remains unmet. See the separate selected-input validation finding for the new regression; this thread stays unresolved. The custom-script support finding remains withdrawn. Coverage is isolated actual adapter sources with mocked boundaries; the full repository suite/build and live-wallet E2E were not run, and no production caller impact or live-wallet exploit is established.
2026-10-22 correction / verification at b882848aafa9dabdf3cb1348d4af6b58c87b0a89
[P3] Preserve selected Taproot key-path compatibility when optional tree metadata is omitted
The separate presigned, unselected-input cases from the earlier follow-up still pass: the presigned input remains unchanged and unfinalized while the separate selected standard input completes. However, the new strict scratch parser at Xverse lines 35–36 and JoyID lines 22–23 rejects a selected, tree-backed key-path input retaining the correct tapInternalKey and a valid output-key signature when optional tapMerkleRoot/tapLeafScript metadata is omitted. Scure interprets that incomplete metadata as an empty-tree commitment. As a result, local auto-finalization throws instead of returning the valid selected input finalized.
Both adapters finalize the identical selected-input fixtures at the previous head, 72d47bfbe9d3e314266e8f2e5115684d1898daca, but reject them at this head. Retaining the correct root or setting autoFinalized: false succeeds. The checks cover explicit/default selection, a request containing the root whose response omits it, and a request already lacking the root. Thus the prior unselected-input correction still holds, but selected-input compatibility regressed; this thread remains open.
Suggested fix
Handle omitted-tree key-path metadata selectively while retaining supplied-commitment validation. Where a response omits metadata present in the request, consider restoring only matching, validated request metadata after verifying transaction/input identity and prevout consistency. Requests already lacking the root need targeted key-path compatibility using the output key, without assuming an empty tree. Preserve rejection of supplied conflicting commitments; do not globally disable script checks again. This is a proposed correction, not a tested patch.
Evidence uses synthetic, independently output-key-verified signatures and isolated actual adapter sources with mocked wallet boundaries, existing compiled support, and dependency aliases. It demonstrates adapter compatibility, not a wallet's ability to discover an undisclosed root. No production caller or live-wallet impact was established. The full monorepo build/suite, live-wallet E2E, and Bitcoin consensus execution were not run.
Generated by ChatGPT · Alter Ego
| const PSBT_OPTIONS: TxOpts = { | ||
| unknown: "ignore", | ||
| proprietary: "ignore", | ||
| disableScriptCheck: true, |
There was a problem hiding this comment.
Generated by ChatGPT · Alter Ego
[P2] Keep commitment checks on inputs being finalized
This option is also used by the scratch transaction in finalizeInput, so it disables commitment checks on selected inputs, beyond allowing unrelated-input passthrough. At 2c45ab669e67ec32b932e5505ccfd86e5e932c6d, isolated before/after tests show both adapters returning finalized witnesses for selected standard inputs with conflicting script commitments that the parent commit rejects. Those witnesses cannot satisfy their declared prevouts.
Suggested fix
Keep compatibility parsing separate from validation of selected inputs and their scratch-finalization path; validate supplied commitments before clearing metadata. Apply the same change to JoyID's mirrored option, and add negative selected-input commitment cases alongside the positive compatibility tests. This is a proposed correction, not a tested patch.
Verification: 111 isolated case groups passed, including expected-rejection and defect-reproduction assertions against actual before/after adapter sources with mocked boundaries. Matching-commitment controls pass. The full repository suite/build, live-wallet E2E, and Bitcoin consensus interpreter were not run. This is a reproduced witness-correctness regression; no live-wallet exploit or fund loss is claimed.
2026-10-22 correction / verification at b882848aafa9dabdf3cb1348d4af6b58c87b0a89
Independent verification confirms the fix for this finding. Both adapters now reject the tested supplied-commitment conflicts before local finalization; matching controls succeed. The previous head, 72d47bfbe9d3e314266e8f2e5115684d1898daca, accepts those conflicting fixtures. Partial-return behavior remains available when finalization is disabled.
Coverage includes representative P2SH, P2WSH, nested P2SH-P2WSH, Taproot internal-key/root/control-block, and stray script-metadata conflicts. These are representative supplied-commitment checks, not exhaustive metadata-permutation coverage. This finding is fully fixed within the verified scope.
The separate omitted-tree compatibility regression introduced by strict scratch parsing is tracked in the existing omitted-metadata thread, which remains open.
Validation comprised 290 isolated expected-outcome case groups, including defect reproductions and controls, using synthetic PSBTs and actual adapter sources with mocked wallet boundaries and existing compiled support. Separately, the two repository test files passed 42 tests at this head and 39 at the previous head using dependency aliases. This was not a fresh frozen-lockfile installation or package-export/build test. The full monorepo build/suite, live-wallet E2E, and Bitcoin consensus execution were not run.
Generated by ChatGPT · Alter Ego
2c45ab6 to
72d47bf
Compare
Refs #517. Supersedes #556.
signPsbtalways returns a PSBT. JoyID is asked not to finalize, and CCC finalizes the inputs JoyID signed.initEccLib.Breaking (playground):
bitcoinfrom@ckb-ccc/playgroundis gone. Useimport * as btc from "@scure/btc-signer".Tested on testnet with JoyID and Xverse.