Repository navigation
Validate checkpoints and authenticated prevouts - #129
Conversation
WalkthroughPrevout validation now authenticates declared witness UTXOs against fetched transactions. ChangesPrevout authentication
Checkpoint validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SubmitTx
participant indexCheckpoints
participant validateCheckpoint
participant ArkadeScript
SubmitTx->>indexCheckpoints: Index checkpoint transactions
indexCheckpoints-->>SubmitTx: Return mappings or validation error
SubmitTx->>validateCheckpoint: Validate Ark input and checkpoint
validateCheckpoint-->>SubmitTx: Return validation result
SubmitTx->>ArkadeScript: Execute script after validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/application/prevout.go (1)
41-51: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the new prevout-field requirements explicit before relying on hard rejections.
decodePrevoutTxsFromFieldaccepts missing fields, butprevOutFetcherForIntent,prevOutFetcherForArkTx, andprevOutFetcherForOnchainTxall reject every submitted input without the requiredPrevArkTxFieldorPrevoutTxField. Update client request population and API/docs before relying on these errors, or make the intent/ark/onchain path fail explicitly for unpopulated inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/prevout.go` around lines 41 - 51, Make missing prevout fields explicit in the intent/Ark/on-chain fetch paths: update prevOutFetcherForIntent, prevOutFetcherForArkTx, and prevOutFetcherForOnchainTx to reject unpopulated inputs with clear errors before relying on authenticatedPrevout. Apply the corresponding handling at internal/application/prevout.go lines 41-51 and 125-137, preserving valid populated-input behavior; also ensure client request population and API/docs reflect the required PrevArkTxField or PrevoutTxField.
🧹 Nitpick comments (7)
internal/application/tx_test.go (3)
190-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert which side rejects the substituted leaf.
The assertion matches "tapleaf does not match ark input".
validateCheckpointemits that text for both the ark input and the checkpoint input. The test therefore passes even if the ark-side check fails instead of the checkpoint-side check.Add an assertion on the
checkpoint tapleaf:prefix.💚 Proposed fix
err := validateCheckpoint(setup.arkPtx, 0, setup.checkpoint, setup.previousOutput, setup.expectedLeaf) + require.ErrorContains(t, err, "checkpoint tapleaf") require.ErrorContains(t, err, "tapleaf does not match ark input")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/tx_test.go` around lines 190 - 195, Update the “substituted checkpoint leaf” test around validateCheckpoint to assert the error contains the checkpoint-specific “checkpoint tapleaf:” prefix, in addition to the existing message assertion, so the test confirms rejection occurs on the checkpoint-input validation path.
25-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover the remaining
indexCheckpointsbranches.The test covers count mismatch, duplicates, and multi-input reuse. Two branches remain uncovered: the nil
arkPtxor nilarkPtx.UnsignedTxguard, and the nil checkpoint guard.Line 61 also mutates the shared
arkPtx. That couples the last assertion to statement order. Convert the assertions to subtests with per-case fixtures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/tx_test.go` around lines 25 - 64, Expand TestIndexCheckpoints with subtests and fresh per-case fixtures so each assertion is independent. Add cases covering nil arkPtx or nil UnsignedTx and a nil checkpoint, while retaining the existing count, duplicate, and multi-input reuse checks; avoid mutating the shared arkPtx by constructing each case through newCheckpoint and newArkTx.
150-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for value preservation and the anchor output.
Two security-relevant branches in
validateCheckpointhave no test:
- Line 258 in
internal/application/tx.gorejects a checkpoint vtxo output value that differs from the authenticated previous output value. This blocks value inflation.- Line 248 rejects a checkpoint output 1 that is not
txutils.AnchorOutput().Add a case that raises
setup.checkpoint.UnsignedTx.TxOut[0].Valuetogether with the matching ark witness UTXO value, and a case that replaces the anchor output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/tx_test.go` around lines 150 - 202, Extend the validateCheckpoint tests with cases covering value inflation and anchor validation. Add a test that increases setup.checkpoint.UnsignedTx.TxOut[0].Value and the corresponding ark witness UTXO value, asserting the authenticated previous-output value error; add another test replacing output 1 with a non-anchor output, asserting the anchor-output validation error.internal/application/prevout.go (1)
78-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant empty-input guard.
Lines 89-91 check for zero checkpoint inputs. Lines 92-94 already reject any count other than one, so the first check is unreachable.
♻️ Proposed simplification
- if len(checkpoint.Inputs) == 0 || len(checkpoint.UnsignedTx.TxIn) == 0 { - return nil, fmt.Errorf("checkpoint has no inputs for input %d", inputIndex) - } if len(checkpoint.Inputs) != 1 || len(checkpoint.UnsignedTx.TxIn) != 1 { return nil, fmt.Errorf("checkpoint must have exactly one input for ark input %d", inputIndex) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/prevout.go` around lines 78 - 106, Remove the redundant zero-length check in the checkpoint validation within the prevout processing loop, keeping the existing exact-one-input validation for both checkpoint.Inputs and checkpoint.UnsignedTx.TxIn. Preserve the surrounding authenticatedPrevout checks and error behavior.internal/application/testdata/onchain_prevout_fetcher.json (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multi-input fixture that passes.
Both multi-input cases now assert an error. The only success case has a single input. No fixture exercises the loop across several inputs where every input carries
PrevoutTxField.Add a fixture with two inputs and both prevout transactions attached, and assert the resulting fetcher entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/testdata/onchain_prevout_fetcher.json` around lines 12 - 15, Add a successful multi-input fixture alongside the existing onchain prevout cases with two inputs, ensuring each input includes its PrevoutTxField. Omit errorContains and assert the fetcher returns the expected entries for both inputs, exercising successful iteration across all inputs.internal/application/prevout_test.go (2)
131-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion resolves before script execution.
SubmitOnchainTxcallsprevOutFetcherForOnchainTxfirst. That call rejects input 1 and returns "failed to create prevout fetcher: witness utxo does not match authenticated prevout for input 1". The Arkade script, the Taproot leaf, and the emulator packet are therefore never exercised by this assertion.The test name implies that
OP_INSPECTINPUTVALUEobserves the lie. Add a comment that states the rejection happens at the prevout-authentication boundary, or add a second case that keeps the declared value honest and asserts that the script reads the authenticated value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/prevout_test.go` around lines 131 - 201, Clarify TestSubmitOnchainTxRejectsAnyOneCanPayPrevoutLie by documenting that SubmitOnchainTx rejects input 1 during prevout authentication before script execution, and update the expected error to include the full “failed to create prevout fetcher” context. Do not imply that OP_INSPECTINPUTVALUE is exercised by this test; add a separate honest-value case only if coverage of script observation is required.
26-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the two error-expectation mechanisms.
The
validfixture list now carrieserrorContains, and a separateinvalidlist already expresses the same intent. Fixtures undervalidthat expect an error also carryexpectedVtxoPkScriptsvalues that the test never asserts.Consider moving error cases into the
invalidlist, or replacing both lists with a single list whereerrorContainsdecides the expectation.Also applies to: 87-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/application/prevout_test.go` around lines 26 - 37, Consolidate the fixture-driven error expectations in the “cases” test and the related assertions: avoid keeping error cases in fix.Valid while ignoring their expectedVtxoPkScripts, and use one consistent mechanism—either move those fixtures to the existing invalid list or make errorContains exclusively determine success versus failure. Ensure successful fixtures still validate expected scripts and error fixtures only assert the expected error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/application/tx.go`:
- Around line 272-296: Update validateTaprootLeaf to iterate over every non-nil
entry in input.TaprootLeafScript, selecting the entry whose tapleaf hash matches
expectedLeaf before validating its control block, leaf version, and commitment.
Return the existing missing/mismatch error when no entry matches, while
preserving the current validation behavior for the selected leaf.
---
Outside diff comments:
In `@internal/application/prevout.go`:
- Around line 41-51: Make missing prevout fields explicit in the
intent/Ark/on-chain fetch paths: update prevOutFetcherForIntent,
prevOutFetcherForArkTx, and prevOutFetcherForOnchainTx to reject unpopulated
inputs with clear errors before relying on authenticatedPrevout. Apply the
corresponding handling at internal/application/prevout.go lines 41-51 and
125-137, preserving valid populated-input behavior; also ensure client request
population and API/docs reflect the required PrevArkTxField or PrevoutTxField.
---
Nitpick comments:
In `@internal/application/prevout_test.go`:
- Around line 131-201: Clarify TestSubmitOnchainTxRejectsAnyOneCanPayPrevoutLie
by documenting that SubmitOnchainTx rejects input 1 during prevout
authentication before script execution, and update the expected error to include
the full “failed to create prevout fetcher” context. Do not imply that
OP_INSPECTINPUTVALUE is exercised by this test; add a separate honest-value case
only if coverage of script observation is required.
- Around line 26-37: Consolidate the fixture-driven error expectations in the
“cases” test and the related assertions: avoid keeping error cases in fix.Valid
while ignoring their expectedVtxoPkScripts, and use one consistent
mechanism—either move those fixtures to the existing invalid list or make
errorContains exclusively determine success versus failure. Ensure successful
fixtures still validate expected scripts and error fixtures only assert the
expected error.
In `@internal/application/prevout.go`:
- Around line 78-106: Remove the redundant zero-length check in the checkpoint
validation within the prevout processing loop, keeping the existing
exact-one-input validation for both checkpoint.Inputs and
checkpoint.UnsignedTx.TxIn. Preserve the surrounding authenticatedPrevout checks
and error behavior.
In `@internal/application/testdata/onchain_prevout_fetcher.json`:
- Around line 12-15: Add a successful multi-input fixture alongside the existing
onchain prevout cases with two inputs, ensuring each input includes its
PrevoutTxField. Omit errorContains and assert the fetcher returns the expected
entries for both inputs, exercising successful iteration across all inputs.
In `@internal/application/tx_test.go`:
- Around line 190-195: Update the “substituted checkpoint leaf” test around
validateCheckpoint to assert the error contains the checkpoint-specific
“checkpoint tapleaf:” prefix, in addition to the existing message assertion, so
the test confirms rejection occurs on the checkpoint-input validation path.
- Around line 25-64: Expand TestIndexCheckpoints with subtests and fresh
per-case fixtures so each assertion is independent. Add cases covering nil
arkPtx or nil UnsignedTx and a nil checkpoint, while retaining the existing
count, duplicate, and multi-input reuse checks; avoid mutating the shared arkPtx
by constructing each case through newCheckpoint and newArkTx.
- Around line 150-202: Extend the validateCheckpoint tests with cases covering
value inflation and anchor validation. Add a test that increases
setup.checkpoint.UnsignedTx.TxOut[0].Value and the corresponding ark witness
UTXO value, asserting the authenticated previous-output value error; add another
test replacing output 1 with a non-anchor output, asserting the anchor-output
validation error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49ffa290-e9c6-43d1-9baa-b3bf40b56bba
📒 Files selected for processing (10)
internal/application/prevout.gointernal/application/prevout_test.gointernal/application/testdata/ark_prevout_fetcher.jsoninternal/application/testdata/onchain_prevout_fetcher.jsoninternal/application/tx.gointernal/application/tx_test.gotest/cross_input_script_validation_test.gotest/pay_2_out_test.gotest/tx_test.gotest/utils_test.go
| func validateTaprootLeaf(input psbt.PInput, expectedLeaf txscript.TapLeaf) error { | ||
| if input.WitnessUtxo == nil || !txscript.IsPayToTaproot(input.WitnessUtxo.PkScript) { | ||
| return fmt.Errorf("witness utxo is not taproot") | ||
| } | ||
| if len(input.TaprootLeafScript) == 0 || input.TaprootLeafScript[0] == nil { | ||
| return fmt.Errorf("missing taproot leaf script") | ||
| } | ||
|
|
||
| leaf := input.TaprootLeafScript[0] | ||
| controlBlock, err := txscript.ParseControlBlock(leaf.ControlBlock) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid control block: %w", err) | ||
| } | ||
| if controlBlock.LeafVersion != leaf.LeafVersion { | ||
| return fmt.Errorf("control block leaf version does not match taproot leaf script") | ||
| } | ||
| if txscript.NewTapLeaf(leaf.LeafVersion, leaf.Script).TapHash() != expectedLeaf.TapHash() { | ||
| return fmt.Errorf("tapleaf does not match ark input") | ||
| } | ||
| if err := txscript.VerifyTaprootLeafCommitment(controlBlock, input.WitnessUtxo.PkScript[2:], leaf.Script); err != nil { | ||
| return fmt.Errorf("tapleaf is not committed by witness utxo: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the codebase selects a taproot leaf script entry elsewhere.
rg -n -C5 'TaprootLeafScript' --type=goRepository: arkade-os/emulator
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching tx.go:"
fd -a 'tx\.go$' . || true
echo
echo "Go files with TaprootLeafScript (case insensitive):"
rg -n -i -C5 'TaprootLeafScript' . -g '*.go' || true
echo
echo "Find validation function context:"
rg -n -C10 'func validateTaprootLeaf|validateTaprootLeaf\(' . -g '*.go' || trueRepository: arkade-os/emulator
Length of output: 28987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "validateCheckpoint call sites:"
rg -n -C8 'validateCheckpoint\(' internal/application/tx.go || true
echo
echo "ReadArkadeScript implementation:"
sed -n '1,160p' pkg/arkade/script.go | cat -n
echo
echo "Go module/dependency hints for btcd txscript/psbt:"
rg -n 'github\.com/lightninglabs/btcwallet|github\.com/lightningnetwork/btcd|package psbt|func.*TaprootLeafScript' go.mod go.sum 2>/dev/null || trueRepository: arkade-os/emulator
Length of output: 7883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PSBT library taproot leaf script handling in module cache, if present:"
go env GOPATH GOMODCACHE 2>/dev/null || true
GOMODCACHE="$(go env GOMODCACHE 2>/dev/null || :)"
if [ -n "$GOMODCACHE" ] && [ -d "$GOMODCACHE" ]; then
find "$GOMODCACHE/github.com/btcsuite/btcd*" -path '*/btcutil/psbt/*' -type f 2>/dev/null | head -20 || true
for f in $(find "$GOMODCACHE/github.com/btcsuite/btcd*" -path '*btcutil/psbt' -name '*.go' 2>/dev/null | head -40); do
if rg -q 'TaprootLeafScript|taproot.*leaf' "$f"; then
echo "### $f"
sed -n '1,220p' "$f" | rg -n -C3 'TaprootLeafScript|Taproot.*leaf|LeafScript|leaf' || true
fi
done
fiRepository: arkade-os/emulator
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Exact Go dependency versions:"
sed -n '1,120p' go.mod | cat -n
echo
echo "List installed btcd/btcutil packages:"
find /home/jailuser/go/pkg/mod/github.com/btcsuite* -maxdepth 5 -type d | sed 's#^/home/jailuser/go/pkg/mod/##' | sort | head -120
echo
echo "BTCD PSBT TaprootLeafScript references:"
find /home/jailuser/go/pkg/mod/github.com/btcsuite* -type f -name '*.go' -print0 | xargs -0 rg -n 'TaprootLeafScript|TaprootTapLeafScript|leafScript|LeafVersion' | head -120Repository: arkade-os/emulator
Length of output: 7430
🌐 Web query:
btcsuite btcutil psbt TaprootLeafScript TaprootTapLeafScript array docs
💡 Result:
In the btcsuite library (btcd), the terms TaprootLeafScript and TaprootTapLeafScript relate to the implementation of BIP 371, which defines Taproot fields for Partially Signed Bitcoin Transactions (PSBT) [1][2]. TaprootLeafScript in the context of a PSBT input (within the btcutil/psbt package) is an array of pointers to TaprootTapLeafScript structs [3][4]. This field is used to hold information required to spend a Taproot input via the script path, including the control block and the revealed script [2][5]. Key structural details include: 1. TaprootTapLeafScript: This struct represents an individual entry in the TaprootLeafScript array. It typically contains: - ControlBlock: The control block for the leaf as specified in BIP 341 [2][4]. - Script: The script for the leaf [2][4]. - LeafVersion: The version of the tapscript leaf [2][4]. 2. Implementation Context: - The PSBT serialization logic handles these fields as key-value pairs (PSBT_IN_TAP_LEAF_SCRIPT = 0x15) [2][4]. - The btcsuite library includes logic to validate, sort, and serialize these entries [3][6][4]. There have been specific updates to ensure that nil entries in this array are handled correctly to avoid serialization panics [3][6]. - These fields are essential for processes such as fee estimation, where the revealed script and control block are used to calculate the weight of the witness stack for a script path spend [5]. For developers, these structures are found within the btcutil/psbt and txscript packages of the btcd repository [7][8][4]. You should consult the source code in the github.com/btcsuite/btcd repository, specifically the psbt/partial_input.go and psbt/taproot.go files, for the most authoritative technical definition and usage patterns [1][3][4].
Citations:
- 1: psbt: add support for new Taproot fields btcsuite/btcd#1847
- 2: https://bips.dev/371/
- 3: psbt: fix nil pointer panic in TaprootLeafScript serialization btcsuite/btcd#2498
- 4: https://github.com/otaliptus/psbt-v2/blob/v0.2.0/partial_input.go
- 5: lnwallet/btcwallet: support taproot script path fee estimation in FundPSBT lightningnetwork/lnd#10504
- 6: psbt: reject nil taproot leaf scripts btcsuite/btcd#2539
- 7: https://pkg.go.dev/github.com/btcsuite/btcd/btcutil/psbt
- 8: https://github.com/btcsuite/btcd/blob/99e4e00345017a70eadc4e1d06353c56b23bb15c/txscript/taproot.go
Match the expected leaf against every TaprootLeafScript entry.
validateTaprootLeaf only validates input.TaprootLeafScript[0], while a PSBT input can expose multiple taproot leaf-script entries. A valid input with the Arkade leaf at a later index will fail at internal/application/tx.go:276. Find the non-nil entry whose tapleaf hash matches expectedLeaf, then validate its control block/script.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/application/tx.go` around lines 272 - 296, Update
validateTaprootLeaf to iterate over every non-nil entry in
input.TaprootLeafScript, selecting the entry whose tapleaf hash matches
expectedLeaf before validating its control block, leaf version, and commitment.
Return the existing missing/mismatch error when no entry matches, while
preserving the current validation behavior for the selected leaf.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
This PR closes two real vulnerabilities — the SIGHASH_ANYONECANPAY prevout lie (#123) and the unauthenticated checkpoint path (#121) — and the authentication chain is correct. The core logic in authenticatedPrevout, validateCheckpoint, and indexCheckpoints is sound and well-tested. Still, several issues need addressing before merge.
Blocking
1. ts-sdk multi-input transactions will break (unacknowledged)
internal/application/prevout.go (new code): prevOutFetcherForArkTx now requires PrevArkTxField for every input:
for inputIndex := range ptx.Inputs {
prevTx, ok := prevoutTxs[inputIndex]
if !ok {
return nil, fmt.Errorf("missing prevout tx for input %d", inputIndex)
}The ts-sdk (ts-sdk/packages/ts-sdk/src/arkade/contract.ts:634) only sets this field for input index 0:
setArkPsbtField(arkTx, 0, PrevArkTxField, coin.sourceTx);When fundingCoins are present they become inputs at indices 1, 2, … (contract.ts:625, 746). Those inputs will carry no PrevArkTxField, and the emulator will reject the transaction. The PR description mentions a go-sdk follow-up but does not mention ts-sdk. Either the ts-sdk fix must land atomically with this PR, or the emulator must continue tolerating missing fields for non-emulator-scripted inputs.
2. go-sdk confirmed broken (acknowledged but no tracking)
The go-sdk (go-sdk/swap/swap.go:383,1570, go-sdk/swap/utils.go:122,534, go-sdk/swap/batch_handler.go:516) never sets PrevArkTxField on ark tx inputs — it only sets ConditionWitnessField. All go-sdk SubmitTx calls will fail immediately after deploy. The PR description says a follow-up is needed; that follow-up must be tracked (issue or linked PR) before merging, otherwise prod clients silently break.
Non-blocking, but should fix
3. BIP322 message-input detection is fragile
internal/application/prevout.go (new code, prevOutFetcherForIntent):
if len(ptx.Inputs) > 1 && ptx.Inputs[0].WitnessUtxo.Value == 0 {Detection relies solely on Value == 0. A legitimate 0-sat VTXO input at index 0 (dust, protocol-level experiments) would be silently reclassified as the BIP322 envelope input and skipped, then authenticated against the wrong expected UTXO. A dedicated PSBT field or the presence of a well-known BIP322 OP_RETURN output would be a more robust signal. Nil is guarded by the preceding computePrevoutFetcher call, so there is no panic risk.
4. validateTaprootLeaf silently ignores all but the first leaf
internal/application/tx.go (new validateTaprootLeaf):
leaf := input.TaprootLeafScript[0]If a caller submits len(TaprootLeafScript) > 1, entries at indices 1+ are never validated. Signing later uses script.Hash() from resolveArkadeScriptSigner (not from the leaf directly), so there is no immediate signing exploit here. However, an assertion (len(input.TaprootLeafScript) != 1 → error) would eliminate the ambiguity and match the protocol invariant that a checkpoint input carries exactly one leaf.
5. signedCheckpointTxs := tx.Checkpoints aliases the request slice
internal/application/tx.go (new SubmitTx, diff line replacing the ordered-copy loop):
signedCheckpointTxs := tx.CheckpointsThe old code reconstructed an ordered slice from orderedCheckpointTxids to guarantee deterministic response order regardless of caller ordering. The new code trusts that the caller's order is correct. indexCheckpoints validates that every ark input has a matching checkpoint, but it does not enforce that the caller-supplied slice is ordered the same way as arkPtx.UnsignedTx.TxIn. If arkd or downstream consumers expect checkpoints to be returned in input order, this could silently produce mis-ordered responses for transactions where the checkpoint slice order differs from the input order.
Positive observations
- The three-layer authentication chain (prevArkTx → checkpoint → arkTx via
authenticatedPrevout) is correct and exactly matches the data that is hash-bound in each transaction. validateCheckpointproperly enforces: shape (1 input, 2 outputs), outpoint binding, anchor output equality, value conservation, and tapleaf commitment viaVerifyTaprootLeafCommitment.indexCheckpointscorrectly catches: count mismatch, nil checkpoints, duplicates, and unmapped ark inputs.- Moving
validateCheckpointbeforescript.Execute/ signing (vs. after in the old code) is the right order. TestSubmitOnchainTxRejectsAnyOneCanPayPrevoutLiedirectly encodes the SIGHASH_ANYONECANPAY attack vector; good regression coverage.TestValidateCheckpointcovers the substituted-leaf and unrelated-destination attacks.- Package-level
sync.Maptest globals (integrationPrevArkTxs,skipPrevArkTxFields) are never reset between runs.go test -count=2will bleed state, though txid uniqueness makes correctness failures unlikely in practice.
768449a to
8fde023
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/application/tx_test.go`:
- Around line 190-195: Require exactly one non-nil TaprootLeafScript before
accessing index zero in validateCheckpoint, signer.signInput, and
ReadArkadeScript; reject empty, nil-entry, and multiple-entry slices. Extend the
relevant tests, including the substituted checkpoint leaf case, to cover all
three malformed inputs and verify they fail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f6f34b3-424a-4593-9f59-0c7baa799fb5
📒 Files selected for processing (7)
internal/application/intent_test.gointernal/application/prevout.gointernal/application/prevout_test.gointernal/application/signer_test.gointernal/application/testdata/onchain_prevout_fetcher.jsoninternal/application/tx.gointernal/application/tx_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/application/testdata/onchain_prevout_fetcher.json
- internal/application/tx.go
- internal/application/prevout_test.go
| t.Run("substituted checkpoint leaf", func(t *testing.T) { | ||
| setup := newSetup(t, false) | ||
| setup.checkpoint.Inputs[0].TaprootLeafScript[0] = setup.foreignLeaf | ||
| err := validateCheckpoint(setup.arkPtx, 0, setup.checkpoint, setup.previousOutput, setup.expectedLeaf) | ||
| require.ErrorContains(t, err, "tapleaf does not match ark input") | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/application/signer.go --items all
rg -n -C 5 'TaprootLeafScript|signInput|validateCheckpoint' \
internal/application/signer.go \
internal/application/tx.go \
internal/application/tx_test.goRepository: arkade-os/emulator
Length of output: 20846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant guard/query behavior and PSBT type definition from vendored/library source if present.
sed -n '18,75p' internal/application/signer.go
sed -n '238,330p' internal/application/tx.go
# Check whether the repo vendor or module cache contains btcd psbt.TaprootTapLeafScript definition/source.
rg -n "type TaprootTapLeafScript|\\bTaprootLeafScript" --glob '*.go' . gopath 2>/dev/null || true
# Read the current test around lines 190-195 without assumptions.
sed -n '176,205p' internal/application/tx_test.goRepository: arkade-os/emulator
Length of output: 13166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the ark script resolver enforces TaprootLeafScript cardinality before indexing entry zero.
sed -n '112,145p' pkg/arkade/script.go
rg -n -C 3 'verifyNonArkdCheckpointSignatures|func .*Signatures|TaprootScriptSpendSig|TaprootLeafScript' internal/application/tx.go internal/application/tx_test.go internal/application/signer_test.goRepository: arkade-os/emulator
Length of output: 21999
Enforce exactly one valid TaprootLeafScript before indexing entry zero.
validateCheckpoint, signer.signInput, and ReadArkadeScript only check len(...) == 0 or input.TaprootLeafScript[0] == nil; they still accept duplicates. Add tests for an empty slice, a nil entry, and multiple entries; add a guard such as len(...) != 1 || input.TaprootLeafScript[0] == nil before indexing leaf zero so malformed PSBTs fail instead of using the first leaf unchecked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/application/tx_test.go` around lines 190 - 195, Require exactly one
non-nil TaprootLeafScript before accessing index zero in validateCheckpoint,
signer.signInput, and ReadArkadeScript; reject empty, nil-entry, and
multiple-entry slices. Extend the relevant tests, including the substituted
checkpoint leaf case, to cover all three malformed inputs and verify they fail.
Source: Learnings
8fde023 to
54db467
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
One commit since last pass (ae624e0). Prior finding #3 fixed; findings #1, #2, #4, #5 remain open (PR merged with them in place).
Prior-finding disposition
1. ts-sdk multi-input transactions will break — Still open
ae624e0 does not touch prevOutFetcherForArkTx. The requirement that every Ark-tx input carry a PrevArkTxField stands. The ts-sdk only sets this field for input index 0 (contract.ts:634); multi-input funding transactions at indices 1+ will still be rejected at runtime. The incremental commit makes this neither better nor worse.
2. go-sdk never sets PrevArkTxField — Still open
Not touched. All go-sdk SubmitTx paths continue to fail on this emulator version. No tracking issue or linked PR is visible.
3. BIP322 message-input detection relied on Value == 0 — Fixed (ae624e0)
chris-ricketts replaced the conditional value-heuristic with an unconditional protocol invariant: input 0 is always the BIP322 message input and the function hard-returns an error for < 2 inputs. equalTxOut now enforces value == 0 AND pkScript match against input 1 as a correctness assertion, not a detection gate. This eliminates the path where a non-zero-value input at index 0 could slip past the BIP322 check. The fix is correct.
4. validateTaprootLeaf silently ignores TaprootLeafScript[1+] — Still open
internal/application/tx.go line indexing into input.TaprootLeafScript[0] without asserting len == 1 is unchanged. No new assertions were added.
5. signedCheckpointTxs := tx.Checkpoints aliases caller slice — Still open
The deterministic ordering concern is unchanged.
Incremental findings (ae624e0 only)
A. New TestPrevOutFetcherForIntentMessageInput — good coverage, one gap
internal/application/prevout_test.go (new, +91 lines): the six sub-cases cover valid, fabricated-value, script-mismatch, missing-field, hash-mismatch, and single-input paths. All directly test the invariant the new code enforces. Coverage is solid.
One gap: there is no test for an intent with exactly 2 inputs where Inputs[1].WitnessUtxo is nil. computePrevoutFetcher guards this (it rejects nil WitnessUtxo on any input and is called first), so there is no panic risk, but the error message ("witness utxo is nil") is less informative than "intent requires real input's witness utxo". Low priority; the nil path is already guarded.
B. Removal of intent-path JSON fixtures
testdata/ark_prevout_fetcher.json: three fixture entries removed ("intent psbt", "missing prevout field", "mismatched prevout hash"). The first two were exercising the old optional-detection code path; the third was a mismatched-hash test. The unit tests in TestPrevOutFetcherForIntentMessageInput replace them with cleaner, more readable coverage. The removal is intentional and correct; the fixture file is now structurally cleaner.
C. 2-input minimum is a protocol change worth documenting
internal/application/prevout.go: the hard len < 2 rejection codifies that no intent proof can ever have a single input. This is correct per the BIP322 + Ark architecture (one signing envelope input + at least one VTXO), but it is a breaking change for any client sending 1-input intent proofs. The PR description does not call this out explicitly. Given findings #1 and #2 are still open, a follow-up compatibility note in the API/SDK docs would help SDK authors understand the full set of new requirements on SubmitTx.
Summary
ae624e0 is a clean, targeted fix for a legitimate prior finding. No new blocking issues are introduced. The two originally-blocking SDK-compatibility issues (#1 go-sdk, #2 ts-sdk) are still unresolved; follow-up tracking for those should be confirmed before deploying this emulator version against production SDK clients.
Emulator v0.0.7 (arkade-os/emulator#129, "validate checkpoints and prevouts") walks every input of a submitted Arkade transaction or intent proof and hard-fails on a missing prevarktx field; v0.0.6 only validated the fields a PSBT happened to carry, so zero fields meant zero checks. The SDK never attached the field, so once the regtest submodule bumped the emulator to v0.0.7 every POST /v1/tx failed with "missing prevout tx for input 0". The transaction an Arkade input must carry is the checkpoint's parent, not the checkpoint: input i spends checkpoint i, which spends the coin, so it is the VTXO's own creating transaction. The emulator reconciles it against the checkpoint's witness utxo. Only the previous transaction's outputs are read (txid, value, script, introspection opcodes), all witness-independent, so a PSBT's unsigned global transaction resolves correctly and no signature propagation has to be waited on. The field also lives in the PSBT unknown map, which no sighash covers, so it is attached after signing, at submit time. - IPrevArkTxProvider / PrevArkTxProvider resolve previous transactions from IVirtualTxStorage, then arkd's indexer, then chain. Indexer results are keyed by the txid parsed out of each body, since arkd returns them in database order rather than request order, and bodies that were not requested are dropped. - ArkadePsbtExtensions.AttachPrevArkTxsAsync / AttachIntentPrevArkTxsAsync annotate an Arkade transaction and a BIP322 intent proof respectively; the proof's synthetic input 0 is left alone. An input already carrying the field keeps it, since the emulator rejects an input bearing two. - ArkadeEmulatorSpendSubmitter annotates before submitting; AddArkadeEmulator registers the provider. - IBitcoinBlockchain.GetRawTransactionAsync, defaulted to NotSupportedException and implemented for Esplora and NBXplorer. Boarding and commitment parents have no off-chain body, so an intent proof registering a boarding input cannot be annotated without it. Unresolvable parents throw with the input index and txid named rather than letting the emulator reject the submission. The batch tree-signing hop still submits to POST /v1/tx with an empty checkpoint list, which v0.0.7 rejects before it looks at prevouts; that needs a different shape and is documented as a limit.
* chore: update regtest to latest and drop the unused ARK_CONTAINER override * chore(ci): don't run e2e swaps tests on CI * feat(arkade): support emulator v0.0.7 prevarktx requirement Emulator v0.0.7 (arkade-os/emulator#129, "validate checkpoints and prevouts") walks every input of a submitted Arkade transaction or intent proof and hard-fails on a missing prevarktx field; v0.0.6 only validated the fields a PSBT happened to carry, so zero fields meant zero checks. The SDK never attached the field, so once the regtest submodule bumped the emulator to v0.0.7 every POST /v1/tx failed with "missing prevout tx for input 0". The transaction an Arkade input must carry is the checkpoint's parent, not the checkpoint: input i spends checkpoint i, which spends the coin, so it is the VTXO's own creating transaction. The emulator reconciles it against the checkpoint's witness utxo. Only the previous transaction's outputs are read (txid, value, script, introspection opcodes), all witness-independent, so a PSBT's unsigned global transaction resolves correctly and no signature propagation has to be waited on. The field also lives in the PSBT unknown map, which no sighash covers, so it is attached after signing, at submit time. - IPrevArkTxProvider / PrevArkTxProvider resolve previous transactions from IVirtualTxStorage, then arkd's indexer, then chain. Indexer results are keyed by the txid parsed out of each body, since arkd returns them in database order rather than request order, and bodies that were not requested are dropped. - ArkadePsbtExtensions.AttachPrevArkTxsAsync / AttachIntentPrevArkTxsAsync annotate an Arkade transaction and a BIP322 intent proof respectively; the proof's synthetic input 0 is left alone. An input already carrying the field keeps it, since the emulator rejects an input bearing two. - ArkadeEmulatorSpendSubmitter annotates before submitting; AddArkadeEmulator registers the provider. - IBitcoinBlockchain.GetRawTransactionAsync, defaulted to NotSupportedException and implemented for Esplora and NBXplorer. Boarding and commitment parents have no off-chain body, so an intent proof registering a boarding input cannot be annotated without it. Unresolvable parents throw with the input index and txid named rather than letting the emulator reject the submission. The batch tree-signing hop still submits to POST /v1/tx with an empty checkpoint list, which v0.0.7 rejects before it looks at prevouts; that needs a different shape and is documented as a limit. * fix(ci): drop the commented-out e2e-swaps job from publish's needs 8c20170 commented out the e2e-swaps job but left it in publish's needs list, so Actions rejected the whole workflow file — "Job 'publish' depends on unknown job 'e2e-swaps'" — and CI failed at startup in 0s without running a single job. * fix(arkade): close three gaps in the emulator v0.0.7 prevarktx support RpcBlockchain did not override GetRawTransactionAsync, contradicting the interface doc's claim that every in-box backend does. It holds an RPCClient already, so it gets the same getrawtransaction/verbose=false implementation as the NBXplorer backend. A backend that genuinely cannot serve raw transactions was swallowed per-txid at debug level, so the caller's "could not be resolved" error named the symptom and hid the cause. PrevArkTxProvider now reports a NotSupportedException once, at warning level, naming the backend and the missing override, and stops retrying the remaining txids. ArkadeBatchSessionExtension routed PostTreeSigning to POST /v1/tx with an empty checkpoint list and no prevarktx fields. Emulator v0.0.7 requires one checkpoint per input plus the field on every input before it inspects anything else, and a tree transaction has neither, so the hop reached the emulator with a request it cannot accept. It now throws NotSupportedException at the call site, matching the existing PreForfeitFinalization guard. CoSignWithEmulatorAsync attaches no prevarktx and submits no checkpoints, so it is subject to the same rejection. Its XML doc now says so and points at AttachPrevArkTxsAsync / AttachIntentPrevArkTxsAsync. * refactor(arkade): make the unreachable co-sign path explicit, drop hardcoded networks CoSignAsync's per-PSBT emulator loop was unreachable: BatchExtensionPhase has two values and both throw above it. A maintainer adding a third phase would have found a loop that had never run. The two guards become a switch over the phase whose default arm throws ArgumentOutOfRangeException, so an unhandled phase fails loudly and the place a future submission arm belongs is named rather than implied. RpcBlockchain and NBXplorerBlockchain both know the network they are connected to, so they parse against it instead of Network.Main. Esplora is constructed from a bare base URI and has none; its comment now says that rather than reading as an oversight. The backend-without-raw-tx-support test used a single txid, so it could not tell whether the loop gives up after the first structural failure. A second case uses two txids and asserts one chain call and one warning.
Summary
PrevArkTxField; the emulator performs no indexer or other network query on the request path.This supersedes closed PR #127 and includes the authenticated-prevout fix for #123.
Validation
go test -v -count=1 ./test/...against a clean rebuilt regtest stack: PASS (361.202s)make testfrom a clean clone: PASSgo vet ./internal/application: PASSgit diff --check: PASSCloses #121.
Closes #123.
Summary by CodeRabbit