swaps: cover issue 881 settlement replay - #1171
Conversation
Extend the lost-response regression through authoritative claim observation and terminal restart. Assert the recovered payment completes once without duplicate funding or recovery side effects. Also replay an authoritative funding miss after restart to prove the negative control remains terminal.
Project the reported incident shape through the real activity store and daemon balance adapter. Assert a completed pay produces one terminal event, the activity amount matches the VTXO debit, and a failed negative control does not alter balance.
|
/gateway review Review this test-only PR for merge readiness. Incident invariant:
Focus on concrete, reachable defects introduced by this PR. Check assertion validity, whether the fakes model the production boundaries faithfully, test determinism, and whether the tests could pass while the #881 path is still broken. Treat style-only or pre-existing debt as non-blocking. Return READY, READY WITH FOLLOW-UPS, or NOT READY. For each blocker, give the trigger, execution path, consequence, existing guard, smallest fix, and regression risk. Stop once the invariant is proven and no concrete PR-introduced blocker remains. |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
Test-only PR that extends the #881 regression coverage: the sdk/swaps test now carries the recovered pay session through authoritative claim observation, durable completion, and a second restart, and the negative-control test gains a terminal-reload assertion. The exactly-once evidence in sdk/swaps/in_swap_test.go is the strongest part of the change — sendPolicyCalls/armRecoveryCalls/cancelCalls pinned across the second restart, plus GetSwapSummary re-read from the store, do genuinely fail if the FSM re-enters funding or re-derives the claim after terminal persistence.
The weaker half is the wallet-projection test. Its two headline invariants — "the SEND amount matches the VTXO balance debit" and "an authoritative failed pay implies no debit" — are asserted against balances the test itself hardcodes through the fake daemon, so neither assertion can fail for a reason connected to the projection under test. It also models "startup replay" by calling the live fan-out twice rather than through the startup backfill path that the sibling tests in the same file use for restart shapes.
Nothing here changes production code, and I found no defect that makes the added assertions wrong. The findings below are about evidential value and determinism, not correctness of the diff.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
Exercise activity replay through the startup backfill and remove a fake balance comparison that only restated its fixture. Bound terminal reload waits and use txid-shaped identities.
|
/gateway re-review Head is now 76e0c14. Re-check only the five prior findings:
Return READY, READY WITH FOLLOW-UPS, or NOT READY. Stop if no concrete PR-introduced blocker remains. |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
READY WITH FOLLOW-UPS. Four of the five prior findings are closed by 76e0c14: the tautological balance deltas are gone (F1), the duplicate terminal summary now arrives through ListSwaps + runtime.backfillActivity with a third idempotent pass (F2), both terminal reloads are bounded by a one-second context (F4), and the txid/payment-hash literals are now 64-hex (F5). F3 is retracted on the basis of the maintainer's statement that ResumePayViaLightning only reloads durable state and that the FSM advances synchronously inside Wait — the non-test sdk/swaps sources are not in this review context, so I cannot re-derive it, but the passing focused race test is consistent with it.
Two residual gaps, both in the new assertions rather than in the code paths under test, both minor: the terminal-restart block cannot distinguish "reloaded the persisted Completed row" from "re-observed the still-armed indexer and re-completed", and the failed-pay negative control asserts status only, leaving the "no funds movement" half of the stated invariant uncovered.
The incident invariant itself is proven end to end: one accepted payment-scoped funding intent (accepted length 1, sendPolicyCalls pinned), one recovery arm/cancel pair, authoritative preimage observation, durable Completed, and a second restart with no new funding or recovery side effect. No PR-introduced blocker.
Status of prior findings
- F1 addressed: Both
GetConfirmedSatdeltas and therequire.Equal(t, after, afterFailure)restatement are gone fromswapwallet/reconciler_test.go. What remains —entry.AmountSat == -paymentAmountandentry.FeeSat == 1at lines 100–101 — is a projection assertion against the projector's output, not a comparison of two fixture-supplied constants. Resolved. - F2 addressed: The duplicate now goes through the real startup shape: live
fanOutSwapUpdateatswapwallet/reconciler_test.go:77, thenswap.listSwapsRespset andruntime.backfillActivity(ctx)at line 92, withrequire.Len(t, events, 1)at line 105 proving suppression. The added third pass at line 130 also covers the FAILED row. Resolved. - F3 withdrawn: Retracting. The finding was conditional on whether the resumed session polls before
Wait; the answer is that it does not, so the post-resume field writes atsdk/swaps/in_swap_test.go:2634–2644and the exactsendPolicyCalls == 3at line 2653 are deterministic. This rests on the maintainer's statement plus the focused race run, not on code I can read here —sdk/swapsnon-test sources are outside this context. - F4 addressed: Both terminal reloads now wrap
Waitin a one-second context (sdk/swaps/in_swap_test.go:2681and:2799), so a regression that re-enters the poll loop fails in a second instead of hanging to the package timeout. Note the fix trades a package timeout for a wall-clock bound inside at.Parallel()test; if this ever flakes under-raceon a loaded runner, widen the bound rather than removing it. - F5 addressed:
SpentByTxIDis now 64 hex characters (sdk/swaps/in_swap_test.go:2637–2638), matching the siblingTestPayViaLightningReturnsClaimPreimage, andfailedHashatswapwallet/reconciler_test.go:56–57is a 64-hex value alongside the equally-shapedpaymentHash. Resolved.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
bhandras
added
the
backport-v0.1.x-branch
|
Successfully created backport PR for |
1 participant
What this changes
The production bug was fixed by #1061. This PR adds the incident-shaped regression that was still missing.
TestPaySessionFundingReplayAfterLostResponsethrough authoritative claim observation, durable completion, and a second daemon restart.There is no production-code change.
Incident path
VHTLCFunded.FundingInitiatedaction kept polling after changing its own state. The live-vHTLC lookup then emittedOnVHTLCFundedagain.VHTLCFunded -> OnVHTLCFunded, so the pay row became FAILED even though the server later claimed the vHTLC.VHTLCFunded. The FSM advances toWaitingForClaim, observes the indexed preimage, and persistsCompleted.The attached #881 daemon log records the separate OOR accounting boundary: the 3,000-sat input was spent and replaced by a 1,999-sat live change VTXO for the 1,001-sat send. The activity mismatch came from the pay FSM's incorrect terminal state, not from a missing VTXO debit.
The wallet regression does not feed a fake balance into the assertion. The incident log is the authoritative evidence for the debit; the test covers the activity projection that was wrong.
Tests
go test ./sdk/swaps -count=1go test -tags='wavewalletrpc swapruntime' ./swapwallet -count=1make lint-changed-local base=origin/mainmake fmt-changed-check base=origin/mainmake tidy-module-checkmake sqlc-checkmake doc-check schema-check sample-conf-checkmake commitmsg-lint range=origin/main..HEADRelated to #881. Production fix: #1061.