multi: reduce remaining alert noise - #1210
Conversation
|
/gateway review |
c46ace5 to
b55029f
Compare
![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 · 🟠 1 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
This PR reclassifies five log paths that fire repeatedly for one underlying condition. Four of the five changes are pure severity/dedup work (block-epoch shutdown, mailbox pull retries, bounded FSM scan misses, proof-floor alerts) and are contained; the new proofNodeFloorAlertDeduper is small, correctly mutex-guarded, and wired so all children of one registry share it.
The exception is commit 1. It is not a logging change — it deletes a call to registerCommitmentConfirmation from processOutbox's checkpoint handling and relies on the FSM outbox to have emitted an equivalent registration. The two registration paths are not obviously equivalent, and the accompanying test asserts only that exactly one registration exists, not that it is the right one. That is the one thing here that can move funds if it is wrong.
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)
| // here creates two notifier subscriptions for the same | ||
| // tx. A restarted actor still re-registers active | ||
| // rounds in Start. | ||
| a.commitmentTxIndex[txid] = keyStr |
There was a problem hiding this comment.
🟠 F1 (Major) — Checkpoint registration removed without proving the FSM's is equivalent · round/actor.go:2934
Deleting the registerCommitmentConfirmation call at the checkpoint leaves the FSM-emitted RegisterConfirmationRequest as the sole steady-state watch for the commitment tx, but nothing in the diff or the loaded context shows that request carries the same watch parameters. If it does not, the round's confirmation either never routes (funds sit in a checkpointed round that never finalizes) or finalizes on fewer confirmations than the operator requires.
Why this matters
The deleted path built its registration deliberately: registerCommitmentConfirmation derives pkScript from confirmationWatchScript(packet.UnsignedTx, vtxoTrees) — with an in-code comment stating it must "watch the validated batch output ... rather than assuming output 0" — and sets TargetConfs: a.cfg.OperatorTerms.MinConfirmations. The only visible representation of the FSM-side message is the one the new test hand-builds at round/actor_test.go:840-845, which carries TargetConfs: 1 and no PkScript at all. The FSM transition that emits the real message is not in file_contents[], so I cannot confirm which shape it uses; this is a major if the FSM request omits the tree-derived pkScript or hardcodes a conf target below OperatorTerms.MinConfirmations, and a non-issue if it mirrors the actor path.
The asymmetry the change introduces is worth naming regardless: restart recovery still registers through registerCommitmentConfirmation (see TestActorRecovery/single_active_round asserting one registration after Start), so a round now gets one watch shape in steady state and a different one after a restart. Those two shapes should be reconciled — either by having the FSM emit the same pkScript/conf-target, or by keeping the actor as the single registration owner and removing the FSM's emission instead.
| // The FSM outbox registration is sufficient. Processing the | ||
| // checkpoint notification must not create a second notifier for | ||
| // the same commitment transaction. | ||
| require.Len(t, h.chainSource.registrations, 1) |
There was a problem hiding this comment.
🟡 F2 (Minor) — Regression test cannot detect a divergent FSM registration · round/actor_test.go:867
The new assertions check len(h.chainSource.registrations) == 1 and that its Txid matches, but the RegisterConfirmationRequest under test is constructed by the test itself, so it proves only that processOutbox no longer double-registers — not that the real FSM emission is a viable substitute for the deleted call. Asserting PkScript and TargetConfs on registrations[0] against what registerCommitmentConfirmation would have produced would turn this into a test that actually locks in F1.
| err, | ||
| slog.Uint64("cursor", state.PullCursor), | ||
| ) | ||
| if *failCount == 0 { |
There was a problem hiding this comment.
🟡 F3 (Minor) — Pull failure counter is shared, so first warning can be lost · serverconn/ingress.go:419
failCount is not private to the pull path — ackPhase, the idle checkpoint flush, and both dispatch-failure branches all call a.sleepBackoff(ctx, &failCount) — so a preceding unrelated failure leaves it non-zero and the first genuine pull failure logs at debug with a misleading consecutive_failures count. Concretely: an idle-flush checkpoint error increments it to 1, and the next Pull failure then takes the else branch and never warns.
| firstAlert := b.cfg.proofNodeFloorAlerts == nil || | ||
| b.cfg.proofNodeFloorAlerts.first(txid) | ||
| if firstAlert { | ||
| b.log.WarnS(ctx, "Proof-node confirmation floor may "+ |
There was a problem hiding this comment.
🟡 F4 (Minor) — Deduped floor warning no longer names any affected VTXO · unroll/actor.go:878
The surviving warning drops target_outpoint (and vtxo_age_blocks / created_height) to the debug line, so an operator paged on "exits could stall" gets a proof txid but no way to identify a single affected VTXO without enabling debug logging. Since the dedup is per proof transaction, keeping the first target's outpoint on the warning costs nothing in alert volume and preserves the entry point for triage.
| ) | ||
| if err != nil { | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block hash lookup "+ |
There was a problem hiding this comment.
🟡 F5 (Minor) — New lines exceed the repo's 80-column limit · chainbackends/lndclient_adapters.go:527
Several added lines run past 80 columns at their nesting depth: chainbackends/lndclient_adapters.go:527 (85) and :569 (92), and unroll/actor.go:878 (87) — the last only because the new if firstAlert { pushed a previously-79-column line one level deeper. CLAUDE.md states an 80-char limit and round/actor.go carries a file-level //nolint:ll, so these will not pass a clean ll run; wrapping the string literals one segment earlier resolves all three.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
b55029f to
3edd47b
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 6 findings
🔴 0 Blocker · 🟠 1 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
Three of the five prior findings landed cleanly: the pull-failure counter is now pull-scoped (F3), the proof-floor warning carries the first affected target's identity (F4), and the double-registration itself is now pinned by require.Len(t, h.chainSource.registrations, 1).
F1 is still the open one. The new test documents the expected registration shape but builds the RegisterConfirmationRequest itself, so nothing verifies what the FSM actually emits — and the shape it documents uses confirmationWatchScript(tx, nil), i.e. the output-0 fallback that the deleted call went out of its way to avoid. The caller IDs on the two registration paths also differ, which removes the dedup backstop the change is relying on. The FSM transition file is not in file_contents[], so I cannot settle it from here.
New commits add three minor concerns, all in the "does the noise reduction actually do what it claims" bucket: the proof-floor dedup keys on an incidental txid rather than the shared ancestor, the pull suppression is one-shot rather than interval-based, and the sharing wiring has no test and fails silently if dropped.
Status of prior findings
- F3 addressed: Fixed at
serverconn/ingress.go:65-68and:425-441— a dedicatedpullFailCount, incremented only on pull failures and reset on a successful pull, so an unrelated ack or checkpoint backoff can no longer swallow the first pull warning. - F4 addressed: Fixed at
unroll/actor.go:877-901— the warning now carriestarget_outpoint,vtxo_age_blocks,created_height, andheight_hintfor the first affected target, with the per-target Debug record retained for the rest.
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)
| "exceed ancestor height; exit could stall", | ||
| nil, | ||
| firstAlert := b.cfg.proofNodeFloorAlerts == nil || | ||
| b.cfg.proofNodeFloorAlerts.first(txid) |
There was a problem hiding this comment.
🟡 F6 (Minor) — Alert dedup keys on an incidental proof node, not the ancestor · unroll/actor.go:876
first(txid) is keyed on whichever proof node the actor happens to submit first, but the condition being alerted (age >= proofNodeHeightHintLookback) is a property of the target VTXO, not of that transaction — proofNodeConfHeightHint's own comment says "txid is used only for the fallback-path breadcrumb below, not to select a floor". Two targets sharing an ancestor collapse to one warning only when their first-processed node coincides, so a target resumed mid-exit (starting from a deeper node) and a freshly admitted sibling both warn; keying on the proof root or desc.Ancestry[i].CommitmentTxID would match what AGENTS.md now documents.
| err, | ||
| slog.Uint64("cursor", state.PullCursor), | ||
| ) | ||
| if *pullFailCount == 0 { |
There was a problem hiding this comment.
🟡 F7 (Minor) — Pull outage warns once, never again · serverconn/ingress.go:425
Suppression is one-shot per episode, so a mailbox outage lasting hours produces a single warning at its start and nothing after — an operator who misses that line has no warn-level signal for the rest of the outage. The same file already models the other episode type differently: per the comment at serverconn/ingress.go:78-82, a stalled dispatch target "is logged on an interval rather than on every re-pull". The markIngressPoll staleness gauge is a backstop here, but an interval re-warn would make the two episode types consistent (noteDispatchDeferral itself is not in the loaded context, so I am going by that comment).
|
|
||
| require.True(t, deduper.first(firstProof)) | ||
| require.False(t, deduper.first(firstProof)) | ||
| require.True(t, deduper.first(secondProof)) |
There was a problem hiding this comment.
🟡 F8 (Minor) — Shared-deduper wiring has no test and fails silently · unroll/proof_floor_alert_test.go:21
The one new test exercises first() sequentially in a single goroutine, so neither property the change depends on is covered: that registryBehavior.childConfig propagates the shared deduper (unroll/registry.go:1495), and that concurrent children cannot both claim the first alert (the -race run in the PR description exercises no concurrency). The propagation gap is the riskier of the two — NewVTXOUnrollActor substitutes a fresh newProofNodeFloorAlertDeduper() when the field is nil, so dropping the registry wiring reverts to per-child dedup with no compile error and no failing test.
| // here creates two notifier subscriptions for the same | ||
| // tx. A restarted actor still re-registers active | ||
| // rounds in Start. | ||
| a.commitmentTxIndex[txid] = keyStr |
There was a problem hiding this comment.
🟠 F1 (Major) — Commitment watch may register the wrong pkScript · round/actor.go:2934 · partially_addressed
Nothing in the diff or the loaded context shows what the FSM emits, so the deleted registration's tree-derived pkScript may not be reproduced. If it is not, rounds whose funds land outside output 0 register a confirmation watch on a pkScript the commitment tx never pays to, the confirmation never routes, and the round never finalizes.
Why this matters
registerCommitmentConfirmation passed inputSigState.VTXOTreePaths into confirmationWatchScript precisely so the watch lands on the validated batch output "rather than assuming output 0" (its own comment). The new test derives its expected script at round/actor_test.go:839-840 as confirmationWatchScript(commitmentTx.UnsignedTx, nil) — exactly the output-0 fallback — and then feeds that value into the outbox itself, so it neither exercises nor constrains the tree-derived path.
Second, concrete asymmetry: the caller IDs diverge. registerCommitmentConfirmation builds fmt.Sprintf("commitment-tx-%s", txid.String()); the FSM message the test mirrors uses "commitment-" + txid.String() (round/actor_test.go:846). chainsource keys registrations by caller ID, so if the Start-time recovery registration and an FSM-emitted one are ever live at the same time they cannot collapse — the double subscription this PR removes has no backstop on that path.
What closes it: a test that drives the real FSM transition with non-nil VTXOTreePaths and asserts the emitted request's PkScript equals confirmationWatchScript(tx, trees) and its CallerID matches the recovery path's format. Absent the FSM transition file I cannot confirm either way.
| require.Len(t, h.chainSource.registrations, 1) | ||
| registration := h.chainSource.registrations[0] | ||
| require.True(t, registration.Txid.IsEqual(&txid)) | ||
| require.Equal(t, pkScript, registration.PkScript) |
There was a problem hiding this comment.
🟡 F2 (Minor) — Registration assertions compare test-supplied values · round/actor_test.go:875 · partially_addressed
The PkScript / TargetConfs assertions were added, but the test computes both at :839-842 and feeds them into the outbox at :848-849, so require.Equal at :875-876 proves only that processOutbox passes the fields through. The require.Len(..., 1) check is the part that earns its keep; locking in F1 needs the request to originate from the FSM, not from the test body.
| if err != nil { | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block hash "+ | ||
| "lookup cancelled during "+ |
There was a problem hiding this comment.
🟡 F5 (Minor) — Rewrapped lines still exceed 80 columns · chainbackends/lndclient_adapters.go:528 · partially_addressed
All three cited lines were restructured, but the new wrapping still overflows at tab-width 8: chainbackends/lndclient_adapters.go:528 ("lookup cancelled during "+ at 7 tabs, 83), :570 ("subscription cancelled",, 81), and unroll/actor.go:886 (slog.String("proof_txid", txid.String()), at 5 tabs, 81). Splitting each one segment earlier clears them.
The round FSM already emits the confirmation registration after it commits the durable InputSigSent state. The checkpoint notification registered the same transaction again under a second caller ID. Both subscriptions delivered the same confirmation. The first completed the round. The second reported that the transaction was no longer indexed. Keep steady-state registration in the FSM outbox. Checkpoint handling only installs the routing index. Restart recovery registers active rounds from durable state. Crash recovery is unchanged. The FSM request uses the validated batch-output script and operator's minimum confirmation target, matching restart recovery. Add regression assertions for both fields and for the single chain notification watch.
3edd47b to
8ad12c3
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
The FSM-side registration is now proven equivalent to the deleted actor-side one: round/vtxo_tree_binding_test.go drives the real forfeitCollectionOutbox and asserts the emitted RegisterConfirmationRequest carries the validated batch-output script, OperatorTerms.MinConfirmations, the round's StartHeight, and the commitment txid (F1, F2). The deduper test now covers both concurrency and childConfig propagation (F8, mostly), and unroll/actor.go's alert lines are back inside 80 columns (part of F5).
What remains is small. Two added lines in chainbackends/lndclient_adapters.go still exceed 80 columns, the per-proof-node dedup key (F6) and the one-shot pull-failure suppression (F7) are unchanged, and the deduper test still hand-builds registryBehavior rather than going through NewUnrollRegistryActor.
One new concern comes out of what the F1 evidence revealed: the conf registration is now emitted from the same forfeitCollectionOutbox whose message ordering is load-bearing for the issue #386 fix, and the new test does not pin that ordering.
Status of prior findings
- F1 addressed:
round/vtxo_tree_binding_test.go:288-292now exercises the realForfeitSignaturesCollectingState.forfeitCollectionOutboxand asserts the emitted request carriesbatchScript(the validated batch output, not output 0),TargetConfs == OperatorTerms.MinConfirmations,HeightHint == env.StartHeight, and the commitment txid — the exact parametersregisterCommitmentConfirmationbuilt.HeightHintdiffering from the actor path's current-best-height query only widens the rescan window, so it is safe. The one thing the loaded context does not show is whether every round that reaches checkpoint transitsForfeitSignaturesCollectingState; the boarding-only recovery case atround/actor_test.go(replays_checkpointed_boarding_input_sigs, which replays aSubmitForfeitSigRequestfor a boarding input) indicates it does. - F2 addressed:
round/actor_test.go:838-841,875-877now derivespkScriptfromconfirmationWatchScriptandtargetConfsfromh.actor.env.OperatorTerms.MinConfirmationsand asserts both onregistrations[0], and the non-tautological version of the same check lives inround/vtxo_tree_binding_test.go. Together they lock in F1. - F3 addressed:
pullFailCountis a dedicated pull-only counter (serverconn/ingress.go:65,400,425), so an unrelated ack or checkpoint backoff can no longer suppress the first genuine pull warning. - F4 addressed: The warning at
unroll/actor.go:878now carriestarget_outpoint,proof_txid,vtxo_age_blocks,lookback,height_hint, andcreated_height, so a paged operator has a triage entry point without enabling debug logging.
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)
| ) | ||
|
|
||
| var registration *RegisterConfirmationRequest | ||
| for _, msg := range outbox { |
There was a problem hiding this comment.
🟡 F9 (Minor) — Conf registration joins an outbox whose ordering is load-bearing · round/vtxo_tree_binding_test.go:282
The conf registration now rides in forfeitCollectionOutbox, whose message ordering is load-bearing for the issue #386 fix, and the new test does not pin where it sits.
round/actor_test.go (forfeit_send_failure_still_arms_timeout) documents the invariant: "The forfeit-collection transitions emit StartTimeoutReq BEFORE the per-VTXO ForfeitRequestToVTXO messages… processOutbox aborts on the first send error, so if the timeout were emitted last it would be skipped." This PR adds a RegisterConfirmationRequest to that same outbox, and it is fallible — it Asks ChainSource, which can fail transiently. If it is emitted ahead of StartTimeoutReq, a failed registration aborts the outbox before the forfeit-collection timeout is armed, and the round waits for signatures forever with forfeit-reserved inputs stranded — the exact #386 failure. I cannot tell from the loaded context where forfeitCollectionOutbox places it (the transition source is not in file_contents[]); this is a major if it precedes StartTimeoutReq.
The new test scans the outbox with a for/break to find the request, which is precisely where the ordering could be pinned: assert the RegisterConfirmationRequest index is greater than the StartTimeoutReq index rather than just that it exists.
| if err != nil { | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block hash "+ | ||
| "lookup cancelled during "+ |
There was a problem hiding this comment.
🟡 F5 (Minor) · chainbackends/lndclient_adapters.go:528 · partially_addressed
unroll/actor.go is fixed (every added line now lands at or under 80 columns), but two added lines in this file still exceed the limit at 8-space tabs: :528 ("lookup cancelled during "+, 7 tabs + 27 = 83) and :570 ("subscription cancelled",, 7 tabs + 25 = 81). Splitting each string one word earlier clears both.
| "exceed ancestor height; exit could stall", | ||
| nil, | ||
| firstAlert := b.cfg.proofNodeFloorAlerts == nil || | ||
| b.cfg.proofNodeFloorAlerts.first(txid) |
There was a problem hiding this comment.
🟡 F6 (Minor) · unroll/actor.go:876 · unresolved
first(txid) is still keyed on whichever proof node the actor happens to submit first, while the alerted condition (age >= proofNodeHeightHintLookback) is a property of the target VTXO. Two targets sharing an ancestor collapse to one warning only when their first-processed node coincides, so a target resumed mid-exit and a freshly admitted sibling still warn twice; keying on the proof root or desc.Ancestry[i].CommitmentTxID would match what unroll/AGENTS.md now documents.
| if *pullFailCount == 0 { | ||
| a.log.WarnS(ctx, "Pull failed, retrying", | ||
| err, | ||
| slog.Uint64("cursor", state.PullCursor), |
There was a problem hiding this comment.
🟡 F7 (Minor) · serverconn/ingress.go:428 · unresolved
Suppression is still one-shot per episode, so a multi-hour mailbox outage produces exactly one warning at its start and nothing after. The same file's dispatch-deferral episode is logged "on an interval rather than on every re-pull" (serverconn/ingress.go:78-82); an interval re-warn here would make the two episode types consistent.
|
|
||
| // Every child config from one registry must share the same deduper. | ||
| registryDeduper := newProofNodeFloorAlertDeduper() | ||
| registry := ®istryBehavior{ |
There was a problem hiding this comment.
🟡 F8 (Minor) · unroll/proof_floor_alert_test.go:44 · partially_addressed
Both properties named in the original finding are now covered: 32 concurrent first() calls yield exactly one claim, and childConfig is asserted to propagate the same pointer to every child. What remains is the constructor: the test hand-builds ®istryBehavior{proofNodeFloorAlerts: registryDeduper}, so dropping proofNodeFloorAlerts: newProofNodeFloorAlertDeduper() from NewUnrollRegistryActor (unroll/registry.go:239) would leave the field nil, silently revert to per-child dedup via the NewVTXOUnrollActor fallback, and still pass. Constructing through NewUnrollRegistryActor (or asserting the field is non-nil after construction) closes it.
8ad12c3 to
eb54846
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 4 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
The four substantive gaps from the last review are closed. forfeitCollectionOutbox now emits RegisterConfirmationRequest with the validated batch-output pkScript, env.OperatorTerms.MinConfirmations, and env.StartHeight (F1), and the new assertions in both test files pin those parameters (F2). The submission/registration pair moved behind the timeout arming with an explicit rationale comment, and the test asserts the index ordering (F9). pullFailCount is now a dedicated counter (F3), the operator warning carries target_outpoint again (F4), and the deduper test exercises both the concurrency guarantee and the registry spawn seam (F8).
What remains is the same three minor items from last round, none of which the new commits touched: the over-80 lines (F5), the txid-based dedup key (F6), and the one-shot-per-episode pull warning (F7). One new item: the predicate that decides whether a Canceled is an alert or noise ships without a test, which is the asymmetry against the deduper — the unroll dedup got a test, the classification predicates did not.
No unresolved major or blocker.
Status of prior findings
- F1 addressed: Fixed at
round/transitions.go:3145-3151— the FSM request now carriesPkScript: pkScript,TargetConfs: env.OperatorTerms.MinConfirmations, andHeightHint: env.StartHeight, matching whatregisterCommitmentConfirmationderived.round/vtxo_tree_binding_test.go:310-313locks all three plus the txid against the validated batch output. - F2 addressed:
round/actor_test.go:872-874now assertsPkScriptandTargetConfsonregistrations[0]againstconfirmationWatchScript(...)andOperatorTerms.MinConfirmations, so the test proves the surviving registration is equivalent rather than merely counting it. - F3 addressed:
serverconn/ingress.go:66adds a pull-onlypullFailCount, threaded throughpullPhaseand reset on a successful pull (:441). An unrelated ack or checkpoint backoff no longer suppresses the first genuine pull warning. - F4 addressed: The warning at
unroll/actor.go:880-902now carriestarget_outpoint,vtxo_age_blocks, andcreated_heightalongsideproof_txid, so a paged operator has a concrete VTXO to triage without enabling debug. - F8 addressed:
unroll/proof_floor_alert_test.go:32-45drives 32 concurrentfirst()calls and asserts exactly one claim, and:47-73spawns two real children throughNewUnrollRegistryActor/registryBehavior.spawnand assertsrequire.Sameon the shared deduper. Dropping thechildConfigwiring atunroll/registry.go:1495would now fail a test instead of silently reverting to per-child dedup. - F9 addressed:
round/transitions.go:3136-3152moves both fallible effects behind the timeout messages with a comment naming theprocessOutboxstop-on-first-error reason, andround/vtxo_tree_binding_test.go:308-309pinsstartIndex < submitIndexandstartIndex < registrationIndex. SincestartIndexrecords the last*StartTimeoutReqseen, the assertion covers every timeout in the outbox, not just the first.
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)
| // the owning block-epoch subscription is shutting down. The context check is | ||
| // required so an independent Canceled status from a live backend remains an | ||
| // actionable warning. | ||
| func isBlockEpochShutdownError(ctx context.Context, err error) bool { |
There was a problem hiding this comment.
🟡 F10 (Minor) — Alert-classification predicate ships untested · chainbackends/lndclient_adapters.go:37
isBlockEpochShutdownError decides whether an operator-actionable Canceled from a live backend stays a warning or is silenced to debug, and no test in this PR covers it — files[] contains no chainbackends test file. Inverting the ctx.Err() == nil guard, or dropping it entirely, would suppress every genuine block-epoch failure with no failing test. Two table cases (live context + codes.Canceled → false; cancelled context + context.Canceled → true) would pin the branch that the whole change rests on.
| if err != nil { | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block hash "+ | ||
| "lookup cancelled during "+ |
There was a problem hiding this comment.
🟡 F5 (Minor) · chainbackends/lndclient_adapters.go:528 · partially_addressed
The rewrapped log.DebugS message lines fit, but the deeper nesting still pushes several added lines past 80 columns at the documented 8-column tab width: chainbackends/lndclient_adapters.go:528 (83) and :571 (81), and in unroll/actor.go the block the new if firstAlert { indented one level — :888 (81), :892 (84), :900 (84), :905 (83). Wrapping each one segment earlier, or hoisting proofNodeHeightHintLookback / int64(b.desc.CreatedHeight) into locals above the WarnS call, resolves them.
| "exceed ancestor height; exit could stall", | ||
| nil, | ||
| firstAlert := b.cfg.proofNodeFloorAlerts == nil || | ||
| b.cfg.proofNodeFloorAlerts.first(txid) |
There was a problem hiding this comment.
🟡 F6 (Minor) · unroll/actor.go:878 · unresolved
Unchanged: first(txid) is still keyed on whichever proof node the actor happens to submit first, while the condition being alerted (age >= proofNodeHeightHintLookback) is a property of the target VTXO. Targets whose frontiers have diverged — a resumed exit starting from a deeper node, or siblings whose shared ancestor already confirmed — each claim a separate first alert. Keying on the proof root or desc.Ancestry[i].CommitmentTxID would match what unroll/AGENTS.md:188-194 now documents.
| slog.Uint64("cursor", state.PullCursor), | ||
| ) | ||
| if *pullFailCount == 0 { | ||
| a.log.WarnS(ctx, "Pull failed, retrying", |
There was a problem hiding this comment.
🟡 F7 (Minor) · serverconn/ingress.go:426 · unresolved
Unchanged: suppression is still one-shot per episode, so a multi-hour mailbox outage produces a single warning at its start. The comment at serverconn/ingress.go:78-82 describes the sibling deferral episode as logged "on an interval rather than on every re-pull"; an interval re-warn here would make the two episode types consistent.
📚 Doc drift advisoryThis PR's Go changes in Proposed
|
Round completion stops each VTXO's block subscription. A block already being processed can race that cancellation, causing GetBlockHash or the notifier error channel to return a Canceled status. The forwarder then exits normally, so paging an operator for each terminated VTXO is noise. Treat Canceled results as expected shutdown only when the owning context is also done. Keep a Canceled status from a live subscription, and every other notifier or block-hash failure, at warning level.
A mailbox outage currently warns on every exponential-backoff attempt. A normal rollout produced eight alerts in about twenty seconds for each client even though every retry carried the same cause and cursor. Keep the first failure at warning level so the dependency outage remains visible. A pull-only counter prevents suppression by unrelated backoff. Log later pull failures at debug with the consecutive count. Successful pulls reset the episode, so a later outage warns again.
GetClientState scans every tracked round under one shared deadline so a slow FSM cannot block the round actor indefinitely. When the deadline is consumed, the handler skips unreadable rounds. The next monitoring poll tries again. Two such misses on testnet recovered without affecting a round. This is diagnostic state from a bounded monitoring read, not an operator action. Keep the error and round key at debug level. Invalid FSM state types and failures in state-changing paths remain warnings or errors.
One old proof transaction can be an ancestor of many target VTXOs. Each target actor warned when its bounded confirmation scan could start after that ancestor. A testnet restart therefore produced 309 warnings for 309 targets even though they referred to only 13 proof transactions. Share a process-local, concurrency-safe alert set across children of one unroll registry. Warn once per proof transaction and include the first affected target. Retain a debug record for every target, age, creation height, and height hint. Restart re-arms the warning. The unresolved recovery risk remains visible. The confirmation floor and exit behavior do not change. This only bounds the number of human alerts generated by one historical-data gap. Test registry sharing and the single winner under concurrent child calls.
eb54846 to
14adf66
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
The fifth pass closes the substantive gaps. forfeitCollectionOutbox now emits the confirmation registration with the validated batch script and OperatorTerms.MinConfirmations (round/transitions.go:3145-3151), and the new test drives the real outbox to pin both the parameters and the message ordering that the #386 timeout invariant depends on (round/vtxo_tree_binding_test.go:308-313). isBlockEpochShutdownError has a table test covering the live-backend-Canceled boundary, the pull-failure counter is now pull-scoped, and the proof-floor deduper has both a concurrency test and a registry-propagation test through the production spawn seam. F1–F4 and F8–F10 are resolved.
What remains is unchanged from the last pass: the dedup key is still the arbitrarily-first-submitted proof node rather than a target-scoped identity (F6), pull-failure suppression is still one-shot for the whole outage (F7), and three added lines still exceed 80 columns (F5).
Two new observations, both minor. The change replaces one universal registration with per-transition emissions, and only the forfeit-bearing transition is covered by a test. Separately, the restart path's height hint diverges from the FSM path's in a way the repo's own comment in createRoundFSMFromDB warns about.
The doc-gardening bot's advisory about stale round/chainbackends/serverconn invariant docs is worth acting on — the processOutbox single-owner rule and the outbox ordering rule are exactly the kind of "breaks if violated" invariant those files carry.
Status of prior findings
- F1 addressed:
forfeitCollectionOutboxemitsRegisterConfirmationRequestwithPkScriptfromconfirmationWatchScriptandTargetConfs: env.OperatorTerms.MinConfirmations(round/transitions.go:3145-3151), andround/vtxo_tree_binding_test.go:310-313asserts the emitted request carries the validated batch script, the operator conf target, the height hint, and the right txid. Resolved. - F2 addressed:
round/actor_test.go:875-876now assertsPkScriptandTargetConfson the single surviving registration, not just its txid. - F3 addressed:
pullFailCountis declared separately atserverconn/ingress.go:68and mutated only insidepullPhase, so an unrelated ack or checkpoint backoff can no longer consume the first-failure warning. - F4 addressed: The warning now carries
target_outpoint,vtxo_age_blocks,height_hint, andcreated_height(unroll/actor.go:885-899), so the alert is triageable without enabling debug. - F8 addressed:
unroll/proof_floor_alert_test.go:27-40covers the concurrent claim (32 goroutines, exactly one winner) and:44-73drives the realNewUnrollRegistryActor+behavior.spawnseam, asserting withrequire.Samethat both children share the registry-lifetime deduper. Dropping thechildConfigwiring now fails a test rather than silently reverting to per-child dedup. - F9 addressed:
round/vtxo_tree_binding_test.go:308-309asserts theStartTimeoutReqindex precedes both the submission and the registration. Because the loop overwritesstartIndexon each match, the assertion binds against the last-emitted timeout, which is the stronger form of the invariant. - F10 addressed:
TestIsBlockEpochShutdownError(chainbackends/lndclient_adapters_test.go:20) covers all four quadrants, including live-backendcodes.Canceled→ false and a cancelled context with an unrelated error → false. Inverting or dropping thectx.Err()guard now fails a test.
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)
| // here creates two notifier subscriptions for the same | ||
| // tx. A restarted actor still re-registers active | ||
| // rounds in Start. | ||
| a.commitmentTxIndex[txid] = keyStr |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F11 (Minor) — Only the forfeit-bearing path is tested for the confirmation watch · round/actor.go:2934
Removing the checkpoint-path registration trades one registration that covered every checkpointed round for per-transition emissions, and only the forfeit-bearing transition (forfeitCollectionOutbox) is exercised by a test — a boarding-only round, which reaches InputSigSent without forfeits and so never runs that helper, has no test asserting it still gets a watch. round/actor_test.go's round_checkpointed case hand-builds the request rather than driving the transition, so it would pass even if the boarding-only path emitted nothing and the commitment tx were left unwatched. Worth a one-line confirmation that the PartialSigsSent → InputSigSent path emits the same request, and a test on it if so.
| // emitted by the FSM can be delivered. The FSM outbox | ||
| // owns the steady-state registration; registering again | ||
| // here creates two notifier subscriptions for the same | ||
| // tx. A restarted actor still re-registers active |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F12 (Minor) — Restart re-registration uses current height as its height hint · round/actor.go:2932
The new comment asserts a restarted actor "still re-registers active rounds in Start", but that path goes through registerCommitmentConfirmation, which derives HeightHint from a live BestHeightRequest rather than the round's StartHeight — so PkScript and TargetConfs match the FSM-emitted request while the height hint does not, and it can sit above the height at which the commitment already confirmed. createRoundFSMFromDB documents exactly this hazard for the same restart path ("not the current height, which could miss confirmations if the tx was already mined"), and restart is precisely when the tx is most likely to have confirmed while the daemon was down. This is pre-existing code the diff does not touch, and I cannot confirm from the loaded context whether the notifier tolerates a hint above the confirmation height; passing round.StartHeight through to the registration would make the two paths agree on all three parameters.
| if err != nil { | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block hash "+ | ||
| "lookup cancelled during "+ |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F5 (Minor) · chainbackends/lndclient_adapters.go:528 · partially_addressed
Three added lines still exceed the 80-column limit at tab-width 8: chainbackends/lndclient_adapters.go:528 ("lookup cancelled during "+, 83), the "subscription cancelled", continuation in the errChan branch (81), and slog.String("proof_txid", txid.String()), inside the WarnS call at unroll/actor.go (81). Splitting each string literal one segment earlier clears all three; if ll is configured with a different tab width in this repo, disregard.
| "exceed ancestor height; exit could stall", | ||
| nil, | ||
| firstAlert := b.cfg.proofNodeFloorAlerts == nil || | ||
| b.cfg.proofNodeFloorAlerts.first(txid) |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F6 (Minor) · unroll/actor.go:878 · unresolved
first(txid) is still keyed on whichever proof node the actor happens to submit first, while the condition being alerted (age >= proofNodeHeightHintLookback) is a property of the target VTXO — proofNodeConfHeightHint's own comment states "txid is used only for the fallback-path breadcrumb below, not to select a floor." Two targets sharing an ancestor collapse to one warning only when their first-processed node coincides, so a target resumed mid-exit from a deeper node and a freshly admitted sibling both warn. Keying on the proof root or desc.Ancestry[i].CommitmentTxID would match what the updated AGENTS.md describes.
| err, | ||
| slog.Uint64("cursor", state.PullCursor), | ||
| ) | ||
| if *pullFailCount == 0 { |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F7 (Minor) · serverconn/ingress.go:425 · unresolved
Suppression is still one-shot per episode, so a multi-hour mailbox outage emits one warning at its start and nothing after; an operator who misses that line has no warn-level signal for the remainder. The same file models the other episode type on an interval instead — per the comment at serverconn/ingress.go:78-82, a stalled dispatch target "is logged on an interval rather than on every re-pull." The markIngressPoll gauge is a backstop, but an interval re-warn would make the two episode types consistent.
|
🚫 Dismissed F5 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F6 (minor) · 🟡 F7 (minor) · 🟡 F11 (minor) · 🟡 F12 (minor) |
|
🚫 Dismissed F6 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F7 (minor) · 🟡 F11 (minor) · 🟡 F12 (minor) |
|
Gateway F6 dismissal rationale: The warning is intentionally keyed by proof transaction, not by target VTXO or proof root. The observed storm was 309 target warnings for 13 distinct proof transactions. Each proof transaction is an independent historical confirmation lookup that can stall recovery. Collapsing all of them under one root could hide a second missing proof node. Thirteen actionable warnings preserve that distinction while removing the 309-target fan-out. Gateway F7 dismissal rationale: Mailbox pull failures use the repository severity contract: the first failure in an outage warns, identical retries are debug, and a successful pull re-arms the warning. Interval warnings would page repeatedly for one unchanged outage. The ingress-poll staleness gauge is the continuous outage signal; logs retain the first transition and recovery episode boundary. Gateway F5 status: All changed lines in Gateway F11 dismissal rationale: The boarding-only Gateway F12 dismissal rationale: The restart recovery height hint is pre-existing behavior outside this PR's changed lines. This PR removes a duplicate steady-state registration; it neither introduces nor changes |
|
🚫 Dismissed F7 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F11 (minor) · 🟡 F12 (minor) |
|
🚫 Dismissed F11 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F12 (minor) |
|
🚫 Dismissed F12 (minor) by @bhandras — no reason given All findings on this PR are addressed or dismissed. |
The forfeit-collection outbox submitted signatures and registered the commitment chain watch before it armed status reconciliation. processOutbox stops at the first failed external effect. A transient failure could therefore skip the timeout after signatures had left the client and strand reserved inputs without a reconciliation path. Arm the status-reconciliation timeout before both fallible effects. Keep the old forfeit-collection timeout cancellation first because the two timers use distinct phase keys. Protocol state and timeout policy do not change. Build the remaining outbox sequence directly: VTXO forfeit signatures, optional boarding signatures, then chain registration. This preserves the server submission order without an insertion index whose meaning changes when the optional timeout is present. Exercise the real FSM outbox and pin the exact mixed-round order, the validated batch script, and the operator confirmation target.
14adf66 to
3fba4f6
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F1 addressed:
forfeitCollectionOutboxnow emits theRegisterConfirmationRequestwithPkScriptfromconfirmationWatchScript,TargetConfs: env.OperatorTerms.MinConfirmations, andHeightHint: env.StartHeight, matching what the deleted actor-side call built. Resolved. - F2 addressed:
round/actor_test.go:867-875now assertsPkScriptandTargetConfsonregistrations[0], andround/vtxo_tree_binding_test.godrives the real transition and checks the same fields againstbatchScript/targetConfs/startHeight/txid. Resolved. - F3 addressed:
ingressLoopnow owns a dedicatedpullFailCount, incremented and reset only inpullPhase, so an unrelated ack or checkpoint backoff can no longer suppress the first genuine pull warning. Resolved. - F4 addressed: The warning at
unroll/actor.gonow carriestarget_outpoint,proof_txid,vtxo_age_blocks,lookback,height_hint, andcreated_height, so the alert is triageable without enabling debug. Resolved. - F8 addressed:
TestProofNodeFloorAlertDedupernow exercises 32 concurrentfirst()callers asserting exactly one winner, and spawns two real registry children assertingrequire.Sameon the shared deduper — covering both the concurrency property and thechildConfigpropagation that would otherwise revert silently. Resolved. - F9 addressed: The test pins the full outbox shape:
require.Len(t, outbox, 5)plusIsTypeonCancelTimeoutReq,StartTimeoutReq,SubmitVTXOForfeitSigsToServer,SubmitForfeitSigRequest, and theRegisterConfirmationRequestat index 4. The reconciliation timeout is now provably ahead of both fallible effects, and the server submission order is unchanged from the pre-refactor insertion. Resolved. - F10 addressed:
TestIsBlockEpochShutdownErrorcovers all four branches, including the load-bearing one — acodes.Canceledstatus on a live context must stay an actionable warning. Inverting or dropping thectx.Err()guard now fails a test. 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)
2 participants
Problem
Several expected lifecycle and retry paths still produce repeated warnings:
Canceledwhile its owning context is shutting down.These paths create many human alerts for one condition.
Changes
Each classification change is a separate commit:
Cancelederrors only when the owning context is already done. Live-backend failures remain warnings.GetClientStatescan misses to debug. State-changing failures remain warnings or errors.Safety
No protocol, persistence, transaction, or recovery policy changes. The outbox ordering now preserves the existing status-reconciliation path if signature submission or chain registration fails, while retaining the established server submission order.
The proof confirmation floor and exit behavior are unchanged. The warning remains visible once per underlying proof transaction.
Testing
go test -tags='dev nolog' ./round ./chainbackends ./serverconn ./unrollgo test -tags='dev nolog' ./round -run TestConfirmationWatchScriptUsesBatchOutput -count=10go test -race -tags='dev nolog' ./unroll -run TestProofNodeFloorAlertDedupermake lint-changed-localmake commitmsg-lint range="origin/main..HEAD"