Repository navigation
Conversation
…be swept The client's L1 claim leaf has no timelock, so a claim can replace the solver's refund until that refund confirms, which on mainnet is priced against a deadline about a week out. The funding gate only required the lockup to live until htlcLocktime + 3h, and this corridor cannot claim a swept lockup, so a client funding with inputs expiring just after that could wait for the sweep, claim L1 and recover the lockup at refund_locktime, taking both legs. The adopted outpoints now record their batch expiry, the refund's fee ramp targets the earlier of refund_locktime and that expiry, less the claim window, and the gate requires the lockup to outlive the refund arming, a two-hour confirm budget and the claim window (htlcLocktime + 5h). Regtest stays exempt, as at the gate.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #354
Overall: the logic is correct and the fix is well-scoped. Two minor defensive notes below; neither blocks merge.
Protocol correctness
onchainRefundConfirmDeadline formula (packages/solver-core/src/core/onchainSend.ts):
Math.min(refundLocktime, lockupExpiresAt ?? refundLocktime) - ARKADE_CLAIM_WINDOW_SECONDSThe ?? refundLocktime identity collapses the Math.min to refundLocktime when no expiry is known, preserving the previous behaviour exactly. When a lockup expiry is earlier it drives the ramp to all-in before the batch can be swept. Timing arithmetic checks out: the floor htlcLocktime + HTLC_REFUND_MTP_MARGIN + LOCKUP_REFUND_CONFIRM_BUDGET + ARKADE_CLAIM_WINDOW_SECONDS leaves a full 2 h LOCKUP_REFUND_CONFIRM_BUDGET for the refund to confirm, then 90 min for the solver to answer a revealed preimage before the batch expires.
Backward compatibility (packages/solver-corridors/src/db/onchainSwaps.ts, column lockup_outpoints): old rows contain {txid, vout, value} with no expiresAt. lockupExpiresAt returns undefined for them (the flatMap produces an empty list), so the fee ramp falls back to the previous refundLocktime-based behaviour. ✓
expiresAt: undefined JSON round-trip (onchainOrchestrator.ts:862): JSON.stringify({…, expiresAt: undefined}) drops the key; JSON.parse then produces an object without it; lockupExpiresAt guards with o.expiresAt === undefined. ✓
Regtest exemption consistency: both the funding gate (expiryBeforeClaimDeadline check) and lockupExpiresAt early-return on network === 'regtest'. ✓
Finding 1 — JSON.parse without error handling (minor)
packages/solver-corridors/src/send/onchainOrchestrator.ts, the new lockupExpiresAt method:
const expiries = (JSON.parse(row.lockupOutpoints) as { expiresAt?: number }[]).flatMap(…)row.lockupOutpoints is non-null here, but JSON.parse will throw on a malformed string. The column is written by a single trusted call site (JSON.stringify(…)), so corruption is unlikely. If it does happen the refund tick throws on every invocation, stalling fee-bump broadcasts for that swap — a silent loss of liveness for real money. A try/catch that falls back to undefined (treating the row as having no expiry information) would bound the blast radius:
private lockupExpiresAt(row: OnchainSendSwapRow): number | undefined {
if (this.deps.network === 'regtest' || row.lockupOutpoints === null) return undefined
let parsed: { expiresAt?: number }[]
try {
parsed = JSON.parse(row.lockupOutpoints)
} catch {
return undefined
}
const expiries = parsed.flatMap((o) => (o.expiresAt === undefined ? [] : [o.expiresAt]))
return expiries.length > 0 ? Math.min(...expiries) : undefined
}Finding 2 — Orchestrator gate test boundary (nit)
test/send/onchainOrchestrator.test.ts:648:
const row = await quoteThenFund(gated(), (deadline) => deadline - 1)
// where deadline = arkadeClaimDeadline(htlcLocktime)After this PR the gate checks against lockupExpiryFloor, which is arkadeClaimDeadline + LOCKUP_REFUND_CONFIRM_BUDGET (+2 h). The test still produces a refusal (the value is ~7 200 s below the new floor), so it is not broken. But it no longer exercises the boundary; a lockup expiring at lockupExpiryFloor - 1 now also fails the gate for a reason the test doesn't cover. The new unit test in test/core/onchainSend.test.ts:111 does reach the right boundary at the pure-function level. Consider updating the orchestrator integration test comment/value to match the floor it is actually verifying against, so future readers don't misinfer what the gate is protecting.
Test coverage
New paths covered:
expiryBeforeClaimDeadlinerefusing an expiry betweenarkadeClaimDeadlineandlockupExpiryFloor—test/core/onchainSend.test.ts:111. ✓- Fee ramp reaching all-in before
lockupExpiresAt − ARKADE_CLAIM_WINDOW_SECONDSwhen that deadline is tighter thanrefundLocktime—test/send/onchainOrchestrator.test.ts:2306. ✓ - Existing
onchainRefundConfirmDeadline(first.refundLocktime)call withoutlockupExpiresAtcontinues to test the unchanged single-argument path. ✓
No cross-repo consumers of the changed exports found in the SDK clones.
…not the claim deadline
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up — #354 @ 6c60243
One test-only commit since last pass. Finding 2 fixed; Finding 1 still open (unchanged). No new findings.
Prior findings closure
Finding 1 — JSON.parse without error handling (packages/solver-corridors/src/send/onchainOrchestrator.ts:878)
Still open. This commit does not touch onchainOrchestrator.ts; the unguarded JSON.parse(row.lockupOutpoints) is exactly as I last saw it. As noted previously: DB corruption here stalls fee-bump broadcasts for that swap on every tick — a liveness risk for real money, but bounded to a single swap. I flagged it as minor and non-blocking; that assessment stands.
Finding 2 — Orchestrator gate test pinned to wrong boundary (test/send/onchainOrchestrator.test.ts:631,655)
Fixed in 6c60243.
Two targeted changes:
quoteThenFundnow passeslockupExpiryFloor(outcome.swap.htlcLocktime)toexpiry()— the helper is now calibrated to the actual gate floor, not the intermediatearkadeClaimDeadline.- A new acceptance case
['expires exactly at the floor', (deadline: number) => deadline]is added, exercising the exact boundary that the gate accepts. Combined with the existingdeadline - 1refusal case, the gate is now bracketed on both sides at the correct value.
The test now reads correctly: deadline - 1 refuses; deadline (the floor exactly) passes; deadline + 86_400 passes; undefined passes. This is the right set for a gate boundary test.
No cross-repo impact — this commit is test-only with no exported-API changes.
What changed
lockup_outpointsnow records each adopted output's batchexpiresAt.min(refund_locktime, earliest lockup expiry) − ARKADE_CLAIM_WINDOW_SECONDS(onchainRefundConfirmDeadline), so it reaches the all-in rate before the lockup can be swept.expiryBeforeClaimDeadline) now requires the lockup to outlivelockupExpiryFloor = htlcLocktime + HTLC_REFUND_MTP_MARGIN + 2h + ARKADE_CLAIM_WINDOW_SECONDS(H+5h), up fromarkadeClaimDeadline(H+3h).main: a lockup expiring between H+3h and H+5h is refused, and a refund racing a lockup that expires long beforerefund_locktimereaches the all-in rate in time.Why
The L1 claim leaf has no timelock, so the client can claim (outbidding the solver's refund) until that refund confirms. On mainnet the refund was priced against
refund_locktime − 90m, about a week out, so it only kept pace with the estimate. The gate only protected the lockup until H+3h, and this corridor has no swept-lockup claim (only Lightning send got one in #340).A client could fund the lockup with inputs expiring just after H+3h, wait for the batch sweep, then claim L1. The solver would hold P with nothing left to claim offchain, and the client would recover the swept lockup at
refund_locktime, taking both legs.With the refund confirming by
expiry − 90m, a cheaper client claim still lands while the solver has 90 minutes to claim offchain. Replacing an all-in refund would cost the client its whole payout in fees.Limits