fix: duplicated swept vtxos in sweep tx event - #1152
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughBatch sweep processing now collects multiple leaf VTXO outpoints, traverses descendant lineages, filters commitment results to preconfirmed VTXOs, deduplicates swept outpoints, and determines fully swept rounds using distinct leaf transaction IDs. ChangesVTXO sweep lineage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BatchSweep
participant leafVtxoOutpoints
participant VtxoRepository
participant SQLDatabase
BatchSweep->>leafVtxoOutpoints: collect eligible leaf outpoints
BatchSweep->>VtxoRepository: request preconfirmed VTXOs or descendants
VtxoRepository->>SQLDatabase: execute lineage or commitment query
SQLDatabase-->>VtxoRepository: return VTXO outpoints
VtxoRepository-->>BatchSweep: return deduplicated candidates
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
30f8eec to
f55d49a
Compare
f55d49a to
59375aa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/core/application/sweeper_test.go (1)
479-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire
GetSweepablePreconfirmedVtxosByCommitmentTxidthroughm.CalledIt currently always returnsnil, nil, so tests can’t inject preconfirmed vtxos for thecommitmentRootSwept == truepath.🤖 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/core/application/sweeper_test.go` around lines 479 - 483, Update mockVtxoRepository.GetSweepablePreconfirmedVtxosByCommitmentTxid to delegate to m.Called with the provided context and commitmentTxid, then return the configured []domain.Outpoint and error values so tests can inject preconfirmed vtxos for the commitmentRootSwept path.internal/core/application/utils.go (1)
94-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the anchor/extension filter into a shared helper.
This exact check (
bytes.Equal(out.PkScript, txutils.ANCHOR_PKSCRIPT) || extension.IsExtension(out.PkScript)) is now duplicated a third time in this file (also indecodeTxandgetNewVtxosFromRound). Consolidating into one predicate reduces the risk of the three sites diverging if the filter criteria change.♻️ Proposed shared helper
+func isSweepableTxOut(pkScript []byte) bool { + return !bytes.Equal(pkScript, txutils.ANCHOR_PKSCRIPT) && !extension.IsExtension(pkScript) +} + func leafVtxoOutpoints(leaf *psbt.Packet) []domain.Outpoint { txid := leaf.UnsignedTx.TxID() outpoints := make([]domain.Outpoint, 0, len(leaf.UnsignedTx.TxOut)) for i, out := range leaf.UnsignedTx.TxOut { - if bytes.Equal(out.PkScript, txutils.ANCHOR_PKSCRIPT) || - extension.IsExtension(out.PkScript) { + if !isSweepableTxOut(out.PkScript) { continue } outpoints = append(outpoints, domain.Outpoint{Txid: txid, VOut: uint32(i)}) } return outpoints }(similarly replace the checks in
decodeTxandgetNewVtxosFromRound)🤖 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/core/application/utils.go` around lines 94 - 107, Extract the duplicated anchor/extension output check into a shared predicate helper in this file, then update leafVtxoOutpoints, decodeTx, and getNewVtxosFromRound to call it. Preserve the existing behavior by returning true for anchor scripts or extension outputs and skipping those outputs at each call site.
🤖 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/core/application/sweeper.go`:
- Around line 689-714: The preconfirmed-vTXO collection logic is duplicated and
has diverging error messages. In internal/core/application/sweeper.go lines
689-714, extract the commitmentRootSwept lookup and per-leaf GetDescendantVtxos
deduplication into a shared helper, correcting the stale error message; in
internal/core/application/admin.go lines 868-895, replace the equivalent block
with a call to that helper so both paths use the same implementation.
- Around line 689-714: Correct the final `sweepErr` log in the sweep flow so it
identifies failure to fetch sweepable preconfirmed vtxos by commitment
transaction ID, rather than descendant lookup. Keep the per-iteration
`descendantsErr` logging in the `GetDescendantVtxos` branch unchanged, and
update only the message associated with `sweepErr` from
`GetSweepablePreconfirmedVtxosByCommitmentTxid`.
In `@internal/infrastructure/db/sqlite/sqlc/query.sql`:
- Around line 371-405: Update the visited-path cycle guards in
SelectDescendantVtxoOutpointsByArkTxid and
SelectVtxosOutpointsByArkTxidRecursive to use delimiter-bounded matching,
ensuring complete outpoint entries are compared rather than substrings. Preserve
the comma-separated path representation while adding delimiters at the
boundaries so values such as txid:1 cannot match txid:12.
---
Nitpick comments:
In `@internal/core/application/sweeper_test.go`:
- Around line 479-483: Update
mockVtxoRepository.GetSweepablePreconfirmedVtxosByCommitmentTxid to delegate to
m.Called with the provided context and commitmentTxid, then return the
configured []domain.Outpoint and error values so tests can inject preconfirmed
vtxos for the commitmentRootSwept path.
In `@internal/core/application/utils.go`:
- Around line 94-107: Extract the duplicated anchor/extension output check into
a shared predicate helper in this file, then update leafVtxoOutpoints, decodeTx,
and getNewVtxosFromRound to call it. Preserve the existing behavior by returning
true for anchor scripts or extension outputs and skipping those outputs at each
call site.
🪄 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: ec779e20-881b-42fb-a9a1-2bd70fc83527
📒 Files selected for processing (17)
internal/core/application/admin.gointernal/core/application/indexer_test.gointernal/core/application/sweeper.gointernal/core/application/sweeper_test.gointernal/core/application/utils.gointernal/core/domain/round.gointernal/core/domain/round_test.gointernal/core/domain/vtxo_repo.gointernal/infrastructure/db/badger/vtxo_repo.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/postgres/vtxo_repo.gointernal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/infrastructure/db/sqlite/vtxo_repo.gointernal/test/e2e/e2e_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/core/application/utils.go (2)
131-142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
domain.Outpointdirectly as the map key to avoid string allocations.In Go, structs with comparable fields (like
stringanduint32) are fully comparable. Usingdomain.Outpointdirectly as the map key eliminates the need to call.String()on every iteration, avoiding unnecessary heap allocations.♻️ Proposed refactor
- seen := make(map[string]struct{}) + seen := make(map[domain.Outpoint]struct{}) for _, leafVtxo := range leafVtxos { descendants, err := vtxoRepo.GetDescendantVtxos(ctx, leafVtxo) if err != nil { log.WithError(err).Error("error while getting descendant vtxos") continue } for _, descendant := range descendants { - if _, ok := seen[descendant.String()]; !ok { + if _, ok := seen[descendant]; !ok { preconfirmedVtxos = append(preconfirmedVtxos, descendant) - seen[descendant.String()] = struct{}{} + seen[descendant] = struct{}{} } }🤖 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/core/application/utils.go` around lines 131 - 142, Update the deduplication map in the descendant-processing loop to use map[domain.Outpoint]struct{} keyed directly by each descendant, replacing String() calls for lookup and insertion while preserving the existing preconfirmedVtxos append behavior.
119-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid variable shadowing and return the initialized empty slice on error.
Using
:=instead ofvar err erroris cleaner, avoids overwriting the initializedpreconfirmedVtxosslice withnilon failure, and explicitly returns the empty slice correctly on the error path.♻️ Proposed refactor
if commitmentRootSwept { - var err error - preconfirmedVtxos, err = vtxoRepo.GetSweepablePreconfirmedVtxosByCommitmentTxid( + res, err := vtxoRepo.GetSweepablePreconfirmedVtxosByCommitmentTxid( ctx, commitmentTxid, ) if err != nil { log.WithError(err). Error("error while getting sweepable preconfirmed vtxos by commitment txid") + return preconfirmedVtxos } - return preconfirmedVtxos + return res }🤖 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/core/application/utils.go` around lines 119 - 128, Update the commitmentRootSwept branch to fetch results using a scoped short declaration for the repository call, avoiding the separate err declaration and shadowing the initialized preconfirmedVtxos slice. On error, log the failure and return the existing initialized empty slice; on success, return the fetched results.
🤖 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.
Nitpick comments:
In `@internal/core/application/utils.go`:
- Around line 131-142: Update the deduplication map in the descendant-processing
loop to use map[domain.Outpoint]struct{} keyed directly by each descendant,
replacing String() calls for lookup and insertion while preserving the existing
preconfirmedVtxos append behavior.
- Around line 119-128: Update the commitmentRootSwept branch to fetch results
using a scoped short declaration for the repository call, avoiding the separate
err declaration and shadowing the initialized preconfirmedVtxos slice. On error,
log the failure and return the existing initialized empty slice; on success,
return the fetched results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 862266ce-d2bc-48f7-9181-87ce1b2ce8fd
📒 Files selected for processing (3)
internal/core/application/admin.gointernal/core/application/sweeper.gointernal/core/application/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/application/sweeper.go
|
Hi @altafan, quick context on the approach. The simple fix for #1138 would be to just filter the leaf vtxos out of the preconfirmed list in So instead of filtering after the fact, I made the queries honest: a renamed Happy to go the simpler route if this looks invasive. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Reviewed at head f8501c1. This PR touches the batch sweep event flow, VTXO descendant walks, and round-fully-swept accounting. Please have a human protocol reviewer sign off before merge.
Correctness of the fix (looks right)
leafVtxoOutpointsininternal/core/application/utils.go:94-105correctly enumerates every non-anchor / non-extension output of a leaf tx, matching the receiver semantics used elsewhere (getNewVtxosFromRound,decodeTx). This is what fixes the duplication in the sweep event.collectPreconfirmedVtxos(utils.go:110-140) consolidates the two identical branches inadmin.saveBatchSweptEventsandsweeper.createBatchSweepTask. Behavior matches the previous inline code, with the dedup map (seen) preserved.- Renaming
GetSweepableVtxosByCommitmentTxid→GetSweepablePreconfirmedVtxosByCommitmentTxidand addingpreconfirmed = trueis right: the caller already passes leaves separately, so returning them again from the repo was the source of the duplicates. The three implementations (postgres/sqlite/badger) all apply the filter (internal/infrastructure/db/{postgres,sqlite}/sqlc/query.sql,internal/infrastructure/db/badger/vtxo_repo.go:770-773). GetDescendantVtxos(new) correctly excludes the seed: postgres usesWHERE depth > 0, sqlite usesWHERE depth > 0, badger seeds thevisitedmap with the seed outpoint but never appends it tooutpoints. Good.Round.Sweep(round.go:214-224) switching from summinglen(LeafVtxos)to counting distinct leaf txids is the right accounting when a leaf tx carries multiple receivers.
Nice side fix
The SQLite SelectVtxosOutpointsByArkTxidRecursive cycle guard is corrected from unbounded LIKE '%X%' to delimiter-bounded LIKE '%,X,%' (query.sql:355-360). The old form could false-match when one outpoint string was a substring of another (e.g. txid:1 inside txid:12), which would prematurely prune a descendant. The new SelectDescendantVtxoOutpointsByArkTxid uses the same fix. Worth calling out because it silently changes results for the existing method too.
Issues to address / flag
-
Latent multi-receiver bug in
updateVtxoExpirationTime(sweeper.go:786) — this callsite still usesextractVtxoOutpoint, which returns only the first non-anchor output. With the same multi-receiver leaves this PR is fixing elsewhere, only the vout-0 vtxo of each leaf gets its expiration refreshed inUpdateVtxosExpiration; the other receivers keep the initialexpireAtfrom round creation. Not introduced by this PR but the exact same underlying bug — please fix in the same PR or file a follow-up. -
extractVtxoOutpointalso doesn't skip extension outputs —leafVtxoOutpointsskips both anchor and extension, but the still-liveextractVtxoOutpoint(sweeper.go:889) only skips anchor. If a leaf ever has layout[anchor, extension, vtxo],extractVtxoOutpointreturns the extension outpoint. Ties into (1); same remediation. -
fullySweptstill depends onr.Changeswhich is empty when the round is loaded from the projection.sweeper.createBatchSweepTask(sweeper.go:683) andadmin.saveBatchSweptEvents(admin.go:514, 776) all load the round viaGetRoundWithCommitmentTxid, which populates the projection fields but never rehydratesr.Changes(seerowsToRoundsinsqlite/round_repo.go:645,postgres/round_repo.go:518). So the newfor _, event := range r.Changes { … BatchSwept … }loop always sees zero past events. This means for a round that gets swept across multiple subtree/expiry tasks,len(sweptLeafTxids) == leavesCountwill never hold andr.Sweptnever becomestruevia this path. The old code had the same defect (countSweptLeafVtxos(r.Changes)was also always 0 in this callflow), so this is a pre-existing bug rather than a regression — but the PR's stated goal is to makeFullySwept"count distinct leaf txids, otherwise multi-receiver leaves would break the accounting," and it still won't for the multi-sweep case. Either fix (load pastBatchSweptevents / query the DB for already-swept leaf txids of the round) or add a comment acknowledging the limitation. This matters for ther.Sweptflag written inon(BatchSwept)at round.go:308 which is used byGetSweepableRounds. -
fullySweptalso fails if any leaf tx has no vtxo outputs (only anchor/extension).leafVtxoOutpointsreturns[], that leaf's txid never joinssweptLeafTxids, and the round can never reachfullySwept. Not a common case, but worth an assertion or a comment. If it is impossible by construction, no action. -
Badger
GetSweepablePreconfirmedVtxosByCommitmentTxidtraversal viaqueue = append(queue, vtxo.ArkTxid)is suspect (badger/vtxo_repo.go:775-778). The subsequent iteration queriesCommitmentTxids Contains(currentTxid)wherecurrentTxidis now an ark txid, not a commitment txid. Unless preconfirmed descendants carry the parent's ark tx id in theirCommitmentTxidsfield (which I don't see indecodeTx), the second+ iterations return nothing. Behavior appears to happen to match the flat postgres query only because both end up returning the direct vtxos whosecommitment_txidmatches. Pre-existing shape, not this PR's fault, but worth a look — if the walk really is a no-op past depth 0 the code should be simplified to a single query. -
collectPreconfirmedVtxosswallows repo errors and returns a partial result (utils.go:127, 137). On a DB error mid-walk the sweep event will list fewer preconfirmed vtxos than were actually swept, then be persisted as the canonical event. Preserving the old logging behavior is fine, but consider propagating the error or aborting the sweep event save so consumers don't get a silently-truncatedSweptVtxos. This is the event that downstream indexers/wallets react to.
Cross-repo
VtxoRepository is internal to arkd; the rename is not visible to SDKs. Grepped ts-sdk / go-sdk / rust-sdk / dotnet-sdk — no references. The wire SweepTx event schema is unchanged; only its contents get de-duplicated, which is what SDKs already assumed.
Test coverage
- e2e assertion for uniqueness added (
internal/test/e2e/e2e_test.go:3425-3432). Good, this is exactly the invariant being fixed. service_test.gocoversGetDescendantVtxosfor a happy-path chain and for the end-of-chain (empty) case. Missing: a fan-out case (one seed with two ark descendants at the same depth) and the cycle guard (which is the fragile bit of the sqlite CTE).round_test.gostill only exercises single-shot Sweep; the multi-sweep/r.Changes-empty case discussed in (3) is not covered.- No unit test for
leafVtxoOutpointsitself despite it now driving the sweep event content.
Please treat (1), (3), and (6) as blocking for the human protocol reviewer.
|
This PR has been open for 3+ days without review. @Dunsin-cyber is anyone looking at this? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — fix: duplicated swept vtxos in sweep tx event
Correct fix for two related bugs.
Bug 1 — single vtxo per leaf: extractVtxoOutpoint returned only the first non-anchor output. Leaf txs that pay multiple receivers (shared leaf) would only sweep one vtxo. The new leafVtxoOutpoints collects every vtxo outpoint from the leaf, skipping anchors and extension outputs. This correctly handles multi-receiver leaves.
Bug 2 — duplicated preconfirmed vtxos: The non-root sweep path used GetAllChildrenVtxos per leaf vtxo and de-duplicated with a seen map. If two leaf vtxos shared a descendant (possible with chains), that descendant appeared multiple times in preconfirmedVtxos, creating duplicate sweep events. The new collectPreconfirmedVtxos helper uses GetDescendantVtxos which deduplicates at the query level.
Code quality: Extracting collectPreconfirmedVtxos removes duplicated logic between admin.go and sweeper.go, which previously had to be kept in sync. Good refactor.
Tests: The interface additions (GetDescendantVtxos, renamed GetSweepablePreconfirmedVtxosByCommitmentTxid) are correctly wired into the mock. No new unit tests for the multi-receiver case — worth adding one to pin the regression if time allows, but not blocking.
Ready to merge.
|
This PR has been open for 30+ days without review. @Dunsin-cyber is anyone looking at this? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — fix: duplicated swept vtxos in sweep tx event
What this does: Fixes a bug where extractVtxoOutpoint was called per leaf tx and returned only the first non-anchor output VTXO. Leaf txs can pay multiple receivers, so subsequent VTXOs in the same leaf were silently dropped. This caused the sweep event to report incomplete vtxo sets, meaning some swept VTXOs might not be marked appropriately.
The fix:
leafVtxoOutpointsreplacesextractVtxoOutpointand collects all VTXO outpoints from a leaf tx. This is the right fix.collectPreconfirmedVtxosis extracted as a shared helper used by bothsaveBatchSweptEvents(admin) and the sweeper, eliminating duplicate logic. Good refactor.- The interface rename
GetSweepableVtxosByCommitmentTxid→GetSweepablePreconfirmedVtxosByCommitmentTxidis a clarity improvement. - New
GetDescendantVtxoson the vtxo repo port is added to mock/test stubs — confirm the production implementation handles depth correctly.
Notes:
- No explicit unit test for
leafVtxoOutpointsitself, but the existing sweep tests cover the path. Worth a targeted test if multi-receiver leaf txs are common. - The change is on the event-reporting side; verify it doesn't affect the actual sweep transaction construction (which appeared to already handle multiple outputs).
Verdict: Straightforward correctness fix. Looks ready to merge.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review: fix duplicated swept VTXOs in sweep tx event
Verdict: looks ready to merge.
What the PR does
extractVtxoOutpoint returned only the first VTXO from a leaf tx. Since leaf txs can pay multiple receivers, sweep events were missing VTXOs at indices > 0. The fix replaces it with leafVtxoOutpoints, which iterates all outputs and collects every VTXO outpoint (skipping anchor and extension outputs). Applied consistently in both sweeper.go and admin.go.
collectPreconfirmedVtxos is a clean extraction of the duplicated pre-confirmed-VTXO collection logic. The port rename GetSweepableVtxosByCommitmentTxid → GetSweepablePreconfirmedVtxosByCommitmentTxid is more accurate. The new GetDescendantVtxos port method is reflected in mock stubs.
Minor questions
leafVtxoOutpointssilently skips outputs it can't parse as a VTXO script. Does it log a warning when a non-anchor, non-extension output fails to parse? Silent skipping is safe but could hide a future parsing bug.- The
GetDescendantVtxosmock returns nil — is this method used in the sweep path covered by existing tests, or is it a forward stub for a follow-up?
Core fix is correct and the refactor is a net improvement. Ready to merge.
|
This PR has been open for 42+ days without a human review. @Dunsin-cyber is anyone looking at this? Arkana reviewed it (looks ready to merge). |
|
This PR has been open for 5+ days without a review. @Dunsin-cyber is anyone looking at this? (fix duplicated swept vtxos in sweep tx event; 5 days without review.) |
|
This PR has been open for 48 days without a review. @Dunsin-cyber is there anything blocking this from getting a look? |
|
This PR has been open for 51+ days without review. @Dunsin-cyber is anyone looking at this? (fix: duplicated swept vtxos in sweep tx event) |
|
This PR has been open for 52+ days without a review decision. @Dunsin-cyber is anyone looking at this? |
|
This PR has been open for 52+ days without a review. @Dunsin-cyber — is anyone looking at this? |
|
This PR has been open for 54 days without review. @Dunsin-cyber is anyone looking at this? |
|
This PR has been open 55 days without review. @Dunsin-cyber is anyone looking at this? |
2 participants
fixes #1138
the sweep event listed the same vtxo twice because the queries used to collect preconfirmed vtxos also returned the leaves.
Fix: make the queries return what their names say, instead of filtering at call sites:
GetAllChildrenVtxosbut depth > 0, seed excluded.This covers both event producers (sweeper + admin sweep) with no filter code to forget.
Also needed: leaf collection now takes every vtxo output of a leaf tx (not just vout 0), and
FullySweptcounts distinct leaf txids, otherwise multi-receiver leaves would be dropped from the event / break the accounting.Summary by CodeRabbit