multi: add itest to check race between channel_ready and update_add_htlc

This commit adds a new itest case to check the race condition found in
issue #7401. In order to control the funding manager's state, a new dev
config for the funding manager is introduced to specify a duration we
should hold before processing remote node's channel_ready message.

A new development config, `DevConfig` is introduced in `lncfg` and will
only have effect if built with flag `integration`. This can also be
extended for future integration tests if more dev-only flags are needed.

config.go

@@ -490,6 +490,10 @@ type Config struct {
 
 	// Estimator is used to estimate routing probabilities.
 	Estimator routing.Estimator
+
+	// Dev specifies configs used for integration tests, which is always
+	// empty if not built with `integration` flag.
+	Dev *lncfg.DevConfig `group:"dev" namespace:"dev"`
 }
 
 // GRPCConfig holds the configuration options for the gRPC server.

funding/manager.go

@@ -337,10 +337,22 @@ func newSerializedKey(pubKey *btcec.PublicKey) serializedPubKey {
 	return s
 }
 
+// DevConfig specifies configs used for integration test only.
+type DevConfig struct {
+	// ProcessChannelReadyWait is the duration to sleep before processing
+	// remote node's channel ready message once the channel as been marked
+	// as `channelReadySent`.
+	ProcessChannelReadyWait time.Duration
+}
+
 // Config defines the configuration for the FundingManager. All elements
 // within the configuration MUST be non-nil for the FundingManager to carry out
 // its duties.
 type Config struct {
+	// Dev specifies config values used in integration test. For
+	// production, this config will always be an empty struct.
+	Dev *DevConfig
+
 	// NoWumboChans indicates if we're to reject all incoming wumbo channel
 	// requests, and also reject all outgoing wumbo channel requests.
 	NoWumboChans bool
@@ -3523,6 +3535,24 @@ func (f *Manager) handleChannelReady(peer lnpeer.Peer,
 	msg *lnwire.ChannelReady) {
 
 	defer f.wg.Done()
+
+	// If we are in development mode, we'll wait for specified duration
+	// before processing the channel ready message.
+	if f.cfg.Dev != nil {
+		duration := f.cfg.Dev.ProcessChannelReadyWait
+		log.Warnf("Channel(%v): sleeping %v before processing "+
+			"channel_ready", msg.ChanID, duration)
+
+		select {
+		case <-time.After(duration):
+			log.Warnf("Channel(%v): slept %v before processing "+
+				"channel_ready", msg.ChanID, duration)
+		case <-f.quit:
+			log.Warnf("Channel(%v): quit sleeping", msg.ChanID)
+			return
+		}
+	}
+
 	log.Debugf("Received ChannelReady for ChannelID(%v) from "+
 		"peer %x", msg.ChanID,
 		peer.IdentityKey().SerializeCompressed())

itest/list_on_test.go

@@ -546,4 +546,8 @@ var allTestCases = []*lntest.TestCase{
 		Name:     "utxo selection funding",
 		TestFunc: testChannelUtxoSelection,
 	},
+	{
+		Name:     "update pending open channels",
+		TestFunc: testUpdateOnPendingOpenChannels,
+	},
 }

itest/lnd_open_channel_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/lightningnetwork/lnd/chainreg"
 	"github.com/lightningnetwork/lnd/funding"
 	"github.com/lightningnetwork/lnd/lnrpc"
+	"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
 	"github.com/lightningnetwork/lnd/lntest"
 	"github.com/lightningnetwork/lnd/lntest/node"
 	"github.com/lightningnetwork/lnd/lntest/rpc"
@@ -488,6 +489,194 @@ func runBasicChannelCreationAndUpdates(ht *lntest.HarnessTest,
 	)
 }
 
