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

@@ -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.

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