multi: make deletion of edge atomic.

When the option SCID is used we need to make sure we update the
channel data in an atomic way so that we don't miss any updates
by our peer.

Author: ziggie1984

funding/manager.go

@@ -530,10 +530,11 @@ type Config struct {
 	// the initiator for channels of the anchor type.
 	MaxAnchorsCommitFeeRate chainfee.SatPerKWeight
 
-	// DeleteAliasEdge allows the Manager to delete an alias channel edge
-	// from the graph. It also returns our local to-be-deleted policy.
-	DeleteAliasEdge func(scid lnwire.ShortChannelID) (
-		*models.ChannelEdgePolicy, error)
+	// UpdateAliasEdge allows the Manager to update an alias channel edge
+	// from the graph. This is necessary because SCID channels change their
+	// scid during their lifetime so we need to make sure to update the
+	// underlying graph.
+	UpdateAliasEdge func(aliasScID, newScID lnwire.ShortChannelID) error
 
 	// AliasManager is an implementation of the aliasHandler interface that
 	// abstracts away the handling of many alias functions.
@@ -3719,24 +3720,18 @@ func (f *Manager) annAfterSixConfs(completeChan *channeldb.OpenChannel,
 					"maps: %v", err)
 			}
 
-			// We'll delete the edge and add it again via
-			// addToGraph. This is because the peer may have
-			// sent us a ChannelUpdate with an alias and we don't
-			// want to relay this.
-			ourPolicy, err := f.cfg.DeleteAliasEdge(baseScid)
+			// We update the SCID so that potential ChannelUpdates
+			// we the alias as the short channel id are also removed
+			// and not relayed to the broader network (because the
+			// alias is not a verifiable channel id).
+			err = f.cfg.UpdateAliasEdge(
+				baseScid, baseScid,
+			)
 			if err != nil {
 				return fmt.Errorf("failed deleting real edge "+
 					"for alias channel from graph: %v",
 					err)
 			}
-
-			err = f.addToGraph(
-				completeChan, &baseScid, nil, ourPolicy,
-			)
-			if err != nil {
-				return fmt.Errorf("failed to re-add to "+
-					"graph: %v", err)
-			}
 		}
 
 		// Create and broadcast the proofs required to make this channel
@@ -3808,24 +3803,17 @@ func (f *Manager) waitForZeroConfChannel(c *channeldb.OpenChannel) error {
 				"six confirmations: %v", err)
 		}
 
-		// TODO: Make this atomic!
-		ourPolicy, err := f.cfg.DeleteAliasEdge(c.ShortChanID())
+		// The underlying graph entry for this channel id needs to be
+		// updated with the new confirmed scid. Moreover channel updates
+		// with the alias id are removed and we do not relay them to the
+		// broader network.
+		err := f.cfg.UpdateAliasEdge(
+			c.ShortChanID(), confChan.shortChanID,
+		)
 		if err != nil {
 			return fmt.Errorf("unable to delete alias edge from "+
 				"graph: %v", err)
 		}
-
-		// We'll need to update the graph with the new ShortChannelID
-		// via an addToGraph call. We don't pass in the peer's
-		// alias since we'll be using the confirmed SCID from now on
-		// regardless if it's public or not.
-		err = f.addToGraph(
-			c, &confChan.shortChanID, nil, ourPolicy,
-		)
-		if err != nil {
-			return fmt.Errorf("failed adding confirmed zero-conf "+
-				"SCID to graph: %v", err)
-		}
 	}
 
 	// Since we have now marked down the confirmed SCID, we'll also need to

funding/manager_test.go

@@ -550,10 +550,10 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 		NotifyOpenChannelEvent:        evt.NotifyOpenChannelEvent,
 		OpenChannelPredicate:          chainedAcceptor,
 		NotifyPendingOpenChannelEvent: evt.NotifyPendingOpenChannelEvent,
