multi: harden write paths for snapshot isolation - #10998
Open
Roasbeef wants to merge 10 commits into
Open
Conversation
In this commit, we relax the isolation level used for read-only transactions on the shared SQL kvdb backend when it's backed by Postgres. Read-write transactions are untouched and remain SERIALIZABLE. lnd currently opens every transaction at SERIALIZABLE. Under Postgres' serializable snapshot isolation, even a read-only transaction takes SIRead predicate locks and fully participates in the serialization conflict graph, so it can both suffer a 40001 abort and cause one for a concurrent writer by acting as a pivot. Since lnd is extremely read heavy, that adds up to a lot of needless abort pressure. REPEATABLE READ in Postgres is snapshot isolation. A read-only transaction still reads from a single consistent snapshot for its whole lifetime, taken when its first statement runs rather than at BEGIN. This is a genuine, if modest, weakening: that snapshot is no longer guaranteed to correspond to a serial ordering of the writers running alongside it, so the read-only transaction anomaly of Fekete and O'Neil becomes possible again. We're fine with that, because these read paths only ever consume a point-in-time view of the database, much like bbolt's View transactions do, and a read feeding a later write in a separate transaction was never protected across that boundary at any isolation level. What we get in return is that the transaction takes no part in the SSI conflict graph at all: no SIRead predicate locks, no exposure to SSI serialization failures, and no chance of aborting a concurrent writer as a pivot. The sqlbase package is shared with the SQLite backend, so we gate this on the driver name. SQLite is always effectively serializable and its driver ignores the requested isolation level entirely, so there's nothing to gain there.
In this commit, we apply the same isolation level change to the native SQL backend that the previous commit applied to the kvdb SQL backend: read-only transactions on Postgres now run at REPEATABLE READ, while read-write transactions stay SERIALIZABLE. A read-only transaction still reads from a single consistent snapshot for its whole lifetime, taken at its first statement. It gives up the guarantee that the snapshot corresponds to a serial ordering of the concurrent writers, which these read paths never relied on, and in return stops participating in Postgres' serializable snapshot isolation conflict graph: no SIRead predicate locks, no exposure to SSI serialization failures, and no aborting a concurrent writer as a pivot. BaseDB is also used for SQLite, so it needs to know which backend it's talking to. We add a BackendType to BaseDB mirroring the one that sqldb/v2 already carries, and set it at the two construction sites.
In this commit, we mirror the isolation level change into sqldb/v2. BaseDB already carries a BackendType here, so we can gate directly on it: read-only transactions on Postgres are opened at REPEATABLE READ, everything else stays SERIALIZABLE.
In this commit, we add the release notes entry for the isolation level change, along with an operator facing note in docs/postgres.md. The latter spells out that reads now run under snapshot isolation, and warns that lnd holds some read transactions open for a long time. The graph cache load and each pathfinding GraphSession keep a snapshot pinned for their whole duration, which is worth knowing before setting idle_in_transaction_session_timeout or statement_timeout.
In this commit, we make syncNewChannel write the peer's link node row unconditionally, rather than returning early when a link node already exists for that peer. The link node prune paths (MarkChanFullyClosed and pruneLinkNode) read the peer's set of open channels and, when that set is empty, delete the peer's link node. A concurrent channel open reads the link node row, sees that it's already there, and skips the write. Each transaction reads what the other writes, but their write sets are disjoint, which is the classic write skew shape: under snapshot isolation both transactions commit, and we're left with an open channel whose peer has no link node. Serializable isolation catches this today, but we're preparing to run write transactions at repeatable read, so instead we make the two transactions collide on the same row. The write is a verbatim re-write of the existing record, so any state that has accumulated for the link node (extra addresses, last seen time) is preserved exactly, and the only observable difference is that one of the two racing transactions now aborts with a retryable serialization error. We also update the isolation assumptions spelled out in the comments of the two prune paths.
In this commit, we make PruneGraphNodes acquire the store's cache mutex before it opens its write transaction. It was the only mutator of the graph that didn't, in either of the two store implementations, while all of its siblings already do. That mutex does more than guard the reject and channel caches: it's also the in-process serialization point against the batched channel edge insertion path. addChannelEdge reads the row of each node an edge attaches to, but doesn't write it. PruneGraphNodes range scans the node bucket and then deletes the nodes it finds to be unreferenced. With no mutual exclusion, the two can interleave such that the prune deletes a node that the edge being added still references, which leaves a dangling edge in the graph. Under serializable isolation the database would abort one of the two, but under snapshot isolation their write sets are disjoint and both commit. The lock ordering here is cacheMu -> DB, which is what the rest of both stores already does. The in-memory graph cache is updated by the ChannelGraph wrapper once the store call returns, exactly as it is for PruneGraph, so nothing further is needed here.
In this commit, we rework the bucket creation path of the SQL kvdb backends so that two transactions racing to create the same bucket no longer leave the loser with a unique constraint violation. Both CreateBucket and CreateBucketIfNotExists used to select the row for the key, and then, if nothing came back, run a bare insert. Under serializable isolation that's fine: the select takes a predicate lock on the index page, so the concurrent insert of the same key shows up as a read/write conflict and the loser is aborted with a retryable serialization failure. Under repeatable read there is no predicate lock, so the loser instead runs head first into the unique index and gets back a 23505 unique violation. MapSQLError turns that into ErrSQLUniqueConstraintViolation, which ExecuteSQLTransactionWithRetry does not retry, so it would be handed straight back to the caller. Since CreateTopLevelBucket delegates here, that's a hard error on paths as central as opening a channel. We instead phrase the insert as an upsert whose update is a no-op. The conflict is then reported as a serialization failure at both isolation levels, which the retry loop knows how to handle: on the retry, the select at the top finds the winner's row. The conflict target of each statement has to line up with the partial unique index that covers the row being inserted, of which there are two, one for top level rows and one for nested rows, so we keep the two flavours of the statement apart just like Put already does. The walletdb semantics are unchanged: CreateBucketIfNotExists returns the existing bucket, CreateBucket returns ErrBucketExists when the bucket is already there (including when it lost the race, since the retry then finds it), and both return ErrIncompatibleValue when the key holds a value rather than a bucket. Two tests are added against the embedded postgres fixture. The first races several transactions creating the same bucket through the real code path and asserts that they all succeed with a single row created. The second races the raw statements at both serializable and repeatable read, and pins down the failure modes: the bare insert yields 23505 under repeatable read, while the upsert yields 40001 everywhere.
In this commit, we close a hole in the bookkeeping that decides when a
watchtower client session may be deleted. A session is only ever deleted
once it has been found closable, and closability is only ever evaluated
in MarkChannelClosed, which walks the set of sessions that have acked an
update for the channel being closed. A session joins that set the first
time it acks an update for the channel, from its own sessionQueue
goroutine, which shares no lock with the channel close path.
So a session that acks its first update for a channel after that channel
was closed is never evaluated for it. If that channel was the last open
channel of the session, then nothing will ever mark the session closable
and it holds on to the tower's storage forever. This is reachable today
purely on ordering, without any concurrency involved.
We fix it by evaluating the session in AckUpdate itself whenever the
channel it acked an update for is already closed. Like the rogue update
path right above it, this only records the session in the closable
sessions bucket, so it is picked up by the closable session handler on
the next startup rather than being handed to the caller. The evaluation
is best effort: an AckUpdate that fails takes down the session queue
that issued it, and the ack itself must be persisted no matter what, so
a failure to work out whether the session has become closable is logged
rather than returned.
The rogue update branch has the same hole, and it isn't enough there to
only consider a session when the rogue count saturates it. A rogue ack
that exhausts a session whose other channels have all been closed makes
that session closable just as well, so we let that branch fall through
to the same evaluation, which subsumes the marking it did inline.
That leaves the concurrent version of the same problem. The channel's
set of sessions is only read by MarkChannelClosed and only added to by
AckUpdate, and the closed height is the other way around, so the two
transactions read what the other writes while writing disjoint rows.
That's write skew: serializable isolation aborts one of them, but under
snapshot isolation both commit, each having assumed the other would
evaluate the session. The same shape exists one level down, between
MarkChannelClosed's evaluation of a session and the concurrent
CommitUpdate or AckUpdate that changes the very state that evaluation
reads. There the consequence is worse than a leak: a session could be
marked closable off a view that has already been invalidated, and then
deleted while it still has updates to make good on.
Both are turned into same-row conflicts by writing a row back with the
value it already holds, which puts a row that a transaction would
otherwise only read into its write set:
- AckUpdate and MarkChannelClosed both write the channel's db-ID row,
so a close can't race the addition of a session to the channel.
- MarkChannelClosed writes the body row of every session it evaluates,
which is the row that CommitUpdate and AckUpdate both write, so an
evaluation can't race a change to the session it is evaluating.
The loser of either race is aborted with a retryable serialization
error, and on the retry it observes the state the winner left behind:
AckUpdate then sees the channel as closed and evaluates its own session,
while MarkChannelClosed sees the new session in the channel's set.
Making retries of AckUpdate more likely brings a pre-existing problem
with it, which we fix here as well. RangeIndex.Add applies its changes
to the in-memory range index as soon as the key/value changes have been
staged, but that index is cached on the ClientDB and outlives the
transaction. A retry on top of an index that a rolled back attempt has
already mutated finds the height covered, stages nothing, and commits an
ack that made it into every part of the database except the range index
it belongs in, after which IsAcked reports an update as backed up that
isn't. AckUpdate now drops the range index it dirtied before each retry
and after giving up for good, so the next reader loads it back from the
database.
The invariant the two of them now maintain together is spelled out on
MarkChannelClosed.
In this commit, we fold the Detail field of a Postgres error into the error we hand back from MapSQLError, but only for the errors that report a transaction conflict. The error string that the driver builds leaves that field out, which is unfortunate because for a conflict it is the only thing that says which kind of conflict was actually hit. A plain write-write conflict reports a concurrent update, whereas an abort that only serializable isolation raises spells out the reason code of the pivot that was cancelled. Being able to tell the two apart in the logs tells an operator how much of their abort pressure would go away by running write transactions at repeatable read, and how much of it is real contention that would remain. The detail of any other error is deliberately left alone. For a constraint violation in particular, Postgres spells out the offending column values, and in our schemas that means payment session keys, invoice hashes and raw key/value bucket keys. None of that belongs in a log line, let alone in an error that may travel back over an RPC. The error parsing exists twice in each module, once for the builds that have SQLite and once for the ones that don't, so the helper is put in a file of its own that carries no build tags rather than being written out twice. While here, the copy that predates SQLite is brought back in line with its twin, which it had drifted from: it did not classify a failed SQL transaction or a detected deadlock as a serialization error at all, so neither would have been retried on the platforms that use it.
In this commit, we add a release notes entry for the four write paths that were hardened so that read-write Postgres transactions can move to repeatable read, which is the other half of the change that already put read-only transactions there in lightningnetwork#10997.
Roasbeef
force-pushed
the
rr-write-hardening
branch
from
July 27, 2026 20:48
e54821e to
af9b296
Compare
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
1 participant
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 harden the handful of write paths that a full audit of lnd's database transaction call sites flagged as unsafe under snapshot isolation, in preparation for eventually running write transactions at REPEATABLE READ on Postgres. This builds on #10997 (which moves read-only transactions to REPEATABLE READ), so only the last four commits are new here until that PR lands.
The audit classified roughly 340 write and read transaction closures across the codebase. Nearly all of them are already safe under snapshot isolation: their invariants are protected by same-row write conflicts (which still abort with a retryable 40001 under REPEATABLE READ), by in-process mutexes, or by unique constraints. Four shapes survived adversarial review, and each gets a targeted fix here. Every fix is correct and non-regressive at the current SERIALIZABLE level too, so this PR can land on its own merits.
The fixes
In
channeldb,syncNewChannelused to return early when the peer's link node already existed, reading a row it never wrote. A concurrentMarkChanFullyCloseddeleting that link node would leave an open channel with no link node under snapshot isolation. We now re-put the existing bytes verbatim, so the row joins the write set and the two transactions conflict on it.In
graph/db,PruneGraphNodeswas the only graph mutator that didn't takecacheMubefore its transaction, so its range-scan-then-delete could interleave with a concurrentaddChannelEdgeand leave an edge referencing a deleted node. It now takes the mutex like its siblings.In
kvdb/sqlbase, bucket creation was a select followed by a bare insert. Losing a creation race today produces a retryable 40001 (the select's predicate lock turns the duplicate into a rw-conflict), but under REPEATABLE READ it produces a 23505 unique violation instead, which the retry loop correctly refuses to retry, so it would surface as a hard error on the channel open path. The insert is now phrased as a no-op upsert whose conflict targets match the two partial unique indexes exactly, which the database reports as a retryable serialization failure in both isolation levels. A new Postgres concurrency test pins down all four combinations (bare insert vs upsert, SERIALIZABLE vs REPEATABLE READ) empirically.In
wtdb, we found and fixed a session leak that exists today even under SERIALIZABLE: a session that acks its first update for a channel after that channel was marked closed is simply never evaluated for closability, and if that was its last channel it holds tower storage forever.AckUpdatenow evaluates its own session when the channel is already closed. The concurrent variant of the same hole (genuine write skew betweenAckUpdateandMarkChannelClosed, including a session-level variant where a session could be deleted while still owing updates) is closed by having both sides touch a shared row, so one of them retries and observes the other. The touches re-write byte-identical values, so DB dumps are unaffected.The last commit surfaces
PgError.Detailon classified Postgres errors, since today an operator cannot tell an SSI-only abort (detail mentions a pivot or rw-conflict) apart from an ordinary write-write conflict (detail says concurrent update) in any lnd log. That's exactly the signal that tells us which paths still rely on SERIALIZABLE, and it's a separable commit if reviewers would rather drop it.With this PR and #10997 in, the remaining step for the isolation work is flipping write transactions to REPEATABLE READ behind a config option, which we'll propose separately once these have had soak time.
See each commit message for a detailed description w.r.t the incremental changes.
Related to #10995.