+// testUpdateOnPendingOpenChannels checks that `update_add_htlc` followed by
+// `channel_ready` is properly handled. In specific, when a node is in a state
+// that it's still processing a remote `channel_ready` message, meanwhile an
+// `update_add_htlc` is received, this HTLC message is cached and settled once
+// processing `channel_ready` is complete.
+func testUpdateOnPendingOpenChannels(ht *lntest.HarnessTest) {
+	// Test funder's behavior. Funder sees the channel pending, but fundee
+	// sees it active and sends an HTLC.
+	ht.Run("pending on funder side", func(t *testing.T) {
+		st := ht.Subtest(t)
+		testUpdateOnFunderPendingOpenChannels(st)
+	})
+
+	// Test fundee's behavior. Fundee sees the channel pending, but funder
+	// sees it active and sends an HTLC.
+	ht.Run("pending on fundee side", func(t *testing.T) {
+		st := ht.Subtest(t)
+		testUpdateOnFundeePendingOpenChannels(st)
+	})
+}
+
+// testUpdateOnFunderPendingOpenChannels checks that when the fundee sends an
+// `update_add_htlc` followed by `channel_ready` while the funder is still
+// processing the fundee's `channel_ready`, the HTLC will be cached and
+// eventually settled.
+func testUpdateOnFunderPendingOpenChannels(ht *lntest.HarnessTest) {
+	// Grab the channel participants.
+	alice, bob := ht.Alice, ht.Bob
+
+	// Restart Alice with the config so she won't process Bob's
+	// channel_ready msg immediately.
+	ht.RestartNodeWithExtraArgs(alice, []string{
+		"--dev.processchannelreadywait=10s",
+	})
+
+	// Make sure Alice and Bob are connected.
+	ht.EnsureConnected(alice, bob)
+
+	// Create a new channel that requires 1 confs before it's considered
+	// open.
+	params := lntest.OpenChannelParams{
+		Amt:     funding.MaxBtcFundingAmount,
+		PushAmt: funding.MaxBtcFundingAmount / 2,
+	}
+	pendingChan := ht.OpenChannelAssertPending(alice, bob, params)
+	chanPoint := &lnrpc.ChannelPoint{
+		FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{
+			FundingTxidBytes: pendingChan.Txid,
+		},
+		OutputIndex: pendingChan.OutputIndex,
+	}
+
+	// Alice and Bob should both consider the channel pending open.
+	ht.AssertNumPendingOpenChannels(alice, 1)
+	ht.AssertNumPendingOpenChannels(bob, 1)
+
+	// Mine one block to confirm the funding transaction.
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// TODO(yy): we've prematurely marked the channel as open before
+	// processing channel ready messages. We need to mark it as open after
+	// we've processed channel ready messages and change the check to,
+	// ht.AssertNumPendingOpenChannels(alice, 1)
+	ht.AssertNumPendingOpenChannels(alice, 0)
+
+	// Bob will consider the channel open as there's no wait time to send
+	// and receive Alice's channel_ready message.
+	ht.AssertNumPendingOpenChannels(bob, 0)
+
+	// Alice and Bob now have different view of the channel. For Bob,
+	// since the channel_ready messages are processed, he will have a
+	// working link to route HTLCs. For Alice, because she hasn't handled
+	// Bob's channel_ready, there's no active link yet.
+	//
+	// Alice now adds an invoice.
+	req := &lnrpc.Invoice{
+		RPreimage: ht.Random32Bytes(),
+		Value:     10_000,
+	}
+	invoice := alice.RPC.AddInvoice(req)
+
+	// Bob sends an `update_add_htlc`, which would result in this message
+	// being cached in Alice's `peer.Brontide` and the payment will stay
+	// in-flight instead of being failed by Alice.
+	bobReq := &routerrpc.SendPaymentRequest{
+		PaymentRequest: invoice.PaymentRequest,
+		TimeoutSeconds: 60,
+		FeeLimitMsat:   noFeeLimitMsat,
+	}
+	bobStream := bob.RPC.SendPayment(bobReq)
+	ht.AssertPaymentStatusFromStream(bobStream, lnrpc.Payment_IN_FLIGHT)
+
+	// Wait until Alice finishes processing Bob's channel_ready.
+	//
+	// NOTE: no effect before fixing the above TODO.
+	ht.AssertNumPendingOpenChannels(alice, 0)
+
+	// Once Alice sees the channel as active, she will process the cached
+	// premature `update_add_htlc` and settles the payment.
+	ht.AssertPaymentStatusFromStream(bobStream, lnrpc.Payment_SUCCEEDED)
+
+	// Close the channel.
+	ht.CloseChannel(alice, chanPoint)
+}
+
+// testUpdateOnFundeePendingOpenChannels checks that when the funder sends an
+// `update_add_htlc` followed by `channel_ready` while the fundee is still
+// processing the funder's `channel_ready`, the HTLC will be cached and
+// eventually settled.
+func testUpdateOnFundeePendingOpenChannels(ht *lntest.HarnessTest) {
+	// Grab the channel participants.
+	alice, bob := ht.Alice, ht.Bob
+
+	// Restart Bob with the config so he won't process Alice's
+	// channel_ready msg immediately.
+	ht.RestartNodeWithExtraArgs(bob, []string{
+		"--dev.processchannelreadywait=10s",
+	})
+
+	// Make sure Alice and Bob are connected.
+	ht.EnsureConnected(alice, bob)
+
+	// Create a new channel that requires 1 confs before it's considered
+	// open.
+	params := lntest.OpenChannelParams{
+		Amt: funding.MaxBtcFundingAmount,
+	}
+	pendingChan := ht.OpenChannelAssertPending(alice, bob, params)
+	chanPoint := &lnrpc.ChannelPoint{
+		FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{
+			FundingTxidBytes: pendingChan.Txid,
+		},
+		OutputIndex: pendingChan.OutputIndex,
+	}
+
+	// Alice and Bob should both consider the channel pending open.
+	ht.AssertNumPendingOpenChannels(alice, 1)
+	ht.AssertNumPendingOpenChannels(bob, 1)
+
+	// Mine one block to confirm the funding transaction.
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// Alice will consider the channel open as there's no wait time to send
+	// and receive Bob's channel_ready message.
+	ht.AssertNumPendingOpenChannels(alice, 0)
+
+	// TODO(yy): we've prematurely marked the channel as open before
+	// processing channel ready messages. We need to mark it as open after
+	// we've processed channel ready messages and change the check to,
+	// ht.AssertNumPendingOpenChannels(bob, 1)
+	ht.AssertNumPendingOpenChannels(bob, 0)
+
+	// Alice and Bob now have different view of the channel. For Alice,
+	// since the channel_ready messages are processed, she will have a
+	// working link to route HTLCs. For Bob, because he hasn't handled
+	// Alice's channel_ready, there's no active link yet.
+	//
+	// Bob now adds an invoice.
+	req := &lnrpc.Invoice{
+		RPreimage: ht.Random32Bytes(),
+		Value:     10_000,
+	}
+	bobInvoice := bob.RPC.AddInvoice(req)
+
+	// Alice sends an `update_add_htlc`, which would result in this message
+	// being cached in Bob's `peer.Brontide` and the payment will stay
+	// in-flight instead of being failed by Bob.
+	aliceReq := &routerrpc.SendPaymentRequest{
+		PaymentRequest: bobInvoice.PaymentRequest,
+		TimeoutSeconds: 60,
+		FeeLimitMsat:   noFeeLimitMsat,
+	}
+	aliceStream := alice.RPC.SendPayment(aliceReq)
+	ht.AssertPaymentStatusFromStream(aliceStream, lnrpc.Payment_IN_FLIGHT)
+
+	// Wait until Bob finishes processing Alice's channel_ready.
+	//
+	// NOTE: no effect before fixing the above TODO.
+	ht.AssertNumPendingOpenChannels(bob, 0)
+
+	// Once Bob sees the channel as active, he will process the cached
+	// premature `update_add_htlc` and settles the payment.
+	ht.AssertPaymentStatusFromStream(aliceStream, lnrpc.Payment_SUCCEEDED)
+
+	// Close the channel.
+	ht.CloseChannel(alice, chanPoint)
+}
+
 // verifyCloseUpdate is used to verify that a closed channel update is of the
 // expected type.
 func verifyCloseUpdate(chanUpdate *lnrpc.ChannelEventUpdate,

lncfg/dev.go

@@ -0,0 +1,23 @@
+//go:build !integration
+
+package lncfg
+
+import "time"
+
+// IsDevBuild returns a bool to indicate whether we are in a development
+// environment.
+//
+// NOTE: always return false here.
+func IsDevBuild() bool {
+	return false
+}
+
+// DevConfig specifies development configs used for production. This struct
+// should always remain empty.
+type DevConfig struct{}
+
+// ChannelReadyWait returns the config value, which is always 0 for production
+// build.
+func (d *DevConfig) ChannelReadyWait() time.Duration {
+	return 0
+}

lncfg/dev_integration.go

@@ -0,0 +1,26 @@
+//go:build integration
+
+package lncfg
+
+import "time"
+
+// IsDevBuild returns a bool to indicate whether we are in a development
+// environment.
+//
+// NOTE: always return true here.
+func IsDevBuild() bool {
+	return true
+}
+
+// DevConfig specifies configs used for integration tests. These configs can
+// only be used in tests and must NOT be exported for production usage.
+//
+//nolint:lll
+type DevConfig struct {
+	ProcessChannelReadyWait time.Duration `long:"processchannelreadywait" description:"Time to sleep before processing remote node's channel_ready message."`
+}
+
+// ChannelReadyWait returns the config value `ProcessChannelReadyWait`.
+func (d *DevConfig) ChannelReadyWait() time.Duration {
+	return d.ProcessChannelReadyWait
+}

server.go

@@ -1276,8 +1276,18 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		return ourPolicy, err
 	}
 
+	// Get the development config for funding manager. If we are not in
+	// development mode, this would be nil.
+	var devCfg *funding.DevConfig
+	if lncfg.IsDevBuild() {
+		devCfg = &funding.DevConfig{
+			ProcessChannelReadyWait: cfg.Dev.ChannelReadyWait(),
+		}
+	}
+
 	//nolint:lll
 	s.fundingMgr, err = funding.NewFundingManager(funding.Config{
+		Dev:                devCfg,
 		NoWumboChans:       !cfg.ProtocolOptions.Wumbo(),
 		IDKey:              nodeKeyDesc.PubKey,
 		IDKeyLoc:           nodeKeyDesc.KeyLocator,