-
Notifications
You must be signed in to change notification settings - Fork 2.2k
chancloser: stabilize remote RBF coop tests #10353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
4bacf0e to
233742c
Compare
|
@yyforyongyu: review reminder |
233742c to
1386d3f
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration( | ||
| initEvent *OfferReceivedEvent, balanceAfterClose, | ||
| absoluteFee btcutil.Amount, sequence uint32, iteration bool, | ||
| sendInit bool) { | ||
| trigger func()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature of this function has been modified, but it lacks a comment explaining its purpose and the new trigger parameter. According to the repository's style guide, all functions should be commented to improve readability and maintainability.
| func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration( | |
| initEvent *OfferReceivedEvent, balanceAfterClose, | |
| absoluteFee btcutil.Amount, sequence uint32, iteration bool, | |
| sendInit bool) { | |
| trigger func()) { | |
| // 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. | |
| func (r *rbfCloserTestHarness) assertSingleRemoteRbfIteration( | |
| initEvent *OfferReceivedEvent, balanceAfterClose, | |
| absoluteFee btcutil.Amount, sequence uint32, iteration bool, | |
| trigger func()) { |
References
- The style guide (lines 17-20) requires every function to be commented with its purpose and assumptions. The comment must start with the function name and be a complete sentence. (link)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
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 letting assertSingleRemoteRbfIteration take a trigger callback. Tests that rely on asynchronously replayed offers now install their expectations first and then fire the event via the callback. All other call sites pass nil, preserving their existing behavior. 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). 2. go test ./lnwallet/chancloser -run TestRbfChannelFlushingTransitions/early_offer 3. The panic reproduces immediately.
1386d3f to
110f7a5
Compare
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 mockCloseSignerexpectations. When that happened, the mock complained thatCreateCloseProposalwas unexpected:Fix this by letting
assertSingleRemoteRbfIterationtake a trigger callback. Tests that rely on asynchronously replayed offers now install their expectations first and then fire the event via the callback. All other call sites pass nil, preserving their existing behavior.Steps to Test
Reproduction (on master)
lnwallet/chancloser/rbf_coop_test.gotime.Sleep(10 * time.Millisecond)before the first call ofcloseHarness.assertSingleRemoteRbfIteration(in functionTestRbfChannelFlushingTransitions).go test ./lnwallet/chancloser -run TestRbfChannelFlushingTransitions/early_offerThe panic reproduces immediately.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.