Conversation
WalkthroughFinalized offchain transactions are now decoded including inputs; if an expired offchain tx has any input vtxo already marked Swept (or fetching inputs errors), the tx is considered swept and newly created output vtxos are marked Swept without running dust/script checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Service
participant TxDecoder
participant VtxoStore
Client->>Service: OffchainTxRequested
Client->>Service: OffchainTxAccepted (ExpiryTimestamp expired)
Client->>Service: OffchainTxFinalized
Service->>TxDecoder: DecodeTx(tx)
TxDecoder-->>Service: inputs (outpoints), outputs (pkScripts, amounts)
Service->>VtxoStore: GetVtxos(decoded inputs)
alt GetVtxos error or any input Swept
VtxoStore-->>Service: error / Swept found
Service->>VtxoStore: CreateVtxos(outputs with Swept = true)
else inputs not swept
VtxoStore-->>Service: inputs not swept
Service->>VtxoStore: CreateVtxos(outputs with Swept = script.IsSubDustScript(...))
end
VtxoStore-->>Service: ack created vtxos
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Review — #999Fix: Finalize swept pending tx SummaryWhen an offchain tx is finalized after its expiry, the new VTXOs may need to be marked as swept if the inputs were already swept by the server. Previously the code unconditionally set What ChangedSingle file:
Security Analysis✅ Safe fallback on error: ✅ Correct swept propagation: If the server swept the VTXO tree (inputs are swept), the outputs of a finalized tx built from those inputs should also be swept — they exist in the same expired tree. This is the right behavior.
Missing test coverageThe PR description mentions two needed test cases:
Neither is included in this PR. These would be valuable to add, especially the second case which verifies that non-swept expired txs produce spendable outputs. VerdictClean, minimal fix with correct safety semantics. The conservative error handling is the right call. Tests should follow to cover both swept and non-swept expiry scenarios. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/db/service.go`:
- Around line 541-543: The code currently sets txSwept = err != nil after
calling s.vtxoStore.GetVtxos, which incorrectly treats DB read errors as
permanent swept state; remove the assignment and instead handle GetVtxos errors
explicitly (e.g., return the error up, log and skip processing, or retry) so
transient read failures don't mark vtxos swept; determine txSwept only from the
actual inputVtxos result (or explicit spent flags) after a successful call to
s.vtxoStore.GetVtxos rather than based on err.
🪄 Autofix (Beta)
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
Run ID: 1cf6a5f5-cea2-48d9-b80f-15ff544c4aea
📒 Files selected for processing (1)
internal/infrastructure/db/service.go
| inputVtxos, err := s.vtxoStore.GetVtxos(ctx, ins) | ||
| // if an error happened, we assume the vtxo is swept. it should never happen but it's to avoid skipping adding vtxo to db | ||
| txSwept = err != nil |
There was a problem hiding this comment.
Do not infer txSwept=true from repository errors.
At Line 543, txSwept = err != nil turns transient DB read failures into permanent swept flags for all new outputs. That can incorrectly lock spendable vtxos.
Proposed fix
if offchainTx.ExpiryTimestamp > 0 &&
time.Now().After(time.Unix(offchainTx.ExpiryTimestamp, 0)) {
inputVtxos, err := s.vtxoStore.GetVtxos(ctx, ins)
- // if an error happened, we assume the vtxo is swept. it should never happen but it's to avoid skipping adding vtxo to db
- txSwept = err != nil
-
- for _, inputVtxo := range inputVtxos {
- if inputVtxo.Swept {
- txSwept = true
- break
- }
- }
+ if err != nil {
+ log.WithError(err).Warn(
+ "failed to infer swept state from input vtxos; falling back to script-based swept check",
+ )
+ } else {
+ for _, inputVtxo := range inputVtxos {
+ if inputVtxo.Swept {
+ txSwept = true
+ break
+ }
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/infrastructure/db/service.go` around lines 541 - 543, The code
currently sets txSwept = err != nil after calling s.vtxoStore.GetVtxos, which
incorrectly treats DB read errors as permanent swept state; remove the
assignment and instead handle GetVtxos errors explicitly (e.g., return the error
up, log and skip processing, or retry) so transient read failures don't mark
vtxos swept; determine txSwept only from the actual inputVtxos result (or
explicit spent flags) after a successful call to s.vtxoStore.GetVtxos rather
than based on err.
|
@louisinger I added the 2 test cases you outlined in the PR description |
|
Iterative review — new commit Good test coverage for the swept-status propagation logic. Two cases are the right ones to test:
Observations:
Overall this is solid test coverage for a critical edge case. LGTM on the test additions. The core logic in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/infrastructure/db/service_test.go (1)
2201-2206: Preserve failure cause in async polling.If projection fails internally, this eventually check times out without surfacing the last DB error, which makes failures harder to triage.
💡 Proposed fix
+ var lastErr error require.Eventually(t, func() bool { vtxos, err := svc.Vtxos().GetVtxos(ctx, []domain.Outpoint{ {Txid: outTxid, VOut: 0}, }) - return err == nil && len(vtxos) > 0 - }, 5*time.Second, 100*time.Millisecond) + lastErr = err + return err == nil && len(vtxos) > 0 + }, 5*time.Second, 100*time.Millisecond, + "projected output vtxo not found (last error: %v)", lastErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/service_test.go` around lines 2201 - 2206, The test's require.Eventually call hides the underlying error from svc.Vtxos().GetVtxos when the predicate keeps returning false; capture the last error inside the polling closure (e.g., a variable lastErr set to err each iteration) and after require.Eventually returns assert or fail with that captured error so the real DB/projection failure is surfaced; modify the test around the require.Eventually invocation that calls svc.Vtxos().GetVtxos to record the last err and then use require.NoError(lastErr) or t.Fatalf with lastErr if the eventually check still fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2036-2042: taprootPkScript currently ignores hex.DecodeString
errors and doesn't validate the decoded key length, which lets malformed fixture
hex produce truncated/zero-padded 34-byte scripts; change taprootPkScript to
validate input by handling the hex.DecodeString error and verifying len(pk) ==
32, and fail fast on invalid input (preferably by accepting a *testing.T
parameter and calling t.Fatalf with a clear message, or return an error) before
constructing the 34-byte script so malformed fixtures cause test failures rather
than silent false positives.
---
Nitpick comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2201-2206: The test's require.Eventually call hides the underlying
error from svc.Vtxos().GetVtxos when the predicate keeps returning false;
capture the last error inside the polling closure (e.g., a variable lastErr set
to err each iteration) and after require.Eventually returns assert or fail with
that captured error so the real DB/projection failure is surfaced; modify the
test around the require.Eventually invocation that calls svc.Vtxos().GetVtxos to
record the last err and then use require.NoError(lastErr) or t.Fatalf with
lastErr if the eventually check still fails.
🪄 Autofix (Beta)
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
Run ID: 5d9b02fb-1af7-439d-8e20-aa647acb2151
📒 Files selected for processing (1)
internal/infrastructure/db/service_test.go
|
Iterative review — new commit Minor test hardening: LGTM for this increment. No concerns. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/infrastructure/db/service_test.go (1)
2072-2074: Prefer per-subtest decoder instance to avoid shared mutable state.Creating one decoder outside the loop makes subtests depend on mutable shared state. Instantiating it inside each
t.Runkeeps isolation tight and future-proofs fort.Parallel().♻️ Suggested refactor
- // The mock decoder is swapped per-subtest to return unique outpoints. - decoder := &mockTxDecoder{} - tests := []struct { name string inputSwept bool expectSwept bool @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + decoder := &mockTxDecoder{} // Fresh db per subtest for clean isolation. dbDir := t.TempDir() svc, err := db.NewService(db.ServiceConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/service_test.go` around lines 2072 - 2074, The test creates a single shared decoder variable (decoder := &mockTxDecoder{}) outside the subtest loop which introduces shared mutable state; change the code to instantiate a fresh mockTxDecoder inside each t.Run (e.g., inside the subtest closure create decoder := &mockTxDecoder{}) so each subtest gets its own decoder instance, update any references in the subtest to use that local decoder, and ensure this enables safe use of t.Parallel() in the subtests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2122-2126: The mock decode function (decoder.decodeFn) currently
ignores its tx argument which can hide wiring bugs; update decoder.decodeFn to
assert the incoming tx equals the expected transaction payload used in the test
(e.g., compare to the test's raw/serialized tx variable) and fail the test if it
differs (using the test's t.Fatalf/t.Helper or require/assert helper). Keep the
returned outTxid, in1, in2 and outputs unchanged but add that assertion at the
start of decoder.decodeFn so wrong payloads cause a visible test failure.
---
Nitpick comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2072-2074: The test creates a single shared decoder variable
(decoder := &mockTxDecoder{}) outside the subtest loop which introduces shared
mutable state; change the code to instantiate a fresh mockTxDecoder inside each
t.Run (e.g., inside the subtest closure create decoder := &mockTxDecoder{}) so
each subtest gets its own decoder instance, update any references in the subtest
to use that local decoder, and ensure this enables safe use of t.Parallel() in
the subtests.
🪄 Autofix (Beta)
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
Run ID: bc5c8405-9aa8-45a8-a183-31c4b29ec4fa
📒 Files selected for processing (1)
internal/infrastructure/db/service_test.go
| decoder.decodeFn = func(tx string) (string, []ports.TxIn, []ports.TxOut, error) { | ||
| return outTxid, []ports.TxIn{in1, in2}, []ports.TxOut{ | ||
| {Amount: 1000, PkScript: outputPkScript}, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Validate the decoded tx argument in the mock to avoid false positives.
The mock ignores tx, so the test can pass even if the projection decodes the wrong transaction payload. Add an assertion path in the mock to make wiring regressions visible.
🧪 Suggested tightening
+ expectedDecodedTx := "mock_final_ark_tx"
@@
- decoder.decodeFn = func(tx string) (string, []ports.TxIn, []ports.TxOut, error) {
+ decoder.decodeFn = func(tx string) (string, []ports.TxIn, []ports.TxOut, error) {
+ if tx != expectedDecodedTx {
+ return "", nil, nil, fmt.Errorf("unexpected tx passed to decoder: %q", tx)
+ }
return outTxid, []ports.TxIn{in1, in2}, []ports.TxOut{
{Amount: 1000, PkScript: outputPkScript},
}, nil
}
@@
- FinalArkTx: "mock_final_ark_tx",
+ FinalArkTx: expectedDecodedTx, import (
"context"
"crypto/rand"
"encoding/hex"
+ "fmt"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/infrastructure/db/service_test.go` around lines 2122 - 2126, The
mock decode function (decoder.decodeFn) currently ignores its tx argument which
can hide wiring bugs; update decoder.decodeFn to assert the incoming tx equals
the expected transaction payload used in the test (e.g., compare to the test's
raw/serialized tx variable) and fail the test if it differs (using the test's
t.Fatalf/t.Helper or require/assert helper). Keep the returned outTxid, in1, in2
and outputs unchanged but add that assertion at the start of decoder.decodeFn so
wrong payloads cause a visible test failure.
2 participants
we need 2 test cases :
@altafan please review
Summary by CodeRabbit
Bug Fixes
Tests