Merge pull request #10780 from ziggie1984/channeldb-tombstone-close

channeldb: tombstone closed channels on KV-SQL backends

Author: ziggie1984

channeldb/channel.go

@@ -418,7 +418,8 @@ func CreateWithBackend(backend kvdb.Backend, modifiers ...OptionModifier) (*DB,
 			linkNodeDB: &LinkNodeDB{
 				backend: backend,
 			},
-			backend: backend,
+			backend:                 backend,
+			tombstoneClosedChannels: opts.tombstoneClosedChannels,
 		},
 		clock:                     opts.clock,
 		dryRun:                    opts.dryRun,
@@ -548,6 +549,12 @@ type ChannelStateDB struct {
 	// backend points to the actual backend holding the channel state
 	// database. This may be a real backend or a cache middleware.
 	backend kvdb.Backend
+
+	// tombstoneClosedChannels is set by OptionTombstoneClosedChannels.
+	// When true, CloseChannel skips deleting nested per-channel state and
+	// relies on the outpointBucket flip to outpointClosed as the
+	// authoritative closed-channel signal.
+	tombstoneClosedChannels bool
 }
 
 // GetParentDB returns the "main" channeldb.DB object that is the owner of this
@@ -621,7 +628,7 @@ func (c *ChannelStateDB) fetchOpenChannels(tx kvdb.RTx,
 
 		// Finally, we both of the necessary buckets retrieved, fetch
 		// all the active channels related to this node.
-		nodeChannels, err := c.fetchNodeChannels(chainBucket)
+		nodeChannels, err := c.fetchNodeChannels(tx, chainBucket)
 		if err != nil {
 			return fmt.Errorf("unable to read channel for "+
 				"chain_hash=%x, node_key=%x: %v",
@@ -637,12 +644,19 @@ func (c *ChannelStateDB) fetchOpenChannels(tx kvdb.RTx,
 
 // fetchNodeChannels retrieves all active channels from the target chainBucket
 // which is under a node's dedicated channel bucket. This function is typically
-// used to fetch all the active channels related to a particular node.
-func (c *ChannelStateDB) fetchNodeChannels(chainBucket kvdb.RBucket) (
-	[]*OpenChannel, error) {
+// used to fetch all the active channels related to a particular node. Channels
+// already flipped to outpointClosed in the outpoint index are skipped silently
+// — readers see only channels that are still considered open.
+func (c *ChannelStateDB) fetchNodeChannels(tx kvdb.RTx,
+	chainBucket kvdb.RBucket) ([]*OpenChannel, error) {
 
 	var channels []*OpenChannel
 
+	// Hoist the outpoint-bucket lookup so the closed-channel check inside
+	// the loop is a per-iteration map probe rather than a tx-level bucket
+	// resolve.
+	opBucket := tx.ReadBucket(outpointBucket)
+
 	// A node may have channels on several chains, so for each known chain,
 	// we'll extract all the channels.
 	err := chainBucket.ForEach(func(chanPoint, v []byte) error {
@@ -651,12 +665,24 @@ func (c *ChannelStateDB) fetchNodeChannels(chainBucket kvdb.RBucket) (
 			return nil
 		}
 
+		// Skip already-closed channels. The chanBucket still exists
+		// on disk on tombstone-enabled backends; the outpoint flip is
+		// the sole signal that the channel should be treated as
+		// closed.
+		isClosed, err := isOutpointClosed(opBucket, chanPoint)
+		if err != nil {
+			return err
+		}
+		if isClosed {
+			return nil
+		}
+
 		// Once we've found a valid channel bucket, we'll extract it
 		// from the node's chain bucket.
 		chanBucket := chainBucket.NestedReadBucket(chanPoint)
 
 		var outPoint wire.OutPoint
-		err := graphdb.ReadOutpoint(
+		err = graphdb.ReadOutpoint(
 			bytes.NewReader(chanPoint), &outPoint,
 		)
 		if err != nil {
@@ -771,6 +797,11 @@ func (c *ChannelStateDB) FetchPermAndTempPeers(
 			return ErrNoChanDBExists
 		}
 
+		// Hoist the outpoint-bucket lookup so the closed-channel check
+		// inside the nested chainBucket.ForEach below is a per-channel
+		// map probe rather than a tx-level bucket resolve.
+		opBucket := tx.ReadBucket(outpointBucket)
+
 		openChanErr := openChanBucket.ForEach(func(nodePub,
 			v []byte) error {
 
@@ -804,6 +835,22 @@ func (c *ChannelStateDB) FetchPermAndTempPeers(
 					return nil
 				}
 
+				// Skip already-closed channels: they are
+				// logically closed even though their
+				// per-channel state still resides under
+				// chainBucket. The closed peer's protected
+				// status is established below via the
+				// historical-channel scan.
+				isClosed, err := isOutpointClosed(
+					opBucket, chanPoint,
+				)
+				if err != nil {
+					return err
+				}
+				if isClosed {
+					return nil
+				}
+
 				chanBucket := chainBucket.NestedReadBucket(
 					chanPoint,
 				)
@@ -976,6 +1023,11 @@ func (c *ChannelStateDB) channelScanner(tx kvdb.RTx,
 			return ErrNoActiveChannels
 		}
 
+		// Hoist the outpoint-bucket lookup so the closed-channel
+		// check inside the per-chain ForEach below pays one tx-level
+		// bucket resolve total instead of one per visited chanKey.
+		opBucket := tx.ReadBucket(outpointBucket)
+
 		// Within the node channel bucket, are the set of node pubkeys
 		// we have channels with, we don't know the entire set, so we'll
 		// check them all.
@@ -1024,6 +1076,19 @@ func (c *ChannelStateDB) channelScanner(tx kvdb.RTx,
 					return err
 				}
 
+				// An already-closed channel is logically gone
+				// and must not be surfaced by lookup-style
+				// scans.
+				isClosed, err := isOutpointClosed(
+					opBucket, targetChanBytes,
+				)
+				if err != nil {
+					return err
+				}
+				if isClosed {
+					return nil
+				}
+
 				chanBucket := chainBucket.NestedReadBucket(
 					targetChanBytes,
 				)
@@ -1187,7 +1252,9 @@ func fetchChannels(c *ChannelStateDB, filters ...fetchChannelsFilter) (
 						"bucket for chain=%x", chainHash[:])
 				}
 
-				nodeChans, err := c.fetchNodeChannels(chainBucket)
+				nodeChans, err := c.fetchNodeChannels(
+					tx, chainBucket,
+				)
 				if err != nil {
 					return fmt.Errorf("unable to read "+
 						"channel for chain_hash=%x, "+

channeldb/close_channel_test.go

@@ -71,6 +71,16 @@ type Options struct {
 	// storeFinalHtlcResolutions determines whether to persistently store
 	// the final resolution of incoming htlcs.
 	storeFinalHtlcResolutions bool
+
+	// tombstoneClosedChannels, when true, instructs CloseChannel to skip
+	// the cascading deletion of nested per-channel state and rely on the
+	// outpoint-index flip to mark the channel as closed. KV-over-SQL
+	// backends (sqlite, postgres) opt in because nested-bucket deletes
+	// inside a write transaction translate into a long-running
+	// ON DELETE CASCADE that holds the database write-lock for many
+	// seconds on long-lived channels. bbolt and etcd leave this off; the
+	// synchronous delete is already cheap there.
+	tombstoneClosedChannels bool
 }
 
 // DefaultOptions returns an Options populated with default values.
@@ -151,3 +161,14 @@ func OptionGcDecayedLog(noGc bool) OptionModifier {
 		o.OptionalMiragtionConfig.MigrationFlags[1] = !noGc
 	}
 }
+
+// OptionTombstoneClosedChannels controls whether CloseChannel skips the
+// cascading deletion of nested per-channel state and relies on the
+// outpoint-index flip to mark the channel as closed. Set this to true on
+// KV-over-SQL backends (sqlite, postgres); leave it false for bbolt and
+// etcd.
+func OptionTombstoneClosedChannels(enabled bool) OptionModifier {
+	return func(o *Options) {
+		o.tombstoneClosedChannels = enabled
+	}
+}

channeldb/db.go

@@ -1072,6 +1072,15 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		)
 	}
 
+	// KV-over-SQL backends (sqlite, postgres) opt in to closing channels
+	// via tombstone markers because nested-bucket deletes inside a write
+	// transaction translate into a long-running ON DELETE CASCADE on the
+	// kvdb-on-SQL schema, holding the database write-lock for many seconds
+	// on long-lived channels. bbolt and etcd keep the synchronous one-shot
+	// close path, where nested-bucket deletion is already cheap.
+	tombstoneClosedChans := cfg.DB.Backend == lncfg.SqliteBackend ||
+		cfg.DB.Backend == lncfg.PostgresBackend
+
 	dbOptions := []channeldb.OptionModifier{
 		channeldb.OptionDryRunMigration(cfg.DryRunMigration),
 		channeldb.OptionStoreFinalHtlcResolutions(
@@ -1081,6 +1090,7 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		channeldb.OptionNoRevLogAmtData(cfg.DB.NoRevLogAmtData),
 		channeldb.OptionGcDecayedLog(cfg.DB.NoGcDecayedLog),
 		channeldb.OptionWithDecayedLogDB(dbs.DecayedLogDB),
+		channeldb.OptionTombstoneClosedChannels(tombstoneClosedChans),
 	}
 
 	// Otherwise, we'll open two instances, one for the state we only need

channeldb/options.go

@@ -296,6 +296,37 @@
     public-only path sees a ~42% speedup; on the previous code it could stall
     for minutes.
 
+* [Tombstone closed channels on KV-over-SQL
+  backends](https://github.com/lightningnetwork/lnd/pull/10780). Closing a
+  long-lived channel previously issued a single `DeleteNestedBucket` inside
+  the close transaction. On the kvdb-on-SQL schema (sqlite, postgres) that
+  delete fans out into a row-by-row `ON DELETE CASCADE` over the channel's
+  revocation log and forwarding-package bucket, holding the database
+  write-lock for many seconds — long enough on channels with millions of
+  states to stall HTLC forwarding, time out htlcswitch retries, and trigger
+  force-close cycles. `CloseChannel` now skips the cascading delete on
+  these backends; the outpoint-index flip from `outpointOpen` to
+  `outpointClosed` (already performed by the existing close path) is the
+  authoritative closed-channel marker, and every reader of the open-channel
+  bucket consults it before treating a channel as open. The bulk historical
+  state — the chanBucket itself, the revocation log, and the per-channel
+  forwarding-package bucket — remains on disk for the channel's lifetime in
+  this database and is reclaimed wholesale by the upcoming native-SQL
+  channel-state migration. bbolt and etcd retain the synchronous one-shot
+  close path, where nested-bucket deletion is already cheap.
+
+  > ⚠️ **Downgrade warning.** On sqlite/postgres, once a channel is
+  > closed under this build the chanBucket and its nested state remain
+  > on disk; the close is signalled only by the `outpointClosed` flip
+  > in the outpoint index. Earlier `lnd` releases do not consult that
+  > flip when iterating `openChannelBucket`, so downgrading to a
+  > pre-0.21 binary after closing channels on these backends will
+  > resurrect those channels as open in `listchannels`,
+  > `pendingchannels`, and the chain-watch path. Operators who close
+  > channels on sqlite/postgres after upgrading should treat the
+  > upgrade as one-way for that database; bbolt and etcd users are unaffected 
+  > because the close path on those backends still deletes the chanBucket.
+
 ## Deprecations
 
 ### ⚠️ **Warning:** Deprecated fields in `lnrpc.Hop` will be removed in release version **0.22**

config_builder.go

@@ -85,13 +85,28 @@ func testWipeForwardingPackages(ht *lntest.HarnessTest) {
 	// close channel should now become pending force closed channel.
 	pendingAB = ht.AssertChannelPendingForceClose(bob, chanPointAB).Channel
 
-	// Check the forwarding pacakges are deleted.
-	require.Zero(ht, pendingAB.NumForwardingPackages)
-
-	// For Alice, the forwarding packages should have been wiped too.
-	pending := ht.AssertChannelPendingForceClose(alice, chanPointAB)
-	pendingAB = pending.Channel
-	require.Zero(ht, pendingAB.NumForwardingPackages)
+	// On backends that close channels via tombstone markers (sqlite,
+	// postgres), the per-channel forwarding-package bucket is left on
+	// disk by design — the synchronous close path's nested-bucket
+	// delete is exactly what tombstoning avoids. The bytes are reclaimed
+	// by the upcoming native-SQL channel-state migration. The unit-test
+	// suite in channeldb covers the tombstone semantics directly, so
+	// here we just skip the post-close fwd-pkg assertions on those
+	// backends while still exercising the rest of the close flow for
+	// backend symmetry.
+	if !ht.UsesClosedChanTombstones() {
+		require.Zero(ht, pendingAB.NumForwardingPackages)
+
+		// For Alice, the forwarding packages should have been wiped
+		// too.
+		pending := ht.AssertChannelPendingForceClose(alice, chanPointAB)
+		pendingAB = pending.Channel
+		require.Zero(ht, pendingAB.NumForwardingPackages)
+	} else {
+		// Still drive Alice's pending-force-close lookup so the rest
+		// of the test stays backend-symmetric.
+		ht.AssertChannelPendingForceClose(alice, chanPointAB)
+	}
 
 	// Alice should one pending sweep.
 	ht.AssertNumPendingSweeps(alice, 1)

docs/release-notes/release-notes-0.21.0.md

@@ -1444,6 +1444,18 @@ func (h *HarnessTest) IsPostgresBackend() bool {
 	return h.manager.dbBackend == node.BackendPostgres
 }
 
+// UsesClosedChanTombstones reports whether the test harness's database
+// backend closes channels via tombstone markers rather than cascading the
+// nested-bucket delete. This is true on the KV-over-SQL backends (sqlite,
+// postgres) and false on bbolt. Tests that observe forwarding-package or
+// revocation-log deletion immediately after a channel close should consult
+// this predicate; on tombstone backends the bulk state remains on disk
+// until the upcoming native-SQL channel-state migration reclaims it.
+func (h *HarnessTest) UsesClosedChanTombstones() bool {
+	return h.manager.dbBackend == node.BackendSqlite ||
+		h.manager.dbBackend == node.BackendPostgres
+}
+
 // fundCoins attempts to send amt satoshis from the internal mining node to the
 // targeted lightning node. The confirmed boolean indicates whether the
 // transaction that pays to the target should confirm. For neutrino backend,