lnwallet: allow opener to dip into its reserve.

[ziggie1984]

Fixes: #8249

See also ACINQ/eclair#2899 for more background.

Although zero-fee commitments are on the horizon it will be probably take longer until all new channels are upgraded to this new type where we basically don't need those exceptions anymore. With the current channel type we need this exception to make splicing work and also protect against some force closes with other node implementation e.g. eclair which excepts the dipping into the reserve in some case.

There are some cases where we should allow the peer paying the channel fees (channel opener) to be dipped into its reserve. These will be detailed in the following to have an easier way to understand this PR.

We need to look at this problem from the perspective of the different messages which are allowed to change the channel state in Lightning causing the commitment fees to potentially increase. (Update_FullFill and Update_Fail are not mentioned here because they do not increase the fee requirements but only resolve htlcs from the commitment).

1. Receive HTLC
2. Send HTLC
3. Receive CommitSig
4. Send CommitSig (alias SignNextCommitment)
5. Send FeeUpdate (Opener only)
6. Receive FeeUpdate (Non-Opener only)

Now we need to look at those messages from the perspective of the channel opener paying the fees and the non-opener.

In general ONLY the peer paying the fees is allowed to be dipped into its reserve. A non-opener can be below the reserve (when a channel without push_amt is opened) but the balance should only increase not decrease hence its important to notice that we are only looking into the cases where the initial balance of one side decreased.

Channel Opener:
Looking into the case when the local reserve is allowed to be dipped.

1. When receiving an Update HTLC should be allowed to be dipped into its local reserve unconditionally (solves the splicing case, resolves stuck channels, helps in the concurrent case where and outgoing and incoming were added at the same time.
2. Sending an HTLC the local reserve is not allowed to be dipped into its reserve, we also have the feeBuffer in place here so this should never happen.
3. Signing the Next State, here we essentially need to skip the LocalReserve Check because objectively speaking we don't need to check this constraint here because all changes and the constraints are already done in the other cases. However I decided to keep this check but just not fail if we fall below the reserve because of the concurrent adding of HTLCs. The problem at this point is that we cannot really know which side dipped the local here into its reserve therefore we just allow it.
4. Same as 3, we basically skip the local reserve check by allow the local reserve to be dipped at this stage.
5. Sending FeeUpdate we already keep an additional buffer and don't allow the local reserve to be dipped here. We will not attempt the FeeUpdate if we are below the local reserve. This is no problem because the state has not locked in yet.
6. Cannot Receive the FeeUpdate because the Opener creates it.

Channel Non-Opener.
We look now when the remote reserve is allowed to be dipped.

1. When receiving an HTLC, we do not allow the peer to be dipped into its reserve, we always evaluate the htlc against the acked outgoing htlcs, so the peer is never allowed to dip himself into the reserve.
2. Sending an HTLC, we allow the dipping into the remote reserve => ONLY add this via a config flag so we don't mess with older LND nodes ?
3. Skip the remote check here same as for the opener case
4. Skip the remote check here same as for the opener case
5. Not applicable
6. Don't allow the Peer to dip into its reserve via the fee update because we check this new proposal against the already ACKED outgoing htlcs. But because we might have already unsigned Outgoing HTLCs from our point of we, this would allow the peer to dip into its reserve implicitly by the weight of the unsigned outgoing HTLCs. => This case is important to prevent force closes with other nodes like eclair, which already takes the dipping into the reserve into account here.

[coderabbitai]

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

• llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[ziggie1984]

Looking for a Concept ACK here and also an Approach ACK before adding tests.

[ziggie1984]

What should be taken into account when deciding whether we should go with this change:

1. We should definitely make sure we do not dip the remote into its reserve when the remote is the opener because all old LND nodes would basically force-close which do not allow the dipping and only allow this after 1-2 releases, is this really worth it then ?
2. Would only prevent force-closes with Eclair nodes because we already take into account an additional HTLC when updating the fee for example.
3. Fix stuck channels which I think there are not many out there and only happen if they use pre 18 LND versions as a channel opener.
4. Adds complexity maybe really hold this change off and go straight to Zero-FeeCommitments.
5. As long as we have no splicing other then the above case with the Fee_Update there is no real win for LND nodes including this change because we already protect us against concurrent cases with the FeeBuffer
6. Wait for solid splicing PRs in LND and decide form there.

So I kinda tend to go straight to Zero-FeeCommitments or even Anchor Channels with no FeeUpdates as soon as Bitcoind 28 is rolled out widely.

Curios what you all think about this.

[t-bast]

So I kinda tend to go straight to Zero-FeeCommitments or even Anchor Channels with no FeeUpdates as soon as Bitcoind 28 is rolled out widely.

That will take a very long time before it's available: I think it's worth improving the reserve requirements today to avoid more force-closed channels (I suspect some force-closed channels happen because of races between update_fee and update_add_htlc because of too strict reserve requirements - they could be avoided with those changes).

The most important thing to release ASAP is to ensure that as the channel opener, you allow receiving HTLCs that make you dip into your reserve. From the channel opener's point of view, it's a 100% win: you don't have anything to lose by dipping into your own channel reserve, it is your peer that may be taking a risk when that happens. We had discussed that behavior a long time ago in spec meetings, and all implementations had committed to ship this behavior, but I'm not sure lnd has actually shipped it since, so it's really time to do it?

Sending an HTLC, we allow the dipping into the remote reserve => ONLY add this via a config flag so we don't mess with older LND nodes ?

If you haven't yet shipped the behavior described above, you may indeed want to only allow this when you think the remote node supports it. That can be implicitly done with good enough accuracy by checking for a new (unrelated) feature that would be added in the same release as these new reserve checks?

[ziggie1984]

Thanks for taking the time @t-bast.

Great I totally agree we should ship the case where we allow incoming HTLCs as the opener to be more in line with other implementations.

If you haven't yet shipped the behavior described above, you may indeed want to only allow this when you think the remote node supports it. That can be implicitly done with good enough accuracy by checking for a new (unrelated) feature that would be added in the same release as these new reserve checks?

Need to think about a good way to also enable sending support in terms of backwards compatibility, your proposal is definitely one.

[morehouse]

I agree we should allow dipping into our reserve as an HTLC receiver. I'm not sure about letting the other party dip into their reserve to pay commitment fees though...

How often do these stuck channels happen? Perhaps we should increase the fee spike buffer instead...