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

Fix blinded path min_final_expiry_delta check #2678

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented May 26, 2023

The spec says that we must return an error if:

  • cltv_expiry < outgoing_cltv_value
  • cltv_expiry < current_block_height + min_final_cltv_expiry_delta

For the second check, we actually tested if:

  • cltv_expiry < max(outgoing_cltv_value, current_block_height) + min_final_cltv_expiry_delta

But that check should only verify that our min_final_expiry_delta requirement is fulfilled, which is unrelated to the outgoing_cltv_value.

It was redundant with the first check, which already guarantees that intermediate nodes inside the blinded path cannot cheat by using a higher cltv_expiry_delta than what the recipient intended.

For context, see lightning/bolts#1066 and https://gist.github.com/t-bast/b1371d357a2c5f3e8c09514a62db7079

The spec says that we must return an error if:

- cltv_expiry < outgoing_cltv_value
- cltv_expiry < current_block_height + min_final_cltv_expiry_delta

For the second check, we actually tested if:

- cltv_expiry < max(outgoing_cltv_value, current_block_height) + min_final_cltv_expiry_delta

But that check should only verify that our `min_final_expiry_delta`
requirement is fulfilled, which is unrelated to the `outgoing_cltv_value`.

It was redundant with the first check, which already guarantees that
intermediate nodes inside the blinded path cannot cheat by using a higher
`cltv_expiry_delta` than what the recipient intended.
@codecov-commenter
Copy link

Codecov Report

Merging #2678 (f7d58cf) into master (e7b4631) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
- Coverage   85.88%   85.87%   -0.01%     
==========================================
  Files         214      214              
  Lines       17591    17590       -1     
  Branches      728      716      -12     
==========================================
- Hits        15108    15106       -2     
- Misses       2483     2484       +1     
Impacted Files Coverage Δ
...r/acinq/eclair/payment/send/PaymentInitiator.scala 86.56% <ø> (ø)
...cinq/eclair/payment/receive/MultiPartHandler.scala 93.33% <100.00%> (-0.04%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Member

@thomash-acinq thomash-acinq left a comment

Choose a reason for hiding this comment

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

I was a bit confused at first but actually all we care about is that the delta between now and when the addHTLC expires is at least nodeParams.channelConf.minFinalExpiryDelta. So this looks good.

@t-bast t-bast merged commit 05ef2f9 into master Jun 8, 2023
@t-bast t-bast deleted the blinded-path-expiry branch June 8, 2023 15:20
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