Repository navigation
fix: verify commitment tx with arkd indexer before signing in SubmitFinalization - #133
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe emulator now configures an arkd indexer, verifies commitment transactions with retries before finalization signing, and tests rejection of unknown commitment transactions. ChangesCommitment indexer validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SubmitFinalization
participant ArkdIndexer
participant ForfeitSigner
SubmitFinalization->>ArkdIndexer: Look up commitment transaction ID
ArkdIndexer-->>SubmitFinalization: Return recognition result
SubmitFinalization->>ForfeitSigner: Sign only recognized commitment inputs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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
🧹 Nitpick comments (1)
test/tx_test.go (1)
787-1023: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared boarding setup into a helper.
Lines 787-1023 duplicate almost the entire body of
TestBoarding(lines 1064-1307). Only the emulator client construction, the commitment tx override, and the final assertions differ. Any change to the boarding flow now needs two edits, and the two copies will drift.Extract the common part into a helper that returns the values both tests need, for example the intent, the intent id, the boarding
client.TapscriptsVtxo, the tree signer session, the emulator client, the wallet, and the explorer. Each test then only setsOverrideCommitmentTxand asserts.🤖 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 `@test/tx_test.go` around lines 787 - 1023, Extract the duplicated boarding setup from TestSubmitFinalizationRejectsUnknownCommitmentTx and TestBoarding into a shared test helper. Have the helper return the common intent, intent ID, boarding client.TapscriptsVtxo, tree signer session, emulator client, wallet, and explorer; keep emulator construction configurable as needed. Refactor both tests to use the helper, leaving only each test’s OverrideCommitmentTx configuration and final assertions in the test bodies.
🤖 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/finalization.go`:
- Around line 23-46: Bound the commitment lookup in the finalization flow around
retryWithBackoff by deriving a context with the configured
commitmentTxLookupTimeout and ensuring it is canceled after the lookup. Update
the retry callback to stop immediately for definitive not-found responses, while
retrying only transport or unavailable errors; preserve the existing arkd
indexer validation and wrapped failure reporting.
---
Nitpick comments:
In `@test/tx_test.go`:
- Around line 787-1023: Extract the duplicated boarding setup from
TestSubmitFinalizationRejectsUnknownCommitmentTx and TestBoarding into a shared
test helper. Have the helper return the common intent, intent ID, boarding
client.TapscriptsVtxo, tree signer session, emulator client, wallet, and
explorer; keep emulator construction configurable as needed. Refactor both tests
to use the helper, leaving only each test’s OverrideCommitmentTx configuration
and final assertions in the test bodies.
🪄 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: 9a7b538c-1cb8-4c84-af8f-2d7690cce56b
📒 Files selected for processing (6)
README.mdinternal/application/finalization.gointernal/application/service.gointernal/config/config.gotest/tx_test.gotest/utils_test.go
| // Sign nothing unless the commitment tx is an arkd-built artifact: a | ||
| // client-chosen commitment tx (e.g. an ark tx spending a pending | ||
| // checkpoint outpoint) would let a replayed proof bypass the arkade | ||
| // script execution that justified it. arkd indexes its commitment txs | ||
| // on round finalization; the write may lag the RoundFinalizationStarted | ||
| // event, so retry. | ||
| if finalization.CommitmentTx == nil { | ||
| return nil, fmt.Errorf("commitment tx is required") | ||
| } | ||
| if s.indexerClient == nil { | ||
| return nil, fmt.Errorf("arkd indexer client is not configured") | ||
| } | ||
| commitmentTxid := finalization.CommitmentTx.UnsignedTx.TxID() | ||
| err := retryWithBackoff(ctx, commitmentTxRetryConfig, | ||
| func() error { | ||
| _, err := s.indexerClient.GetCommitmentTx(ctx, commitmentTxid) | ||
| return err | ||
| }, nil, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf( | ||
| "commitment tx %s not known to arkd indexer: %w", commitmentTxid, err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the total lookup time and stop retrying on a definitive "not found".
The retry loop retries every error, including a permanent not-found response from the indexer. With these parameters, a request with an unknown commitment tx blocks the handler goroutine for several seconds before it fails (200ms, 400ms, 800ms, 1.6s, 2s, 2s, 2s with jitter). An unauthenticated caller can submit many such requests and hold goroutines and indexer connections. The integration test also pays this cost on every run.
Two mitigations:
- Derive a bounded context for the lookup, so the total wait has a hard ceiling.
- Treat a definitive not-found response as terminal instead of retryable, and retry only transport or unavailable errors.
🛡️ Proposed bound on total lookup time
commitmentTxid := finalization.CommitmentTx.UnsignedTx.TxID()
- err := retryWithBackoff(ctx, commitmentTxRetryConfig,
+ lookupCtx, cancel := context.WithTimeout(ctx, commitmentTxLookupTimeout)
+ defer cancel()
+ err := retryWithBackoff(lookupCtx, commitmentTxRetryConfig,
func() error {
- _, err := s.indexerClient.GetCommitmentTx(ctx, commitmentTxid)
+ _, err := s.indexerClient.GetCommitmentTx(lookupCtx, commitmentTxid)
return err
}, nil,
)Add the ceiling next to the retry configuration:
const commitmentTxLookupTimeout = 5 * time.SecondAlso applies to: 314-321
🤖 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/finalization.go` around lines 23 - 46, Bound the
commitment lookup in the finalization flow around retryWithBackoff by deriving a
context with the configured commitmentTxLookupTimeout and ensuring it is
canceled after the lookup. Update the retry callback to stop immediately for
definitive not-found responses, while retrying only transport or unavailable
errors; preserve the existing arkd indexer validation and wrapped failure
reporting.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #133
The commitment-tx indexer gate closes a real attack surface: before this change a client who had previously obtained the emulator's signature on an intent proof could replay that proof during SubmitFinalization with a client-chosen commitment tx, getting forfeit signatures without arkd having actually produced a batch for those VTXOs. Placing the gate unconditionally before any signing — forfeits and boarding path alike — is the correct location.
Protocol / security
Boarding path is covered. When all signedInputs are consumed by forfeits the function returns early at internal/application/finalization.go:93; when inputs remain (boarding), execution reaches finalization.go:98. The indexer gate runs before either branch, so both paths are protected. ✓
txid as proof. commitmentTxid is the Bitcoin double-SHA256 of the unsigned transaction (finalization.go new block, UnsignedTx.TxID()). A matching txid proves content identity — no crafted PSBT metadata can satisfy the check with a different transaction. ✓
PSBT metadata trust boundary. Forfeit output validation continues to use prevouts from the intent proof (which carries the emulator's own signature) and the connector tree, not from the commitment PSBT itself. The indexer check does not introduce a new trusted source for those values. ✓
Findings
1. Missing retry log — internal/application/finalization.go (new retryWithBackoff call)
onErr: nil is passed, so the retry loop can spin up to 8 attempts over ≈6 s with zero log output. finalizeRetryConfig's caller in tx.go:253 logs on every attempt; this site should do the same. When the indexer lags or is unreachable, operators currently have no signal that finalization is retrying rather than hanging.
// suggested
func(attempt int, e error) {
log.WithField("txid", commitmentTxid).WithField("attempt", attempt).
Warnf("commitment tx not yet indexed, retrying: %s", e)
},2. MinAttempts: 5 ignores ctx for the first four retries — internal/application/finalization.go:313
The retryWithBackoff implementation (tx.go:393) calls time.Sleep without checking ctx.Done() for attempt < MinAttempts. With the current config that is up to ≈200 + 400 + 800 + 1 600 ms ≈ 3 s of uncancellable sleep. This matches the pattern used by arkdConnectRetryConfig (MinAttempts=0 there, but the design is the same), and is probably acceptable for finalization. Worth a short comment explaining the intentional trade-off, similar to the one on arkdConnectRetryConfig.
3. Mutable package-level commitmentTxRetryConfig — internal/application/finalization.go:311 and finalization_test.go:126
TestSubmitFinalizationRejectsUnknownCommitmentTx saves and restores the global in t.Cleanup, which is safe today because no test in this package calls t.Parallel(). If that changes, there is a data race. A parameter-injection approach (accepting a retryConfig via a functional option or passing it through service) would be more robust, but this is a style concern not a correctness defect.
4. MaxElapsed: 0 on commitmentTxRetryConfig — internal/application/finalization.go:311
MaxElapsed unset means the elapsed-time budget check (tx.go:388) is skipped, leaving only MaxAttempts: 8 to bound the loop. That is sufficient, but it is asymmetric with finalizeRetryConfig. Acceptable as-is; a comment noting the omission is intentional would help future readers.
No cross-repo breakage
Service interface signature is unchanged. New() gains arkdIndexerURL but its only caller is config.AppService (updated in this PR). ts-sdk, rust-sdk, and dotnet-sdk are consumers of the emulator's gRPC protocol, not its Go API, and the on-wire contract is unchanged.
Tests
TestSubmitFinalizationRejectsUnknownCommitmentTx correctly verifies that:
SubmitFinalizationreturns an error containing"not known to arkd indexer", and- the forfeit input carries no signature after rejection.
Existing forfeit-output-validation tests are correctly updated to inject &mockIndexerClient{} (the happy-path mock that always returns &indexer.CommitmentTx{}), so they continue to exercise the signing logic.
A test covering the happy path with a slow indexer (i.e., first N calls return an error, then succeed) would strengthen confidence in the retry path, but is not a blocker.
Summary
One actionable item before merge: add retry logging (finding 1). Findings 2–4 are informational. Overall the change is a sound security improvement with correct placement, adequate test coverage, and no cross-repo impact.
Summary
SubmitFinalizationpreviously signed any structurally valid commitment tx carrying inputs that matched a signed intent proof. It now confirms the commitment tx is an artifact produced by arkd before signing anything — forfeits included.SubmitFinalization: the commitment tx's txid must resolve viaindexer.GetCommitmentTx. Unknown txid → rejection, no signatures produced.RoundFinalizationStartedevent and the indexer projection write in arkd (deferredsaveEventsinstartFinalization).EMULATOR_ARKD_INDEXER_URL; falls back toEMULATOR_ARKD_URLwhen unset, matching deployments where the indexer shares the arkd endpoint. No breaking change.indexer.Indexerclient alongside the existing transport client (built inNew, closed inClose).Test
TestSubmitFinalizationRejectsUnknownCommitmentTx: runs a boarding settlement while the batch handler overrides the commitment tx with a locally-built PSBT; expects finalization to fail withnot known to arkd indexer.delegateBatchEventsHandlergains anOverrideCommitmentTxhook (zero value = no override), honored by both the forfeit and boarding finalization paths in test utils.Notes
@msinkec / @louisinger please review
Summary by CodeRabbit
New Features
Bug Fixes
Tests