-
Notifications
You must be signed in to change notification settings - Fork 12
multi: reduce remaining alert noise #1210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0305d16
2aac64d
309fdee
c1c790f
d0482c8
3fba4f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package chainbackends | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "log/slog" | ||
| "time" | ||
|
|
@@ -15,6 +16,8 @@ import ( | |
| "github.com/lightningnetwork/lnd/chainntnfs" | ||
| fn "github.com/lightningnetwork/lnd/fn/v2" | ||
| "github.com/lightningnetwork/lnd/lnwallet/chainfee" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| // lndRegistrationTimeout bounds how long a conf/spend registration call into | ||
|
|
@@ -27,6 +30,19 @@ import ( | |
| // genuinely wedged backend surfaces as an error rather than a silent hang. | ||
| const lndRegistrationTimeout = 15 * time.Second | ||
|
|
||
| // isBlockEpochShutdownError reports whether an LND call only failed because | ||
| // 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 { | ||
| if ctx.Err() == nil { | ||
| return false | ||
| } | ||
|
|
||
| return errors.Is(err, context.Canceled) || | ||
| status.Code(err) == codes.Canceled | ||
| } | ||
|
|
||
| // LndClientTxBroadcaster implements TxBroadcaster using | ||
| // lndclient.WalletKitClient. | ||
| type LndClientTxBroadcaster struct { | ||
|
|
@@ -507,6 +523,20 @@ func (n *LndClientChainNotifier) RegisterBlockEpochNtfn( | |
| ctx, int64(height), | ||
| ) | ||
| if err != nil { | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block hash "+ | ||
| "lookup cancelled during "+ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 All three cited lines were restructured, but the new wrapping still overflows at tab-width 8: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The rewrapped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 Dismissed by @bhandras 🟡 Three added lines still exceed the 80-column limit at tab-width 8:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /gateway dismiss |
||
| "subscription shutdown", | ||
| slog.Any("err", err), | ||
| slog.Int( | ||
| "height", | ||
| int(height), | ||
| ), | ||
| ) | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| log.WarnS( | ||
| ctx, | ||
| "Failed to get block hash", | ||
|
|
@@ -535,7 +565,18 @@ func (n *LndClientChainNotifier) RegisterBlockEpochNtfn( | |
|
|
||
| case err, ok := <-errChan: | ||
| if ok && err != nil { | ||
| log.WarnS(ctx, "Block epoch error", err) | ||
| if isBlockEpochShutdownError(ctx, err) { | ||
| log.DebugS(ctx, "Block epoch "+ | ||
| "subscription cancelled", | ||
| slog.Any("err", err), | ||
| ) | ||
| } else { | ||
| log.WarnS( | ||
| ctx, | ||
| "Block epoch error", | ||
| err, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| return | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2333,8 +2333,10 @@ func (a *RoundClientActor) handleGetState(ctx context.Context, | |
| for keyStr, roundFSM := range a.rounds { | ||
| roundState, err := fsmState(scanCtx, roundFSM.FSM) | ||
| if err != nil { | ||
| a.log.WarnS(ctx, "Failed to get FSM state for round", | ||
| err, | ||
| a.log.DebugS( | ||
| ctx, | ||
| "Skipped round with unreadable FSM state", | ||
| slog.Any("err", err), | ||
| slog.String("key", string(keyStr)), | ||
| ) | ||
|
|
||
|
|
@@ -2923,12 +2925,13 @@ func (a *RoundClientActor) processOutbox(ctx context.Context, | |
| inputSigState.CommitmentTx, | ||
| ) | ||
|
|
||
| // Index for confirmation routing and register. | ||
| // Index the transaction before the confirmation request | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 Dismissed by @bhandras 🟡 The new comment asserts a restarted actor "still re-registers active rounds in Start", but that path goes through
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /gateway dismiss |
||
| // rounds in Start. | ||
| a.commitmentTxIndex[txid] = keyStr | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Deleting the Why this mattersThe deleted path built its registration deliberately: The asymmetry the change introduces is worth naming regardless: restart recovery still registers through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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
Second, concrete asymmetry: the caller IDs diverge. What closes it: a test that drives the real FSM transition with non-nil There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 Dismissed by @bhandras 🟡 Removing the checkpoint-path registration trades one registration that covered every checkpointed round for per-transition emissions, and only the forfeit-bearing transition (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /gateway dismiss |
||
| a.registerCommitmentConfirmation( | ||
| ctx, txid, roundFSM.CommitmentTx, | ||
| inputSigState.VTXOTreePaths, | ||
| ) | ||
|
|
||
| a.log.InfoS(ctx, "Round checkpoint processed", | ||
| slog.String("round_id", m.RoundID.String()), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -835,8 +835,20 @@ func TestActorProcessOutbox(t *testing.T) { | |
| // Set up a round in InputSigSentState, simulating the state | ||
| // after the round has completed through partial sigs. | ||
| commitmentTx := h.setupRoundInInputSigSentState(roundID) | ||
| txid := commitmentTx.UnsignedTx.TxHash() | ||
| pkScript := confirmationWatchScript( | ||
| commitmentTx.UnsignedTx, nil, | ||
| ) | ||
| targetConfs := h.actor.env.OperatorTerms.MinConfirmations | ||
|
|
||
| outbox := []ClientOutMsg{ | ||
| &RegisterConfirmationRequest{ | ||
| CallerID: "commitment-" + txid.String(), | ||
| Txid: &txid, | ||
| PkScript: pkScript, | ||
| TargetConfs: targetConfs, | ||
| HeightHint: h.actor.env.StartHeight, | ||
| }, | ||
| &RoundCheckpointedNotification{ | ||
| RoundID: roundID, | ||
| }, | ||
|
|
@@ -850,10 +862,18 @@ func TestActorProcessOutbox(t *testing.T) { | |
| keyStr := RoundKeyStr(roundID.KeyString()) | ||
| require.Contains(t, h.actor.rounds, keyStr) | ||
|
|
||
| txid := commitmentTx.UnsignedTx.TxHash() | ||
| indexedKeyStr, exists := h.actor.commitmentTxIndex[txid] | ||
| require.True(t, exists) | ||
| require.Equal(t, keyStr, indexedKeyStr) | ||
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new assertions check |
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The |
||
| require.Equal(t, targetConfs, registration.TargetConfs) | ||
| }) | ||
|
|
||
| t.Run("new_boarding_creates_round", func(t *testing.T) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
F10(Minor) — Alert-classification predicate ships untested ·chainbackends/lndclient_adapters.go:37isBlockEpochShutdownErrordecides whether an operator-actionableCanceledfrom a live backend stays a warning or is silenced to debug, and no test in this PR covers it —files[]contains nochainbackendstest file. Inverting thectx.Err() == nilguard, 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.