chancloser: fix flakes in chancloser tests #10052

guggero · 2025-07-08T14:22:47Z

Fixed by gemini-cli.

Attempts to fix the following flake:

--- FAIL: TestRbfCloseErr (0.00s)
    --- FAIL: TestRbfCloseErr/send_offer_restart (0.00s)
        rbf_coop_test.go:381: no dust field set, expected: no dust
        rbf_coop_test.go:617: unexpected state transition: ClosingNegotiation(local=LocalOfferSent(proposed_fee=0.00010100 BTC), remote=<nil>)
        rbf_coop_test.go:381: no dust field set, expected: no dust
    --- FAIL: TestRbfCloseErr/recv_offer_restart (0.00s)
        rbf_coop_test.go:696: unexpected state transition: ClosingNegotiation(local=<nil>, remote=ClosePending(txid=4ebd325a4b394cff8c57e8317ccf5a8d0e2bdf1b8526f8aad6c8e43d8240621a, party=Remote, fee_rate=1010 sat/vb))
--- FAIL: TestRbfCloseClosingNegotiationRemote (0.05s)
    --- FAIL: TestRbfCloseClosingNegotiationRemote/recv_offer_rbf_loop_iterations (0.00s)
        rbf_coop_test.go:696: unexpected state transition: ClosingNegotiation(local=LocalCloseStart, remote=ClosePending(txid=4ebd325a4b394cff8c57e8317ccf5a8d0e2bdf1b8526f8aad6c8e43d8240621a, party=Remote, fee_rate=1110 sat/vb))

gemini-code-assist

Summary of Changes

Hello @guggero, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses and fixes flaky tests within the chancloser package. The primary goal is to enhance the stability and reliability of cooperative channel closing tests, particularly those involving RBF and Taproot fast closes, by refining test logic related to transaction broadcasting and correcting assertion expectations for dust outputs.

Highlights

Improved Test Determinism: Refactored the TestTaprootFastClose in chancloser_test.go to use distinct broadcast channels for Alice and Bob. This change replaces a single shared broadcast signal with separate channels (aliceBroadcast and bobBroadcast), ensuring that the test correctly waits for the specific party's transaction broadcast, thereby preventing potential race conditions and improving test reliability.
Corrected RBF Test Assertions: Updated the rbf_coop_test.go file to correct an assertion parameter within the assertSingleRbfIteration function. The parameter passed to expectHalfSignerIteration was changed from noDustExpect to dustExpect, indicating a more accurate expectation regarding dust outputs during RBF (Replace-by-Fee) cooperative closing iterations, which resolves a source of test flakiness.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request aims to fix flaky tests in the chancloser package. The changes are well-targeted and improve test stability.

In chancloser_test.go, the TestTaprootFastClose test is refactored to use two separate channels for broadcast signals from Alice and Bob, instead of a single shared channel. This is an excellent change that eliminates potential race conditions and makes the test more deterministic.

In rbf_coop_test.go, a bug in the assertSingleRbfIteration test helper is fixed. The function was previously ignoring its dustExpect parameter. This has been corrected to properly propagate the parameter, ensuring the helper function behaves as intended.

The changes are correct and improve the quality of the test suite. I have no further suggestions.

yyforyongyu

Thank you🙏

yyforyongyu

hmm looks like we still got one more flake

yyforyongyu · 2025-07-09T12:47:26Z

Makefile

 	$(UNIT_BENCH)

+#? unit-flake: Run the unit tests continuously until one fails
+unit-flake: $(BTCD_BIN)


nit: name it flakehunter-unit-all to keep them consistent below?

guggero · 2025-07-09T12:53:04Z

hmm looks like we still got one more flake

Yeah, whenever I think I've patched one flake, another pops up... Not sure this is easy to fix. Or I (and Gemini) are just missing something simple... Tried quite a few iterations locally.

yyforyongyu · 2025-07-15T12:18:32Z

Yeah, whenever I think I've patched one flake, another pops up... Not sure this is easy to fix. Or I (and Gemini) are just missing something simple... Tried quite a few iterations locally.

yeah why don't we merge what we have so far and then fix it in another PR? Think as long as the number of flaky tests decreases it's still a win.

guggero · 2025-07-15T14:23:39Z

yeah why don't we merge what we have so far and then fix it in another PR? Think as long as the number of flaky tests decreases it's still a win.

Makes sense. I pushed up one more commit that increases some of the timeouts, perhaps that also helps a bit.

yyforyongyu

Thanks for the fix! LGTM❤️

ellemouton

Thanks for the flake fixes!

just a few questions

lnwallet/chancloser/chancloser_test.go

netann/chan_status_manager_test.go