-		DeleteAliasEdge: func(scid lnwire.ShortChannelID) (
-			*models.ChannelEdgePolicy, error) {
+		UpdateAliasEdge: func(
+			aliasScID, newScID lnwire.ShortChannelID) error {
 
-			return nil, nil
+			return nil
 		},
 		AliasManager: aliasMgr,
 		// For unit tests we default to false meaning that no funds
@@ -674,7 +674,7 @@ func recreateAliceFundingManager(t *testing.T, alice *testNode) {
 		ZombieSweeperInterval: oldCfg.ZombieSweeperInterval,
 		ReservationTimeout:    oldCfg.ReservationTimeout,
 		OpenChannelPredicate:  chainedAcceptor,
-		DeleteAliasEdge:       oldCfg.DeleteAliasEdge,
+		UpdateAliasEdge:       oldCfg.UpdateAliasEdge,
 		AliasManager:          oldCfg.AliasManager,
 		AuxLeafStore:          oldCfg.AuxLeafStore,
 		AuxSigner:             oldCfg.AuxSigner,

graph/db/graph.go

@@ -1178,6 +1178,70 @@ func (c *ChannelGraph) addChannelEdge(tx kvdb.RwTx,
 	return chanIndex.Put(b.Bytes(), chanKey[:])
 }
 
+// ReAddChannelEdge removes the edge with the given channel ID from the
+// database and adds the new edge to guarantee atomicity.
+// This is important for option-scid-alias channel which might over the course
+// of their lifetime change their SCID (e.g. public zero-conf channels).
+func (c *ChannelGraph) ReAddChannelEdge(
+	chanID uint64, newEdge *models.ChannelEdgeInfo,
+	ourPolicy *models.ChannelEdgePolicy) error {
+
+	c.cacheMu.Lock()
+	defer c.cacheMu.Unlock()
+
+	err := kvdb.Update(c.db, func(tx kvdb.RwTx) error {
+		edges := tx.ReadWriteBucket(edgeBucket)
+		if edges == nil {
+			return ErrEdgeNotFound
+		}
+		edgeIndex := edges.NestedReadWriteBucket(edgeIndexBucket)
+		if edgeIndex == nil {
+			return ErrEdgeNotFound
+		}
+		chanIndex := edges.NestedReadWriteBucket(channelPointBucket)
+		if chanIndex == nil {
+			return ErrEdgeNotFound
+		}
+		nodes := tx.ReadWriteBucket(nodeBucket)
+		if nodes == nil {
+			return ErrGraphNodeNotFound
+		}
+
+		var rawChanID [8]byte
+		byteOrder.PutUint64(rawChanID[:], chanID)
+
+		// We don't mark this channel as zombie, because we are readding
+		// it immediately after deleting it below.
+		err := c.delChannelEdgeUnsafe(
+			edges, edgeIndex, chanIndex, nil,
+			rawChanID[:], false, false,
+		)
+		if err != nil {
+			return err
+		}
+
+		// Now we add the channel with the new edge info
+		err = c.addChannelEdge(tx, newEdge)
+		if err != nil {
+			return err
+		}
+
+		// Also add the new channel update from our side.
+		_, err = updateEdgePolicy(tx, ourPolicy, c.graphCache)
+
+		return err
+	}, func() {})
+	if err != nil {
+		return err
+	}
+
+	// Remove the Cache entries.
+	c.rejectCache.remove(chanID)
+	c.chanCache.remove(chanID)
+
+	return nil
+}
+
 // HasChannelEdge returns true if the database knows of a channel edge with the
 // passed channel ID, and false otherwise. If an edge with that ID is found
 // within the graph, then two time stamps representing the last time the edge

peer/brontide.go

@@ -2268,8 +2268,7 @@ func messageSummary(msg lnwire.Message) string {
 		return fmt.Sprintf("chan_id=%v", msg.ChanID)
 
 	case *lnwire.ChannelReady:
-		return fmt.Sprintf("chan_id=%v, next_point=%x",
-			msg.ChanID, msg.NextPerCommitmentPoint.SerializeCompressed())
+		return fmt.Sprintf("%v", msg)
 
 	case *lnwire.Shutdown:
 		return fmt.Sprintf("chan_id=%v, script=%x", msg.ChannelID,

server.go

@@ -1380,24 +1380,44 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		return nil, err
 	}
 
-	// Wrap the DeleteChannelEdges method so that the funding manager can
+	// Wrap the `ReAddChannelEdge` method so that the funding manager can
 	// use it without depending on several layers of indirection.
-	deleteAliasEdge := func(scid lnwire.ShortChannelID) (
-		*models.ChannelEdgePolicy, error) {
+	updateAliasEdge := func(
+		aliasScID, newScID lnwire.ShortChannelID) error {
 
 		info, e1, e2, err := s.graphDB.FetchChannelEdgesByID(
-			scid.ToUint64(),
+			aliasScID.ToUint64(),
 		)
 		if errors.Is(err, graphdb.ErrEdgeNotFound) {
 			// This is unlikely but there is a slim chance of this
 			// being hit if lnd was killed via SIGKILL and the
 			// funding manager was stepping through the delete
 			// alias edge logic.
-			return nil, nil
+			return nil
 		} else if err != nil {
-			return nil, err
+			return err
 		}
 
+		// The AuthProof is only available when the channel is public
+		// and therefore announced to the broader network after six
+		// confirmation. This happens after readding the edge with the
+		// confirmed scid.
+		newEdgeInfo := models.ChannelEdgeInfo{
+			ChannelID:        newScID.ToUint64(),
+			ChannelPoint:     info.ChannelPoint,
+			Capacity:         info.Capacity,
+			ChainHash:        info.ChainHash,
+			NodeKey1Bytes:    info.NodeKey1Bytes,
+			NodeKey2Bytes:    info.NodeKey2Bytes,
+			BitcoinKey1Bytes: info.BitcoinKey1Bytes,
+			BitcoinKey2Bytes: info.BitcoinKey2Bytes,
+			Features:         info.Features,
+			ExtraOpaqueData:  info.ExtraOpaqueData,
+		}
+
+		// We also readd the channel policy from our side with the new
+		// short channel id.
+		//
 		// Grab our key to find our policy.
 		var ourKey [33]byte
 		copy(ourKey[:], nodeKeyDesc.PubKey.SerializeCompressed())
@@ -1410,14 +1430,43 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		}
 
 		if ourPolicy == nil {
-			// Something is wrong, so return an error.
-			return nil, fmt.Errorf("we don't have an edge")
+			// We should always have our policy available. If that
+			// is not the case there might be an error in the
+			// ChannelUpdate msg logic so we return early.
+			return fmt.Errorf("edge policy not found")
+		}
+
+		// Update the policy data, this invalidates the signature
+		// therefore we need to resign the data.
+		ourPolicy.ChannelID = newEdgeInfo.ChannelID
+		chanUpdate := netann.UnsignedChannelUpdateFromEdge(
+			&newEdgeInfo, ourPolicy)
+
+		data, err := chanUpdate.DataToSign()
+		if err != nil {
+			return err
 		}
 
-		err = s.graphDB.DeleteChannelEdges(
-			false, false, scid.ToUint64(),
+		nodeSig, err := cc.MsgSigner.SignMessage(
+			nodeKeyDesc.KeyLocator, data, true,
 		)
-		return ourPolicy, err
+		if err != nil {
+			return err
+		}
+
+		sig, err := lnwire.NewSigFromSignature(nodeSig)
+		if err != nil {
+			return err
+		}
+		ourPolicy.SetSigBytes(sig.ToSignatureBytes())
+
+		// Delete the old edge information under the alias SCID and add
+		// the updated data with the new SCID.
+		err = s.graphDB.ReAddChannelEdge(
+			aliasScID.ToUint64(), &newEdgeInfo, ourPolicy,
+		)
+
+		return err
 	}
 
 	// For the reservationTimeout and the zombieSweeperInterval different
@@ -1612,7 +1661,7 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		EnableUpfrontShutdown:         cfg.EnableUpfrontShutdown,
 		MaxAnchorsCommitFeeRate: chainfee.SatPerKVByte(
 			s.cfg.MaxCommitFeeRateAnchors * 1000).FeePerKWeight(),
-		DeleteAliasEdge:      deleteAliasEdge,
+		UpdateAliasEdge:      updateAliasEdge,
 		AliasManager:         s.aliasMgr,
 		IsSweeperOutpoint:    s.sweeper.IsSweeperOutpoint,
 		AuxFundingController: implCfg.AuxFundingController,