Update fee after channel becomes active #8876
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Approved CI run. |
|
This is very interesting, some lock-up is triggered if this timer is set to a small value. Couldn't that be a sign of a bigger issue somewhere? If it's set to 10-60 minutes as it is by now then the tests work fine. If I set it to less than 10 minutes it hangs. Could be a race condition between the tests? |
|
So the actual failing test is as well as setting the original PR line to |
|
Many tests seem to rely on update fee never happening unless explicitly requested. It either breaks their expectations or the link. Since the tests usually complete in under 2 minutes and the default fee update interval is at least 10 minutes those tests have never broken before. Not sure how to fix it reliably, I'd like the fee update to happen in 1-10 seconds after the link is established but then too many tests break so I set it to 1 minute now. Another way could be patching the mock links like this: diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go
index 9a72197ec..70e8e71f0 100644
--- a/htlcswitch/test_utils.go
+++ b/htlcswitch/test_utils.go
@@ -1185,7 +1185,7 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer,
}
}
}()
-
+ link.(*channelLink).updateFeeTimer.Reset(time.Minute * 10)
return link, nil
}and then adding a separate test to check for this short interval working as expected. Please tell me what solution is better for the project. |
There was a problem hiding this comment.
|
Yes, I tried this before and the tests failed in the same way. Changing the timer period is better in this case because we can delay the fee update if needed. Currently the update period is 10-60 minutes, all tests are faster than that, no test accounts for a random fee update in the middle of the run (in production it can happen of course in a similar scenario). If we make a fee update soon after a channel becomes active, the test gets an unexpected update and misbehaves. Either many tests should be fixed (other issues could be discovered during that but idk tho) or we pretend nothing changed and test the short update period separately. |
| l.updateFeeTimer = time.NewTimer(l.randomFeeUpdateTimeout()) | ||
| // this timer will fire in one minute after the channel is ready | ||
| // and it will reset to a random interval after that | ||
| l.updateFeeTimer = time.NewTimer(time.Minute) |
There was a problem hiding this comment.
Linking back to the issue,
Open the app, do any payments, close it in under 10 minutes
Though less likely, if the following happened we'd still have this issue or?
Open the app, do any payments, close it in under 1 minutes
| @@ -856,6 +856,10 @@ func TestChannelLinkCancelFullCommitment(t *testing.T) { | |||
| n := newTwoHopNetwork( | |||
| t, channels.aliceToBob, channels.bobToAlice, testStartingHeight, | |||
| ) | |||
| // This test takes a long time to finish and the link breaks if the fee | |||
| // update times out during it, blocking the test indefinitely | |||
| n.aliceChannelLink.updateFeeTimer.Reset(time.Minute * 10) | |||
There was a problem hiding this comment.
This mitigation works, though it may introduce flakiness to the unit tests since we are now relying on them to be finished in under 1min. We should still give the "send out a fee update immediately on startup" a shot, and I'll help figure out how to best handle the tests.
There was a problem hiding this comment.
Many tests break if we immediately send an update. If you're up to the task then sure, this change is trivial enough. But I still think it's better to set the timer to 0 instead (achieving the same effect without forcing it in all cases), reset it to 10 minutes for the tests + write a separate test for this instant update. I'll take a look in a day or two!
There was a problem hiding this comment.
@yyforyongyu Hi, maybe you could take this over? I really don't have time now it seems...
There was a problem hiding this comment.
sure, thanks for all the input so far!
|
@rkfg, remember to re-request review from reviewers when ready |
|
!lightninglabs-deploy mute |
Change Description
See #8790, PR recreated for several reasons. The fee update timer is initially set to zero to trigger the fee update check right after the channel becomes active, the next update interval will be random (between 10 and 60 minutes) as usual.
Steps to Test
Have a low commitment fee, make the estimated fee high and reconnect the peer. The commitment fee should update.
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.