Merge pull request #9602 from lightningnetwork/yy-pending-remote-commit

multi: make sure HTLCs are locked in the itest

contractcourt/chain_watcher.go

@@ -131,6 +131,35 @@ func (c *CommitSet) toActiveHTLCSets() map[HtlcSetKey]htlcSet {
 	return htlcSets
 }
 
+// String return a human-readable representation of the CommitSet.
+func (c *CommitSet) String() string {
+	if c == nil {
+		return "nil"
+	}
+
+	// Create a descriptive string for the ConfCommitKey.
+	commitKey := "none"
+	c.ConfCommitKey.WhenSome(func(k HtlcSetKey) {
+		commitKey = k.String()
+	})
+
+	// Create a map to hold all the htlcs.
+	htlcSet := make(map[string]string)
+	for k, htlcs := range c.HtlcSets {
+		// Create a map for this particular set.
+		desc := make([]string, len(htlcs))
+		for i, htlc := range htlcs {
+			desc[i] = fmt.Sprintf("%x", htlc.RHash)
+		}
+
+		// Add the description to the set key.
+		htlcSet[k.String()] = fmt.Sprintf("count: %v, htlcs=%v",
+			len(htlcs), desc)
+	}
+
+	return fmt.Sprintf("ConfCommitKey=%v, HtlcSets=%v", commitKey, htlcSet)
+}
+
 // ChainEventSubscription is a struct that houses a subscription to be notified
 // for any on-chain events related to a channel. There are three types of
 // possible on-chain events: a cooperative channel closure, a unilateral

contractcourt/channel_arbitrator.go

