Add `NoopAdd` HTLCs

[GeorgeTsagk]

Description

Adds a new type of HTLC named "noop add", which behaves identically to normal HTLCs except for the settling part. If upon settlement the receiver has an above dust balance, then the amount is returned back to the sender and the only thing that ends up being updated is the aux leaf of the commitment, which will successfully reflect any overlay changes.

Replacement for https://github.com/lightningnetwork/lnd/pull/9430

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

:label: 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.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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.
  • Explain this complex logic.
  • 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 explain this code block.
  • @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 explain its main purpose.
  • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this 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]

NACK, in my opinion we should be very careful progressing in this direction:

1. This needs proper documentation, what is the exact use case, to understand the reasoning behind this you have to know the specifics of the TapAss design

2. Looks like a bandaid to the ChannelState maschine a very crucial part of the LND software, it needs way more tests to also verify normal behaviour is not touched

3. Moreover the case where we currently check if the amount is above the reserve (and treat it as a NOOP or an ADD) does not work, and should be observable via itests, from different perspectives we would credit different parties. So the so called NOOP is an ADD in some cases which is super confusing.

[GeorgeTsagk]

NACK, in my opinion we should be very careful progressing in this direction:

1. This needs proper documentation, what is the exact use case, to understand the reasoning behind this you have to know the specifics of the TapAss design
2. Looks like a bandaid to the ChannelState maschine a very crucial part of the LND software, it needs way more tests to also verify normal behaviour is not touched
3. Moreover the case where we currently check if the amount is above the reserve (and treat it as a NOOP or an ADD) does not work, and should be observable via itests, from different perspectives we would credit different parties. So the so called NOOP is an ADD in some cases which is super confusing.

1. Will add proper documentation, thanks for pointing out.
2. Will add some unit coverage that exposes the noop_add path, as far as preserving the old behavior isn't the existing unit coverage enough? Unless what you're saying is that we need to run the entire coverage but with noop_add enabled?
3. What do you mean by "does not work"? This behavior is currently only exposed when running LND side by side with Tapd, see the LiT itest PR where most of payments default to noop_add HTLCs (it's tapd that sets the TLV flag for LND to flag it as noop). Yeap it's true that a noop can be a normal add and that's if the receiver is below the chan reserve.

Thanks for your attention and the review so far Will address all of the above points

[GeorgeTsagk]

Pushed a version which adds more docs, and simplifies the crediting logic

Also in order to avoid violating the chan reserve check I now take into account the accumulated balance delta for the party we're performing the check for. So now we check if balance + delta exceeds reserve, to make the call on if we apply the noop.

I added the "failure" path which reverts to old behavior https://github.com/lightningnetwork/lnd/blob/08080831fc10cbac641e7537df6bac1f49c2ef56/lnwallet/channel.go#L10087-L10100

[ziggie1984]

I think this change before further work needs an design ACK from @yyforyongyu who has the most expertise in the htlcswitch package. So I would keep myself as a third reviewer.

[ZZiigguurraatt]

For any new TLV, I think they need to be properly documented in https://github.com/Roasbeef/blips/blob/tap-blip/blip-tap.md and any other relevant places. The taproot assets protocol is getting more and more hard to understand because of missing definitions (see also https://github.com/lightninglabs/taproot-assets/issues/1446).

[Roasbeef]

And if the no-op goes on chain, it looks and behaves the same way as a normal "add" HTLC?

Yes it should behave as a normal add HTLC. Parallel HTLCs will each have an non-dust output on the commitment transaction, but once settled off-chain, we'll credit the sender.

There's another optimization that we can do here to instead just aggregate many aux HTLCs into a single one progressively. The need to have similar time locks to do it safely though. Going even further, there's a more elaborate path where there's a single HTLC that commits to a tree of future HTLCs, CTV style. I think that's out of scope for now, as we'd want to add something like that to the BOLT specs themselves, as they're a way to effectively make commitment transactions never grow in size with additional HTLCs, which makes the process of just force closing much cheaper.

[yyforyongyu]

/gemini review

[GeorgeTsagk]

Pushed a version that addresses the critical bug reported here

I correctly cast the delta value and adjust the balanceAboveReserve calculation accordingly

Enhanced the table driven tests for evaluateNoOpHTLC with cases where we have negative deltas too

[GeorgeTsagk]

/gemini review

[lightninglabs-deploy]

@roasbeef: review reminder @yyforyongyu: review reminder @georgetsagk, remember to re-request review from reviewers when ready

[GeorgeTsagk]

I think this new type adds lots of complexity to the existing flow, and the expectation from the usage of Noop adds must be explicitly documented (maybe in tapd). I think the purpose of noop can be defeated via multiple ways,

• if the remote sets a large channel reserve, sats will always be claimed by the receiver.
• if the remote receives lots of payments and refuses to settle, they can claim them via FC - this can be mitigated via restricting the num of inflight payments.
• if the custom channel is used to send normal HTLCs, the receiver is in danger as the payments can be claimed back.

I guess these are handled in the tapd side?

Thanks for the great points @yyforyongyu

I'd like to reply to the last 2 bullets:

The fact that the HTLCs still actually carry the amount while in-flight is also sort of a feature. We want to be able to materialize the HTLCs on chain if that needs to happen, as the metadata that are present in the outpoint must be preserved (that's where the state of the custom channels resides). Below dust amounts would contribute towards fees and the HTLCs would get lost into the void.

if the custom channel is used to send normal HTLCs

Yes, the custom channel can be used to send normal HTLCs but it would never do so and also set the noop TLV flag for it. That's a strong responsibility of the external software -- to make sane decisions around when an HTLC should be treated as a noop.

[GeorgeTsagk]

Rebased on latest master

[Roasbeef]

if the remote receives lots of payments and refuses to settle, they can claim them via FC - this can be mitigated via restricting the num of inflight payments.

Correct that since we only apply the noop on settle, each pending HTLC still needs that above-dust-amount to anchor it to the commitment transaction. If they go to chain, then the receiver can pull the committed amount, and also that above-dust-amount.

The only direct way to get rid of this final limitation would be to switch to a zero-value output. However, those are disallowed by default policy. May we need the degens to ~ab~use this feature so it becomes standard....

if the custom channel is used to send normal HTLCs, the receiver is in danger as the payments can be claimed back.

So normal HTLCs wouldn't be marked as noop. Only these special HTLCs would be, currently we drive that type based on the set of custom records we see. Another approach here would be to add another aux interface to allow that check to be dynamic.

Left a comment to error out instead of warn to make this case explicit.