From 8e10da03c707e684d3283508e6058e22d70d1ab7 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 19 Nov 2019 20:12:07 -0800 Subject: [PATCH 1/4] lntest/itest: move chanpoint instantiation down to where it's used --- lntest/itest/lnd_test.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index 5817e85db..f58191003 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -2247,9 +2247,19 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf("unable to assert channel existence: %v", err) } - // Finally, immediately close the channel. This function will also - // block until the channel is closed and will additionally assert the - // relevant channel closing post conditions. + // Disconnect Alice-peer from Bob-peer and get error causes by one + // active channel with detach node is existing. + if err := net.DisconnectNodes(ctxt, alice, bob); err == nil { + t.Fatalf("Bob's peer was disconnected from Alice's"+ + " while one active channel is existing: err %v", err) + } + + // Check existing connection. + assertNumConnections(t, alice, bob, 1) + + // Finally, immediately close the channel. This function will also block + // until the channel is closed and will additionally assert the relevant + // channel closing post conditions. chanPoint := &lnrpc.ChannelPoint{ FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{ FundingTxidBytes: pendingUpdate.Txid, @@ -2257,16 +2267,6 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { OutputIndex: pendingUpdate.OutputIndex, } - // Disconnect Alice-peer from Bob-peer and get error - // causes by one active channel with detach node is existing. - if err := net.DisconnectNodes(ctxt, net.Alice, net.Bob); err == nil { - t.Fatalf("Bob's peer was disconnected from Alice's"+ - " while one active channel is existing: err %v", err) - } - - // Check existing connection. - assertNumConnections(t, net.Alice, net.Bob, 1) - ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout) closeChannelAndAssert(ctxt, t, net, net.Alice, chanPoint, true) From c9c72167688f4d536f642ac3e5aca7308afd0b85 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 19 Nov 2019 20:12:29 -0800 Subject: [PATCH 2/4] lntest/itest: remove global alice and bob from disconnect test This will allow us to start Alice and Bob with additional arguments once we switch to making unsafe-disconnect true by default. --- lntest/itest/lnd_test.go | 84 ++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 29 deletions(-) diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index f58191003..753368fdf 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -2172,13 +2172,38 @@ func testOpenChannelAfterReorg(net *lntest.NetworkHarness, t *harnessTest) { closeReorgedChannelAndAssert(ctxt, t, net, net.Alice, chanPoint, false) } -// testDisconnectingTargetPeer performs a test which -// disconnects Alice-peer from Bob-peer and then re-connects them again +// testDisconnectingTargetPeer performs a test which disconnects Alice-peer from +// Bob-peer and then re-connects them again func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { ctxb := context.Background() + alice, err := net.NewNode("Alice", nil) + if err != nil { + t.Fatalf("unable to create new node: %v", err) + } + defer shutdownAndAssert(net, t, alice) + + bob, err := net.NewNode("Bob", nil) + if err != nil { + t.Fatalf("unable to create new node: %v", err) + } + defer shutdownAndAssert(net, t, bob) + + // Start by connecting Alice and Bob with no channels. + ctxt, _ := context.WithTimeout(ctxb, defaultTimeout) + if err := net.ConnectNodes(ctxt, alice, bob); err != nil { + t.Fatalf("unable to connect Alice's peer to Bob's: err %v", err) + } + // Check existing connection. - assertNumConnections(t, net.Alice, net.Bob, 1) + assertNumConnections(t, alice, bob, 1) + + // Give Alice some coins so she can fund a channel. + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + err = net.SendCoins(ctxt, btcutil.SatoshiPerBitcoin, alice) + if err != nil { + t.Fatalf("unable to send coins to carol: %v", err) + } chanAmt := lnd.MaxBtcFundingAmount pushAmt := btcutil.Amount(0) @@ -2186,22 +2211,23 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { // Create a new channel that requires 1 confs before it's considered // open, then broadcast the funding transaction const numConfs = 1 - ctxt, _ := context.WithTimeout(ctxb, channelOpenTimeout) - pendingUpdate, err := net.OpenPendingChannel(ctxt, net.Alice, net.Bob, - chanAmt, pushAmt) + ctxt, _ = context.WithTimeout(ctxb, channelOpenTimeout) + pendingUpdate, err := net.OpenPendingChannel( + ctxt, alice, bob, chanAmt, pushAmt, + ) if err != nil { t.Fatalf("unable to open channel: %v", err) } - // At this point, the channel's funding transaction will have - // been broadcast, but not confirmed. Alice and Bob's nodes - // should reflect this when queried via RPC. + // At this point, the channel's funding transaction will have been + // broadcast, but not confirmed. Alice and Bob's nodes should reflect + // this when queried via RPC. ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - assertNumOpenChannelsPending(ctxt, t, net.Alice, net.Bob, 1) + assertNumOpenChannelsPending(ctxt, t, alice, bob, 1) - // Disconnect Alice-peer from Bob-peer and get error - // causes by one pending channel with detach node is existing. - if err := net.DisconnectNodes(ctxt, net.Alice, net.Bob); err == nil { + // Disconnect Alice-peer from Bob-peer and get error causes by one + // pending channel with detach node is existing. + if err := net.DisconnectNodes(ctxt, alice, bob); err == nil { t.Fatalf("Bob's peer was disconnected from Alice's"+ " while one pending channel is existing: err %v", err) } @@ -2209,7 +2235,7 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { time.Sleep(time.Millisecond * 300) // Check existing connection. - assertNumConnections(t, net.Alice, net.Bob, 1) + assertNumConnections(t, alice, bob, 1) fundingTxID, err := chainhash.NewHash(pendingUpdate.Txid) if err != nil { @@ -2223,15 +2249,15 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { block := mineBlocks(t, net, numConfs, 1)[0] assertTxInBlock(t, block, fundingTxID) - // At this point, the channel should be fully opened and there should - // be no pending channels remaining for either node. + // At this point, the channel should be fully opened and there should be + // no pending channels remaining for either node. time.Sleep(time.Millisecond * 300) ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - assertNumOpenChannelsPending(ctxt, t, net.Alice, net.Bob, 0) + assertNumOpenChannelsPending(ctxt, t, alice, bob, 0) - // The channel should be listed in the peer information returned by - // both peers. + // The channel should be listed in the peer information returned by both + // peers. outPoint := wire.OutPoint{ Hash: *fundingTxID, Index: pendingUpdate.OutputIndex, @@ -2239,11 +2265,11 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { // Check both nodes to ensure that the channel is ready for operation. ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - if err := net.AssertChannelExists(ctxt, net.Alice, &outPoint); err != nil { + if err := net.AssertChannelExists(ctxt, alice, &outPoint); err != nil { t.Fatalf("unable to assert channel existence: %v", err) } ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - if err := net.AssertChannelExists(ctxt, net.Bob, &outPoint); err != nil { + if err := net.AssertChannelExists(ctxt, bob, &outPoint); err != nil { t.Fatalf("unable to assert channel existence: %v", err) } @@ -2268,13 +2294,13 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { } ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout) - closeChannelAndAssert(ctxt, t, net, net.Alice, chanPoint, true) + closeChannelAndAssert(ctxt, t, net, alice, chanPoint, true) - // Disconnect Alice-peer from Bob-peer without getting error - // about existing channels. + // Disconnect Alice-peer from Bob-peer without getting error about + // existing channels. var predErr error err = wait.Predicate(func() bool { - if err := net.DisconnectNodes(ctxt, net.Alice, net.Bob); err != nil { + if err := net.DisconnectNodes(ctxt, alice, bob); err != nil { predErr = err return false } @@ -2286,19 +2312,19 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { } // Check zero peer connections. - assertNumConnections(t, net.Alice, net.Bob, 0) + assertNumConnections(t, alice, bob, 0) // Finally, re-connect both nodes. ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - if err := net.ConnectNodes(ctxt, net.Alice, net.Bob); err != nil { + if err := net.ConnectNodes(ctxt, alice, bob); err != nil { t.Fatalf("unable to connect Alice's peer to Bob's: err %v", err) } // Check existing connection. - assertNumConnections(t, net.Alice, net.Bob, 1) + assertNumConnections(t, alice, net.Bob, 1) // Cleanup by mining the force close and sweep transaction. - cleanupForceClose(t, net, net.Alice, chanPoint) + cleanupForceClose(t, net, alice, chanPoint) } // testFundingPersistence is intended to ensure that the Funding Manager From aa3e74043daa4ad1ded01f932ce56ee7665ea19b Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 19 Nov 2019 20:12:48 -0800 Subject: [PATCH 3/4] lntest/harness: improve error message for AssertChannelExists predErr wasn't always being set when the predicate failed, replace with wait.NoError. --- lntest/harness.go | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/lntest/harness.go b/lntest/harness.go index d017b75dc..524177504 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -1195,28 +1195,25 @@ func (n *NetworkHarness) AssertChannelExists(ctx context.Context, req := &lnrpc.ListChannelsRequest{} - var predErr error - pred := func() bool { + return wait.NoError(func() error { resp, err := node.ListChannels(ctx, req) if err != nil { - predErr = fmt.Errorf("unable fetch node's channels: %v", err) - return false + return fmt.Errorf("unable fetch node's channels: %v", err) } for _, channel := range resp.Channels { if channel.ChannelPoint == chanPoint.String() { - return channel.Active + if channel.Active { + return nil + } + + return fmt.Errorf("channel %s inactive", + chanPoint) } - } - return false - } - if err := wait.Predicate(pred, time.Second*15); err != nil { - return fmt.Errorf("channel not found: %v", predErr) - } - - return nil + return fmt.Errorf("channel %s not found", chanPoint) + }, 15*time.Second) } // DumpLogs reads the current logs generated by the passed node, and returns From 216b933d9bba4c1d7aa91ea98905f23a7ab4d95b Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 19 Nov 2019 20:13:04 -0800 Subject: [PATCH 4/4] config+itest: allow unsafe disconnect by default This commit beings the process of deprecating unsafe-disconnect. Many moons ago this was disallowed to prevent concurency bugs surrounding reconnect. Despite the name, it has been safe to enable this feature for well over a year, as several PRs have been merged that addressed the possible issues that existed when the feature was added. --- config.go | 3 +- lntest/itest/lnd_test.go | 60 +++++++++++++++++++++++----------------- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/config.go b/config.go index 20b2fe0ef..064315a09 100644 --- a/config.go +++ b/config.go @@ -271,7 +271,7 @@ type config struct { Profile string `long:"profile" description:"Enable HTTP profiling on given port -- NOTE port must be between 1024 and 65535"` - UnsafeDisconnect bool `long:"unsafe-disconnect" description:"Allows the rpcserver to intentionally disconnect from peers with open channels. USED FOR TESTING ONLY."` + UnsafeDisconnect bool `long:"unsafe-disconnect" description:"DEPRECATED: Allows the rpcserver to intentionally disconnect from peers with open channels. THIS FLAG WILL BE REMOVED IN 0.10.0"` UnsafeReplay bool `long:"unsafe-replay" description:"Causes a link to replay the adds on its commitment txn after starting up, this enables testing of the sphinx replay logic."` MaxPendingChannels int `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."` BackupFilePath string `long:"backupfilepath" description:"The target location of the channel backup file"` @@ -389,6 +389,7 @@ func loadConfig() (*config, error) { Dir: defaultLitecoindDir, RPCHost: defaultRPCHost, }, + UnsafeDisconnect: true, MaxPendingChannels: DefaultMaxPendingChannels, NoSeedBackup: defaultNoSeedBackup, MinBackoff: defaultMinBackoff, diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index 753368fdf..9df319b20 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -2173,17 +2173,25 @@ func testOpenChannelAfterReorg(net *lntest.NetworkHarness, t *harnessTest) { } // testDisconnectingTargetPeer performs a test which disconnects Alice-peer from -// Bob-peer and then re-connects them again +// Bob-peer and then re-connects them again. We expect Alice to be able to +// disconnect at any point. func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { ctxb := context.Background() - alice, err := net.NewNode("Alice", nil) + // We'll start both nodes with a high backoff so that they don't + // reconnect automatically during our test. + args := []string{ + "--minbackoff=1m", + "--maxbackoff=1m", + } + + alice, err := net.NewNode("Alice", args) if err != nil { t.Fatalf("unable to create new node: %v", err) } defer shutdownAndAssert(net, t, alice) - bob, err := net.NewNode("Bob", nil) + bob, err := net.NewNode("Bob", args) if err != nil { t.Fatalf("unable to create new node: %v", err) } @@ -2227,15 +2235,15 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { // Disconnect Alice-peer from Bob-peer and get error causes by one // pending channel with detach node is existing. - if err := net.DisconnectNodes(ctxt, alice, bob); err == nil { + if err := net.DisconnectNodes(ctxt, alice, bob); err != nil { t.Fatalf("Bob's peer was disconnected from Alice's"+ " while one pending channel is existing: err %v", err) } time.Sleep(time.Millisecond * 300) - // Check existing connection. - assertNumConnections(t, alice, bob, 1) + // Assert that the connection was torn down. + assertNumConnections(t, alice, bob, 0) fundingTxID, err := chainhash.NewHash(pendingUpdate.Txid) if err != nil { @@ -2256,6 +2264,12 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { assertNumOpenChannelsPending(ctxt, t, alice, bob, 0) + // Reconnect the nodes so that the channel can become active. + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + if err := net.ConnectNodes(ctxt, alice, bob); err != nil { + t.Fatalf("unable to connect Alice's peer to Bob's: err %v", err) + } + // The channel should be listed in the peer information returned by both // peers. outPoint := wire.OutPoint{ @@ -2275,13 +2289,19 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { // Disconnect Alice-peer from Bob-peer and get error causes by one // active channel with detach node is existing. - if err := net.DisconnectNodes(ctxt, alice, bob); err == nil { + if err := net.DisconnectNodes(ctxt, alice, bob); err != nil { t.Fatalf("Bob's peer was disconnected from Alice's"+ " while one active channel is existing: err %v", err) } // Check existing connection. - assertNumConnections(t, alice, bob, 1) + assertNumConnections(t, alice, bob, 0) + + // Reconnect both nodes before force closing the channel. + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + if err := net.ConnectNodes(ctxt, alice, bob); err != nil { + t.Fatalf("unable to connect Alice's peer to Bob's: err %v", err) + } // Finally, immediately close the channel. This function will also block // until the channel is closed and will additionally assert the relevant @@ -2298,17 +2318,9 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) { // Disconnect Alice-peer from Bob-peer without getting error about // existing channels. - var predErr error - err = wait.Predicate(func() bool { - if err := net.DisconnectNodes(ctxt, alice, bob); err != nil { - predErr = err - return false - } - return true - }, time.Second*15) - if err != nil { + if err := net.DisconnectNodes(ctxt, alice, bob); err != nil { t.Fatalf("unable to disconnect Bob's peer from Alice's: err %v", - predErr) + err) } // Check zero peer connections. @@ -3595,9 +3607,8 @@ func testSphinxReplayPersistence(net *lntest.NetworkHarness, t *harnessTest) { defer shutdownAndAssert(net, t, dave) // Next, we'll create Carol and establish a channel to from her to - // Dave. Carol is started in both unsafe-replay and unsafe-disconnect, - // which will cause her to replay any pending Adds held in memory upon - // reconnection. + // Dave. Carol is started in both unsafe-replay which will cause her to + // replay any pending Adds held in memory upon reconnection. carol, err := net.NewNode("Carol", []string{"--unsafe-replay"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) @@ -11541,7 +11552,7 @@ func testSwitchOfflineDelivery(net *lntest.NetworkHarness, t *harnessTest) { // Carol -> Dave -> Alice -> Bob // // First, we'll create Dave and establish a channel to Alice. - dave, err := net.NewNode("Dave", []string{"--unsafe-disconnect"}) + dave, err := net.NewNode("Dave", nil) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -11871,7 +11882,7 @@ func testSwitchOfflineDeliveryPersistence(net *lntest.NetworkHarness, t *harness // Carol -> Dave -> Alice -> Bob // // First, we'll create Dave and establish a channel to Alice. - dave, err := net.NewNode("Dave", []string{"--unsafe-disconnect"}) + dave, err := net.NewNode("Dave", nil) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -12208,7 +12219,7 @@ func testSwitchOfflineDeliveryOutgoingOffline( // Carol -> Dave -> Alice -> Bob // // First, we'll create Dave and establish a channel to Alice. - dave, err := net.NewNode("Dave", []string{"--unsafe-disconnect"}) + dave, err := net.NewNode("Dave", nil) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -12948,7 +12959,6 @@ func testSendUpdateDisableChannel(net *lntest.NetworkHarness, t *harnessTest) { carol, err := net.NewNode("Carol", []string{ "--minbackoff=10s", - "--unsafe-disconnect", "--chan-enable-timeout=1.5s", "--chan-disable-timeout=3s", "--chan-status-sample-interval=.5s",