diff --git a/lnwallet/chancloser/mock.go b/lnwallet/chancloser/mock.go index a2cc4270c..970c6b036 100644 --- a/lnwallet/chancloser/mock.go +++ b/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 { diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go index df19b1fd3..58b02f6ea 100644 --- a/lnwallet/chancloser/rbf_coop_test.go +++ b/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() })