multi: add opt-in repeatable read isolation for Postgres write transactions

channeldb/channel.go

@@ -1545,11 +1545,29 @@ func syncNewChannel(tx kvdb.RwTx, c *OpenChannel, addrs []net.Addr,
 		return err
 	}
 
-	// If a LinkNode for this identity public key already exists,
-	// then we can exit early.
+	// If a LinkNode for this identity public key already exists, then we
+	// don't want to clobber the state that has accumulated for it (extra
+	// addresses, last seen time, etc). We do however re-write the existing
+	// record verbatim instead of skipping the write entirely.
+	//
+	// This idempotent re-put is what makes this transaction conflict with a
+	// concurrent transaction that prunes the very same link node (see
+	// ChannelStateDB.pruneLinkNode and MarkChanFullyClosed). Those prune
+	// paths read the peer's set of open channels and delete the link node
+	// when that set is empty. Were we to skip the write here, then under
+	// snapshot isolation both transactions could commit: the pruner would
+	// not see our new channel, and we would not see its deletion, leaving
+	// an open channel behind with no link node. By always touching the link
+	// node row, one of the two transactions is instead aborted with a
+	// retryable serialization error.
 	nodePub := c.IdentityPub.SerializeCompressed()
-	if nodeInfoBucket.Get(nodePub) != nil {
-		return nil
+	if existing := nodeInfoBucket.Get(nodePub); existing != nil {
+		// The returned slice may point directly into the database's
+		// memory, so we copy it before handing it back to Put.
+		linkNodeBytes := make([]byte, len(existing))
+		copy(linkNodeBytes, existing)
+
+		return nodeInfoBucket.Put(nodePub, linkNodeBytes)
 	}
 
 	// Next, we need to establish a (possibly) new LinkNode relationship

channeldb/db.go

@@ -1516,10 +1516,17 @@ func (c *ChannelStateDB) MarkChanFullyClosed(chanPoint *wire.OutPoint) error {
 			return nil
 		}
 
-		// If there are no open channels with this peer, prune the
-		// link node. We do this within the same transaction to avoid
-		// a race condition where a new channel could be opened
-		// between this check and the deletion.
+		// If there are no open channels with this peer, prune the link
+		// node. We do this within the same transaction as the read
+		// above so that a channel that is opened concurrently cannot
+		// slip in between the check and the deletion.
+		//
+		// NOTE: This is only safe because syncNewChannel always writes
+		// the peer's link node row, even when that row already exists.
+		// The write turns what would otherwise be a pair of
+		// transactions with disjoint write sets (write skew) into a
+		// same-row conflict, which the database reports as a retryable
+		// serialization failure.
 		log.Infof("Pruning link node %x with zero open "+
 			"channels from database",
 			remotePub.SerializeCompressed())
@@ -1541,13 +1548,17 @@ func (c *ChannelStateDB) MarkChanFullyClosed(chanPoint *wire.OutPoint) error {
 // channels exist. It will double-check within a write transaction to avoid a
 // race condition where a channel could be opened between the initial check
 // and the deletion.
+//
+// NOTE: The double-check below only rules out a concurrent channel open
+// because syncNewChannel always writes the peer's link node row, even when
+// that row already exists. Without that write the two transactions would have
+// disjoint write sets and could both commit under snapshot isolation
+// (REPEATABLE READ), leaving an open channel with no link node. See the
+// comment in syncNewChannel for the full argument.
 func (c *ChannelStateDB) pruneLinkNode(remotePub *btcec.PublicKey) error {
 	return kvdb.Update(c.backend, func(tx kvdb.RwTx) error {
 		// Double-check for open channels to avoid deleting a link node
 		// if a channel was opened since the caller's initial check.
-		//
-		// NOTE: This avoids a race condition where a channel could be
-		// opened between the initial check and the deletion.
 		openChannels, err := c.fetchOpenChannels(tx, remotePub)
 		if err != nil {
 			return err

channeldb/nodes_test.go

@@ -372,3 +372,140 @@ func TestCreateLinkNodes(t *testing.T) {
 	require.Equal(t, wire.MainNet, fetchedNode4.Network,
 		"node4 should have correct network")
 }
+
+// linkNodeWriteCounterTx wraps a kvdb.RwTx so that all writes to the top-level
+// link node bucket are counted.
+type linkNodeWriteCounterTx struct {
+	kvdb.RwTx
+
+	writes *int
+}
+
+// CreateTopLevelBucket returns a write counting bucket if the requested bucket
+// is the link node bucket, and otherwise defers to the wrapped transaction.
+func (t *linkNodeWriteCounterTx) CreateTopLevelBucket(
+	key []byte) (kvdb.RwBucket, error) {
+
+	bucket, err := t.RwTx.CreateTopLevelBucket(key)
+	if err != nil || !bytes.Equal(key, nodeInfoBucket) {
+		return bucket, err
+	}
+
+	return &linkNodeWriteCounterBucket{
+		RwBucket: bucket,
+		writes:   t.writes,
+	}, nil
+}
+
+// linkNodeWriteCounterBucket wraps a kvdb.RwBucket and counts the number of
+// values written to it.
+type linkNodeWriteCounterBucket struct {
+	kvdb.RwBucket
+
+	writes *int
+}
+
+// Put counts the write before deferring to the wrapped bucket.
+func (b *linkNodeWriteCounterBucket) Put(key, value []byte) error {
+	*b.writes++
+
+	return b.RwBucket.Put(key, value)
+}
+
+// TestSyncNewChannelWritesLinkNode tests that syncNewChannel always writes the
+// peer's link node row, even when a link node for that peer already exists. The
+// unconditional write is what causes a channel open to conflict with a
+// concurrent link node prune under snapshot isolation. The write must however
+// be a verbatim re-write, so that any state that has accumulated for the link
+// node (such as extra addresses) is left untouched.
+func TestSyncNewChannelWritesLinkNode(t *testing.T) {
+	t.Parallel()
+
+	fullDB, err := MakeTestDB(t)
+	require.NoError(t, err, "unable to make test database")
+
+	cdb := fullDB.ChannelStateDB()
+
+	channel := createTestChannelState(t, cdb)
+	pub := channel.IdentityPub
+
+	addr1, err := net.ResolveTCPAddr("tcp", "10.0.0.1:9000")
+	require.NoError(t, err, "unable to create test addr")
+	addr2, err := net.ResolveTCPAddr("tcp", "10.0.0.2:9000")
+	require.NoError(t, err, "unable to create test addr")
+
+	syncChannel := func(addrs ...net.Addr) int {
+		var writes int
+		err := kvdb.Update(cdb.backend, func(tx kvdb.RwTx) error {
+			countTx := &linkNodeWriteCounterTx{
+				RwTx:   tx,
+				writes: &writes,
+			}
+
+			return syncNewChannel(
+				countTx, channel, addrs, cdb.backend,
+			)
+		}, func() {
+			writes = 0
+		})
+		require.NoError(t, err, "unable to sync channel")
+
+		return writes
+	}
+
+	// rawLinkNode returns the bytes that are stored for the peer's link
+	// node, so that the record can be compared byte for byte.
+	rawLinkNode := func() []byte {
+		var raw []byte
+		err := kvdb.View(cdb.backend, func(tx kvdb.RTx) error {
+			bucket := tx.ReadBucket(nodeInfoBucket)
+			require.NotNil(t, bucket)
+
+			value := bucket.Get(pub.SerializeCompressed())
+			raw = make([]byte, len(value))
+			copy(raw, value)
+
+			return nil
+		}, func() {
+			raw = nil
+		})
+		require.NoError(t, err, "unable to read link node")
+
+		return raw
+	}
+
+	// The first sync creates the link node from scratch, which obviously
+	// writes the link node row.
+	require.Equal(t, 1, syncChannel(addr1))
+
+	linkNode, err := cdb.linkNodeDB.FetchLinkNode(pub)
+	require.NoError(t, err, "unable to fetch link node")
+	require.Len(t, linkNode.Addresses, 1)
+
+	// Accumulate some extra state for the link node that a naive re-write
+	// of the link node would clobber.
+	updated := NewLinkNode(
+		cdb.linkNodeDB, linkNode.Network, pub, addr1, addr2,
+	)
+	require.NoError(t, updated.Sync())
+
+	before := rawLinkNode()
+	require.NotEmpty(t, before)
+
+	// A second sync (of another channel with the same peer) must still
+	// write the link node row, but it must leave the existing record
+	// exactly as it was.
+	channel.FundingOutpoint.Index++
+	require.Equal(t, 1, syncChannel(addr1))
+
+	// The stored record must be byte for byte what it was before the sync.
+	// Nothing about the link node may be re-derived or re-serialized here,
+	// since the only reason for the write is the row conflict it creates.
+	require.Equal(t, before, rawLinkNode())
+
+	linkNode, err = cdb.linkNodeDB.FetchLinkNode(pub)
+	require.NoError(t, err, "unable to fetch link node")
+	require.Len(t, linkNode.Addresses, 2)
+	require.Equal(t, addr1.String(), linkNode.Addresses[0].String())
+	require.Equal(t, addr2.String(), linkNode.Addresses[1].String())
+}

docs/postgres.md

@@ -52,6 +52,56 @@ resource exhaustion in case LND experiencing high concurrent load:
 * `db.postgres.channeldb-with-global-lock=false` to run the channeldb_kv table
   with a single writer (default is false).
 
+## Transaction isolation
+
+`lnd` opens read-write transactions at the `SERIALIZABLE` isolation level by
+default. Read-only transactions are opened at `REPEATABLE READ`, which in
+Postgres is snapshot isolation: the transaction reads from a single consistent
+snapshot, taken when its first statement runs. Reads therefore acquire no
+`SIRead` predicate locks and take no part in Postgres' serializable snapshot
+isolation conflict graph, which substantially cuts the number of `40001`
+serialization failures that `lnd` and its concurrent writers have to retry
+through.
+
+Operators should be aware that some of `lnd`'s read transactions are long
+lived. Loading the graph cache at startup and each `GraphSession` used for
+pathfinding hold a read transaction open for their full duration, and each
+holds its snapshot for that whole time. Two consequences follow. First,
+Postgres cannot vacuum row versions that are still visible to an open snapshot,
+so a very slow or stuck read transaction delays cleanup and can bloat tables.
+Second, such a session sits in the `idle in transaction` state whenever `lnd`
+is computing between queries, so if `idle_in_transaction_session_timeout` is
+configured it must be generous enough to cover a full pathfinding pass or
+Postgres will terminate the transaction mid-flight. The same caution applies to
+`statement_timeout` for the individual queries these transactions run.
+
+The default for read-write transactions can be relaxed with:
+
+* `db.postgres.tx-isolation=repeatable-read` to also run read-write
+  transactions at `REPEATABLE READ` (default is `serializable`).
+
+**This option is experimental and opt-in.** Under `SERIALIZABLE`, Postgres
+detects and aborts any pair of transactions whose interleaving is not
+equivalent to running them one after the other. That safety costs a lot of
+aborted transactions on a busy node, each of which `lnd` has to retry.
+
+`REPEATABLE READ` on Postgres is snapshot isolation. It still rules out dirty
+reads, non-repeatable reads, phantom reads and lost updates: a transaction that
+writes a row that another in-flight transaction has already written is aborted
+with a serialization failure. The one anomaly it permits is *write skew*, where
+two transactions each read what the other writes but write to disjoint sets of
+rows, so neither of them conflicts and both are allowed to commit.
+
+The write paths that were known to be exposed to write skew have been changed
+to either conflict on a shared row or to serialize in process, which is what
+makes this option safe enough to offer at all. It nonetheless stays off by
+default until it has accumulated soak time on real nodes. If you turn it on and
+observe database inconsistencies, set it back to `serializable` and open an
+issue.
+
+The option has no effect on SQLite, which only ever admits a single writer and
+so is always effectively serializable.
+
 ## Important note about replication
 
 In case a replication architecture is planned, streaming replication should be avoided, as the master does not verify the replica is indeed identical, but it will only forward the edits queue, and let the slave catch up autonomously; synchronous mode, albeit slower, is paramount for `lnd` data integrity across the copies, as it will finalize writes only after the slave confirmed successful replication.

docs/release-notes/release-notes-0.22.0.md

@@ -55,6 +55,13 @@
   the reported network statistics such as total network capacity, channel
   count and max out degree.
 
+* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/10998) in the
+  watchtower client where a retried `AckUpdate` transaction could commit an
+  acknowledgment everywhere except in the acked-update index it belongs in,
+  after which the client would consider a state backed up that the tower had
+  never been told about. Only the SQL backends could retry a transaction, so
+  `bbolt` was never affected.
+
 # New Features
 
 ## Functional Enhancements
@@ -96,6 +103,23 @@
 
 ## Performance Improvements
 
+* [Read-only Postgres transactions now run at `REPEATABLE READ` instead of
+  `SERIALIZABLE`](https://github.com/lightningnetwork/lnd/pull/10997). In
+  Postgres that is snapshot isolation: a read-only transaction still reads from
+  a single consistent snapshot for its whole lifetime, taken when its first
+  statement runs. That snapshot is no longer guaranteed to correspond to a
+  serial ordering of the writers running alongside it, which is acceptable
+  because `lnd`'s read paths only consume a point-in-time view and never
+  depended on being ordered against writers in other transactions. In exchange,
+  such a transaction takes no part in Postgres' serializable snapshot isolation
+  conflict graph: it acquires no `SIRead` predicate locks, is not itself subject
+  to SSI serialization failures, and can no longer cause a concurrent writer to
+  be aborted as a pivot. Since `lnd` is very read heavy, this removes a large
+  amount of needless abort pressure. Read-write transactions are unaffected and
+  remain `SERIALIZABLE`, and the SQLite backend is untouched. See
+  [docs/postgres.md](../postgres.md) for the operator-facing note on long-lived
+  read transactions.
+
 ## Deprecations
 
 # Technical and Architectural Updates
@@ -138,6 +162,63 @@
 
 ## Database
 
+* [Four database write paths were hardened against snapshot
+  isolation](https://github.com/lightningnetwork/lnd/pull/10998), preparing for
+  read-write Postgres transactions to move from `SERIALIZABLE` to `REPEATABLE
+  READ`, the way [read-only transactions already
+  did](https://github.com/lightningnetwork/lnd/pull/10997). Under snapshot
+  isolation a pair of transactions that each read what the other writes, but
+  whose write sets don't overlap, both commit rather than one of them being
+  aborted, so each of these paths was changed to conflict on a shared row or to
+  serialize in process instead:
+
+  * A channel open now always writes the peer's link node row, so that it can't
+    race a link node prune that runs when the peer's last channel is closed.
+
+  * `PruneGraphNodes` now takes the cache mutex in both graph stores, like every
+    other graph mutator does, so that a node prune can't interleave with a
+    channel edge being added for that node.
+
+  * Bucket creation in the SQL kvdb backends is now phrased as an upsert, so
+    that two transactions racing to create the same bucket see a retryable
+    serialization failure rather than a unique constraint violation, which is
+    not retried.
+
+  * The watchtower client now evaluates a session for closability when it acks
+    an update for a channel that has already been closed, and the channel close
+    and ack paths conflict on a shared row. This also fixes a pre-existing leak
+    where a session that acked its first update for a channel only after that
+    channel was closed would never be marked closable, and so would hold on to
+    the tower's storage forever.
+
+* [Read-write Postgres transactions can now optionally be run at `REPEATABLE
+  READ`](https://github.com/lightningnetwork/lnd/pull/10999) via the new
+  `db.postgres.tx-isolation` option, which accepts `serializable` (the default)
+  and `repeatable-read`. This is the final piece of the work that
+  [moved read-only transactions to `REPEATABLE
+  READ`](https://github.com/lightningnetwork/lnd/pull/10997) and then [hardened
+  the write paths that snapshot isolation
+  exposes](https://github.com/lightningnetwork/lnd/pull/10998); both of those
+  are prerequisites for it.
+
+  Under `SERIALIZABLE`, Postgres aborts any pair of transactions whose
+  interleaving isn't equivalent to running them one after the other, and on a
+  busy node that costs a lot of retries. `REPEATABLE READ` is snapshot
+  isolation, which still rules out dirty reads, non-repeatable reads, phantom
+  reads and lost updates, and leaves only write skew on the table. The write
+  paths known to be exposed to write skew were hardened in the PR above.
+
+  **The option is experimental and stays off by default** until it has
+  accumulated soak time on real nodes. See `docs/postgres.md` before enabling
+  it.
+
+* [`KVStore.DeleteNode` now takes the graph store's cache
+  mutex](https://github.com/lightningnetwork/lnd/pull/10999) like every other
+  graph mutator, so that a node deletion can't interleave with a channel edge
+  being added for that node. The method is currently only reachable from tests,
+  so this isn't a live bug, but it's the same shape as the `PruneGraphNodes`
+  fix above.
+
 ## Code Health
 
 ## Tooling and Documentation