From 40f58da16156eb07f594b77e2ec4326a1c767187 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 26 Mar 2025 21:17:53 -0700 Subject: [PATCH 1/2] lnwallet/chancloser: fix flake in TestRbfCloseClosingNegotiationLocal In this commit, we fix a flake in the rbf loop sub-test for the TestRbfCloseClosingNegotiationLocal test case. The fix here is that when we go from ClosePending for an RBF iteration loop, we first transition to LocalCloseStart. However we only do this extra transition if we're doing an iteration (starting from ClosePending). To fix this, we add a new bool that tracks if this is an iteration or not. We can then also eliminate the extra assertion at the end, as we'll terminate in `ClosePending` which is checked by `assertLocalClosePending()` in `assertSingleRbfIteration`. Fixes https://github.com/lightningnetwork/lnd/issues/9526. --- lnwallet/chancloser/rbf_coop_test.go | 40 ++++++++++++++-------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go index ab2526e7771..df19b1fd36a 100644 --- a/lnwallet/chancloser/rbf_coop_test.go +++ b/lnwallet/chancloser/rbf_coop_test.go @@ -508,7 +508,7 @@ func (d dustExpectation) String() string { // message to the remote party, and all the other intermediate steps. func (r *rbfCloserTestHarness) expectHalfSignerIteration( initEvent ProtocolEvent, balanceAfterClose, absoluteFee btcutil.Amount, - dustExpect dustExpectation) { + dustExpect dustExpectation, iteration bool) { ctx := context.Background() numFeeCalls := 2 @@ -569,8 +569,16 @@ func (r *rbfCloserTestHarness) expectHalfSignerIteration( } case *SendOfferEvent: - expectedStates = []RbfState{&ClosingNegotiation{}} + + // If we're in the middle of an iteration, then we expect a + // transition from ClosePending -> LocalCloseStart. + if iteration { + expectedStates = append( + expectedStates, &ClosingNegotiation{}, + ) + } + case *ChannelFlushed: // If we're sending a flush event here, then this means that we // also have enough balance to cover the fee so we'll have @@ -585,10 +593,6 @@ func (r *rbfCloserTestHarness) expectHalfSignerIteration( // We should transition from the negotiation state back to // itself. - // - // TODO(roasbeef): take in expected set of transitions!!! - // * or base off of event, if shutdown recv'd know we're doing a full - // loop r.assertStateTransitions(expectedStates...) // If we examine the final resting state, we should see that @@ -610,7 +614,7 @@ func (r *rbfCloserTestHarness) expectHalfSignerIteration( func (r *rbfCloserTestHarness) assertSingleRbfIteration( initEvent ProtocolEvent, balanceAfterClose, absoluteFee btcutil.Amount, - dustExpect dustExpectation) { + dustExpect dustExpectation, iteration bool) { ctx := context.Background() @@ -618,6 +622,7 @@ func (r *rbfCloserTestHarness) assertSingleRbfIteration( // the RBF loop, ending us in the LocalOfferSent state. r.expectHalfSignerIteration( initEvent, balanceAfterClose, absoluteFee, noDustExpect, + iteration, ) // Now that we're in the local offer sent state, we'll send the @@ -1299,7 +1304,7 @@ func TestRbfChannelFlushingTransitions(t *testing.T) { // flow. closeHarness.expectHalfSignerIteration( &flushEvent, balanceAfterClose, absoluteFee, - noDustExpect, + noDustExpect, false, ) }) } @@ -1418,7 +1423,7 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) { // pending state. closeHarness.assertSingleRbfIteration( sendOfferEvent, balanceAfterClose, absoluteFee, - noDustExpect, + noDustExpect, false, ) }) @@ -1434,7 +1439,7 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) { // event to advance the state machine. closeHarness.expectHalfSignerIteration( sendOfferEvent, balanceAfterClose, absoluteFee, - noDustExpect, + noDustExpect, false, ) // We'll now craft the local sig received event, but this time @@ -1489,7 +1494,7 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) { // proper field is set. closeHarness.expectHalfSignerIteration( sendOfferEvent, balanceAfterClose, absoluteFee, - remoteDustExpect, + remoteDustExpect, false, ) }) @@ -1516,7 +1521,7 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) { dustBalance := btcutil.Amount(100) closeHarness.expectHalfSignerIteration( sendOfferEvent, dustBalance, absoluteFee, - localDustExpect, + localDustExpect, false, ) }) @@ -1581,7 +1586,7 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) { // assuming we start in this negotiation state. closeHarness.assertSingleRbfIteration( sendOfferEvent, balanceAfterClose, absoluteFee, - noDustExpect, + noDustExpect, false, ) // Next, we'll send in a new SendOfferEvent event which @@ -1596,12 +1601,7 @@ func TestRbfCloseClosingNegotiationLocal(t *testing.T) { // initiate a new local sig). closeHarness.assertSingleRbfIteration( localOffer, balanceAfterClose, absoluteFee, - noDustExpect, - ) - - // We should terminate in the negotiation state. - closeHarness.assertStateTransitions( - &ClosingNegotiation{}, + noDustExpect, true, ) }) @@ -2004,7 +2004,7 @@ func TestRbfCloseErr(t *testing.T) { // initiate a new local sig). closeHarness.assertSingleRbfIteration( localOffer, balanceAfterClose, absoluteFee, - noDustExpect, + noDustExpect, false, ) // We should terminate in the negotiation state. From eb863e46af2bdd0b4db3098a4b89ff339fb0b039 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 10 Jun 2025 18:44:34 -0700 Subject: [PATCH 2/2] 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. --- lnwallet/chancloser/mock.go | 23 +++++ lnwallet/chancloser/rbf_coop_test.go | 127 ++++++++++++++------------- 2 files changed, 87 insertions(+), 63 deletions(-) diff --git a/lnwallet/chancloser/mock.go b/lnwallet/chancloser/mock.go index a2cc4270c1e..970c6b03652 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 df19b1fd36a..58b02f6eafe 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() })