feat(ark-client): narrow settle() to expired/recoverable VTXOs#227
feat(ark-client): narrow settle() to expired/recoverable VTXOs#227bonomat wants to merge 3 commits into
Conversation
Rename the existing full-renewal settle to settle_all, and introduce a new settle that only renews VTXOs in the server's recoverable bucket plus any confirmed/pre-confirmed VTXOs the client sees as expired. This keeps periodic renewals cheap by leaving healthy VTXOs and boarding outputs untouched.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe batch settlement API is refactored to provide two methods with distinct scopes. The original ChangesSettlement API refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Protocol-Critical Review — REQUEST CHANGES
This PR touches settlement logic (VTXO handling, round lifecycle). Requires human sign-off even if all issues are resolved.
BLOCKER: Silent semantic break for all existing settle() callers
Severity: Critical
The old settle() settled all VTXOs + boarding outputs. This PR renames it to settle_all() and introduces a new settle() that only settles recoverable() VTXOs (expired, swept, or sub-dust — see ark-core/src/server.rs:405).
The problem: every existing caller of .settle() silently gets different behavior without a compile error. The method signature is identical, but the semantics change from "settle everything" to "settle only recoverable VTXOs."
Affected callers within this repo (all unchanged by this PR):
ark-client-sample/src/main.rs:611— CLIsettlecommand now silently skips boarding outputse2e-tests/tests/e2e_arkade_script.rs:77,194e2e-tests/tests/e2e_two_party.rs:61,113,133e2e-tests/tests/e2e_concurrent_settlement.rs:67,74,80,141,148,154e2e-tests/tests/e2e_delegate.rs:41e2e-tests/tests/e2e_multisig_delegate.rs:52e2e-tests/tests/e2e_sub_dust.rs:34,64,86e2e-tests/tests/e2e_continue_pending_tx.rs:38e2e-tests/tests/e2e_continue_pending_tx_multi_input.rs:49e2e-tests/tests/e2e_key_discovery.rs:56e2e-tests/tests/e2e_send_onchain_vtxo_and_boarding_output.rs:59e2e-tests/tests/e2e_assets.rs:31,110e2e-tests/tests/boltz_submarine.rs:46e2e-tests/tests/fulmine_delegator_smoke.rs:52
Most of these tests fund a boarding address then call settle() to bring funds on-chain into the Ark. After this PR, settle() will return Ok(None) (no recoverable VTXOs exist at that point), and subsequent balance assertions will fail or time out.
Fix: Either:
- Update all existing callers to
settle_all(), OR - Use a different name for the new narrow method (e.g.,
settle_expired(),settle_recoverable()) and keepsettle()with its current semantics
Option 2 is strongly preferred — it avoids a semver-breaking rename and the new name is self-documenting.
Issue: New settle() discards expired confirmed/pre-confirmed VTXOs (doc mismatch)
Severity: Medium
File: ark-client/src/batch.rs:119-120 (new code)
The PR description says:
"a new settle that only renews VTXOs in the server's recoverable bucket plus any confirmed/pre-confirmed VTXOs the client sees as expired"
But the implementation only collects from vtxo_list.recoverable():
let outpoints: Vec<OutPoint> = vtxo_list.recoverable().map(|v| v.outpoint).collect();The recoverable() bucket in VtxoList::new() (ark-core/src/vtxo_list.rs:31) is populated by is_recoverable() which checks is_expired() || is_swept || amount < dust. However, the classification in VtxoList::new() is order-dependent: a VTXO that is expired but NOT spent/swept/sub-dust would hit the is_recoverable() check first and land in recoverable. So expired VTXOs should be covered... but the doc claim about "confirmed/pre-confirmed VTXOs the client sees as expired" suggests there might be an edge case where client and server disagree on expiry (as noted in server.rs:413-414), and such VTXOs would remain in confirmed/pre_confirmed instead of recoverable.
If client-server clock skew can cause VTXOs to be considered expired by the client but not the server, then recoverable() won't include them. Clarify whether this is intentional.
Issue: No tests for the new settle() method
Severity: Medium
No unit or e2e tests verify:
settle()correctly filters to only recoverable VTXOssettle()returnsOk(None)when no recoverable VTXOs existsettle()successfully renews expired VTXOssettle()leaves healthy VTXOs untouched
For a protocol-critical settlement path, test coverage is essential.
Note: Cross-repo impact is contained
The other SDKs (ts-sdk, go-sdk, dotnet-sdk) have their own settle() implementations and are not affected by this Rust-side rename. No cross-repo breakage.
Summary
The core idea is sound — narrowing periodic settlement to only expired/recoverable VTXOs saves batch fees and is a good optimization. But the execution has a critical flaw: reusing the settle() name silently changes behavior for all callers. Use a distinct name like settle_recoverable() and add tests.
There was a problem hiding this comment.
🚨 Protocol-Critical Review — REQUEST CHANGES
This PR touches settlement logic (VTXO handling, round lifecycle). Requires human sign-off even if all issues are resolved.
BLOCKER: Silent semantic break for all existing settle() callers
Severity: Critical
The old settle() settled all VTXOs + boarding outputs. This PR renames it to settle_all() and introduces a new settle() that only settles recoverable() VTXOs (expired, swept, or sub-dust — see ark-core/src/server.rs:405).
The problem: every existing caller of .settle() silently gets different behavior without a compile error. The method signature is identical, but the semantics change from "settle everything" to "settle only recoverable VTXOs."
Affected callers within this repo (all unchanged by this PR):
- `ark-client-sample/src/main.rs:611` — CLI `settle` command now silently skips boarding outputs
- `e2e-tests/tests/e2e_arkade_script.rs:77,194`
- `e2e-tests/tests/e2e_two_party.rs:61,113,133`
- `e2e-tests/tests/e2e_concurrent_settlement.rs:67,74,80,141,148,154`
- `e2e-tests/tests/e2e_delegate.rs:41`
- `e2e-tests/tests/e2e_multisig_delegate.rs:52`
- `e2e-tests/tests/e2e_sub_dust.rs:34,64,86`
- `e2e-tests/tests/e2e_continue_pending_tx.rs:38`
- `e2e-tests/tests/e2e_continue_pending_tx_multi_input.rs:49`
- `e2e-tests/tests/e2e_key_discovery.rs:56`
- `e2e-tests/tests/e2e_send_onchain_vtxo_and_boarding_output.rs:59`
- `e2e-tests/tests/e2e_assets.rs:31,110`
- `e2e-tests/tests/boltz_submarine.rs:46`
- `e2e-tests/tests/fulmine_delegator_smoke.rs:52`
Most of these tests fund a boarding address then call `settle()` to bring funds on-chain into the Ark. After this PR, `settle()` will return `Ok(None)` (no recoverable VTXOs exist at that point), and subsequent balance assertions will fail or time out.
Fix: Either:
- Update all existing callers to `settle_all()`, OR
- Use a different name for the new narrow method (e.g., `settle_expired()`, `settle_recoverable()`) and keep `settle()` with its current semantics
Option 2 is strongly preferred — it avoids a semver-breaking rename and the new name is self-documenting.
Issue: New settle() discards expired confirmed/pre-confirmed VTXOs (doc mismatch)
Severity: Medium
The PR description says:
"a new settle that only renews VTXOs in the server's recoverable bucket plus any confirmed/pre-confirmed VTXOs the client sees as expired"
But the implementation only collects from `vtxo_list.recoverable()`:
```rust
let outpoints: Vec = vtxo_list.recoverable().map(|v| v.outpoint).collect();
```
The `recoverable()` bucket in `VtxoList::new()` (`ark-core/src/vtxo_list.rs:31`) is populated by `is_recoverable()` which checks `is_expired() || is_swept || amount < dust`. The classification is order-dependent: a VTXO that is expired but NOT spent/swept will hit `is_recoverable()` first and land correctly in `recoverable`. However, the doc claim about "confirmed/pre-confirmed VTXOs the client sees as expired" raises a concern: if client-server clock skew causes the server to not mark a VTXO as expired (see `server.rs:413-414` note about clock disagreement), that VTXO would land in `confirmed`/`pre_confirmed` instead of `recoverable`, and the new `settle()` would miss it.
Clarify whether this edge case is intentional or needs handling.
Issue: No tests for the new settle() method
Severity: Medium
No unit or e2e tests verify:
- `settle()` correctly filters to only recoverable VTXOs
- `settle()` returns `Ok(None)` when no recoverable VTXOs exist
- `settle()` successfully renews expired VTXOs
- `settle()` leaves healthy VTXOs untouched
For a protocol-critical settlement path, test coverage is essential.
Note: Cross-repo impact is contained
The other SDKs (ts-sdk, go-sdk, dotnet-sdk) have their own `settle()` implementations and are not affected by this Rust-side rename. No cross-repo breakage.
Summary
The core idea is sound — narrowing periodic settlement to only expired/recoverable VTXOs saves batch fees and is a good optimization. But the execution has a critical flaw: reusing the `settle()` name silently changes behavior for ~20 callers without a compile error. Use a distinct name like `settle_recoverable()` and add tests.
🤖 Review by Arkana
|
imho breaking change is not an issue as I assume this is the preferred behaviour - settle_all is the exception (if it is even ever needed). |
There was a problem hiding this comment.
We may also want to introduce a method which settles all boarding outputs. It's already possible by passing specific outputs to settle_vtxos, but it's not as convenient.
I mention this because at least in the e2e tests, which are now all red, we used settle to convert boarding outputs into VTXOs.
EDIT: perhaps this makes more sense: #227 (review).
luckysori
left a comment
luckysori
left a comment
There was a problem hiding this comment.
Actually, let's reconsider. Shouldn't default settle always include boarding outputs? I would assume they were created because the user wanted to convert them into VTXOs.
Always pull confirmed boarding outputs into the smart settle, alongside expired/recoverable VTXOs. Freshly funded coins should enter the Ark without forcing the caller to fall back to settle_all.
There was a problem hiding this comment.
🚨 Protocol-Critical Follow-up Review (commits 04fb1cff, 30ebbcbb)
Previous blocker (silent semantic break for all callers) is largely resolved — good work. Including boarding outputs in settle() was the right call, and the test migrations to settle_all() are correct.
Two remaining issues:
BUG: e2e_assets.rs:110 still calls settle() — will break
Severity: High
File: e2e-tests/tests/e2e_assets.rs:110
At this point Alice holds pre-confirmed VTXOs from issue_asset() (not recoverable). The new narrow settle() will return Ok(None), and the balance assertions at lines 117-131 will fail.
Fix: Change to alice.settle_all(&mut rng).await.unwrap();
Ongoing: No tests for the new settle() path
Severity: Medium
Still no unit or e2e test that exercises the new narrow settle() and verifies:
- Returns
Ok(None)when only healthy VTXOs exist (no boarding, no recoverable) - Settles recoverable VTXOs while leaving healthy ones untouched
- Correctly includes boarding outputs alongside recoverable VTXOs
For a protocol-critical settlement path, at least one e2e test should demonstrate the filtering behavior.
Notes (non-blocking)
-
ark-client-sample/src/main.rs:611: The CLIsettlecommand now uses the narrowsettle(). This is arguably the intended UX (periodic renewal), but worth a conscious decision — a user who just received a send and runssettlefrom CLI won't see their pre-confirmed VTXOs get settled. Consider whether the CLI should usesettle_all()or at least document the behavior. -
Minor perf:
settle()callsfetch_commitment_transaction_inputs()for boarding outpoints, thensettle_vtxos()calls it again internally to filter. Two round-trips where one would suffice. Non-blocking but worth noting for future cleanup.
🤖 Review by Arkana
Sub-dust recoverable VTXOs can only be rescued when their combined value clears the server's dust threshold. Document that callers holding isolated sub-dust amounts need to fall back to settle_all, which can roll them in alongside a healthy VTXO acting as carrier value.
bonomat
force-pushed
the
feat/smart-settle
branch
from
30ebbcb to
b7043e8
Compare
There was a problem hiding this comment.
✅ Follow-up Review (commit b7043e8d)
Previous high-severity finding (e2e_assets.rs:110 still calling settle()) is now fixed → settle_all(). Good.
Verified the force-push didn't sneak in other changes — only the e2e_assets.rs fix on top of the prior commits.
Remaining callers using settle() — verified correct
All ~15 remaining settle() callers are boarding-first flows (fund boarding address → settle → assert confirmed balance). Since the new settle() includes boarding outputs, these will work correctly. The callers migrated to settle_all() are the ones holding pre-confirmed VTXOs that need full settlement. Split looks right.
Still outstanding (medium, non-blocking)
-
No tests for new
settle()filtering path — no test verifies thatsettle()skips healthy VTXOs, returnsOk(None)when nothing is eligible, or correctly combines recoverable VTXOs with boarding outputs. For protocol-critical code this is a gap, but I'm not blocking on it given the human reviewer approved. -
Double
fetch_commitment_transaction_inputs()call —settle()fetches boarding inputs, thensettle_vtxos()fetches them again internally. Minor perf waste, non-blocking. -
Sub-dust doc note (
batch.rs:128-131) is helpful — accurately describes the limitation where isolated sub-dust amounts needsettle_all()as carrier.
🤖 Review by Arkana
2 participants
Rename the existing full-renewal settle to settle_all, and introduce a new settle that only renews VTXOs in the server's recoverable bucket plus any confirmed/pre-confirmed VTXOs the client sees as expired. This keeps periodic renewals cheap by leaving healthy VTXOs and boarding outputs untouched.
Summary by CodeRabbit
New Features
Refactoring
Tests