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.

Sign up for GitHub

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

Jump to bottom

Correct final_cltv handling in blinded paths #1066

Closed
wants to merge 2 commits into from
Closed

Correct final_cltv handling in blinded paths #1066

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b18e78
Fix undeclared reference in onion-routing
TheBlueMatt Apr 21, 2023
2dc0500
Correct final_cltv handling in blinded paths
TheBlueMatt Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions 04-onion-routing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,17 @@ The creator of `encrypted_recipient_data` (usually, the recipient of payment):
- `total_fee_proportional_millionths(n+1) = ((total_fee_proportional_millionths(n) + fee_proportional_millionths(n+1)) * 1000000 + total_fee_proportional_millionths(n) * fee_proportional_millionths(n+1) + 1000000 - 1) / 1000000`
- MUST create the `encrypted_recipient_data` from the `encrypted_data_tlv` as required in [Route Blinding](#route-blinding).

The writer of `tlv_payload`:
The writer of the TLV `payload`:

- For every node inside a blinded route:
- MUST include the `encrypted_recipient_data` provided by the recipient
- For the first node in the blinded route:
- MUST include the `blinding_point` provided by the recipient in `current_blinding_point`
- If it is the final node:
- MUST include `amt_to_forward`, `outgoing_cltv_value` and `total_amount_msat`.
- `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.
Comment on lines +265 to +267
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
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

- MUST NOT include any other tlv field.
- For every node outside of a blinded route:
- MUST include `amt_to_forward` and `outgoing_cltv_value`.
Expand Down Expand Up @@ -310,7 +313,7 @@ The reader:
- MUST return an error if the payload contains other tlv fields than `encrypted_recipient_data`, `current_blinding_point`, `amt_to_forward`, `outgoing_cltv_value` and `total_amount_msat`.
- MUST return an error if `amt_to_forward`, `outgoing_cltv_value` or `total_amount_msat` are not present.
- MUST return an error if `amt_to_forward` is below what it expects for the payment.
- MUST return an error if incoming `cltv_expiry` < `outgoing_cltv_value`.
- MUST return an error if incoming `cltv_expiry` < `outgoing_cltv_value` + `min_final_cltv_expiry_delta`.
- MUST return an error if incoming `cltv_expiry` < `current_block_height` + `min_final_cltv_expiry_delta`.
- Otherwise (it is not part of a blinded route):
- MUST return an error if `blinding_point` is set in the incoming `update_add_htlc` or `current_blinding_point` is present.
Expand Down