multi: never report a local db error to the peer - #10996
Open
Roasbeef wants to merge 5 commits into
Open
Conversation
Roasbeef
force-pushed
the
db-err-link-failure
branch
from
July 27, 2026 19:21
c8dbef4 to
953c230
Compare
In this commit, we add a single predicate that answers the question callers outside of this package actually care about: was this error our own database's fault, or was it caused by the data we were handed? The distinction matters most on the channel state machine paths. A serialization failure, an exhausted retry loop, a dropped connection, a canceled query, a kv store that was closed out from under us or a disk that filled up all say something about our local infrastructure and nothing at all about our channel peer. Callers that report failures to the peer on the wire need to be able to tell those apart, so that a busy database is never dressed up as a protocol violation. The predicate is deliberately a denylist rather than an allowlist, and a conservative one: it only matches errors that unambiguously mean local trouble, so a genuine protocol violation is never mistaken for one and silently swallowed. It covers the bbolt backed kv stores as well as the SQL ones. Note that we match on the walletdb sentinels rather than on bbolt itself, since walletdb already converts the two bbolt errors that can surface on a write path. That keeps this package's imports to the walletdb root, which is stdlib only. We also add ErrRetryCanceled here, which the transaction retry loop will use in a follow up commit to label an interrupted retry loop.
In this commit, we make the transaction retry loop both more patient and more honest about why it gave up. Two leaks are fixed first. If the context was canceled while we were waiting to retry, we used to return the bare serialization error, whose Error() is nothing but the raw postgres text. Callers had no way to tell an aborted retry loop apart from a transaction that truly failed. We now wrap that error with ErrRetryCanceled, keeping the original error in the chain so errors.Is and errors.As still work. Likewise, exhausting the retry budget used to return a naked ErrRetriesExceeded, throwing away the error that actually caused the failure. We now attach it, along with the attempt count and the elapsed time. The retry loop itself is now bounded by a RetryConfig with two independent budgets: a maximum number of attempts, and a maximum amount of wall clock time. Either one stops the loop, and so does a canceled context or a closed quit channel. This lives behind a new entry point, ExecuteSQLTransactionWithRetryConfig. The original ExecuteSQLTransactionWithRetry keeps its signature and delegates, since it is exported from a separately tagged module that downstream projects depend on. The kvdb backend, which holds our channel state, switches over to the time based budget. Surfacing a serialization error there is expensive: it fails the link that was trying to persist a state transition. With the backoff capped at a second, the old count of 50 attempts gave up after roughly 46 seconds, which isn't much of a contention burst to ride out. Blocking for the two minutes we now allow is strictly cheaper than losing the link. A longer budget is only safe if it can't hold up shutdown, and the context these backends are handed is not canceled until lnd's main function returns, which is far too late. So we thread the daemon's shutdown signal down to the kv backends and select on it in the backoff. Retries now stop the moment lnd starts shutting down, well inside systemd's default kill timeout. Note that the first attempt of every transaction is still made, it is only the retries that are abandoned. Finally, the per-attempt logic moves into its own closure, so the deferred rollback runs once per attempt instead of piling up until the whole loop returns.
In this commit, we swap a %v for a %w in AdvanceCommitChainTail. The neighbouring write in the same function already wrapped its error properly, so this was an oversight rather than a deliberate choice. It matters because this is the write that produced the "unable to restore remote unsigned local updates" error in issue lightningnetwork#10995. Formatting the underlying error with %v flattened it to a string, which meant the serialization error underneath was invisible to errors.Is and errors.As by the time it reached the link.
In this commit, we stop translating our own database problems into protocol errors on the wire. In issue lightningnetwork#10995, a postgres serialization failure surfaced while we were persisting an incoming revocation. The link failed with ErrInvalidRevocation, which is on the whitelist of failures we report to the peer, so we sent the peer an lnwire.Error saying "invalid revocation". The peer, following the spec, force closed the channel. The reporter found a dozen other occurrences of the same underlying failure where the peer happened to tolerate it: the link failed, both sides reconnected, the channel reestablish flow resynced cleanly, and nothing was lost. So the destructive outcome was entirely ours to avoid. We add a new ErrInternalDBError code, deliberately left off the ShouldSendToPeer whitelist, paired with a disconnect action and no force close. Every failure site in the upstream message handlers now runs its error through linkFailureForDBErr, which swaps in that failure when the error came from our own database and otherwise leaves the caller's failure untouched. The same treatment is applied to the paths that hang off those handlers and can hit disk: the forwarding package writes, the sphinx replay database, the invoice registry, and the commitment we sign in response. We give the channel reestablish error path the same treatment, since that's the flow we're now relying on to put both sides back in sync. A database error there used to land in the unspecified branch and ask the peer to recover a channel that was perfectly fine. The cooperative close path had the same shape of bug in a different place: it relayed the raw text of whatever error it hit straight to the peer in an lnwire.Error. It now sends a fixed generic string, following the pattern the funding manager already uses, and sends nothing at all when the error came from our own database. Failing a link this way is deliberately quiet, which on its own reads just like a peer that keeps flapping. So we also count these failures, and once a few of them land close together we say plainly that the database looks unhealthy. Note that this is logged at the error level rather than the critical level: in lnd a critical log requests a daemon shutdown, and tearing the node down over a contended database would be a worse outcome than the failure being reported. The kvdb layer avoids critical logs for this same class of error. Failing the link locally is a cheap outcome. We disconnect, reconnect, and the reestablish flow puts both sides back in sync. Losing the channel is not.
Roasbeef
force-pushed
the
db-err-link-failure
branch
from
July 27, 2026 21:01
953c230 to
9eea8c7
Compare
Roasbeef
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we fix the class of failure behind #10995, where a postgres serialization failure (SQLSTATE 40001) on the revocation path was reported to the peer as an invalid revocation, and the peer (CLN, following the spec) responded by force closing the channel. The reporter found a dozen other occurrences of the same underlying failure where the peer happened to tolerate the error: the link failed, both sides reconnected, and the channel reestablish flow resynced cleanly. So the destructive outcome was entirely ours to avoid: a local database hiccup should never be dressed up as a protocol violation on the wire.
Root cause
The failing chain had three links. First,
AdvanceCommitChainTailwrapped the database error with%vinstead of%w(the neighbouring write in the same function already used%w), flattening the typed serialization error into a plain string, which is why the raw postgres text ended up verbatim inside anlnwire.Error. Second, the transaction retry wrapper had two leak paths of its own: a context cancellation during backoff returned the bare serialization error, and an exhausted retry budget returned a nakedErrRetriesExceededthat discarded the underlying cause. Third, and most important, the link treated any error from the revocation and commitment paths as peer misbehavior, sending an error to the peer via theShouldSendToPeerwhitelist.The fix
We add a
sqldb.IsInternalDBErrorpredicate that answers the question the link actually cares about: was this error our own database's fault, or was it caused by the data we were handed? The predicate isn't limited to the SQL backends: it also matcheswalletdb.ErrDbNotOpen,walletdb.ErrTxNotWritable(which is what bbolt's write-path errors surface as through the walletdb layer), a wrappedENOSPC, dropped connections, and canceled queries, so a disk-full bbolt node gets the same protection as a contended postgres one. A newErrInternalDBErrorlink failure code is deliberately left off theShouldSendToPeerwhitelist, paired with a disconnect action and no force close: we recycle the connection and let the reestablish flow put both sides back in sync. Every failure site in the upstream message handlers now runs its error throughlinkFailureForDBErr, which swaps in the internal failure only when the error classifies as ours, and otherwise leaves the caller's failure untouched (a genuine invalid revocation still fails the channel exactly as before). The reestablish error path itself gets the same treatment, and the coop close path inpeerno longer puts raw error text on the wire at all: a fixed string goes out for genuine failures, and a database failure just recycles the connection.We also make the retry loop more patient, and honest about shutdown. Surfacing a serialization error on the channel state path is expensive (it fails the link mid state transition), so the old budget of 50 attempts, roughly 46 seconds with the backoff capped at a second, wasn't much of a contention burst to ride out. The loop is now bounded by a
RetryConfigwith independent attempt and wall clock budgets, and the kvdb backend switches to a 2 minute elapsed time budget. To keep that budget from holding shutdown hostage, the interceptor's shutdown channel is plumbed down through the database builder into the retry loop, so in-flight backoffs abort the moment shutdown begins rather than afterMainreturns. The oldExecuteSQLTransactionWithRetrysignature is preserved as a thin wrapper for downstream consumers of thesqldbmodule, with the config-based form alongside it.Since the new behavior is deliberately quiet on the wire, we add an operator-facing signal in its place: repeated DB-caused link failures within a short window escalate to a distinct log message naming the database as the likely culprit, so "my database is dying" and "my peer is flapping" are distinguishable from the logs. (Deliberately not
Criticalf, which lnd's shutdown logger treats as a daemon-stopping event; killing the node over database contention would be strictly worse than the bug being fixed.)A link level test drives a full commitment dance against a store that fails
AdvanceCommitChainTail(the exact write from the issue) and asserts the link fails without sending anything to the peer, without force closing, and with the disconnect action, plus a control case proving a non-database error still surfaces to the peer as before, and a drain assertion that nolnwire.Errororlnwire.Warningreaches the peer's message queue.Notes for reviewers:
kvdb/go.modgains areplacefor the localsqldbmodule sinceRetryConfigis a new symbol there, so the usual submodule tagging order applies at release time; andNewDefaultDatabaseBuilder/lncfg.DB.GetBackendsgained a shutdown-channel parameter, which downstreams that construct their ownImplementationCfg(e.g. litd) will need to thread through.See each commit message for a detailed description w.r.t the incremental changes.
Fixes #10995.