lnwallet/chancloser: fix fllake in TestRbfCloseClosingNegotiationLocal/send_offer_rbf_wrong_local_script

In this commit, we fix a flake in the
`TestRbfCloseClosingNegotiationLocal/send_offer_rbf_wrong_local_script`
test.

This flake can happen if the test shuts down _before_ the state machine
is actually able to process the sent event. In this case, the
expectations are triggered, and we find that the error isn't sent.

To resolve this, we create a new wrapper function that'll use a sync
channel send to assert that the error has been sent before we exit the
test.

Author: Roasbeef

lnwallet/chancloser/mock.go

@@ -2,6 +2,7 @@ package chancloser
 
 import (
 	"sync/atomic"
+	"testing"
 
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcutil"
@@ -140,10 +141,32 @@ func (m *mockChanObserver) DisableChannel() error {
 
 type mockErrorReporter struct {
 	mock.Mock
+	errorReported chan error
+}
+
+// newMockErrorReporter creates a new mockErrorReporter.
+func newMockErrorReporter(t *testing.T) *mockErrorReporter {
+	return &mockErrorReporter{
+		// Buffer of 1 allows ReportError to send without blocking
+		// if the test isn't immediately ready to receive.
+		errorReported: make(chan error, 1),
+	}
 }
 
 func (m *mockErrorReporter) ReportError(err error) {
+	// Keep existing behavior of forwarding to mock.Mock
 	m.Called(err)
+
+	// Non-blockingly send the error to the channel.
+	select {
+	case m.errorReported <- err:
+
+	// If the channel is full or no one is listening, this prevents
+	// ReportError from blocking. This might happen if ReportError is called
+	// multiple times and the test only waits for the first, or if the test
+	// times out waiting.
+	default:
+	}
 }
 
 type mockCloseSigner struct {

lnwallet/chancloser/rbf_coop_test.go

@@ -146,14 +146,10 @@ func assertUnknownEventFail(t *testing.T, startingState ProtocolState) {
 		})
 		defer closeHarness.stopAndAssert()
 
-		closeHarness.expectFailure(ErrInvalidStateTransition)
-
-		closeHarness.chanCloser.SendEvent(
+		closeHarness.sendEventAndExpectFailure(
 			context.Background(), &unknownEvent{},
+			ErrInvalidStateTransition,
 		)
-
-		// There should be no further state transitions.
-		closeHarness.assertNoStateTransitions()
 	})
 }
 
@@ -239,6 +235,31 @@ func (r *rbfCloserTestHarness) assertExpectations() {
 	r.signer.AssertExpectations(r.T)
 }
 
+// sendEventAndExpectFailure is a helper to expect a failure, send an event,
+// and wait for the error report.
+func (r *rbfCloserTestHarness) sendEventAndExpectFailure(
+	ctx context.Context, event ProtocolEvent, expectedErr error) {
+
+	r.T.Helper()
+
+	r.expectFailure(expectedErr)
+
+	r.chanCloser.SendEvent(ctx, event)
+
+	select {
+	case reportedErr := <-r.errReporter.errorReported:
+		r.T.Logf("Test received error report: %v", reportedErr)
+
+		if !errors.Is(reportedErr, expectedErr) {
+			r.T.Fatalf("reported error (%v) is not the "+
+				"expected error (%v)", reportedErr, expectedErr)
+		}
+
+	case <-time.After(1 * time.Second):
+		r.T.Fatalf("timed out waiting for error to be "+
+			"reported by mockErrorReporter for event %T", event)
+	}
+}
 func (r *rbfCloserTestHarness) stopAndAssert() {
 	r.T.Helper()
 
@@ -728,7 +749,7 @@ func newRbfCloserTestHarness(t *testing.T,
 
 	feeEstimator := &mockFeeEstimator{}
 	mockObserver := &mockChanObserver{}
-	errReporter := &mockErrorReporter{}
+	errReporter := newMockErrorReporter(t)
 	mockSigner := &mockCloseSigner{}
 
 	harness := &rbfCloserTestHarness{
@@ -838,14 +859,13 @@ func TestRbfChannelActiveTransitions(t *testing.T) {
 		defer closeHarness.stopAndAssert()
 
 		closeHarness.failNewAddrFunc()
-		closeHarness.expectFailure(errfailAddr)
 
 		// We don't specify an upfront shutdown addr, and don't specify
 		// on here in the vent, so we should call new addr, but then
 		// fail.
-		closeHarness.chanCloser.SendEvent(ctx, &SendShutdown{})
-
-		// We shouldn't have transitioned to a new state.
+		closeHarness.sendEventAndExpectFailure(
+			ctx, &SendShutdown{}, errfailAddr,
+		)
 		closeHarness.assertNoStateTransitions()
 	})
 
@@ -902,16 +922,13 @@ func TestRbfChannelActiveTransitions(t *testing.T) {
 
 		// Next, we'll emit the recv event, with the addr of the remote
 		// party.
-		closeHarness.chanCloser.SendEvent(
-			ctx, &ShutdownReceived{
-				ShutdownScript: remoteAddr,
-				BlockHeight:    1,
-			},
+		event := &ShutdownReceived{
+			ShutdownScript: remoteAddr,
+			BlockHeight:    1,
+		}
+		closeHarness.sendEventAndExpectFailure(
+			ctx, event, ErrThawHeightNotReached,
 		)
-
-		// We expect a failure as the block height is less than the
-		// start height.
-		closeHarness.expectFailure(ErrThawHeightNotReached)
 	})
 
 	// When we receive a shutdown, we should transition to the shutdown
@@ -998,18 +1015,16 @@ func TestRbfShutdownPendingTransitions(t *testing.T) {
 		})
 		defer closeHarness.stopAndAssert()
 
-		// We should fail as the shutdown script isn't what we
-		// expected.
-		closeHarness.expectFailure(ErrUpfrontShutdownScriptMismatch)
-
 		// We'll now send in a ShutdownReceived event, but with a
 		// different address provided in the shutdown message. This
 		// should result in an error.
-		closeHarness.chanCloser.SendEvent(ctx, &ShutdownReceived{
+		event := &ShutdownReceived{
 			ShutdownScript: localAddr,
-		})
+		}
 
-		// We shouldn't have transitioned to a new state.
+		closeHarness.sendEventAndExpectFailure(
+			ctx, event, ErrUpfrontShutdownScriptMismatch,
+		)
 		closeHarness.assertNoStateTransitions()
 	})
 
