Skip to content
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

Correct final_cltv handling in blinded paths #1066

Closed

Conversation

TheBlueMatt
Copy link
Collaborator

When paying a blinded path, we don't have a CLTV delta at each hop
available, but rather only a total CLTV delta for the entire
blinded path.

However, the onion format currently still requires that we specify
an outgoing_cltv_value for the final hop. As the sender, we don't
have a sensible value to put there, as we don't know which part of
the total CLTV delta belongs to the recipient.

Thus, the sensible solution is to have the sender increment by the
amount they expect to receive, using zero otherwise. This works as
long as the sender isn't adding any additional CLTV delta for
privacy reasons. If the sender is doing so, they can use the final
CLTV field to communicate this.

This change proposes this, which appears to reflect what some
implementations do today.

When paying a blinded path, we don't have a CLTV delta at each hop
available, but rather only a total CLTV delta for the entire
blinded path.

However, the onion format currently still requires that we specify
an `outgoing_cltv_value` for the final hop. As the sender, we don't
have a sensible value to put there, as we don't know which part of
the total CLTV delta belongs to the recipient.

Thus, the sensible solution is to have the sender increment by the
amount they expect to receive, using zero otherwise. This works as
long as the sender isn't adding any additional CLTV delta for
privacy reasons. If the sender is doing so, they can use the final
CLTV field to communicate this.

This change proposes this, which appears to reflect what some
implementations do today.
Comment on lines +265 to +267
- `outgoing_cltv_value` MUST be set to any additional CLTV delta which was added in addition
to the required amount for the full blinded path for privacy. This may be zero. This MUST
NOT include any additional amount added to compensate for a block during payment relay.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we clarify that outgoing_cltv_value will still be absolute? It sounds like it's set to a delta right now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh? I understood that it should be a delta? I'm not really sure how it could be an absolute block height given we don't actually know what the block height is at the recipient node?

Comment on lines +265 to +267
- `outgoing_cltv_value` MUST be set to any additional CLTV delta which was added in addition
to the required amount for the full blinded path for privacy. This may be zero. This MUST
NOT include any additional amount added to compensate for a block during payment relay.
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I understand what you mean because that's very close to what we implemented in eclair, I find this really hard to read, especially for new readers trying to understand blinded payments without a lot of implementation context. What about detailing more explicitly every step of the construction of outgoing_cltv_value, like this:

- When creating `outgoing_cltv_value`:
  - MUST use the current block height as a baseline
  - SHOULD add a random number of blocks for additional privacy

Why don't you want the sender to include an additional amount to compensate for a block found during payment relay? I don't see a good reason to prevent that, senders should be free to do so if they want to (especially since adding a random value to protect the sender privacy actually is also a way to compensate for blocks found during payment relay)?

Also, I think that what may be missing and is confusing for implementers is that recipients should include their min_final_cltv_expiry_delta in the cltv-expiry-delta for their blinded paths. That wasn't included here because I figured it was the responsibility of the Bolt 12 PR to add this since it's where we actually create blinded paths, but maybe it's worth adding here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

like this:

A few questions - I'd understood previously that this deliberately did not include the current block height, only the offset - the sender doesn't actually know what the block height should be, and if we're treating it like every other hop in the blinded path the CLTV value is just a delta against the blinded path.

Further, there's a question here of who should add the blinding, should it be the recipient when creating the blinded path (IIRC this is mentioned somewhere else), in which case we really SHOULD NOT add a random number (and maybe we remove this entirely, but that breaks compat), or should it be the sender?

Why don't you want the sender to include an additional amount to compensate for a block found during payment relay? I don't see a good reason to prevent that, senders should be free to do so if they want to (especially since adding a random value to protect the sender privacy actually is also a way to compensate for blocks found during payment relay)?

Because that would lead to unnecessary failures - a recipient node needs to treat the received HTLC as if the CLTV delta the HTLC has was reduced by the amount communicated by the sender. If we include the, eg, one block buffer then a block being found while the HTLC is in transit will spuriously fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for coming back late on this, I created the following gist to detail through examples how the spec currently expects nodes to behave: https://gist.github.com/t-bast/b1371d357a2c5f3e8c09514a62db7079

Eclair was not correctly implementing one of the final checks, which I'm fixing in ACINQ/eclair#2678

I think this works, but this is quite subtle, please don't trust me on this and challenge it as much as possible!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for coming back late on this, I created the following gist to detail through examples how the spec currently expects nodes to behave: https://gist.github.com/t-bast/b1371d357a2c5f3e8c09514a62db7079

Eclair was not correctly implementing one of the final checks, which I'm fixing in ACINQ/eclair#2678

I think this works, but this is quite subtle, please don't trust me on this and challenge it as much as possible!

@t-bast Just took another look at this doc because we ran into some CLTV expiry issues. It may seem obvious, but FYI I think the spec is missing that the receiver needs to include their min_final_cltv_expiry_delta in the aggregated CLTV delta. In the example aggregation calculation in proposals/route_blinding.md, the receiver does not add their final delta. Let me know if I'm missing something here!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's correct! I'll prepare a PR to add this to the example in proposals/route_blinding.md. Note that the recipient may also use a dummy hop, in which case they don't necessarily need to add a min_final_cltv_expiry_delta, they would just cover it with the cltv_expiry_delta of the dummy hop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in #1131

@ProofOfKeags
Copy link
Contributor

Seems this can be closed in favor of #1097 ?

@TheBlueMatt
Copy link
Collaborator Author

Superseded.

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.

4 participants