Skip to content

Success hold times #1261

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

Closed
wants to merge 3 commits into from
Closed

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented May 27, 2025

Extending the idea proposed in #1044 to the success case.

LDK implementation: lightningdevkit/rust-lightning#3801

@thomash-acinq
Copy link

I think this should be merged in #1044. It was put in separate PR because we thought #1044 was ready but since there are still discussions around the unit to use (which also affects this PR), it would make sense to put both in the same package.

@joostjager
Copy link
Collaborator Author

Would you use a single feature bit for both? In that case, I think it would delay deployment of attributable failures. It's more work to also extend the success path.

@thomash-acinq
Copy link

Yes I would use the same feature bit (which should be renamed if it applies to fulfill too?). It's barely more work and attributable failures is already delayed anyways.

@joostjager
Copy link
Collaborator Author

Maybe not barely more work in Eclair. You guys always have such compact ways to get things done. That's a compliment.

But wouldn't say this is true for all implementations? LND team seems to be focusing on the failure case initially only. The main problem to solve is attribution for failures, and hold times are an extra.

@t-bast
Copy link
Collaborator

t-bast commented Jun 4, 2025

I agree with @thomash-acinq that it would make more sense to include in the same spec feature right now, as it is much cleaner from a protocol point of view and means we won't have to handle the technical debt of having two distinct feature bits while at some point everyone who supports one will support the other.

It doesn't matter if an implementation ships with only attributable failures in update_fail_htlc initially: it will simply look as if they chose to not include an attributable failure in update_fulfill_htlc, which they can always do even when they fully support the feature? As long as we've tested cross-compatibility for failure, I'd be surprised if we don't notice an issue that would affect fulfills, as it uses exactly the same cryptographic mechanism?

@joostjager
Copy link
Collaborator Author

joostjager commented Jun 4, 2025

It doesn't matter if an implementation ships with only attributable failures in update_fail_htlc initially: it will simply look as if they chose to not include an attributable failure in update_fulfill_htlc, which they can always do even when they fully support the feature?

Maybe at some point in the future, nodes may be penalized for advertising support and not generating/back-propagating attribution data on the update_fulfill_htlc.

That may be a long way off though, allowing for enough time to complete the implementation?

As long as we've tested cross-compatibility for failure, I'd be surprised if we don't notice an issue that would affect fulfills, as it uses exactly the same cryptographic mechanism?

It is the same mechanism indeed. But in LDK the implementation is also compact, so interop between Eclair and LDK seems relatively low effort to achieve.


The `update_fulfill_htlc` message contains an optional `attribution_data` field, similar to the one used in the failure
case described above. The only difference is that there is no failure message that is covered by the HMACs. With
attribution data, the sender can obtain hold times as reported and committed to by each hop along the payment path.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention blinded routes, nodes inside a blinded must not set attribution_data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@joostjager
Copy link
Collaborator Author

I wonder what to call this feature that combines attributable failures, failure hold times and success hold times. option_htlc_telemetry?

@thomash-acinq
Copy link

I was thinking option_attribution_data.

@joostjager
Copy link
Collaborator Author

Merged this PR into #1044

@joostjager joostjager closed this Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants