From d28242c66417448774a45665f0da80f6034dac7c Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Mon, 12 Jun 2023 23:41:20 +0800
Subject: [PATCH] peer: return an error from `updateNextRevocation` and patch
 unit tests

This commit makes the `updateNextRevocation` to return an error and
further feed it through the request's error chan so it'd be handled by
the caller.
---
 peer/brontide.go      | 23 ++++++++++++-------
 peer/brontide_test.go | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/peer/brontide.go b/peer/brontide.go
index e85e3d8b0..3e84042de 100644
--- a/peer/brontide.go
+++ b/peer/brontide.go
@@ -3734,7 +3734,7 @@ func (p *Brontide) attachChannelEventSubscription() error {
 
 // updateNextRevocation updates the existing channel's next revocation if it's
 // nil.
-func (p *Brontide) updateNextRevocation(c *channeldb.OpenChannel) {
+func (p *Brontide) updateNextRevocation(c *channeldb.OpenChannel) error {
 	chanPoint := &c.FundingOutpoint
 	chanID := lnwire.NewChanIDFromOutPoint(chanPoint)
 
@@ -3744,32 +3744,35 @@ func (p *Brontide) updateNextRevocation(c *channeldb.OpenChannel) {
 	// currentChan should exist, but we perform a check anyway to avoid nil
 	// pointer dereference.
 	if !loaded {
-		p.log.Errorf("missing active channel with chanID=%v", chanID)
-		return
+		return fmt.Errorf("missing active channel with chanID=%v",
+			chanID)
 	}
 
 	// currentChan should not be nil, but we perform a check anyway to
 	// avoid nil pointer dereference.
 	if currentChan == nil {
-		p.log.Errorf("found nil active channel with chanID=%v", chanID)
-		return
+		return fmt.Errorf("found nil active channel with chanID=%v",
+			chanID)
 	}
 
 	// If we're being sent a new channel, and our existing channel doesn't
 	// have the next revocation, then we need to update the current
 	// existing channel.
 	if currentChan.RemoteNextRevocation() != nil {
-		return
+		return nil
 	}
 
 	p.log.Infof("Processing retransmitted ChannelReady for "+
 		"ChannelPoint(%v)", chanPoint)
 
 	nextRevoke := c.RemoteNextRevocation
+
 	err := currentChan.InitNextRevocation(nextRevoke)
 	if err != nil {
-		p.log.Errorf("unable to init chan revocation: %v", err)
+		return fmt.Errorf("unable to init next revocation: %w", err)
 	}
+
+	return nil
 }
 
 // addActiveChannel adds a new active channel to the `activeChannels` map. It
@@ -3855,7 +3858,11 @@ func (p *Brontide) handleNewActiveChannel(req *newChannelMsg) {
 
 		// Handle it and close the err chan on the request.
 		close(req.err)
-		p.updateNextRevocation(newChan)
+
+		// Update the next revocation point.
+		if err := p.updateNextRevocation(newChan); err != nil {
+			p.log.Errorf(err.Error())
+		}
 
 		return
 	}
diff --git a/peer/brontide_test.go b/peer/brontide_test.go
index cf71e0b92..4b0929574 100644
--- a/peer/brontide_test.go
+++ b/peer/brontide_test.go
@@ -1118,3 +1118,56 @@ func TestPeerCustomMessage(t *testing.T) {
 	require.Equal(t, remoteKey, receivedCustom.peer)
 	require.Equal(t, receivedCustomMsg, &receivedCustom.msg)
 }
+
+// TestUpdateNextRevocation checks that the method `updateNextRevocation` is
+// behave as expected.
+func TestUpdateNextRevocation(t *testing.T) {
+	t.Parallel()
+
+	require := require.New(t)
+
+	// TODO(yy): create interface for lnwallet.LightningChannel so we can
+	// easily mock it without the following setups.
+	notifier := &mock.ChainNotifier{
+		SpendChan: make(chan *chainntnfs.SpendDetail),
+		EpochChan: make(chan *chainntnfs.BlockEpoch),
+		ConfChan:  make(chan *chainntnfs.TxConfirmation),
+	}
+	broadcastTxChan := make(chan *wire.MsgTx)
+	mockSwitch := &mockMessageSwitch{}
+
+	alicePeer, bobChan, err := createTestPeer(
+		t, notifier, broadcastTxChan, noUpdate, mockSwitch,
+	)
+	require.NoError(err, "unable to create test channels")
+
+	// testChannel is used to test the updateNextRevocation function.
+	testChannel := bobChan.State()
+
+	// Update the next revocation for a known channel should give us no
+	// error.
+	err = alicePeer.updateNextRevocation(testChannel)
+	require.NoError(err, "expected no error")
+
+	// Test an error is returned when the chanID cannot be found in
+	// `activeChannels` map.
+	testChannel.FundingOutpoint = wire.OutPoint{Index: 0}
+	err = alicePeer.updateNextRevocation(testChannel)
+	require.Error(err, "expected an error")
+
+	// Test an error is returned when the chanID's corresponding channel is
+	// nil.
+	testChannel.FundingOutpoint = wire.OutPoint{Index: 1}
+	chanID := lnwire.NewChanIDFromOutPoint(&testChannel.FundingOutpoint)
+	alicePeer.activeChannels.Store(chanID, nil)
+
+	err = alicePeer.updateNextRevocation(testChannel)
+	require.Error(err, "expected an error")
+
+	// TODO(yy): should also test `InitNextRevocation` is called on
+	// `lnwallet.LightningWallet` once it's interfaced.
+}
+
+// TODO(yy): add test for `addActiveChannel` and `handleNewActiveChannel` once
+// we have interfaced `lnwallet.LightningChannel` and
+// `*contractcourt.ChainArbitrator`.