Refactor channel params: extract commitment params#3116
Merged
Conversation
t-bast
commented
Jun 23, 2025
| requireConfirmedInputs = requireConfirmedInputs, | ||
| requestFunding_opt = c.requestFunding_opt, | ||
| localChannelParams = localParams, | ||
| proposedCommitParams = nodeParams.channelConf.commitParams(c.fundingAmount, unlimitedMaxHtlcValueInFlight = false), |
Member
Author
There was a problem hiding this comment.
Note that when rebasing private feature branches, we must correctly set unlimitedMaxHtlcValueInFlight to true for mobile wallet users. Same in the non-initiator case below.
We previously allowed plugins to update parts of our `LocalParams` in the `OpenChannelInterceptor` flow: this was an attempt to allow adding funds to a dual-funded channel that wasn't initiated by us and set `max-htlc-value-in-flight` correctly (taking into account the added funding amount). This isn't used yet, and we will compute `max-htlc-value-in-flight` later in the following commits, so we remove this option, which simplifies the code.
We extract some of the channel parameters that are currently in our `LocalParams` and `RemoteParams` classes into a `CommitParams` class. While we currently simply create instances of that class based on our `LocalParams` and `RemoteParams`, the goal is to later move those values out of `LocalParams` and `RemoteParams` to clearly split params that apply to the channel for its entire lifetime from params that can be updated on a per-commitment basis. This is mostly straightforward refactoring, but there is one detail worth reviewing in-depth: we preivously used the `to_self_delay` from the *remote* parameters when creating our *local* commitments. We now instead initially set the `to_self_delay` in our *local* `CommitParams` based on the remote `to_remote_delay`, which has been renamed to make it more clear. This is a bit subtle, but hopefully will be better contained once we split `CommitParams` from `ChannelParams` in a future PR. We also refactor our unit tests to simplify the creation of the "init" messages. This should make it less painful whenever we have to update these tests.
We change our channel update helper to make it more compatible with the future `CommitParams` changes. This is a trivial refactoring without any change in behavior.
t-bast
force-pushed
the
introduce-commit-params
branch
from
3ce1335 to
8fbb684
Compare
Member
Author
|
Rebased to fix minor conflict with #3117 |
sstone
reviewed
Jun 26, 2025
sstone
approved these changes
Jun 26, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We extract some of the channel parameters that are currently in our
LocalParamsandRemoteParamsclasses into aCommitParamsclass. While we currently simply create instances of that class based on ourLocalParamsandRemoteParams, the goal is to later move those values out ofLocalParamsandRemoteParamsto clearly split params that apply to the channel for its entire lifetime from params that can be updated on a per-commitment basis.This is mostly straightforward refactoring, but there is one detail worth reviewing in-depth: we previously used the
to_self_delayfrom the remote parameters when creating our local commitments. We now instead initially set theto_self_delayin our localCommitParamsbased on the remoteto_remote_delay, which has been renamed to makeit more clear. This is a bit subtle, but hopefully will be better contained once we split
CommitParamsfromChannelParamsin a future PR.This is split into several independent commits: the bulk of the work is in the second commit.
I have tested e2e on regtest against
eclair/masterthat I didn't mess up the commitment params: I've used oneeclairinstance usingmasterand another using this branch, with different configurations fordust_limit,to_self_delay,max_accepted_htlcs,htlc_minimum,max_htlc_value_in_flight_msatand made payments that exercised those limits. Everything worked well 👍