diff --git a/config.go b/config.go index 706fb7892..e2ee0ca68 100644 --- a/config.go +++ b/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. diff --git a/funding/manager.go b/funding/manager.go index f4452470b..ddd92491d 100644 --- a/funding/manager.go +++ b/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()) diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 970dc32e3..613e572fe 100644 --- a/itest/list_on_test.go +++ b/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, + }, } diff --git a/itest/lnd_open_channel_test.go b/itest/lnd_open_channel_test.go index 12b13a14f..674d9a62b 100644 --- a/itest/lnd_open_channel_test.go +++ b/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, diff --git a/lncfg/dev.go b/lncfg/dev.go new file mode 100644 index 000000000..ac47b0f5e --- /dev/null +++ b/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 +} diff --git a/lncfg/dev_integration.go b/lncfg/dev_integration.go new file mode 100644 index 000000000..c55b2efa7 --- /dev/null +++ b/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 +} diff --git a/server.go b/server.go index 5b421ae2f..e3fc80113 100644 --- a/server.go +++ b/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,