round: release forfeit reservations via a status reconcile on round death - #992
Conversation
Roasbeef
added
the
dst
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd50c7216c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(round.Intents.Forfeits) > 0 && | ||
| a.env.StatusReconcileTimeout > 0 { |
There was a problem hiding this comment.
When the daemon restarts with an active refresh round, the production DB reconstruction does not populate Round.Intents.Forfeits (it only restores boarding intents in dbRoundToDomainRound, and the reconstructed InputSigSentState only gets Boarding/VTXOs). That makes this condition false for exactly the reloaded forfeit-bearing rounds described in the comment, so no status-reconcile timer is armed after restart; if the operator failure was missed during the crash window, the client never sends QueryRoundStatus and the pending-forfeit reservation can remain stranded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and it ran deeper than the guard: even with the timer armed, the dead-answer release would have been a no-op after a restart, since it releases the in-memory forfeit set that the reload never repopulated. Fixed in the new db: rebuild reloaded round forfeit sets from vtxo rows commit: both reload paths (dbRoundToDomainRound and reconstructInputSigSentState) now rebuild Intents.Forfeits from the Forfeiting VTXO rows via the persisted forfeit_round_id, pinned by TestRoundStoreReloadRebuildsForfeitSet against a real store.
cd50c72 to
4425f8a
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 3m 30s —— View job Review: forfeit release via status reconcileI read the full diff (
Overall this is a careful, well-reasoned fix. The double-spend hazard is handled correctly (release gated on an authoritative dead answer, not on the timeout or notification), the exponential backoff is bounded, the RoundID mismatch guard is present, the confirmation watch stays registered as a fraud backstop, and the DB rebuild closes the real cross-restart hole the codex bot flagged. Tests cover the important branches. A few things worth addressing before merge: 1. Stale doc comment on
|
4425f8a to
2972194
Compare
|
Addressed the review in the latest push (tip 2972194):
On the restart re-arm seam: it is exercised end to end by the DST crash-door catch on lumos#81 (server crash, silence, probe from the re-armed timer, release), which reruns green against this tip. A focused Go unit test for the actor reload loop is a fair follow-up. |
In this commit, we add the wire surface for the wavelength#844 status reconcile: a client->server QueryRoundStatus RPC and the ClientRoundStatusReport push event that answers it, carrying a RoundLifecycleStatus classification (in-flight, broadcast, confirmed, or dead). A client that has already sent its per-VTXO forfeit signatures cannot release the forfeit reservations on a round-failure notification alone: the operator may hold fully-signed forfeit txs, and a blind release risks a double-spend if the round's commitment later confirms. The saving grace is that a forfeit tx spends a connector output of the commitment, and the operator persists a finalized round atomically with its VTXOs before the commitment is ever broadcast. A ROUND_STATUS_DEAD answer (no live FSM, no durable row) therefore proves the commitment can never confirm, which is exactly the proof-of-death that makes the release safe. The generated code is regenerated via make rpc.
2972194 to
bb02e71
Compare
In this commit, we fix wavelength#844: a round failure arriving after the client's forfeit signatures have left the box no longer strands the VTXO in Forfeiting for the life of the batch. The releaseForfeitsOnFailure wrapper (the #653 fix) deliberately stops at PartialSigsSentState, because past that point the operator may hold fully-signed forfeit txs and a blind release risks a double-spend. The result was that InputSigSentState had no release path at all: the coin sat stranded until the #823 startup sweep or batch expiry rescued it. The fix is a status reconcile rather than a table edit. A BoardingFailed that lands in InputSigSentState with forfeits at stake now parks in the state (PendingFailure) while a QueryRoundStatusOutbox probes the operator for the round's authoritative lifecycle status. Only a dead answer, meaning the round never finalized so its commitment can never confirm, fails the round through releaseForfeitsOnFailure, which returns the inputs to LiveState and retires the originating job on a terminal-for-job code. Any other answer holds the reservations. The same probe covers the lumos#618 silence door, where a crashed operator never sends a failure at all: a status-reconcile timeout (armed when the forfeit signatures are emitted, re-armed per probe, re-armed on restart reload) fires in the silence and drives the identical query. The timeout alone never releases; only the operator's answer does. Boarding-only rounds keep the old immediate-failure behavior, and a non-positive StatusReconcileTimeout opts out entirely. The timer only arms when forfeits are actually at stake, matching the gate every consumer applies, and repeated unanswered probes back off exponentially (capped at 16x the base window) so an operator that predates the status RPC sees a bounded probe cadence rather than a fixed-rate loop forever. The dead-answer release also spells out its trust boundary in the code: the proof of death is the operator's own self-report, sound against an honest-but-faulty operator (the failure mode this reconcile exists for), while the commitment confirmation watch stays registered so a fraudulent later broadcast still surfaces as a detected conflict.
In this commit, we register the inbound dispatch route for ClientRoundStatusReport, so the operator's answer to a QueryRoundStatus probe reaches the round FSM as a RoundStatusReported event through the same push-event path the other round messages use. Without the route the daemon would drop the report on the floor and the reconcile would spin on its retry timeout forever.
In this commit, we close the restart gap in the status-reconcile release. The reconcile keys every decision on Intents.Forfeits, but that set only ever lived in memory: dbRoundToDomainRound and reconstructInputSigSentState rebuilt boarding intents alone, so after a restart a forfeit-bearing round looked boarding-only. The re-arm guard in the actor's reload loop never fired, and even a hand-armed timer would have released nothing, since the dead-answer path releases the (empty) in-memory forfeit list. The strand the reconcile exists to fix simply reopened across every restart. The durable ground truth was already there: MarkVTXOForfeiting stamps each Forfeiting VTXO row with the binding forfeit_round_id. We add a ListForfeitingVTXOsByRound query over those rows and rebuild the forfeit set in both reload paths. The rebuilt requests carry the outpoint and amount with no custom spend paths, which is exactly right: custom (caller-supplied) forfeit inputs never enter the wallet store and their signing contexts die with the process, so everything the query returns is a standard wallet forfeit for the release path. The new TestRoundStoreReloadRebuildsForfeitSet pins the behavior against a real store: commit an InputSigSent round, mark two VTXOs Forfeiting against it, and require both reload paths to surface the pair (and only the pair) as standard forfeits.
bb02e71 to
ff13cd5
Compare
Roasbeef
added
the
backport-v0.1.x-branch
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-992-to-v0.1.x-branch
git worktree add --checkout .worktree/backport-992-to-v0.1.x-branch backport-992-to-v0.1.x-branch
cd .worktree/backport-992-to-v0.1.x-branch
git reset --hard HEAD^
git cherry-pick -x ff13cd57bfac9513aec001cb0c698ceb65979da7
git push --force-with-lease |
…anch [v0.1.x-branch] Backport #992: round: release forfeit reservations via a status reconcile on round death
1 participant
In this PR, we fix #844: a refresh or leave round that fails after the client has already sent its per-VTXO forfeit signatures no longer strands the VTXO in
Forfeitingfor the life of the batch. The strand needed no crash to trigger; a single lostSubmitVTXOForfeitSigson the wire was enough, and the coin stayed wedged until the #823 startup sweep or batch expiry rescued it.The reason this was never a one-line table edit is the double-spend hazard the issue lays out: once the forfeit signatures leave the box, the operator may hold fully-signed forfeit txs for the coin, so a blind runtime release would mark the VTXO spendable while a signed forfeit for it exists. The
releaseForfeitsOnFailurewrapper (the #653 fix) deliberately stops atPartialSigsSentStatefor exactly this reason. The saving grace is that a forfeit tx spends a connector output of the round's commitment tx, and (since the lumos#631 fix) the operator persists a finalized round atomically with its VTXOs before the commitment is ever broadcast. So "the operator has no record of this round" is a proof that the commitment can never confirm, and that proof is what makes the release safe.The status reconcile
The fix turns that proof into protocol. A
BoardingFailedthat lands inInputSigSentStatewith forfeits at stake now parks in the state while a newQueryRoundStatusprobe asks the operator for the round's authoritative lifecycle status (in-flight, broadcast, confirmed, or dead). Only a dead answer fails the round throughreleaseForfeitsOnFailure, returning the inputs toLiveStateand retiring the originating job on a terminal-for-job code. Any other answer holds the reservations and keeps waiting, since the commitment may still confirm.The same probe covers the lumos#618 silence door, where a crashed operator never sends a failure notification at all: a status-reconcile timeout is armed when the forfeit signatures are emitted (and re-armed on restart reload), fires in the silence, and drives the identical query. The timeout alone never releases anything; only the operator's answer does. Boarding-only rounds keep the old immediate-failure behavior, and a non-positive
StatusReconcileTimeoutopts out entirely, restoring the pre-fix behavior.The server half (answering the query from the live FSM first, then the durable round store) lands in lightninglabs/lumos alongside a submodule bump; the two halves are wire-compatible either way — a client probing an old server just retries on its reconcile timeout, which is today's strand behavior, no worse.
Verification
Beyond the unit tests in
round/status_reconcile_test.go(park-and-probe, dead-releases, non-dead-holds, mismatched-report guard, re-probe loop, terminal-job retirement), the fix was verified end to end under the darepo DST harness (systest/dston lumos PR #81), which is what found #844 in the first place: both deterministic catches — the timeout door and the #618 server-crash door — now drive the real client and server actors through the reconcile and watch the coin return toLiveon both stores. The full DST suite and a 500-seed workload soak are green with the strand allowlist deleted; the ops that used to classify the known bug now assert the recovery instead.See each commit message for the incremental breakdown.
Review hardening
An adversarial review pass over the combined client+server diff (plus the codex bot, which flagged the same hole) caught a defect that silently defeated the fix across a client restart: the forfeit set only ever lived in memory. A reloaded round looked boarding-only, so the restart re-arm guard never fired, and even a hand-armed timer would have released nothing since the dead-answer path releases the (empty) in-memory set. The new
db:commit closes this by rebuildingIntents.Forfeitsfrom the Forfeiting VTXO rows (each row already carries the bindingforfeit_round_id) in both reload paths, andTestRoundStoreReloadRebuildsForfeitSetpins it against a real store. Everything the rebuild returns is a standard wallet forfeit, which is exactly right: custom caller-supplied forfeit inputs never enter the wallet store and their signing contexts die with the process.The pass also hardened the probe loop: reconcile probes now back off exponentially (capped at 16x the base window) so an operator predating the status RPC sees a bounded cadence instead of a fixed-rate loop forever, and the timer only arms when forfeits are actually at stake. The dead-answer release now spells out its trust boundary in the code: the proof of death is the operator's own self-report, sound against an honest-but-faulty operator (the failure mode this reconcile exists for), while the commitment confirmation watch stays registered so a fraudulent later broadcast still surfaces as a detected conflict rather than passing silently.