allow unrolled VTXOs to join batches with configurable expiry margin

[bitcoin-coder-bob]

closes #703

Allow unrolled VTXOs to rejoin a batch via the collaborative (boarding) path instead of rejecting them outright. When an unrolled VTXO still has sufficient CSV time remaining before its exit path becomes available it is treated as a boarding input so it can be swept back into the Ark without requiring unilateral exit.

Why do we want this? An unrolled VTXO is stuck on-chain then the user's funds are out of the Ark and the only way back in was to wait for the CSV to expire, then sweep the funds into a new boarding UTXO. That's slow and costs an extra transaction. With this change, the user can immediately rejoin the Ark by presenting the unrolled VTXO as a boarding input in the next batch, skipping the CSV wait and the extra on-chain step. Funds go from on-chain back to off-chain in a single round.

Changes

• RegisterIntent: Instead of returning VTXO_ALREADY_UNROLLED, unrolled VTXOs are now appended as boarding inputs with isUnrolledVtxo=true and tracked in vtxoInputs for state settlement.
• Boarding validation: processBoardingInputs and validateBoardingInput use the unilateral exit delay (not boarding exit delay) when verifying unrolled VTXO scripts and expiry. Reason: Because the VTXO's script was originally built with the unilateral exit delay when it was created inside the Ark's VTXO tree. Unrolling just reveals that output on-chain, it doesn't rewrite the script. The CSV lock embedded in the tapscript is still the unilateral exit delay. The boarding exit delay is a separate value used for UTXOs that users create outside the Ark (boarding UTXOs). Those are constructed by the user's wallet with the boarding-specific delay. So when Validate() checks the script at line 3864, it needs to match what's actually in the tapscript. If we passed boardingExitDelay, validation would fail because the on-chain script contains unilateralExitDelay.
• Expiry safety check: New checkUnrolledVtxoExpiry ensures the VTXO's CSV doesn't expire before the batch can finalize. Margin defaults to session duration but is configurable via UNROLLED_VTXO_MIN_EXPIRY_MARGIN.
• Fee computation: Unrolled VTXOs are excluded from the VTXO fee computation since they are already counted as onchain/boarding inputs. Reason: The fee manager applies different fee programs to onchain vs offchain inputs (line 36-39). Unrolled VTXOs are already added to boardingInputs (and therefore included in the onchainInputs list passed to ComputeIntentFees via boardingInput.witnessUtxo). If we didn't filter them out of vtxoInputs, they'd be double-counted — charged the onchain input fee and the offchain input fee.
• Forfeit skip: RequiresForfeit() returns false for unrolled VTXOs since they have no off-chain state to forfeit.
• Config: New UNROLLED_VTXO_MIN_EXPIRY_MARGIN env var (seconds, default 0 = fallback to session duration). It is the minimum remaining CSV time an unrolled VTXO must have before the server accepts it into a batch. If the VTXO's exit path becomes available too soon, the batch might not finalize in time then the user could unilaterally exit mid-round, taking their funds while also being included in the new VTXO tree. When set to 0 (default), it falls back to sessionDuration which is the time a single round takes to complete. A custom value lets operators require a larger buffer if needed.
• Tests: Unit tests for checkUnrolledVtxoExpiry and an e2e test covering the full unrolled-VTXO-rejoins-batch flow.

Summary by CodeRabbit

• New Features

  • Unrolled VTXOs can re-enter collaborative settlement batches.
  • Configurable minimum expiry margin for rejoining VTXOs (UNROLLED_VTXO_MIN_EXPIRY_MARGIN).

• Bug Fixes

  • Exclude rejoining VTXOs from intent fee calculations.
  • Stronger expiry checks to prevent VTXOs expiring too soon during settlement.
  • Boarding processing selects correct exit-delay for rejoining VTXOs.
  • Recoverable listings exclude unrolled VTXOs.

• Tests

  • Unit tests for expiry-margin logic and an end-to-end test for unrolled VTXO rejoin.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: enabling unrolled VTXOs to participate in batches with a configurable expiry margin.
