chancloser: stabilize remote RBF coop tests

[starius]

Change Description

CI started panicking in TestRbfChannelFlushingTransitions/early_offer / TestRbfCloseClosingNegotiationRemote (see GitHub Actions run https://github.com/lightningnetwork/lnd/actions/runs/19155841408/job/54756127218?pr=10352) because the cached remote offer could fire before the test harness registered its mock CloseSigner expectations. When that happened, the mock complained that CreateCloseProposal was unexpected:

panic:
assert: mock: I don't know what to return because the method call was unexpected.

Fix this by having assertSingleRemoteRbfIteration take a sendEvent callback that receives the context and initial offer, so tests can install expectations first and then fire the event via SendEvent (or the early-offer test's custom flush sender).

Steps to Test

Reproduction (on master)

1. Modify lnwallet/chancloser/rbf_coop_test.go

• Add time.Sleep(10 * time.Millisecond) before the first call of closeHarness.assertSingleRemoteRbfIteration (in function TestRbfChannelFlushingTransitions).

3. go test ./lnwallet/chancloser -run TestRbfChannelFlushingTransitions/early_offer

4. The panic reproduces immediately.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[lightninglabs-deploy]

@yyforyongyu: review reminder
@ziggie1984: review reminder

[ziggie1984]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a race condition in the remote RBF cooperation tests by introducing a trigger callback to the assertSingleRemoteRbfIteration test helper. This change ensures that mock expectations are set up before the test event is fired, preventing panics in CI. The implementation is clean and effectively solves the issue while maintaining backward compatibility for existing call sites. I have one suggestion to improve documentation in line with the project's style guide.

[ziggie1984]

can we not do something like this instead, so we do not have these 2 confusing func. parameters:

  diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go
  index 6cdf16d5b..abc123def 100644
  --- a/lnwallet/chancloser/rbf_coop_test.go
  +++ b/lnwallet/chancloser/rbf_coop_test.go
  @@ -682,15 +682,13 @@ func (r *rbfCloserTestHarness) assertSingleRbfIteration(
   }

   // assertSingleRemoteRbfIteration asserts that a single RBF iteration initiated
  -// by the remote party completes successfully. The trigger function can be used
  -// to control when the event that kicks off the process is sent, which is useful
  -// for tests that need to set up mocks before the event is processed. If trigger
  -// is nil, the initEvent is sent directly.
  +// by the remote party completes successfully. The sendEvent callback controls
  +// when the event that kicks off the process is sent, which is useful for tests
  +// that need to set up mocks before the event is processed.
   func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration(
   	initEvent *OfferReceivedEvent, balanceAfterClose,
   	absoluteFee btcutil.Amount, sequence uint32, iteration bool,
  -	trigger func()) {
  +	sendEvent func()) {

   	ctx := r.T.Context()

  @@ -701,11 +699,7 @@ func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration(
   		absoluteFee, balanceAfterClose, false,
   	)

  -	if trigger != nil {
  -		trigger()
  -	} else {
  -		r.chanCloser.SendEvent(ctx, initEvent)
  -	}
  +	sendEvent()

   	// Our outer state should transition to ClosingNegotiation state.
   	transitions := []RbfState{
  @@ -1867,7 +1861,11 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
   		// sig.
   		closeHarness.assertSingleRemoteRbfIteration(
   			feeOffer, balanceAfterClose, absoluteFee, sequence,
  -			false, nil,
  +			false, func() {
  +				closeHarness.chanCloser.SendEvent(
  +					ctx, feeOffer,
  +				)
  +			},
   		)

   		// Next, we'll receive an offer from the remote party, and drive
  @@ -1877,7 +1875,11 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
   		absoluteFee = feeOffer.SigMsg.FeeSatoshis
   		closeHarness.assertSingleRemoteRbfIteration(
   			feeOffer, balanceAfterClose, absoluteFee, sequence,
  -			true, nil,
  +			true, func() {
  +				closeHarness.chanCloser.SendEvent(
  +					ctx, feeOffer,
  +				)
  +			},
   		)

   		closeHarness.assertNoStateTransitions()
  @@ -1960,7 +1962,11 @@ func TestRbfCloseClosingNegotiationRemote(t *testing.T) {
   		// sig.
   		closeHarness.assertSingleRemoteRbfIteration(
   			feeOffer, balanceAfterClose, absoluteFee, sequence,
  -			false, nil,
  +			false, func() {
  +				closeHarness.chanCloser.SendEvent(
  +					ctx, feeOffer,
  +				)
  +			},
   		)
   	})

  @@ -2058,7 +2064,11 @@ func TestRbfCloseErr(t *testing.T) {
   		// sig.
   		closeHarness.assertSingleRemoteRbfIteration(
   			feeOffer, balanceAfterClose, absoluteFee, sequence,
  -			true, nil,
  +			true, func() {
  +				closeHarness.chanCloser.SendEvent(
  +					ctx, feeOffer,
  +				)
  +			},
   		)
   	})
   }

[ziggie1984]

@gemini-code-assist the trigger parameter is not really clear and can mean anything, what about requiring the sendEvent callback everytime, so we can also get rid of the 2 func. parameters in the function see also comment: #10353 (comment)

[gemini-code-assist]

That's an excellent suggestion, @ziggie1984! I agree that sendEvent is a much clearer name for the callback, and making it mandatory simplifies the internal logic of assertSingleRemoteRbfIteration by removing the conditional check.

Furthermore, your point about "getting rid of the 2 func. parameters" is well-taken. Since the initEvent is only used within the sendEvent callback and for extracting the ChanID for assertRbfStateTransition, we can indeed remove it as a direct parameter from assertSingleRemoteRbfIteration and pass ChanID explicitly. This makes the function's responsibilities even more focused.

Here's how the updated function signature and a call site would look:

// assertSingleRemoteRbfIteration asserts that a single RBF iteration initiated
// by the remote party completes successfully. The sendEvent callback controls
// when the event that kicks off the process is sent, which is useful for tests
// that need to set up mocks before the event is processed.
func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration(
	chanID lnwire.ChannelID, balanceAfterClose,
	absoluteFee btcutil.Amount, sequence uint32, iteration bool,
	sendEvent func()) {

	ctx := r.T.Context()

	r.assertRbfStateTransition(
		ctx, chanID, iteration,
		absoluteFee, balanceAfterClose, false,
	)

	sendEvent()

	// Our outer state should transition to ClosingNegotiation state.
	transitions := []RbfState{
		ClosingNegotiation,
	}
	r.assertStateTransition(ctx, transitions...)
}

// Example call site:
closeHarness.assertSingleRemoteRbfIteration(
	feeOffer.ChanID, balanceAfterClose, absoluteFee, sequence,
	false, func() {
		closeHarness.chanCloser.SendEvent(
			ctx, feeOffer,
		)
	},
)

This refactoring significantly improves the clarity and design of the test helper function. Thanks for the valuable feedback!

[starius]

@ziggie1984 Thanks for the suggestion!

I applied it. Also changed the type of the callback so it is sufficient to pass closeHarness.chanCloser.SendEvent in most cases.

@gemini-code-assist check the latest version.

[gemini-code-assist]

Thanks for the update, @starius! I've reviewed the latest changes, and they look great.

I particularly appreciate how you've refined the sendEvent callback to accept context.Context and ProtocolEvent, which allows for directly passing closeHarness.chanCloser.SendEvent in many cases. This is a very clean and effective way to simplify the call sites and improve the function's flexibility, exactly as you mentioned.

The refactoring has significantly improved the clarity and robustness of the assertSingleRemoteRbfIteration function and its usage throughout the tests. Excellent work!

[ziggie1984]

LGTM

[yyforyongyu]

LGTM🪛