test(abstract-utxo): Pearl transaction flow and live-node cross-validation - #9367
test(abstract-utxo): Pearl transaction flow and live-node cross-validation#9367manojkumar138 wants to merge 1 commit into
Conversation
abdf94d to
ca4ab4e
Compare
ca4ab4e to
4d12acc
Compare
|
@claude review |
|
Claude finished @manojkumar138's task —— View job Code Review for Pearl Transaction TestsReview Tasks:
SummaryThis PR adds comprehensive test coverage for Pearl (Duplex) cryptocurrency transaction flows with 42 well-structured tests across two complementary files. The approach is methodical and the test quality is excellent. Strengths✅ Excellent Test Architecture
✅ Robust Implementation Quality
✅ Strong External Validation
Code Quality ObservationstransactionFlow.ts
sandboxVectors.ts
Areas for ConsiderationMinor Suggestions
Documentation ExcellenceThe comments and documentation are exceptional:
Test Coverage Assessment
Verdict✅ Approve - This is exemplary test code that demonstrates:
The known divergence from sandbox taptree is well-documented and the tests appropriately capture this difference rather than hiding it. The PR achieves its stated goals and provides solid coverage for the Pearl transaction flow within the constraints of offline testing. |
There was a problem hiding this comment.
Pull request overview
Adds comprehensive unit-test coverage for Pearl’s fixed-script (Taproot) transaction lifecycle in abstract-utxo, including an offline PSBT flow and a second suite that cross-validates key outputs against vectors captured from a real pearld regtest node. This fits the codebase by strengthening confidence in the wasm-only Pearl path without requiring @bitgo/utxo-lib network support.
Changes:
- Adds an end-to-end offline PSBT lifecycle suite for Pearl (address derivation → PSBT build → signing/verification → finalize/extract → serialize/deserialize → parse).
- Adds a live-node vector cross-validation suite for tx deserialization/txid, address encoding round-trips, witness shape, and taptree-depth assertions, while explicitly pinning the known sandbox-vs-SDK taptree divergence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| modules/abstract-utxo/test/unit/impl/pearl/unit/transactionFlow.ts | New offline end-to-end PSBT lifecycle + policy/serialization/parse + script-type restriction tests for Pearl (wasm-only). |
| modules/abstract-utxo/test/unit/impl/pearl/unit/sandboxVectors.ts | New tests that validate wasm-utxo behavior against real pearld regtest vectors (txid, scriptPubKey↔address, witness shape/depth) and pin the known divergence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('builds p2trMusig2 inputs even though key-path signing is a later phase', function () { | ||
| for (const chain of MUSIG2_CHAINS) { | ||
| const psbt = buildPsbt('pearl', { signer: 'user', cosigner: 'bitgo' }, chain); | ||
| // Signing needs MuSig2 nonce rounds, so finalizing here fails by design. | ||
| assert.throws(() => psbt.finalizeAllInputs(), `chain ${chain} should need nonces`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Correct, and worth chasing down rather than just adding a signature call. Fixed in 38e46183fb.
I checked what the actual behavior is before fixing this, since I wanted the assertion to be precise rather than just "less wrong":
chain 40 sign(user) -> [0] sign(bitgo) -> [0]
verifySignature(user) = false
verifySignature(bitgo) = false
finalizeAllInputs() -> "Signature aggregation error: At least 2 public nonces are required, got 0"
sign() reports both input indices even for musig2, because it doesn't know the signature it produced needs a completed nonce round to be valid — it always looks "reported." The real distinguishing evidence is that verifySignature is false for a key that did call sign(), and that finalize fails on the nonce-aggregation error specifically, not a bare "unsigned input" error.
The test now:
- signs with both keys (matching how a real 2-of-2 musig2 spend would proceed)
- asserts
verifySignatureisfalsefor both — the signature exists but isn't usable without nonces - asserts
finalizeAllInputs()throws matching/At least 2 public nonces are required/, not just "throws"
One thing I checked before committing to this shape: even without step 1, finalizeAllInputs() on a never-signed musig2 input throws the same nonce error message — wasm-utxo apparently checks nonce state before it checks signature completeness. So the bare regex match alone would technically have passed either way. But keeping the sign() calls is still the right fix, not just a nice-to-have: without them the test's own narrative is wrong (claiming "signing needs nonce rounds" while never signing), and this version demonstrates the actual mechanism your comment was pointing at — that sign()'s apparent success is misleading for musig2, consistent with the same caveat already documented for the taproot script-path leaves earlier in this file.
Full suite: 1594 passing, no regressions.
…ation Cover the Pearl (Duplex) fixed-script lifecycle with 42 offline tests across two files. transactionFlow.ts (27 tests) exercises the full PSBT lifecycle - address generation, build, sign, verify, finalize, extract, serialize round-trip, parse - directly through fixedScriptWallet.BitGoPsbt. Pearl has no @bitgo/utxo-lib network, so none of the shared UTXO test helpers apply; AcidTest, testutil.constructPsbt and the utxoCoins fixtures all resolve a network. All three taptree leaves are exercised. Note the canonical signer order is user+bitgo, user+backup and backup+bitgo: wasm-utxo rejects the reversed forms with "Could not find control block for leaf script", so the third leaf is backup+bitgo rather than bitgo+backup as the TDD writes it. verifySignature, not the return of sign(), is used to assert signing succeeded - sign() reports the inputs it touched and returns the input index even for a key outside the chosen leaf. sandboxVectors.ts (15 tests) checks our output against a real pearld 1.0.2 regtest node from sandboxing (BitGo/coins-sandbox PR 898), rather than against itself. It deserializes the node's own raw coinbase transaction and computes the node's own txid; encodes the node's real p2tr scriptPubKeys and matches the node's bech32m data part exactly; and reproduces the witness shape (4 stack items, 64-byte BIP-340 signatures, leaf version 0xc0) and per-leaf taptree depth the node accepted. The sandbox spends used a NUMS internal key and 70-byte leaf scripts; BitGo's p2tr chain derives an internal key from the wallet keys and produces 68-byte leaves. This is BitGo's house construction rather than anything Pearl-specific - the same input built for btc yields a byte-identical witness - but it means the sandbox's on-chain-verified signatures describe a different script than the SDK generates and cannot be asserted against our output. Three tests pin this divergence, since it means BitGo's actual Pearl taptree has not yet been accepted by a pearld node; closing that needs a regtest broadcast of an SDK-built transaction. Both files were mutation-tested rather than trusted on a green run: reversing the taptree leaf order, substituting a taproot chain into the legacy-rejection list, corrupting the node txid, corrupting a node scriptPubKey byte, and using the wrong merkle depth each fail the suite as expected. Broadcasting and p2trMusig2 key-path signing are out of scope. Broadcast needs a live pearld node or Pearl in the BitGo test environment, neither of which exists yet (tracked in the wallet-platform onboarding epic). Musig2 key-path needs MuSig2 nonce rounds and is a later phase per the TDD; those chains are asserted to be buildable and to fail finalization without nonces, so the gap is visible rather than silent. The musig2 case signs before finalizing and asserts verifySignature is false and the finalize error names the missing nonces specifically, so it cannot be confused with a generic missing-signature failure. TICKET: CECHO-1806 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
4d12acc to
38e4618
Compare
|
Thanks both — one real fix landed, one accepted as-is. Copilot: musig2 test was weaker than it claimed — fixedThe finding was correct: the test asserted Full suite: 1594 passing, no regressions. Commitlint clean, single commit. Claude: approved, minor suggestions not takenVerdict was Approve, with three optional style suggestions (stricter
Nothing else outstanding. PR is squashed to one commit, CI green (21 pass / 4 skipping last run, docker-build flake self-resolved on retry). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
modules/abstract-utxo/test/unit/impl/pearl/unit/transactionFlow.ts:386
- This test only asserts that an error is thrown, but not why it is thrown. Since the intent here is specifically to reject reversed signer/cosigner orderings that have no tapleaf, matching the expected error message (or a stable substring) will prevent false positives if
buildPsbt()ever starts throwing for an unrelated reason (e.g. an input/output construction issue).
assert.throws(() => buildPsbt('pearl', pair), `${pair.signer}+${pair.cosigner} is not a leaf in the taptree`);
Summary
Test coverage for the Pearl (Duplex) fixed-script lifecycle — 42 tests across two files:
transactionFlow.tssandboxVectors.tsThe split is deliberate. The first file builds transactions whose inputs reference txids that never existed, so it can only prove our libraries agree with themselves. The second checks against output captured from a live
pearld1.0.2 regtest node during sandboxing (coins-sandbox#898).Scope — what is and isn't covered
sha256txids, not real UTXOs)Single-input/single-output only; no dust boundaries, multi-recipient, or RBF.
What is genuinely node-verified
Transaction deserialization. Given the node's raw coinbase hex, our library computes the node's own txid
272e5b8a…d295and round-trips byte-exactly. The node published both the bytes and the txid, so agreement is external.Address encoding. For the node's real p2tr scriptPubKeys, our bech32m data part matches the node exactly — e.g.
efxxurpnufae39uq0gcquq3msh27rhdau2rjh38mf9ndlhflk9js. Only HRP and checksum differ, as they must: the node ran regtest (rprl) and wasm-utxo has no Pearl regtestCoinName. Decoding restores the node's scriptPubKey exactly.Witness shape. 4 stack items, 64-byte BIP-340 signatures with no sighash byte, leaf version
0xc0— the shape the node accepted.Taptree depth. Control block size is
1 + 32 + 32*depth, so it reveals each leaf's position. The measured depths confirm the TDD's taptree:user+bitgoshallow at depth 1,user+backupandbackup+bitgoat depth 2. The node's 65-byte sample was auser+bitgospend, which is why it saw depth 1.The sandbox spends set the BIP-341 NUMS point as the internal key and produced 70-byte leaf scripts. BitGo's
p2trchain instead derives an internal key from the wallet keys and produces 68-byte leaves.This is BitGo's house construction, not a Pearl bug — building the same input for
btcyields a byte-identical witness, and the derived internal key is identical across both coins. But it has two consequences worth attention:Three tests pin this so it cannot drift unnoticed, including one that fails if BitGo ever moves to a NUMS internal key (at which point the sandbox vectors become comparable).
Worth noting the TDD lists "NUMS internal key" as a chain parameter, which is misleading — it was a property of the sandbox's hand-rolled script. Anything else derived from that section may inherit the same assumption.
Three assumptions the tests corrected
The TDD's third taptree leaf is written backwards. It says
[bitgo+backup]; wasm-utxo only acceptsbackup+bitgoand rejects the reverse withCould not find control block for leaf script.sign()'s return value proves nothing. It returns the input index even for a key outside the chosen leaf.verifySignatureis the source of truth; finalization enforces the leaf.One test was vacuous until mutation testing caught it. "Rejects non-taproot chains" passed even with chain 30 substituted in, because chain 30 also throws without a
signPath— a different error. It now supplies asignPathand matches/Unsupported script type/.Mutation testing
I mutated each suite rather than trusting a green run. All caught:
user+backupVerification
abstract-utxo: 1594 passing (1552 → +42), zero failures. Prettier and eslint clean.Next step to close the remaining gap
A regtest broadcast of an SDK-built transaction — that is the only thing that proves BitGo's Pearl taptree is consensus-valid. Blocked by the TDD's own risks: no public
pearldDocker image, and ZK-PoW CGo crashes on Apple Silicon.Related
TICKET: CECHO-1806
🤖 Generated with Claude Code