The param previously was unused so this was likely an oversight or copy/paste error. This does not change any behavior, as the method was always called with the constant previously used. But it makes things more explicit.

This doesn't change the behavior but makes the use of the broadcast channels more clear.

guggero · 2025-07-18T12:08:30Z

Okay, after investing HUMAN debugging minutes, I actually found the flake :D

ellemouton

Nice, thanks!! ✅

These two new goals take all the usual flags like pkg=, case=, nocache= and so on.

This fixes the actual flake: We didn't wait for next iterations to _not_ come in, so depending on timing, we could read another transition in a second run. But making sure we expect all transitions at the same time and then make sure no further transitions come in makes this not depend on time.

These tests previously only worked because we didn't properly wait for all transitions to come in. We fix them by correctly asserting the expected number of state transitions.

Fixes another flake in the unit-race test: Sometimes we miss startup events if there's a high CPU load (in CI for example). To avoid that, we register our subscriber before starting the state machine.

guggero · 2025-07-21T12:47:05Z

Added another flake fix, this time for the race unit test. Also fixed some more formatting issues.

ellemouton

🧹 🧼

ellemouton · 2025-07-23T09:23:56Z

lnwallet/chancloser/rbf_coop_test.go

+	// We register our subscriber before starting the state machine, to make
+	// sure we don't miss any events.


gemini-code-assist bot reviewed Jul 8, 2025

View reviewed changes

yyforyongyu approved these changes Jul 8, 2025

View reviewed changes

guggero added no-itest no-changelog labels Jul 8, 2025

guggero force-pushed the rbf-close-err branch from b307372 to a663188 Compare July 8, 2025 18:31

yyforyongyu reviewed Jul 9, 2025

View reviewed changes

guggero force-pushed the rbf-close-err branch from a663188 to d3821dc Compare July 15, 2025 14:21

guggero requested review from ellemouton and yyforyongyu July 15, 2025 14:23

yyforyongyu approved these changes Jul 16, 2025

View reviewed changes

ellemouton reviewed Jul 16, 2025

View reviewed changes

lnwallet/chancloser/chancloser_test.go Show resolved Hide resolved

netann/chan_status_manager_test.go Outdated Show resolved Hide resolved

guggero force-pushed the rbf-close-err branch from d3821dc to 1be66d0 Compare July 18, 2025 12:06

guggero added 2 commits July 18, 2025 14:07

chancloser: use param instead of constant

899131f

The param previously was unused so this was likely an oversight or copy/paste error. This does not change any behavior, as the method was always called with the constant previously used. But it makes things more explicit.

chancloser: use separate broadcast channels

8023583

This doesn't change the behavior but makes the use of the broadcast channels more clear.

guggero force-pushed the rbf-close-err branch from 1be66d0 to 3397cc7 Compare July 18, 2025 12:07

guggero requested a review from ellemouton July 18, 2025 12:08

ellemouton approved these changes Jul 21, 2025

View reviewed changes

guggero added 6 commits July 21, 2025 13:48

chancloser: fix nolint comments, fix formatting

0860f2d

make: add flakehunter-unit-all and flakehunter-unit-race

f96ad47

These two new goals take all the usual flags like pkg=, case=, nocache= and so on.

chancloser: increase timeouts

263d070

chancloser: fix error tests

15d8f96

These tests previously only worked because we didn't properly wait for all transitions to come in. We fix them by correctly asserting the expected number of state transitions.

chancloser: fix flake by registering subscriber first

1882939

Fixes another flake in the unit-race test: Sometimes we miss startup events if there's a high CPU load (in CI for example). To avoid that, we register our subscriber before starting the state machine.

guggero force-pushed the rbf-close-err branch from 3397cc7 to 1882939 Compare July 21, 2025 11:54

guggero requested a review from ellemouton July 21, 2025 12:47

ellemouton approved these changes Jul 23, 2025

View reviewed changes

guggero merged commit 6f09d96 into lightningnetwork:master Jul 23, 2025
26 checks passed

guggero deleted the rbf-close-err branch July 23, 2025 09:47

guggero mentioned this pull request Aug 6, 2025

release: create v0.19.3-rc1 branch #10134

Merged

		// We register our subscriber before starting the state machine, to make
		// sure we don't miss any events.

chancloser: fix flakes in chancloser tests #10052

chancloser: fix flakes in chancloser tests #10052

Uh oh!

Conversation

guggero commented Jul 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

guggero commented Jul 9, 2025

Uh oh!

yyforyongyu commented Jul 15, 2025

Uh oh!

guggero commented Jul 15, 2025

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guggero commented Jul 18, 2025

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

guggero commented Jul 21, 2025

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

ellemouton Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants