← Watch feed
lnwallet/chancloser: remove racy "no more states" check
Public commit record
95/100 · Strong
What the developer wrote
Authored by Elle Mouton
lnwallet/chancloser: remove racy "no more states" check
The assertStateTransitions helper contained a non-blocking select
after consuming expected states to verify no additional transitions
occurred. This check is inherently racy: the state machine goroutine
can emit the next transition before the non-blocking select runs,
causing spurious "unexpected state transition" failures.
This was the primary cause of TestRbfCloseErr flakes (~29% failure
rate under coverage instrumentation).
To preserve strictness, add a post-Stop() quiet-period check in
stopAndAssert(). After the state machine is stopped, no further
transitions should be produced, so draining the subscriber channel
there is deterministic and catches any unexpected stragglers.
Also fix the CloseErr restart test paths (send_offer_restart,
recv_offer_restart) which were under-consuming transitions. Both
ClosePending and CloseErr produce an extra ClosingNegotiation
emission on restart via internal requeue, so the helpers now take
an explicit expectExtraTransition flag rather than the ambiguous
iteration bool.
Finally, register the state subscriber before Start() to avoid
racing with the initial state notification emitted by driveMachine.
The old ordering (Start then RegisterStateEvents) could miss the
first transition entirely on slow CI runners, causing a permanent
timeout in assertStartupAssertions.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode✓ Mentions testing or verification
What changed, and why it matters
This commit fixes flaky automated tests for a Lightning Network channel-closing feature. It removes a timing-sensitive check that sometimes falsely reported extra state changes, and adjusts the tests to expect the correct number of state transitions after restarts. There is no change to production code or user-facing behavior.
Recommended action
No security action required. Treat as a routine test-flake fix. Reviewers may optionally verify that the post-Stop quiet-period check adequately preserves the original test strictness.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch modifies lnwallet/chancloser/rbf_coop_test.go only. It removes a non-blocking select in assertStateTransitions that asserted no further state-machine transitions existed, because that check raced with the state-machine goroutine and caused false failures (notably ~29% flake rate for TestRbfCloseErr under coverage). It adds a deterministic post-Stop() quiet-period check in stopAndAssert, renames an ambiguous iteration bool to expectExtraTransition, and registers the state subscriber before Start() so the initial transition is not missed. No production logic is altered.
Changed components
lnwallet/chancloser/rbf_coop_test.goInspect captured patch +34 / −35
diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go
index 6a9389f..848d334 100644
--- a/lnwallet/chancloser/rbf_coop_test.go
+++ b/lnwallet/chancloser/rbf_coop_test.go
@@ -128,13 +128,6 @@ func assertStateTransitions[Event any, Env protofsm.Environment](
require.IsType(t, expectedState, newState)
}
-
- // We should have no more states.
- select {
- case newState := <-stateSub.NewItemCreated.ChanOut():
- t.Fatalf("unexpected state transition: %v", newState)
- default:
- }
}
// unknownEvent is a dummy event that is used to test that the state machine
@@ -280,6 +273,10 @@ func (r *rbfCloserTestHarness) stopAndAssert() {
defer r.chanCloser.RemoveStateSub(r.stateSub)
r.chanCloser.Stop()
+ // After Stop(), no further state transitions should be produced.
+ // Wait for a short quiet window to catch any unexpected stragglers.
+ r.assertNoStateTransitions()
+
r.assertExpectations()
}
@@ -577,7 +574,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, iteration bool) {
+ dustExpect dustExpectation, expectExtraTransition bool) {
ctx := context.Background()
numFeeCalls := 2
@@ -667,9 +664,10 @@ 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 {
+ // If we expect an extra transition (e.g. restarting from
+ // ClosePending or CloseErr), then we'll see an additional
+ // ClosingNegotiation emission from the internal requeue.
+ if expectExtraTransition {
expectedStates = append(
expectedStates, &ClosingNegotiation{},
)
@@ -726,7 +724,7 @@ func (r *rbfCloserTestHarness) expectHalfSignerIteration(
func (r *rbfCloserTestHarness) assertSingleRbfIteration(
initEvent ProtocolEvent, balanceAfterClose, absoluteFee btcutil.Amount,
- dustExpect dustExpectation, iteration bool) {
+ dustExpect dustExpectation, expectExtraTransition bool) {
ctx := context.Background()
@@ -734,7 +732,7 @@ func (r *rbfCloserTestHarness) assertSingleRbfIteration(
// the RBF loop, ending us in the LocalOfferSent state.
r.expectHalfSignerIteration(
initEvent, balanceAfterClose, absoluteFee, noDustExpect,
- iteration,
+ expectExtraTransition,
)
// Now that we're in the local offer sent state, we'll send the
@@ -791,7 +789,7 @@ func newPartialSigWithNonceTlv[T tlv.TlvType](psn lnwire.PartialSigWithNonce,
// that includes nonce handling for taproot channels.
func (r *rbfCloserTestHarness) assertSingleRbfIterationWithNonce(
initEvent ProtocolEvent, balanceAfterClose, absoluteFee btcutil.Amount,
- dustExpect dustExpectation, iteration bool,
+ dustExpect dustExpectation, expectExtraTransition bool,
nextCloseeNonce lnwire.Musig2Nonce) {
ctx := context.Background()
@@ -800,7 +798,7 @@ func (r *rbfCloserTestHarness) assertSingleRbfIterationWithNonce(
// the RBF loop, ending us in the LocalOfferSent state.
r.expectHalfSignerIteration(
initEvent, balanceAfterClose, absoluteFee, noDustExpect,
- iteration,
+ expectExtraTransition,
)
// Now that we're in the local offer sent state, we'll send the response
@@ -835,8 +833,8 @@ func (r *rbfCloserTestHarness) assertSingleRbfIterationWithNonce(
func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration(
initEvent *OfferReceivedEvent, balanceAfterClose,
- absoluteFee btcutil.Amount, sequence uint32, iteration bool,
- sendInit bool) {
+ absoluteFee btcutil.Amount, sequence uint32,
+ expectExtraTransition bool, sendInit bool) {
ctx := context.Background()
@@ -853,11 +851,10 @@ func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration(
}
// Our outer state should transition to ClosingNegotiation state.
- // If this is an iteration, we go ClosePending -> RemoteCloseStart ->
- // ClosePending, producing two ClosingNegotiation transitions. We
- // consume them in a single call to avoid the "no more states" check
- // draining the second transition before we can assert it.
- if iteration {
+ // When restarting from ClosePending or CloseErr, the internal
+ // requeue produces two ClosingNegotiation transitions. We consume
+ // them in a single call to keep assertions deterministic.
+ if expectExtraTransition {
r.assertStateTransitions(
&ClosingNegotiation{}, &ClosingNegotiation{},
)
@@ -1039,10 +1036,13 @@ func newRbfCloserTestHarness(t *testing.T,
).Return(nil)
chanCloser := protofsm.NewStateMachine(protoCfg)
- chanCloser.Start(ctx)
+ // Register the state subscriber before Start() to avoid racing
+ // with the initial state notification emitted by driveMachine.
harness.stateSub = chanCloser.RegisterStateEvents()
+ chanCloser.Start(ctx)
+
harness.chanCloser = &chanCloser
return harness
@@ -2962,16 +2962,13 @@ func TestRbfCloseErr(t *testing.T) {
TargetFeeRate: rbfFeeBump,
}
- // Now we expect that another full RBF iteration takes place (we
- // initiate a new local sig).
+ // Now we expect that another full RBF iteration takes place
+ // (we initiate a new local sig). Restarting from CloseErr
+ // produces an extra ClosingNegotiation transition from the
+ // internal requeue, same as a regular iteration.
closeHarness.assertSingleRbfIteration(
localOffer, balanceAfterClose, absoluteFee,
- noDustExpect, false,
- )
-
- // We should terminate in the negotiation state.
- closeHarness.assertStateTransitions(
- &ClosingNegotiation{},
+ noDustExpect, true,
)
})
@@ -3010,12 +3007,14 @@ func TestRbfCloseErr(t *testing.T) {
sequence := uint32(mempool.MaxRBFSequence)
- // As we're already in the negotiation phase, we'll now trigger
- // a new iteration by having the remote party send a new offer
- // sig.
+ // As we're already in the negotiation phase, we'll now
+ // trigger a new iteration by having the remote party send
+ // a new offer sig. Restarting from CloseErr produces an
+ // extra ClosingNegotiation transition from the internal
+ // requeue, same as a regular iteration.
closeHarness.assertSingleRemoteRbfIteration(
feeOffer, balanceAfterClose, absoluteFee, sequence,
- false, true,
+ true, true,
)
})
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.