@@ -947,9 +947,9 @@ func (c *ChannelArbitrator) stateStep(
 	// If we're in the default state, then we'll check our set of actions
 	// to see if while we were down, conditions have changed.
 	case StateDefault:
-		log.Debugf("ChannelArbitrator(%v): new block (height=%v) "+
+		log.Debugf("ChannelArbitrator(%v): examining active HTLCs in "+
-			"examining active HTLC's", c.cfg.ChanPoint,
+			"block %v, confCommitSet: %v", c.cfg.ChanPoint,
-			triggerHeight)
+			triggerHeight, lnutils.LogClosure(confCommitSet.String))
 
 		// As a new block has been connected to the end of the main
 		// chain, we'll check to see if we need to make any on-chain
@@ -2155,6 +2155,20 @@ func (c *ChannelArbitrator) checkRemoteDanglingActions(
 			continue
 		}
 
+		preimageAvailable, err := c.isPreimageAvailable(htlc.RHash)
+		if err != nil {
+			log.Errorf("ChannelArbitrator(%v): failed to query "+
+				"preimage for dangling htlc=%x from remote "+
+				"commitments diff", c.cfg.ChanPoint,
+				htlc.RHash[:])
+
+			continue
+		}
+
+		if preimageAvailable {
+			continue
+		}
+
 		// Dust htlcs can be canceled back even before the commitment
 		// transaction confirms. Dust htlcs are not enforceable onchain.
 		// If another version of the commit tx would confirm we either
@@ -2824,7 +2838,8 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32,
 		case beat := <-c.BlockbeatChan:
 			bestHeight = beat.Height()
 
-			log.Debugf("ChannelArbitrator(%v): new block height=%v",
+			log.Debugf("ChannelArbitrator(%v): received new "+
+				"block: height=%v, processing...",
 				c.cfg.ChanPoint, bestHeight)
 
 			err := c.handleBlockbeat(beat)

docs/release-notes/release-notes-0.19.0.md

@@ -246,6 +246,7 @@ close transaction.
     * `lncli abandonchannel` (`Lightning.AbandonChannel` RPC)
     * `lncli restorechanbackup` (`Lightning.RestoreChannelBackups` RPC)
     * `lncli verifychanbackup` (`Lightning.VerifyChanBackup` RPC)
+
 * The `ForwardInterceptor`'s `MODIFY` option will
   [merge](https://github.com/lightningnetwork/lnd/pull/9240) any custom
   range TLVs provided with the existing set of records on the HTLC,
@@ -255,6 +256,11 @@ close transaction.
 `ProofMatureDelta` used in gossip to be configurable via
 `--gossip.announcement-conf`, with a default value of 6.
 
+* [Added a boolean field
+  `LockedIn`](https://github.com/lightningnetwork/lnd/pull/9602) in
+  `lnrpc.HTLC`. This field is used to indicate whether a given HTLC has been
+  locked in by the remote peer.
+
 ## lncli Updates
 
 * [Fixed](https://github.com/lightningnetwork/lnd/pull/9605) a case where

htlcswitch/link.go

@@ -1582,6 +1582,11 @@ loop:
 		select {
 		case item := <-l.hodlQueue.ChanOut():
 			htlcResolution = item.(invoices.HtlcResolution)
+
+		// No need to process it if the link is broken.
+		case <-l.cg.Done():
+			return ErrLinkShuttingDown
+
 		default:
 			break loop
 		}
@@ -2052,6 +2057,9 @@ func (l *channelLink) cleanupSpuriousResponse(pkt *htlcPacket) {
 func (l *channelLink) handleUpstreamMsg(ctx context.Context,
 	msg lnwire.Message) {
 
+	l.log.Tracef("receive upstream msg %v, handling now... ", msg.MsgType())
+	defer l.log.Tracef("handled upstream msg %v", msg.MsgType())
+
 	// First check if the message is an update and we are capable of
 	// receiving updates right now.
 	if msg.MsgType().IsChannelUpdate() && !l.quiescer.CanRecvUpdates() {

htlcswitch/switch.go

@@ -2243,6 +2243,9 @@ func (s *Switch) getLinkByMapping(pkt *htlcPacket) (ChannelLink, error) {
 	chanID := pkt.outgoingChanID
 	aliasID := s.cfg.IsAlias(chanID)
 
+	log.Debugf("Querying outgoing link using chanID=%v, aliasID=%v", chanID,
+		aliasID)
+
 	// Set the originalOutgoingChanID so the proper channel_update can be
 	// sent back if the option-scid-alias feature bit was negotiated.
 	pkt.originalOutgoingChanID = chanID
@@ -2299,6 +2302,9 @@ func (s *Switch) getLinkByMapping(pkt *htlcPacket) (ChannelLink, error) {
 	// forward over it and this is a channel where the option-scid-alias
 	// feature bit was negotiated.
 	if link.IsUnadvertised() {
+		log.Debugf("Link is unadvertised, chanID=%v, baseScid=%v",
+			chanID, baseScid)
+
 		return nil, ErrChannelLinkNotFound
 	}
 

itest/flakes.go

@@ -1,7 +1,6 @@
 package itest
 
 import (
-	"runtime"
 	"time"
 
 	"github.com/btcsuite/btcd/btcutil"
@@ -72,39 +71,6 @@ func flakeTxNotifierNeutrino(ht *lntest.HarnessTest) {
 	}
 }
 
-// flakeSkipPendingSweepsCheckDarwin documents a flake found only in macOS
-// build. When running in macOS, we might see three anchor sweeps - one from the
-// local, one from the remote, and one from the pending remote:
-//   - the local one will be swept.
-//   - the remote one will be marked as failed due to `testmempoolaccept` check.
-//   - the pending remote one will not be attempted due to it being uneconomical
-//     since it was not used for CPFP.
-//
-// The anchor from the pending remote may or may not appear, which is a bug
-// found only in macOS - when updating the commitments, the channel state
-// machine somehow thinks we still have a pending remote commitment, causing it
-// to sweep the anchor from that version.
-//
-// TODO(yy): fix the above bug in the channel state machine.
-func flakeSkipPendingSweepsCheckDarwin(ht *lntest.HarnessTest,
-	node *node.HarnessNode, num int) {
-
-	// Skip the assertion below if it's on macOS.
-	if isDarwin() {
-		ht.Logf("Skipped AssertNumPendingSweeps for node %s",
-			node.Name())
-
-		return
-	}
-
-	ht.AssertNumPendingSweeps(node, num)
-}
-
-// isDarwin returns true if the test is running on a macOS.
-func isDarwin() bool {
-	return runtime.GOOS == "darwin"
-}
-
 // flakeInconsistentHTLCView documents a flake found that the `ListChannels` RPC
 // can give inaccurate HTLC states, which is found when we call
 // `AssertHTLCNotActive` after a commitment dance is finished. Suppose Carol is

itest/lnd_multi-hop_force_close_test.go

@@ -422,8 +422,7 @@ func runLocalClaimOutgoingHTLC(ht *lntest.HarnessTest,
 	if ht.IsNeutrinoBackend() {
 		numSweeps = 2
 	}
-
+	ht.AssertNumPendingSweeps(bob, numSweeps)
-	flakeSkipPendingSweepsCheckDarwin(ht, bob, numSweeps)
 
 	// We expect to see tow txns in the mempool,
 	// 1. Bob's force close tx.
@@ -744,6 +743,7 @@ func runMultiHopReceiverPreimageClaim(ht *lntest.HarnessTest,
 
 	// Stop Bob so he won't be able to settle the incoming htlc.
 	restartBob := ht.SuspendNode(bob)
+	ht.AssertPeerNotConnected(carol, bob)
 
 	// Settle invoice. This will just mark the invoice as settled, as there
 	// is no link anymore to remove the htlc from the commitment tx. For
@@ -752,6 +752,9 @@ func runMultiHopReceiverPreimageClaim(ht *lntest.HarnessTest,
 	// channel arbitrator won't go to chain.
 	carol.RPC.SettleInvoice(preimage[:])
 
+	// Carol should still have one incoming HTLC on channel Bob -> Carol.
+	ht.AssertNumActiveHtlcs(carol, 1)
+
 	// We now advance the block height to the point where Carol will force
 	// close her channel with Bob, broadcast the closing tx but keep it
 	// unconfirmed.
@@ -779,8 +782,7 @@ func runMultiHopReceiverPreimageClaim(ht *lntest.HarnessTest,
 	if ht.IsNeutrinoBackend() {
 		numSweeps = 2
 	}
-
+	ht.AssertNumPendingSweeps(carol, numSweeps)
-	flakeSkipPendingSweepsCheckDarwin(ht, carol, numSweeps)
 
 	// We expect to see tow txns in the mempool,
 	// 1. Carol's force close tx.
@@ -1690,6 +1692,7 @@ func runLocalClaimIncomingHTLC(ht *lntest.HarnessTest,
 
 	// Suspend Bob to force Carol to go to chain.
 	restartBob := ht.SuspendNode(bob)
+	ht.AssertPeerNotConnected(carol, bob)
 
 	// Settle invoice. This will just mark the invoice as settled, as there
 	// is no link anymore to remove the htlc from the commitment tx. For
@@ -1698,6 +1701,9 @@ func runLocalClaimIncomingHTLC(ht *lntest.HarnessTest,
 	// channel arbitrator won't go to chain.
 	carol.RPC.SettleInvoice(preimage[:])
 
+	// Carol should still have one incoming HTLC on channel Bob -> Carol.
+	ht.AssertNumActiveHtlcs(carol, 1)
+
 	// We now advance the block height to the point where Carol will force
 	// close her channel with Bob, broadcast the closing tx but keep it
 	// unconfirmed.
@@ -1967,6 +1973,7 @@ func runLocalClaimIncomingHTLCLeased(ht *lntest.HarnessTest,
 
 	// Suspend Bob to force Carol to go to chain.
 	restartBob := ht.SuspendNode(bob)
+	ht.AssertPeerNotConnected(carol, bob)
 
 	// Settle invoice. This will just mark the invoice as settled, as there
 	// is no link anymore to remove the htlc from the commitment tx. For
@@ -1975,6 +1982,9 @@ func runLocalClaimIncomingHTLCLeased(ht *lntest.HarnessTest,
 	// channel arbitrator won't go to chain.
 	carol.RPC.SettleInvoice(preimage[:])
 
+	// Carol should still have one incoming HTLC on channel Bob -> Carol.
+	ht.AssertNumActiveHtlcs(carol, 1)
+
 	// We now advance the block height to the point where Carol will force
 	// close her channel with Bob, broadcast the closing tx but keep it
 	// unconfirmed.
@@ -2307,6 +2317,7 @@ func runLocalPreimageClaim(ht *lntest.HarnessTest,
 
 	// Suspend bob, so Carol is forced to go on chain.
 	restartBob := ht.SuspendNode(bob)
+	ht.AssertPeerNotConnected(carol, bob)
 
 	// Settle invoice. This will just mark the invoice as settled, as there
 	// is no link anymore to remove the htlc from the commitment tx. For
@@ -2315,6 +2326,9 @@ func runLocalPreimageClaim(ht *lntest.HarnessTest,
 	// channel arbitrator won't go to chain.
 	carol.RPC.SettleInvoice(preimage[:])
 
+	// Carol should still have one incoming HTLC on channel Bob -> Carol.
+	ht.AssertNumActiveHtlcs(carol, 1)
+
 	ht.Logf("Invoice expire height: %d, current: %d", invoiceExpiry,
 		ht.CurrentHeight())
 
@@ -2334,8 +2348,7 @@ func runLocalPreimageClaim(ht *lntest.HarnessTest,
 	if ht.IsNeutrinoBackend() {
 		numSweeps = 2
 	}
-
+	ht.AssertNumPendingSweeps(carol, numSweeps)
-	flakeSkipPendingSweepsCheckDarwin(ht, carol, numSweeps)
 
 	// We should see two txns in the mempool, we now a block to confirm,
 	// - Carol's force close tx.
@@ -2547,6 +2560,7 @@ func runLocalPreimageClaimLeased(ht *lntest.HarnessTest,
 
 	// Suspend bob, so Carol is forced to go on chain.
 	restartBob := ht.SuspendNode(bob)
+	ht.AssertPeerNotConnected(carol, bob)
 
 	// Settle invoice. This will just mark the invoice as settled, as there
 	// is no link anymore to remove the htlc from the commitment tx. For
@@ -2555,6 +2569,9 @@ func runLocalPreimageClaimLeased(ht *lntest.HarnessTest,
 	// channel arbitrator won't go to chain.
 	carol.RPC.SettleInvoice(preimage[:])
 
+	// Carol should still have one incoming HTLC on channel Bob -> Carol.
+	ht.AssertNumActiveHtlcs(carol, 1)
+
 	ht.Logf("Invoice expire height: %d, current: %d", invoiceExpiry,
 		ht.CurrentHeight())
 
@@ -2574,8 +2591,7 @@ func runLocalPreimageClaimLeased(ht *lntest.HarnessTest,
 	if ht.IsNeutrinoBackend() {
 		numSweeps = 2
 	}
-
+	ht.AssertNumPendingSweeps(carol, numSweeps)
-	flakeSkipPendingSweepsCheckDarwin(ht, carol, numSweeps)
 
 	// We should see two txns in the mempool, we now a block to confirm,
 	// - Carol's force close tx.
@@ -2964,6 +2980,7 @@ func runHtlcAggregation(ht *lntest.HarnessTest,
 	// close. However, Carol will cancel her invoices to prevent force
 	// closes, so we shut her down for now.
 	restartCarol := ht.SuspendNode(carol)
+	ht.AssertPeerNotConnected(bob, carol)
 
 	// We'll now mine enough blocks to trigger Bob's broadcast of his
 	// commitment transaction due to the fact that the Carol's HTLCs are
@@ -2982,8 +2999,7 @@ func runHtlcAggregation(ht *lntest.HarnessTest,
 	if ht.IsNeutrinoBackend() {
 		numSweeps = 2
 	}
-
+	ht.AssertNumPendingSweeps(bob, numSweeps)
-	flakeSkipPendingSweepsCheckDarwin(ht, bob, numSweeps)
 
 	// Bob's force close tx and anchor sweeping tx should now be found in
 	// the mempool.

itest/lnd_sweep_test.go

@@ -1139,6 +1139,11 @@ func testSweepHTLCs(ht *lntest.HarnessTest) {
 	// NOTE: We need to subtract 1 from the deadline as the budget must be
 	// used up before the deadline.
 	blocksLeft := outgoingHTLCDeadline - outgoingFuncPosition - 1
+
+	ht.Logf("Bob has incoming sweep tx: %v, outgoing sweep tx: %v, "+
+		"blocksLeft=%v, entering fee bumping now...",
+		incomingSweep.TxHash(), outgoingSweep.TxHash(), blocksLeft)
+
 	for i := int32(0); i < blocksLeft; i++ {
 		// Mine an empty block.
 		ht.MineEmptyBlocks(1)

lnrpc/lightning.proto

@@ -1381,6 +1381,13 @@ message HTLC {
 
     // Index identifying the htlc on the forwarding channel.
     uint64 forwarding_htlc_index = 7;
+
+    /*
+    Whether the HTLC is locked in. An HTLC is considered locked in when the
+    remote party has sent us the `revoke_and_ack` to irrevocably commit this
+    HTLC.
+    */
+    bool locked_in = 8;
 }
 
 enum CommitmentType {

lnrpc/lightning.swagger.json

@@ -5302,6 +5302,10 @@
           "type": "string",
           "format": "uint64",
           "description": "Index identifying the htlc on the forwarding channel."
+        },
+        "locked_in": {
+          "type": "boolean",
+          "description": "Whether the HTLC is locked in. An HTLC is considered locked in when the\nremote party has sent us the `revoke_and_ack` to irrevocably commit this\nHTLC."
         }
       }
     },

lntest/harness_assertion.go

@@ -1239,6 +1239,9 @@ func (h *HarnessTest) AssertNumActiveHtlcs(hn *node.HarnessNode, num int) {
 	old := hn.State.HTLC
 
 	err := wait.NoError(func() error {
+		// pendingHTLCs is used to print unacked HTLCs, if found.
+		var pendingHTLCs []string
+
 		// We require the RPC call to be succeeded and won't wait for
 		// it as it's an unexpected behavior.
 		req := &lnrpc.ListChannelsRequest{}
@@ -1246,10 +1249,20 @@ func (h *HarnessTest) AssertNumActiveHtlcs(hn *node.HarnessNode, num int) {
 
 		total := 0
 		for _, channel := range nodeChans.Channels {
-			total += len(channel.PendingHtlcs)
+			for _, htlc := range channel.PendingHtlcs {
+				if htlc.LockedIn {
+					total++
+				}
+
+				rHash := fmt.Sprintf("%x", htlc.HashLock)
+				pendingHTLCs = append(pendingHTLCs, rHash)
+			}
 		}
 		if total-old != num {
-			return errNumNotMatched(hn.Name(), "active HTLCs",
+			desc := fmt.Sprintf("active HTLCs: unacked HTLCs: %v",
+				pendingHTLCs)
+
+			return errNumNotMatched(hn.Name(), desc,
 				num, total-old, total, old)
 		}
 
@@ -1291,17 +1304,22 @@ func (h *HarnessTest) assertHTLCActive(hn *node.HarnessNode,
 
 		// Check all payment hashes active for this channel.
 		for _, htlc := range ch.PendingHtlcs {
-			h := hex.EncodeToString(htlc.HashLock)
+			rHash := hex.EncodeToString(htlc.HashLock)
-			if h != target {
+			if rHash != target {
 				continue
 			}
 
 			// If the payment hash is found, check the incoming
 			// field.
 			if htlc.Incoming == incoming {
-				// Found it and return.
+				// Return the result if it's locked in.
-				result = htlc
+				if htlc.LockedIn {
-				return nil
+					result = htlc
+					return nil
+				}
+
+				return fmt.Errorf("htlc(%x) not locked in",
+					payHash)
 			}
 
 			// Otherwise we do have the HTLC but its direction is
@@ -1311,14 +1329,13 @@ func (h *HarnessTest) assertHTLCActive(hn *node.HarnessNode,
 				have, want = "incoming", "outgoing"
 			}
 
-			return fmt.Errorf("node[%s] have htlc(%v), want: %s, "+
+			return fmt.Errorf("htlc(%x) has wrong direction - "+
-				"have: %s", hn.Name(), payHash, want, have)
+				"want: %s, have: %s", payHash, want, have)
 		}
 
-		return fmt.Errorf("node [%s:%x] didn't have: the payHash %x",
+		return fmt.Errorf("htlc not found using payHash %x", payHash)
-			hn.Name(), hn.PubKey[:], payHash)
 	}, DefaultTimeout)
-	require.NoError(h, err, "timeout checking pending HTLC")
+	require.NoError(h, err, "%s: timeout checking pending HTLC", hn.Name())
 
 	return result
 }

netann/chan_status_manager_test.go

@@ -130,8 +130,8 @@ type mockGraph struct {
 	updates chan *lnwire.ChannelUpdate1
 }
 
-func newMockGraph(t *testing.T, numChannels int,
+func newMockGraph(t *testing.T, numChannels int, startEnabled bool,
-	startActive, startEnabled bool, pubKey *btcec.PublicKey) *mockGraph {
+	pubKey *btcec.PublicKey) *mockGraph {
 
 	g := &mockGraph{
 		channels:  make([]*channeldb.OpenChannel, 0, numChannels),
@@ -320,9 +320,7 @@ func newManagerCfg(t *testing.T, numChannels int,
 	require.NoError(t, err, "unable to generate key pair")
 	privKeySigner := keychain.NewPrivKeyMessageSigner(privKey, testKeyLoc)
 
-	graph := newMockGraph(
+	graph := newMockGraph(t, numChannels, startEnabled, privKey.PubKey())
-		t, numChannels, startEnabled, startEnabled, privKey.PubKey(),
-	)
 	htlcSwitch := newMockSwitch()
 
 	cfg := &netann.ChanStatusConfig{

peer/brontide.go

@@ -2404,7 +2404,7 @@ func messageSummary(msg lnwire.Message) string {
 			msg.ID, msg.Reason)
 
 	case *lnwire.UpdateFulfillHTLC:
-		return fmt.Sprintf("chan_id=%v, id=%v, pre_image=%x, "+
+		return fmt.Sprintf("chan_id=%v, id=%v, preimage=%x, "+
 			"custom_records=%v", msg.ChanID, msg.ID,
 			msg.PaymentPreimage[:], msg.CustomRecords)
 

rpcserver.go

@@ -4842,6 +4842,14 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel,
 		)
 	}
 
+	// Create a set of the HTLCs found in the remote commitment, which is
+	// used to decide whether the HTLCs from the local commitment has been
+	// locked in or not.
+	remoteHTLCs := fn.NewSet[[32]byte]()
+	for _, htlc := range dbChannel.RemoteCommitment.Htlcs {
+		remoteHTLCs.Add(htlc.RHash)
+	}
+
 	for i, htlc := range localCommit.Htlcs {
 		var rHash [32]byte
 		copy(rHash[:], htlc.RHash[:])
@@ -4893,6 +4901,7 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel,
 			HtlcIndex:           htlc.HtlcIndex,
 			ForwardingChannel:   forwardingChannel,
 			ForwardingHtlcIndex: forwardingHtlcIndex,
+			LockedIn:            remoteHTLCs.Contains(rHash),
 		}
 
 		// Add the Pending Htlc Amount to UnsettledBalance field.