From 16067ca908378f1115766fab177a56536b024119 Mon Sep 17 00:00:00 2001 From: eugene Date: Thu, 23 Jun 2022 12:57:52 -0400 Subject: [PATCH] funding: defer sending channel_update until received funding_locked This is required by BOLT#07 as otherwise the counter-party will discard the channel_update as they may not consider the channel "ready" or reorg-safe. Most other implementations besides eclair have work-arounds for this, but it is nice to be spec-compliant. --- funding/manager.go | 56 +++++++++++++++++- funding/manager_test.go | 123 +++++++++++++++++++++++----------------- 2 files changed, 125 insertions(+), 54 deletions(-) diff --git a/funding/manager.go b/funding/manager.go index 56d575f4f..d10872bf6 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -981,6 +981,22 @@ func (f *Manager) stateStep(channel *channeldb.OpenChannel, // fundingLocked was sent to peer, but the channel was not added to the // router graph and the channel announcement was not sent. case fundingLockedSent: + // We must wait until we've received the peer's funding locked + // before sending a channel_update according to BOLT#07. + received, err := f.receivedFundingLocked( + channel.IdentityPub, chanID, + ) + if err != nil { + return fmt.Errorf("failed to check if funding locked "+ + "was received: %v", err) + } + + if !received { + // We haven't received FundingLocked, so we'll continue + // to the next iteration of the loop. + return nil + } + var peerAlias *lnwire.ShortChannelID if channel.IsZeroConf() { // We'll need to wait until funding_locked has been @@ -1003,7 +1019,7 @@ func (f *Manager) stateStep(channel *channeldb.OpenChannel, peerAlias = &foundAlias } - err := f.addToRouterGraph(channel, shortChanID, peerAlias, nil) + err = f.addToRouterGraph(channel, shortChanID, peerAlias, nil) if err != nil { return fmt.Errorf("failed adding to "+ "router graph: %v", err) @@ -2871,6 +2887,44 @@ func (f *Manager) sendFundingLocked(completeChan *channeldb.OpenChannel, return nil } +// receivedFundingLocked checks whether or not we've received a FundingLocked +// from the remote peer. If we have, RemoteNextRevocation will be set. +func (f *Manager) receivedFundingLocked(node *btcec.PublicKey, + chanID lnwire.ChannelID) (bool, error) { + + // If the funding manager has exited, return an error to stop looping. + // Note that the peer may appear as online while the funding manager + // has stopped due to the shutdown order in the server. + select { + case <-f.quit: + return false, ErrFundingManagerShuttingDown + default: + } + + // Check whether the peer is online. If it's not, we'll wait for them + // to come online before proceeding. This is to avoid a tight loop if + // they're offline. + connected := make(chan lnpeer.Peer, 1) + var peerKey [33]byte + copy(peerKey[:], node.SerializeCompressed()) + f.cfg.NotifyWhenOnline(peerKey, connected) + + select { + case <-connected: + case <-f.quit: + return false, ErrFundingManagerShuttingDown + } + + channel, err := f.cfg.FindChannel(node, chanID) + if err != nil { + log.Errorf("Unable to locate ChannelID(%v) to determine if "+ + "FundingLocked was received", chanID) + return false, err + } + + return channel.RemoteNextRevocation != nil, nil +} + // extractAnnounceParams extracts the various channel announcement and update // parameters that will be needed to construct a ChannelAnnouncement and a // ChannelUpdate. diff --git a/funding/manager_test.go b/funding/manager_test.go index 604844cd2..d10a0c766 100644 --- a/funding/manager_test.go +++ b/funding/manager_test.go @@ -1317,6 +1317,14 @@ func TestFundingManagerNormalWorkflow(t *testing.T) { // Check that the state machine is updated accordingly assertFundingLockedSent(t, alice, bob, fundingOutPoint) + // Exchange the fundingLocked messages. + alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) + bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) + + // Check that they notify the breach arbiter and peer about the new + // channel. + assertHandleFundingLocked(t, alice, bob) + // Make sure both fundingManagers send the expected channel // announcements. assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) @@ -1328,14 +1336,6 @@ func TestFundingManagerNormalWorkflow(t *testing.T) { // OpenStatusUpdate_ChanOpen update waitForOpenUpdate(t, updateChan) - // Exchange the fundingLocked messages. - alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) - bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) - - // Check that they notify the breach arbiter and peer about the new - // channel. - assertHandleFundingLocked(t, alice, bob) - // Notify that six confirmations has been reached on funding transaction. alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ Tx: fundingTx, @@ -1749,6 +1749,15 @@ func TestFundingManagerOfflinePeer(t *testing.T) { bobPubKey, peer) } + // Before sending on the con chan, update Alice's NotifyWhenOnline + // function so that the next invocation in receivedFundingLocked will + // use this new function. + alice.fundingMgr.cfg.NotifyWhenOnline = func(peer [33]byte, + connected chan<- lnpeer.Peer) { + + connected <- bob + } + // Restore the correct sendMessage implementation, and notify that Bob // is back online. bob.sendMessage = workingSendMessage @@ -1762,6 +1771,14 @@ func TestFundingManagerOfflinePeer(t *testing.T) { // The state should now be fundingLockedSent assertDatabaseState(t, alice, fundingOutPoint, fundingLockedSent) + // Exchange the fundingLocked messages. + alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) + bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) + + // Check that they notify the breach arbiter and peer about the new + // channel. + assertHandleFundingLocked(t, alice, bob) + // Make sure both fundingManagers send the expected channel // announcements. assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) @@ -1773,14 +1790,6 @@ func TestFundingManagerOfflinePeer(t *testing.T) { // OpenStatusUpdate_ChanOpen update waitForOpenUpdate(t, updateChan) - // Exchange the fundingLocked messages. - alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) - bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) - - // Check that they notify the breach arbiter and peer about the new - // channel. - assertHandleFundingLocked(t, alice, bob) - // Notify that six confirmations has been reached on funding transaction. alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ Tx: fundingTx, @@ -2169,17 +2178,6 @@ func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { // Check that the state machine is updated accordingly assertFundingLockedSent(t, alice, bob, fundingOutPoint) - // Make sure both fundingManagers send the expected channel - // announcements. - assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) - - // Check that the state machine is updated accordingly - assertAddedToRouterGraph(t, alice, bob, fundingOutPoint) - - // The funding transaction is now confirmed, wait for the - // OpenStatusUpdate_ChanOpen update - waitForOpenUpdate(t, updateChan) - // Send the fundingLocked message twice to Alice, and once to Bob. alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) @@ -2208,6 +2206,17 @@ func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { // Expected } + // Make sure both fundingManagers send the expected channel + // announcements. + assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) + + // Check that the state machine is updated accordingly + assertAddedToRouterGraph(t, alice, bob, fundingOutPoint) + + // The funding transaction is now confirmed, wait for the + // OpenStatusUpdate_ChanOpen update + waitForOpenUpdate(t, updateChan) + // Notify that six confirmations has been reached on funding transaction. alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ Tx: fundingTx, @@ -2272,6 +2281,14 @@ func TestFundingManagerRestartAfterChanAnn(t *testing.T) { // Check that the state machine is updated accordingly assertFundingLockedSent(t, alice, bob, fundingOutPoint) + // Exchange the fundingLocked messages. + alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) + bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) + + // Check that they notify the breach arbiter and peer about the new + // channel. + assertHandleFundingLocked(t, alice, bob) + // Make sure both fundingManagers send the expected channel // announcements. assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) @@ -2288,14 +2305,6 @@ func TestFundingManagerRestartAfterChanAnn(t *testing.T) { // we expect her to be able to handle it correctly. recreateAliceFundingManager(t, alice) - // Exchange the fundingLocked messages. - alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) - bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) - - // Check that they notify the breach arbiter and peer about the new - // channel. - assertHandleFundingLocked(t, alice, bob) - // Notify that six confirmations has been reached on funding transaction. alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ Tx: fundingTx, @@ -2444,14 +2453,6 @@ func TestFundingManagerPrivateChannel(t *testing.T) { // Check that the state machine is updated accordingly assertFundingLockedSent(t, alice, bob, fundingOutPoint) - // Make sure both fundingManagers send the expected channel - // announcements. - assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) - - // The funding transaction is now confirmed, wait for the - // OpenStatusUpdate_ChanOpen update - waitForOpenUpdate(t, updateChan) - // Exchange the fundingLocked messages. alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) @@ -2460,6 +2461,14 @@ func TestFundingManagerPrivateChannel(t *testing.T) { // channel. assertHandleFundingLocked(t, alice, bob) + // Make sure both fundingManagers send the expected channel + // announcements. + assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) + + // The funding transaction is now confirmed, wait for the + // OpenStatusUpdate_ChanOpen update + waitForOpenUpdate(t, updateChan) + // Notify that six confirmations has been reached on funding transaction. alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ Tx: fundingTx, @@ -2557,6 +2566,14 @@ func TestFundingManagerPrivateRestart(t *testing.T) { // Check that the state machine is updated accordingly assertFundingLockedSent(t, alice, bob, fundingOutPoint) + // Exchange the fundingLocked messages. + alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) + bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) + + // Check that they notify the breach arbiter and peer about the new + // channel. + assertHandleFundingLocked(t, alice, bob) + // Make sure both fundingManagers send the expected channel // announcements. assertChannelAnnouncements(t, alice, bob, capacity, nil, nil) @@ -2570,14 +2587,6 @@ func TestFundingManagerPrivateRestart(t *testing.T) { // OpenStatusUpdate_ChanOpen update waitForOpenUpdate(t, updateChan) - // Exchange the fundingLocked messages. - alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) - bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) - - // Check that they notify the breach arbiter and peer about the new - // channel. - assertHandleFundingLocked(t, alice, bob) - // Notify that six confirmations has been reached on funding transaction. alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ Tx: fundingTx, @@ -2905,15 +2914,23 @@ func TestFundingManagerCustomChannelParameters(t *testing.T) { // After the funding transaction is mined, Alice will send // fundingLocked to Bob. - _ = assertFundingMsgSent( + fundingLockedAlice := assertFundingMsgSent( t, alice.msgChan, "FundingLocked", ).(*lnwire.FundingLocked) // And similarly Bob will send funding locked to Alice. - _ = assertFundingMsgSent( + fundingLockedBob := assertFundingMsgSent( t, bob.msgChan, "FundingLocked", ).(*lnwire.FundingLocked) + // Exchange the fundingLocked messages. + alice.fundingMgr.ProcessFundingMsg(fundingLockedBob, bob) + bob.fundingMgr.ProcessFundingMsg(fundingLockedAlice, alice) + + // Check that they notify the breach arbiter and peer about the new + // channel. + assertHandleFundingLocked(t, alice, bob) + // Make sure both fundingManagers send the expected channel // announcements. // Alice should advertise the default MinHTLC value of