@@ -1459,13 +1474,9 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) {
 			},
 		}
 
-		// We expect that the state machine fails as we received more
-		// than one error.
-		closeHarness.expectFailure(ErrTooManySigs)
-
-		// We should fail as the remote party sent us more than one
-		// signature.
-		closeHarness.chanCloser.SendEvent(ctx, localSigEvent)
+		closeHarness.sendEventAndExpectFailure(
+			ctx, localSigEvent, ErrTooManySigs,
+		)
 	})
 
 	// Next, we'll verify that if the balance of the remote party is dust,
@@ -1545,8 +1556,6 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) {
 
 		// The remote party will send a ClosingSig message, but with the
 		// wrong local script. We should expect an error.
-		closeHarness.expectFailure(ErrWrongLocalScript)
-
 		// We'll send this message in directly, as we shouldn't get any
 		// further in the process.
 		// assuming we start in this negotiation state.
@@ -1561,7 +1570,9 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) {
 				},
 			},
 		}
-		closeHarness.chanCloser.SendEvent(ctx, localSigEvent)
+		closeHarness.sendEventAndExpectFailure(
+			ctx, localSigEvent, ErrWrongLocalScript,
+		)
 	})
 
 	// In this test, we'll assert that we're able to restart the RBF loop
@@ -1705,21 +1716,19 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 		})
 		defer closeHarness.stopAndAssert()
 
-		// We should fail as they sent a sig, but can't pay for fees.
-		closeHarness.expectFailure(ErrRemoteCannotPay)
-
 		// We'll send in a new fee proposal, but the proposed fee will
 		// be higher than the remote party's balance.
-		feeOffer := &OfferReceivedEvent{
+		event := &OfferReceivedEvent{
 			SigMsg: lnwire.ClosingComplete{
 				CloserScript: remoteAddr,
 				CloseeScript: localAddr,
 				FeeSatoshis:  absoluteFee * 10,
 			},
 		}
-		closeHarness.chanCloser.SendEvent(ctx, feeOffer)
 
-		// We shouldn't have transitioned to a new state.
+		closeHarness.sendEventAndExpectFailure(
+			ctx, event, ErrRemoteCannotPay,
+		)
 		closeHarness.assertNoStateTransitions()
 	})
 
@@ -1747,12 +1756,9 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 		})
 		defer closeHarness.stopAndAssert()
 
-		// We should fail as they included the wrong sig.
-		closeHarness.expectFailure(ErrCloserNoClosee)
-
 		// Our balance is dust, so we should reject this signature that
 		// includes our output.
-		feeOffer := &OfferReceivedEvent{
+		event := &OfferReceivedEvent{
 			SigMsg: lnwire.ClosingComplete{
 				FeeSatoshis:  absoluteFee,
 				CloserScript: remoteAddr,
@@ -1764,9 +1770,9 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 				},
 			},
 		}
-		closeHarness.chanCloser.SendEvent(ctx, feeOffer)
-
-		// We shouldn't have transitioned to a new state.
+		closeHarness.sendEventAndExpectFailure(
+			ctx, event, ErrCloserNoClosee,
+		)
 		closeHarness.assertNoStateTransitions()
 	})
 
@@ -1778,12 +1784,9 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 		})
 		defer closeHarness.stopAndAssert()
 
-		// We should fail as they included the wrong sig.
-		closeHarness.expectFailure(ErrCloserAndClosee)
-
 		// Both balances are above dust, we should reject this
 		// signature as it excludes an output.
-		feeOffer := &OfferReceivedEvent{
+		event := &OfferReceivedEvent{
 			SigMsg: lnwire.ClosingComplete{
 				FeeSatoshis:  absoluteFee,
 				CloserScript: remoteAddr,
@@ -1795,9 +1798,9 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 				},
 			},
 		}
-		closeHarness.chanCloser.SendEvent(ctx, feeOffer)
-
-		// We shouldn't have transitioned to a new state.
+		closeHarness.sendEventAndExpectFailure(
+			ctx, event, ErrCloserAndClosee,
+		)
 		closeHarness.assertNoStateTransitions()
 	})
 
@@ -1875,11 +1878,9 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 
 		// The remote party will send a ClosingComplete message, but
 		// with the wrong local script. We should expect an error.
-		closeHarness.expectFailure(ErrWrongLocalScript)
-
 		// We'll send our remote addr as the Closee script, which should
 		// trigger an error.
-		feeOffer := &OfferReceivedEvent{
+		event := &OfferReceivedEvent{
 			SigMsg: lnwire.ClosingComplete{
 				FeeSatoshis:  absoluteFee,
 				CloserScript: remoteAddr,
@@ -1891,9 +1892,9 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
 				},
 			},
 		}
-		closeHarness.chanCloser.SendEvent(ctx, feeOffer)
-
-		// We shouldn't have transitioned to a new state.
+		closeHarness.sendEventAndExpectFailure(
+			ctx, event, ErrWrongLocalScript,
+		)
 		closeHarness.assertNoStateTransitions()
 	})