Linked Issues check	✅ Passed	The PR addresses the primary objective from #703: allowing unrolled VTXOs to join batches collaboratively when their unilateral CSV remains sufficiently far.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to enabling unrolled VTXO batch re-entry: config/env variables, validation logic, fee computation fixes, forfeit behavior updates, and corresponding tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bob/unrolled-vtxo-allow-join-batch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[arkanaai]

[Arkana Review — PR #961: allow unrolled VTXOs to join batches with configurable expiry margin]

Reviewed at: 2026-03-14 14:00 UTC

---

Summary

This PR re-opens the collaborative path for unrolled VTXOs: instead of hard-rejecting them at RegisterIntent, they are treated as boarding inputs validated with the unilateral exit delay (not boarding exit delay) and subject to a configurable CSV expiry margin.

Overall the approach is correct. A few things worth discussing before merge:

---

What looks good

Exit delay selection (processBoardingInputs / validateBoardingInput): Correctly uses unilateralExitDelay for unrolled VTXOs and boardingExitDelay for regular boarding. The expectedExitDelay branching is clean.

Fee double-counting fix: Filtering unrolled VTXOs out of feeVtxoInputs is correct — they are already counted as onchain/boarding inputs so including them in the VTXO fee path would overstate the fee.

IsEligibleForSweep fix: The !v.Unrolled guard is necessary — without it the sweeper could claim an unrolled VTXO that is waiting to rejoin, which would be a loss-of-funds scenario. Good catch.

checkUnrolledVtxoExpiry: Using sessionDuration as the fallback margin is correct — the VTXO must have at least one full session duration of CSV time remaining to give the batch time to finalize.

E2E test (TestUnrolledVtxoRejoinBatch): Good coverage of the full lifecycle. Unit tests for checkUnrolledVtxoExpiry with explicit edge cases are a nice addition.

---

Questions / potential issues

1. Double-entry in vtxoInputs + boardingUtxos

The unrolled VTXO is appended to both boardingUtxos and vtxoInputs in RegisterIntent. The comment says it is for state tracking (settled after batch finalization). Two questions:

• Does the batch finalization path handle a VTXO that appears in vtxoInputs but has Unrolled == true gracefully? The standard VTXO input path may try to validate forfeit txs or connector chain linkage — does it skip that for unrolled inputs?
• Is there a guard preventing the same outpoint from being submitted in both the VTXO input list and the boarding list by a crafty client (not via this code path but via the raw API)?

2. Script validation / tapscript ownership

In validateBoardingInput, the script is validated against s.signerPubkey and expectedExitDelay. This ensures the operator key is embedded. However, is there a check that the tapscripts provided by the client in the PSBT actually match the on-chain script of the unrolled outpoint? If a client can substitute tapscripts at re-entry time, they could present a valid-looking script that does not correspond to the actual UTXO. Arkd normally resolves this by looking up the outpoint from the indexer and comparing — confirming that path is exercised here too.

3. defaultUnrolledVtxoMinExpiryMargin = 0 semantics

A margin of 0 means "fall back to sessionDuration". The env var comment says the same. Worth documenting this explicitly, since operators who want to disable the margin check entirely have no way to do so — they would need to set an extremely small value. Is 0 really "disabled" or "use session duration"? This is fine behavior, just worth a clear config doc note.

4. isUnrolledVtxo flag origin

The isUnrolledVtxo field is set based on vtxo.Unrolled from the store lookup earlier in RegisterIntent, then carried on the boardingIntentInput struct. So this is correct for the normal path. Just confirming: is there any path where a non-unrolled VTXO could arrive at processBoardingInputs with isUnrolledVtxo = true?

---

Nits

• Config comment says 0 means fallback to sessionDuration — "fall back" (two words) for consistency with Go stdlib phrasing.
• The E2E test uses offchainAddr.Tapscripts for the boarding UTXO — this assumes the offchain address tapscripts match the unrolled VTXO. Worth adding an assertion or a comment explaining why this is expected to hold.

---

Verdict

Concept is correct and the feature is valuable — restoring collaborative re-entry for unrolled VTXOs without weakening the exit guarantee is the right approach. Questions 1 and 2 above should be addressed before merge.

— Arkana

[bitcoin-coder-bob]

[Arkana Review — PR #961: allow unrolled VTXOs to join batches with configurable expiry margin]

Reviewed at: 2026-03-14 14:00 UTC

Summary

This PR re-opens the collaborative path for unrolled VTXOs: instead of hard-rejecting them at RegisterIntent, they are treated as boarding inputs validated with the unilateral exit delay (not boarding exit delay) and subject to a configurable CSV expiry margin.

Overall the approach is correct. A few things worth discussing before merge:

What looks good

Exit delay selection (processBoardingInputs / validateBoardingInput): Correctly uses unilateralExitDelay for unrolled VTXOs and boardingExitDelay for regular boarding. The expectedExitDelay branching is clean.

Fee double-counting fix: Filtering unrolled VTXOs out of feeVtxoInputs is correct — they are already counted as onchain/boarding inputs so including them in the VTXO fee path would overstate the fee.

IsEligibleForSweep fix: The !v.Unrolled guard is necessary — without it the sweeper could claim an unrolled VTXO that is waiting to rejoin, which would be a loss-of-funds scenario. Good catch.

checkUnrolledVtxoExpiry: Using sessionDuration as the fallback margin is correct — the VTXO must have at least one full session duration of CSV time remaining to give the batch time to finalize.

E2E test (TestUnrolledVtxoRejoinBatch): Good coverage of the full lifecycle. Unit tests for checkUnrolledVtxoExpiry with explicit edge cases are a nice addition.

Questions / potential issues

1. Double-entry in vtxoInputs + boardingUtxos

The unrolled VTXO is appended to both boardingUtxos and vtxoInputs in RegisterIntent. The comment says it is for state tracking (settled after batch finalization). Two questions:

• Does the batch finalization path handle a VTXO that appears in vtxoInputs but has Unrolled == true gracefully? The standard VTXO input path may try to validate forfeit txs or connector chain linkage — does it skip that for unrolled inputs?
• Is there a guard preventing the same outpoint from being submitted in both the VTXO input list and the boarding list by a crafty client (not via this code path but via the raw API)?

2. Script validation / tapscript ownership

In validateBoardingInput, the script is validated against s.signerPubkey and expectedExitDelay. This ensures the operator key is embedded. However, is there a check that the tapscripts provided by the client in the PSBT actually match the on-chain script of the unrolled outpoint? If a client can substitute tapscripts at re-entry time, they could present a valid-looking script that does not correspond to the actual UTXO. Arkd normally resolves this by looking up the outpoint from the indexer and comparing — confirming that path is exercised here too.

3. defaultUnrolledVtxoMinExpiryMargin = 0 semantics

A margin of 0 means "fall back to sessionDuration". The env var comment says the same. Worth documenting this explicitly, since operators who want to disable the margin check entirely have no way to do so — they would need to set an extremely small value. Is 0 really "disabled" or "use session duration"? This is fine behavior, just worth a clear config doc note.

4. isUnrolledVtxo flag origin

The isUnrolledVtxo field is set based on vtxo.Unrolled from the store lookup earlier in RegisterIntent, then carried on the boardingIntentInput struct. So this is correct for the normal path. Just confirming: is there any path where a non-unrolled VTXO could arrive at processBoardingInputs with isUnrolledVtxo = true?

Nits

• Config comment says 0 means fallback to sessionDuration — "fall back" (two words) for consistency with Go stdlib phrasing.
• The E2E test uses offchainAddr.Tapscripts for the boarding UTXO — this assumes the offchain address tapscripts match the unrolled VTXO. Worth adding an assertion or a comment explaining why this is expected to hold.

Verdict

Concept is correct and the feature is valuable — restoring collaborative re-entry for unrolled VTXOs without weakening the exit guarantee is the right approach. Questions 1 and 2 above should be addressed before merge.

— Arkana

response to questions 1 and 2:

Q1: Does batch finalization handle unrolled VTXOs in vtxoInputs gracefully?

Yes. The key guard is domain.Vtxo.RequiresForfeit() (internal/core/domain/vtxo.go):

  func (v Vtxo) RequiresForfeit() bool {                                                                                                                                                                   
      return !v.Swept && !v.IsNote() && !v.Unrolled                   
  }                    

This returns false for unrolled VTXOs, so the forfeit tx creation and connector chain linkage paths are never entered for them. On the commitment tx side, unrolled VTXOs are signed as boarding inputs
(via isUnrolledVtxo flag on boardingIntentInput), not through the connector tree.

Additionally, fee computation explicitly filters them out of vtxoInputs to avoid double-counting since they're already counted as boarding inputs:

  for _, v := range vtxoInputs {                                                                                                                                                                           
      if !v.Unrolled {                                                
          feeVtxoInputs = append(feeVtxoInputs, v)
      }                                                                                                                                                                                                    
  }

Q2: Is there a guard against a crafty client submitting the same outpoint in both lists via the raw API?

Partially. There's a seenOutpoints map in RegisterIntent (around line 1570) that prevents the same outpoint from appearing more than once in the proof transaction inputs:

  if _, seen := seenOutpoints[outpoint]; seen {                                                                                                                                                            
      return "", errors.INVALID_INTENT_PROOF.New(                                                                                                                                                          
          "duplicated input %s", outpoint.String(),                                                                                                                                                        
      )                                                                                                                                                                                                    
  }                                                                                                                                                                                                        

However, this only deduplicates within the single proof tx input list. The code path that decides whether an input is a VTXO vs. boarding input is based on a DB lookup (Unrolled flag on the stored
vtxo), not on what the client claims. A regular VTXO won't have Unrolled == true in the DB, so it won't be added to boardingUtxos. And a real boarding UTXO won't exist in the vtxo store at all, so it
can't enter vtxoInputs.

In short: the DB state is the authority, not the client's input classification, so a crafty client can't force an outpoint into both paths. The dual-entry for unrolled VTXOs is an intentional
server-side decision based on the stored Unrolled flag.

[arkanaai]

🔍 Arkana PR Review — arkd#961

Summary

Allows unrolled VTXOs to rejoin the Ark collaboratively by treating them as boarding inputs with the unilateral exit delay. Adds a configurable UNROLLED_VTXO_MIN_EXPIRY_MARGIN to gate how close to CSV expiry an unrolled VTXO can be when re-entering a batch. Clean feature with good test coverage (unit + e2e).

Protocol Correctness ✅

1. Boarding path for unrolled VTXOs — Correct choice. Unrolled VTXOs are on-chain UTXOs, so treating them as boarding inputs with unilateralExitDelay (not boardingExitDelay) is the right validation path. The exit delay types match the original VTXO tree construction.

2. RequiresForfeit() returns false for unrolled — Correct. An unrolled VTXO has already been broadcast on-chain, so there is no off-chain state to forfeit. The operator does not need a forfeit transaction to prevent double-spending — the UTXO is already publicly committed.

3. Expiry margin check (checkUnrolledVtxoExpiry) — Sound design. Fallback to sessionDuration when margin=0 ensures there is always enough time for the batch to finalize. Custom margin overrides cleanly.

4. Fee computation — Correctly filters unrolled VTXOs out of feeVtxoInputs since they are already counted as on-chain boarding inputs. No double-counting.

Security Review

⚠️ Dual tracking — verify no double-spend vector:
The unrolled VTXO is appended to both boardingUtxos and vtxoInputs (lines ~1690-1698). boardingUtxos handles on-chain validation/signing; vtxoInputs handles state tracking (marking as settled after finalization).

Please verify that downstream batch finalization does not attempt to spend the same outpoint twice — once as a boarding input and once as a VTXO input. If both lists feed into the same PSBT builder independently, this could produce an invalid transaction or a silent conflict. The fee filtering suggests awareness of this, but the actual PSBT construction path should be checked.

✅ CSV expiry validation — The csvExpiresAt.Before(now) check runs before the margin check, so an already-expired unrolled VTXO is rejected before the margin logic even fires. Good layering.

✅ Exit delay distinction — Using s.unilateralExitDelay for unrolled VTXOs vs s.boardingExitDelay for normal boarding is correct and prevents accepting an unrolled VTXO with a mismatched timelock.

Nits

1. Boundary condition in checkUnrolledVtxoExpiry — csvExpiresAt.Before(now.Add(margin)) means a VTXO expiring exactly at now + margin passes (since Before is strict <). The test confirms this is intentional. Just flagging that the boundary is "margin is exclusive" — operators should set margin with this in mind.

2. Config documentation — Consider documenting the interaction between SETTLEMENT_MIN_EXPIRY_GAP and UNROLLED_VTXO_MIN_EXPIRY_MARGIN — they serve related but distinct purposes and an operator might confuse them.

Cross-Repo Impact

• SDK impact: Client SDKs need to know how to construct the boarding input from an unrolled VTXO (the e2e test shows the pattern via Settle(WithFunds(...))). If this merges, SDK docs/examples should reflect the new flow.
• No gRPC API surface change — this is server-side validation logic only.

Verdict

Solid feature. The dual-tracking concern is the main thing to verify before merge — otherwise LGTM.

[bitcoin-coder-bob]

@arakanaai regarding your double spend concerns: dual tracking is by design and there's no double-spend vector. The same outpoint enters the PSBT exactly once (as a boarding input). The vtxoInputs list never feeds into transaction construction, only into state management.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

internal/test/e2e/e2e_test.go (2)

423-439: Use a timeout-scoped context for NotifyIncomingFunds to reduce flakiness.

If no event arrives, this goroutine can block until the test context is canceled. A short context.WithTimeout here would make failures deterministic in CI.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/test/e2e/e2e_test.go` around lines 423 - 439, The
NotifyIncomingFunds goroutine can block indefinitely; wrap the test context with
a short timeout and pass that into alice.NotifyIncomingFunds to fail fast—create
a timeout context (e.g., ctxTimeout, cancel := context.WithTimeout(ctx,
shortDuration)) before launching the goroutine, defer cancel, use ctxTimeout
when calling alice.NotifyIncomingFunds (instead of ctx), and ensure wg.Done
still runs and incomingErr receives the returned error so the test
deterministically fails when no event arrives.

---

414-420: Add a negative-case assertion for wrong tapscript binding.

This new test covers the happy path. Consider adding a companion case that passes an intentionally wrong tapscript and expects rejection, so script/outpoint binding is explicitly protected in test coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/test/e2e/e2e_test.go` around lines 414 - 420, Add a negative test
that verifies binding between tapscript and outpoint is enforced: after the
existing happy-path where Receive unrolls VTXO and constructs boardingUtxo
(types.Utxo with Outpoint unrolledVtxo.Outpoint and Tapscripts
offchainAddr.Tapscripts), create a companion case that substitutes a
wrong/foreign tapscript (e.g., modify offchainAddr.Tapscripts or build a
different tapscript array) and call Receive (or the same code path that consumes
boardingUtxo); assert that the call is rejected/fails (error returned or
explicit validation failure) to ensure script/outpoint binding is checked.
Reference the existing symbols: Receive, boardingUtxo, unrolledVtxo,
offchainAddr.Tapscripts, and types.Utxo when adding the negative assertion.

🤖 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/config/config.go`:
- Line 428: The assignment of UnrolledVtxoMinExpiryMargin currently takes
viper.GetInt64(UnrolledVtxoMinExpiryMargin) without bounds checking; add a
validation step where you read the value into a local (e.g., val :=
viper.GetInt64(UnrolledVtxoMinExpiryMargin)), verify val >= 0, and if not return
a configuration error (or fail fast) with a clear message referencing
UnrolledVtxoMinExpiryMargin; apply the same non-negative validation to the other
occurrence of UnrolledVtxoMinExpiryMargin in the file so negative values are
rejected rather than accepted silently.

In `@internal/core/domain/vtxo.go`:
- Line 70: The recoverable VTXO filter that currently uses the negation of
RequiresForfeit() is too broad after RequiresForfeit() changed; update the
recoverable condition (the filter in indexer.go that checks
!vtxo.RequiresForfeit()) to also check the Unrolled flag by adding &&
!vtxo.Unrolled so unrolled outputs are excluded from recoverable VTXOs, keeping
semantics aligned with RequiresForfeit() in internal/core/domain/vtxo.go and the
client types.

---

Nitpick comments:
In `@internal/test/e2e/e2e_test.go`:
- Around line 423-439: The NotifyIncomingFunds goroutine can block indefinitely;
wrap the test context with a short timeout and pass that into
alice.NotifyIncomingFunds to fail fast—create a timeout context (e.g.,
ctxTimeout, cancel := context.WithTimeout(ctx, shortDuration)) before launching
the goroutine, defer cancel, use ctxTimeout when calling
alice.NotifyIncomingFunds (instead of ctx), and ensure wg.Done still runs and
incomingErr receives the returned error so the test deterministically fails when
no event arrives.
- Around line 414-420: Add a negative test that verifies binding between
tapscript and outpoint is enforced: after the existing happy-path where Receive
unrolls VTXO and constructs boardingUtxo (types.Utxo with Outpoint
unrolledVtxo.Outpoint and Tapscripts offchainAddr.Tapscripts), create a
companion case that substitutes a wrong/foreign tapscript (e.g., modify
offchainAddr.Tapscripts or build a different tapscript array) and call Receive
(or the same code path that consumes boardingUtxo); assert that the call is
rejected/fails (error returned or explicit validation failure) to ensure
script/outpoint binding is checked. Reference the existing symbols: Receive,
boardingUtxo, unrolledVtxo, offchainAddr.Tapscripts, and types.Utxo when adding
the negative assertion.

🪄 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: 78c5a505-410c-4b8c-8d98-6ed1a7f44a58

📥 Commits

Reviewing files that changed from the base of the PR and between 4b4ff48 and d1b121d.

📒 Files selected for processing (6)

• internal/config/config.go
• internal/core/application/service.go
• internal/core/application/service_test.go
• internal/core/application/types.go
• internal/core/domain/vtxo.go
• internal/test/e2e/e2e_test.go

[arkanaai]

Followup review — new commits since last review: d1b121d9 (master merge) + 9999fee5 (exclude unrolled VTXOs from recoverable filter)

indexer.go — recoverable filter fix ✅

if !vtxo.RequiresForfeit() && !vtxo.Spent && !vtxo.Unrolled {

Good catch. Without this, unrolled VTXOs would appear in the recoverable list (since RequiresForfeit() now returns false for them), which would let the operator attempt recovery sweeps on outputs the user may be actively re-boarding. The !vtxo.Unrolled guard is the correct fix — unrolled VTXOs are either being re-boarded or waiting for CSV expiry, neither of which should trigger recovery.

Note: RequiresForfeit() already returns false for unrolled VTXOs (from the earlier commit), so the !vtxo.Unrolled check here is technically redundant with the forfeit guard for the current recoverable filter logic. But it's the right defensive addition — if the recoverable filter criteria ever change to not depend on RequiresForfeit(), this explicit exclusion keeps unrolled VTXOs out regardless.

No new concerns. Previous review findings (negative margin validation) still apply.

[arkanaai]

This PR has been open for 7+ days without review. @bitcoin-coder-bob is anyone looking at this?

[coderabbitai]

Actionable comments posted: 3

🤖 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/core/application/service.go`:
- Around line 3936-3944: The checkUnrolledVtxoExpiry function currently falls
back to s.sessionDuration when s.unrolledVtxoMinExpiryMargin == 0, but
startRound can switch the effective round duration to scheduledSession.Duration;
update the fallback so the margin is based on the actual active round duration
used by startRound (i.e., if a scheduledSession is set use
scheduledSession.Duration, otherwise use s.sessionDuration or the same variable
startRound uses for the active round), keeping the primary symbol references
s.unrolledVtxoMinExpiryMargin, s.sessionDuration, scheduledSession.Duration and
the checkUnrolledVtxoExpiry function to locate and change the margin
calculation.
- Around line 1958-1968: EstimateIntentFee classifies DB-backed (unrolled) VTXOs
differently than RegisterIntent, causing mismatched fee quotes; update
EstimateIntentFee to mirror RegisterIntent's logic by filtering vtxoInputs the
same way (use feeVtxoInputs / check v.Unrolled) and pass the same onchainInputs
and feeVtxoInputs into s.feeManager.ComputeIntentFees so DB-backed unrolled
VTXOs are treated as onchain inputs rather than offchainOutputs; ensure the same
symbols (vtxoInputs, feeVtxoInputs, v.Unrolled, onchainInputs, offchainOutputs,
ComputeIntentFees, EstimateIntentFee, RegisterIntent) are used to locate and
align the classification logic.
- Around line 1604-1617: The current early "continue" in the loop that appends
an unrolled VTXO to boardingUtxos and vtxoInputs skips the normal asset
bookkeeping and leaves assetInputs incomplete; update the handling in the loop
(where boardingIntentInput is created and vtxoInputs appended) to also record
any asset metadata into assetInputs the same way the normal asset-input path
does (use the same mapping logic applied later for psbtInput/vtxo), or remove
the premature continue and fall through to the existing asset-input code path so
that validateBoardingInput (called later in processBoardingInputs) sees a
complete assetInputs map for isUnrolledVtxo entries; ensure you reference
boardingUtxos, vtxoInputs, assetInputs, boardingIntentInput, psbtInput, vtxo and
isUnrolledVtxo when making the change.

🪄 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: 2c83a29e-1a7c-4003-9ed0-379688a80c16

📥 Commits

Reviewing files that changed from the base of the PR and between 9999fee and eb9bad4.

📒 Files selected for processing (7)

• internal/config/config.go
• internal/core/application/indexer.go
• internal/core/application/service.go
• internal/core/application/types.go
• internal/core/domain/vtxo.go
• internal/core/domain/vtxo_test.go
• internal/test/e2e/e2e_test.go

✅ Files skipped from review due to trivial changes (2)

• internal/core/application/indexer.go
• internal/core/application/types.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/core/domain/vtxo.go
• internal/config/config.go
• internal/test/e2e/e2e_test.go

[coderabbitai]

🧹 Nitpick comments (1)

internal/core/application/service.go (1)

3949-3973: LGTM with a documentation suggestion.

The margin calculation correctly addresses the scheduled-session concern by using the larger of sessionDuration and scheduledSession.Duration when the explicit margin is not configured (zero). This ensures unrolled VTXOs have sufficient time before CSV expiry regardless of which session type is active.

One minor clarification for maintainability: the naming unrolledVtxoMinExpiryMargin suggests a floor/minimum, but when set to a positive value it acts as a direct override rather than a minimum. Consider adding a brief comment in the config documentation clarifying that a non-zero value replaces (not supplements) the session-duration fallback.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/application/service.go` around lines 3949 - 3973, The comment
requests clarifying that unrolledVtxoMinExpiryMargin acts as an override rather
than a minimum: update the documentation/config comment for the
unrolledVtxoMinExpiryMargin setting (and add a short inline comment near
service.checkUnrolledVtxoExpiry or where unrolledVtxoMinExpiryMargin is defined)
to state that when unrolledVtxoMinExpiryMargin > 0 it replaces the
session-duration-based margin (sessionDuration / scheduledSession.Duration)
instead of being applied as a floor or additive value; reference the variable
name unrolledVtxoMinExpiryMargin and the checkUnrolledVtxoExpiry function so
maintainers can find and read the clarified comment.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/core/application/service.go`:
- Around line 3949-3973: The comment requests clarifying that
unrolledVtxoMinExpiryMargin acts as an override rather than a minimum: update
the documentation/config comment for the unrolledVtxoMinExpiryMargin setting
(and add a short inline comment near service.checkUnrolledVtxoExpiry or where
unrolledVtxoMinExpiryMargin is defined) to state that when
unrolledVtxoMinExpiryMargin > 0 it replaces the session-duration-based margin
(sessionDuration / scheduledSession.Duration) instead of being applied as a
floor or additive value; reference the variable name unrolledVtxoMinExpiryMargin
and the checkUnrolledVtxoExpiry function so maintainers can find and read the
clarified comment.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2743ebc5-caac-42ab-8d89-686e6f2b1740

📥 Commits

Reviewing files that changed from the base of the PR and between eb9bad4 and 90fa39c.

⛔ Files ignored due to path filters (1)

• api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.go is excluded by !**/gen/**

📒 Files selected for processing (2)

• internal/core/application/service.go
• internal/core/application/service_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/core/application/service_test.go

[arkanaai]

🔍 Arkana PR Review — arkd#961 (follow-up, new commits since 2026-03-14)

Reviewed at: 2026-04-23
Focus: Changes since last review (commits from 2026-03-18 through 2026-04-23)

---

Changes since last review

Seven substantive commits landed since my March 14 review:

1. correct unrolled VTXO handling in fee estimation, asset tracking, and expiry margin — Adds unrolled VTXO handling to EstimateIntentFee and asset input tracking.
2. simplify checkUnrolledVtxoExpiry and enforce margin >= sessionDuration — Eliminates the 0 = fallback ambiguity by requiring margin > 0 && margin >= sessionDuration in config validation.
3. exclude unrolled VTXOs from recoverable filter — Adds !vtxo.Unrolled guard in GetVtxos recoverable filtering.
4. assert CompleteUnroll finds no funds after unrolled VTXO rejoins batch — E2E test now verifies the on-chain UTXO is consumed after batch re-entry.
5. Update envs & Add CompleteUnroll to all unilateral exit integration tests — Broader test coverage + env config for dev/light/docker-compose.
6. Merge commits + lint fixes.

---

Resolution of previous concerns

All four questions from my March 14 review are now addressed:

Q1 (double-entry in vtxoInputs + boardingUtxos): ✅ Resolved. RequiresForfeit() now returns false for unrolled VTXOs (!v.Unrolled guard at vtxo.go:79). This means ForfeitTxsStore.Init() correctly skips them, EndFinalization() won't demand forfeit txs for them, and CountSpentVtxos() excludes them. The dual-list approach is safe.

Q2 (script validation / tapscript ownership): ✅ Confirmed safe. processBoardingInputs calls newBoardingInput which computes the taproot key from client-provided tapscripts and compares the derived P2TR script against the on-chain output (utils.go:209, bytes.Equal). Additionally, processBoardingInputs itself compares witness UTXO PkScript against the fetched on-chain prevout script. A client cannot substitute fake tapscripts.

Q3 (defaultUnrolledVtxoMinExpiryMargin = 0 semantics): ✅ Resolved completely. Default is now 300 (5 minutes). Config validation rejects <= 0 and enforces >= sessionDuration. No ambiguity remains.

Q4 (isUnrolledVtxo flag origin): ✅ Still correct. Only set from the DB lookup of vtxo.Unrolled in RegisterIntent. No path for a non-unrolled VTXO to arrive at processBoardingInputs with isUnrolledVtxo = true.

---

New changes review

EstimateIntentFee fix (service.go:2298-2306): Correctly mirrors RegisterIntent — unrolled VTXOs are counted as onchain inputs for fee purposes. Without this, fee estimates would be wrong for clients attempting to rejoin with unrolled VTXOs. ✅

checkUnrolledVtxoExpiry simplification (service.go:3974-3983): Clean. The function is now a straight comparison: csvExpiresAt.Before(now.Add(margin)). No fallback logic needed since config enforces margin >= sessionDuration. The boundary test correctly validates that csvExpiresAt == now + margin is accepted (not expired). ✅

Config validation (config.go:497-507): Solid. Two checks: margin > 0 and margin >= sessionDuration. Error messages include both values for debuggability. ✅

Recoverable filter (indexer.go:298): Correct — unrolled VTXOs should not appear in the recoverable list since they're on-chain awaiting CSV expiry or batch re-entry. ✅

E2E test improvements: TestUnrolledVtxoRejoinBatch now asserts CompleteUnroll returns "no mature funds available" after the VTXO rejoins a batch — confirming the on-chain UTXO is consumed. The require.Eventually polling pattern for indexing is better than fixed sleeps. CompleteUnroll assertions added to existing unilateral exit tests too. ✅

Env files (arkd.dev.env, arkd.light.env, docker-compose.regtest.yml): Margin set to 10 seconds in test environments — appropriate for fast regtest rounds where sessionDuration is also ~10s. ✅

---

One remaining nit

The api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.go changes (reordering trie.New arguments) appear to be auto-generated from a protobuf rebuild. Not harmful, but worth confirming these weren't accidentally included — they're unrelated to this PR's intent.

---

Verdict

Approved. All previous concerns addressed. New changes are correct and well-tested. The EstimateIntentFee fix and config hardening are meaningful improvements over the original submission.

⚠️ Protocol-critical: This PR modifies VTXO handling, forfeit logic, exit paths, and round lifecycle. Requires human sign-off before merge per protocol review policy.

— Arkana