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

BOLT 4: don't allow a "fee" for the final node. #711

Conversation

rustyrussell
Copy link
Collaborator

I recently made a cut & paste bug with the protocol tests, and
paid an HTLC of amount 100M msat, but with only a 1M msat amt_to_forward
in the hop_data. To my surprise, it was accepted.

This is because we allow overpaying the routing fee (considered 0
for the final hop). This doesn't make sense for the final hop: anything
but exact equality implies a bug, or that the previous node took the
wrong amount from the payment.

Signed-off-by: Rusty Russell rusty@rustcorp.com.au

I recently made a cut & paste bug with the protocol tests, and
paid an HTLC of amount 100M msat, but with only a 1M msat `amt_to_forward`
in the hop_data.  To my surprise, it was accepted.

This is because we allow overpaying the routing fee (considered 0
for the final hop).  This doesn't make sense for the final hop: anything
but exact equality implies a bug, or that the previous node took the
wrong amount from the payment.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell added the Meeting Discussion Raise at next meeting label Dec 3, 2019
@t-bast
Copy link
Collaborator

t-bast commented Dec 3, 2019

We're also currently using <= as a receiver, but as a sender we make sure the HTLC amountMsat equals the onion's amountToForward.

Nothing blocking on our side to merge this change, ACK aabf4e6.

@cdecker
Copy link
Collaborator

cdecker commented Dec 9, 2019

ACK

Copy link
Collaborator

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

ACK, turns out lnd already implements it this way :)

@rustyrussell
Copy link
Collaborator Author

Given all three acks, and the meeting comment that this is a "no-brainer", I'm merging now.

@rustyrussell rustyrussell merged commit 2422630 into lightning:master Dec 13, 2019
t-bast pushed a commit that referenced this pull request Jan 8, 2020
Update a requirement that was missed in #711
t-bast added a commit to ACINQ/eclair that referenced this pull request Jan 15, 2020
* Final recipient should not collect a fee: see 
lightning/bolts#711

* Fix Sphinx small privacy leak: see
lightning/bolts#697
niftynei pushed a commit to niftynei/lightning-rfc that referenced this pull request Jan 28, 2020
niftynei pushed a commit to niftynei/lightning-rfc that referenced this pull request Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meeting Discussion Raise at next meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants