fix(swap): keep a verified deposit's claim window from lapsing#593
Conversation
A taker on the manual path sends source funds, then relays them with `alw swap post-tx`. submit_swap_claim requires reserved_until >= now, and if the reservation lapses in between there is no claim, no Swap, no timeout and no refund — the deposit is simply gone, to the miner, in silence. Three TAO sends were lost this way today. Two independent gaps, one on each side of the send. Validator: confirm_deposit now slides reserved_until forward when a verified deposit arrives with little runway left, before submitting the claim. Evidence-gated — the deposit has already matched the pinned reservation, so we only extend for a taker who demonstrably sent funds, never on request. This is the same justification the crank extends under, one step earlier in the lifecycle: the crank can only act once a Swap exists, and here there is no Swap precisely because the claim has not landed. Best-effort; a failed extension still attempts the claim, since the reservation may have just enough runway and a claim that lands beats a clean error path. CLI: the manual path now prints the deadline as a wall-clock instant plus the raw reserved_until, and says plainly that missing it is unrecoverable. _SEND_MARGIN_SECS is checked once, at reservation time; after that the send and the relay happen outside this process and nothing re-checks it. A bare countdown is stale the moment it prints and useless in an agent's log, hence the absolute time. When --send was not used it also points at `alw swap now --send`, which keeps send + relay + watch in-process where the guard still applies. Limits, stated plainly: neither change recovers an already-expired reservation (the contract forbids extending one), and neither helps a taker who sends with no reservation at all. This closes the honest race — deposit verified, runway nearly gone — not every way to lose funds here. The structural gap remains that source funds go to the miner before a Swap exists, with no escrow.
LandynDev
left a comment
There was a problem hiding this comment.
Two comments, one worth fixing before merge. Everything else looks solid — the guards mirror the contract exactly (strictly-later, ceiling, live-only), the extension confirms before the claim, and the test coverage is good.
| CLAIM_RELAY_MARGIN_SECS = 90 | ||
|
|
||
|
|
||
| def _extend_for_claim(client, miner_pk, reservation, now: int) -> None: |
There was a problem hiding this comment.
now here is the one stamped in confirm_deposit before verify_transaction runs — and that's a source-chain RPC that can take seconds on BTC. By the time this executes, the clock is stale in the wrong direction: the runway check can see "ample" when real runway is already under 90s, and when it does extend, now + CLAIM_RELAY_MARGIN_SECS buys less than 90s of real time. That's the same race this PR is closing, just narrower.
Suggest recapturing inside the helper and dropping the param:
def _extend_for_claim(client, miner_pk, reservation) -> None:
now = int(time.time())There was a problem hiding this comment.
Good catch — fixed in e720255, exactly as suggested: now = int(time.time()) inside the helper, param dropped.
Worth spelling out that it cut both ways, since the second half is the nastier one: with a stale clock the runway check reads "ample" on a window that is already short (so no extension at all), and when it does extend, now + CLAIM_RELAY_MARGIN_SECS is measured from a point already seconds in the past — the extension buys less than 90s of real runway. Both failure modes are the race this PR is closing.
Added test_confirm_measures_runway_after_the_source_rpc: it hangs verify_transaction for 60s on a fake clock against a reservation with 120s left, then asserts the extension fired and targeted now_after_rpc + margin. Off the pre-RPC clock that reservation looks ample and never extends — I verified the test fails on the old signature before restoring the fix.
| f'([cyan]{max(0, reserved_until - now)}s[/cyan] from now, reserved_until={reserved_until}). ' | ||
| 'Miss it and the deposit is unrecoverable — there is no refund.' | ||
| ] | ||
| if not want_send: |
There was a problem hiding this comment.
Small gap: with --send, if _auto_send_wizard falls back to manual (missing creds, wrong wallet), we still land on this notice — but want_send=True suppresses the warning, even though that user is now sending outside alw too. That's arguably the audience that needs it most: an agent whose auto-send silently degraded.
Suggest splitting the line: always print the "forfeits the pre-send safety check" warning when the manual notice is reached; only gate the alw swap now --send hint on want_send (it's redundant for someone who just tried it). Non-blocking.
There was a problem hiding this comment.
Agreed, and taken — also in e720255. Split exactly as you proposed:
- unconditional:
Sending outsidealwforfeits the pre-send safety check. - gated on
not want_send:`alw swap now --send` does the send, the relay and the watch in one step.
You are right that reaching this notice at all means the send is happening outside the process — _auto_send_wizard returning False (no source provider, wallet does not control the pinned from_addr, declined confirm, coldkey unlock failed) is the only way past it with want_send=True, and none of those leave the guard applying. The old gate keyed off intent when the only thing that matters is outcome.
Test split to match: test_deadline_notice_always_warns_that_the_guard_is_forfeit (both values of want_send) and test_deadline_notice_hints_at_send_only_when_it_was_not_asked_for.
…n manual send Review follow-ups. 1. `_extend_for_claim` took `now` from `confirm_deposit`, stamped *before* `verify_transaction` — a source-chain RPC that can burn seconds on BTC. The stale clock cut both ways: the runway check could read "ample" while the real window was already under the margin, and an extension computed off it bought less than 90s of real time. It now re-reads the clock itself; the param is gone. 2. On the manual path the "forfeits the pre-send safety check" warning was gated on `want_send`, so a `--send` run that fell back to manual (missing creds, wrong wallet) never saw it — the audience that needs it most. The warning is now unconditional; only the `alw swap now --send` hint stays gated, since it's redundant for someone who just asked for it. Tests: the new runway test hangs the source RPC on a fake clock and asserts the extension fires — it fails against the pre-fix signature. Deadline-notice tests split to cover warning-always / hint-gated. 804 -> 806 passed. ruff check + format clean.
The failure
A taker on the manual path sends source funds, then relays them with
alw swap post-tx.submit_swap_claimrequiresreserved_until >= now. If the reservation lapses in between:no claim → no Swap → no timeout → no refund. The deposit is gone, sitting in the miner's wallet, silently. There is no recovery instruction in the contract — I checked all 24.
Three TAO sends (~1.5 TAO) were lost exactly this way today by an external agent.
There are two independent gaps, one on each side of the send. Both are small; keeping them together because they only make sense as a pair.
1. Validator — extend before claiming
confirm_depositslidesreserved_untilforward when a verified deposit arrives with little runway left, then submits the claim.Evidence-gated on purpose. The deposit has already matched the pinned reservation by the time this runs, so we only ever extend for a taker who demonstrably sent funds — never on request. That's the same justification the crank extends under (
_extend_reservation_action), one step earlier in the lifecycle: the crank can only help once a Swap exists, and here there is no Swap precisely because the claim hasn't landed. That's the hole.Best-effort — a failed extension still attempts the claim. The reservation may have just enough runway, and a claim that lands is worth more than a clean error path.
CLAIM_RELAY_MARGIN_SECS = 90is deliberately much shorter than the CLI's 180s pre-send margin: at this point only the claim tx remains, the send and relay are done. Once the Swap exists the crank owns every later extension.2. CLI — state the deadline
_SEND_MARGIN_SECSis checked once, at reservation time. On the manual path the send and the relay then happen outside this process, and nothing re-checks it. That's why the existing guard didn't save the incident above — it passed,swap nowexited, and the send went out minutes later viabtcli.So the manual path now prints the deadline as a wall-clock instant plus the raw
reserved_until, and says plainly that missing it is unrecoverable. A bare countdown is stale the moment it prints and useless in an agent's log. When--sendwasn't used it also points atalw swap now --send, which keeps send + relay + watch in-process where the guard still applies.Extracted to
_deadline_linesso the wording is testable rather than buried in aconsole.print.Limits — stated plainly
extend_reservation.rs:43), and that's correct.The structural gap remains: source funds go to the miner before a Swap exists, with no escrow. Worth a separate discussion; I didn't want to bundle a design change into a fix.
On raising
reservation_ttl_secsinsteadIt was raised as a simpler alternative, and it is one instruction (
set_reservation_ttl, already admin-settable — no code needed). I'd argue against it:The evidence-gated lever is strictly better here: it costs nothing when unused, can't be griefed (you must produce a real deposit), and is bounded by
max_extend_at, which is frozen at creation and separately admin-tunable viaset_max_total_extensionif we want more headroom.Tests
pytest tests/→ 804 passed. ruff check + format clean.New coverage:
max_extend_at, but still attempts the claimreserved_until, runway from the injectednow--sendhint appears only on the unguarded path