Skip to content

BOLT 4: Report outgoing HTLC values when appropriate#538

Merged
sstone merged 1 commit intomasterfrom
bolt4-outgoing-values
Jan 21, 2019
Merged

BOLT 4: Report outgoing HTLC values when appropriate#538
sstone merged 1 commit intomasterfrom
bolt4-outgoing-values

Conversation

@pm47
Copy link
Collaborator

@pm47 pm47 commented Jan 5, 2019

For some relaying errors it makes more sense to report the values of the outgoing HTLC rather than the incoming HTLC.

For some relaying errors it makes more sense to report the values of the outgoing HTLC rather than the incoming HTLC.
pm47 added a commit to ACINQ/eclair that referenced this pull request Jan 5, 2019
...and found bugs!

Note that there is something fishy in BOLT 4, filed a PR:
lightning/bolts#538

Also, first try of softwaremill's quicklens lib (in scope test for now)
@TheBlueMatt
Copy link
Collaborator

Isn't this backwards incompatible? Do we want to just append the outgoing value instead?

@Roasbeef
Copy link
Collaborator

Roasbeef commented Jan 7, 2019

@pm47 can you spell out the rationale for this change? If a node gives you an HTLC with an incorrect value, and the the onion checked, then why is it useful to send back the onion data to the sending node? Whereas right now, we'd send back the incorrect htlc value that was extended.

@pm47
Copy link
Collaborator Author

pm47 commented Jan 7, 2019

Isn't this backwards incompatible? Do we want to just append the outgoing value instead?

Technically yes but I really don't think that's a big deal, those values are just reported as hints and I don't think any implementation does anything more than display or log them.

@pm47 can you spell out the rationale for this change? If a node gives you an HTLC with an incorrect value, and the the onion checked, then why is it useful to send back the onion data to the sending node? Whereas right now, we'd send back the incorrect htlc value that was extended.

In the following scenario B refuses to forward the payment because htlc2.amountMsat< updBC.htlc_minimum_msat (htlc2 is never sent). It doesn't make sense to return htlc1.amountMsat because this is not what caused the error.

A ---- htlc1 ----> B ---- htlc2 ----> C

Also, I think that was clearly the intent of the spec, because if you look at the very similar incorrect_cltv_expiry, we are already supposed to return htlc2.cltv_expiry, not htlc1.cltv_expiry, so it is inconsistent.

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM

lnd does this already, just felt more natural when coding it up

@sstone sstone merged commit 137106a into master Jan 21, 2019
@sstone sstone deleted the bolt4-outgoing-values branch January 21, 2019 20:16
pm47 added a commit to ACINQ/eclair that referenced this pull request Jan 21, 2019
* relay to channel with lowest possible balance

Our current channel selection is very simplistic: we relay to the
channel with the largest balance. As time goes by, this leads to all
channels having the same balance.

A better strategy is to relay to the channel which has the smallest
balance but has enough to process the payment. This way we save larger
channels for larger payments, and also on average channels get depleted
one after the other.

* added tests...

...and found bugs!

Note that there is something fishy in BOLT 4, filed a PR:
lightning/bolts#538

Also, first try of softwaremill's quicklens lib (in scope test for now)

* minor: fixed typo (h/t @btcontract)
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.

5 participants