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

routing+htlcswitch: finalize switch of CLTV delta directionality in path finding and link forwarding #1447

Merged
merged 8 commits into from Jun 27, 2018

Conversation

Projects
None yet
3 participants
@Roasbeef
Member

Roasbeef commented Jun 26, 2018

In this commit, we fix an old-ish bug within the project. This bug was first introduced due to an incomplete migration to the swapped directionality of the CLTV value within the ChannelUpdate message. Now the value from A -> B is B's policy, while in the past, this value was swapped.

Within the newRoute method in the router, we failed to carry this over, and were still using the CLTV delta of the current hop, rather than that of the prior hop (the one later in the route). Additionally, the expiry values within the spec json example were also incorrect, causing this issue to go undetected until now.

This "failure to propagate the CLTV direction swap" carried over to the htlcswitch package where the generateHops method had a similar bug which caused it to generate an invalid payload. Within the link before this PR, we would actually apply the time lock policy verification at the incoming link instead of the outgoing link. As a result, if a node had two links with distinct CLTV policies (p1.delta > p2.delta), all HTLC's routing in the direction of the greater delta would fail as the link was erroneously applying the incoming policy instead of the outgoing policy.

In order to fix the above bug, we've extended the HtlcSatifiesPolicy method to also factor in the time lock information for that HTLC. We've also added a test which fails without the set of changes included in this PR.

We should follow up this PR with a change to unify the hop payload generation logic across the routing and htlcswitch packages. This way, future fixes/modifications can be made in a single location, rather than across to packages.

One last issue still linger: we don't store the min cltv delta used for a particular invoices on disk. As a result, atm, we always just use our current routing policy which is incorrect if the user ever specifies a manual final cltv value that's lower than the value of the accepting link.

Fixes #1431 .
Fixes #1398.

@Roasbeef Roasbeef added this to the 0.5 milestone Jun 26, 2018

@wpaulino

Nice catch on the tests! Once this and the fee changes land, we should have a way better payment UX.

LGTM ⚡️

}
defer cleanUp()
fmt.Println(testStartingHeight)

This comment has been minimized.

@wpaulino

wpaulino Jun 26, 2018

Collaborator

Stray debugging print 😛

This comment has been minimized.

@Roasbeef

Roasbeef Jun 26, 2018

Member

Fixed!

// delta should equal the outgoing time lock. Otherwise, whether the
// sender messed up, or an intermediate node tampered with the HTLC.
if incomingTimeout-timeDelta < outgoingTimeout {
log.Errorf("Incoming htlc(%x) has incorrect time-lock value: "+

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

l.errorf, and above? will help make sure we get the sid in logs

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Fixed.

@@ -81,8 +81,10 @@ type ChannelLink interface {
// Otherwise, a valid protocol failure message should be returned in
// order to signal to the source of the HTLC, the policy consistency
// issue.
HtlcSatifiesPolicy(payHash [32]byte,
incomingAmt, amtToForward lnwire.MilliSatoshi) lnwire.FailureMessage
HtlcSatifiesPolicy(payHash [32]byte, incomingAmt lnwire.MilliSatoshi,

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

seems like we could just pass in the htlcPacket here? is a little weird since the struct is not public, but otherwise the number of args is kinda unwieldy. not a blocker tho

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Hmm, yeh agree we can pass in a struct instead here as the set of args is starting to grow a bit. Would prefer to do it in a follow up, as the htlcPacket struct itself could be streamlined a bit by just embedding the HopPayload struct for example.

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

Yeah that sounds like a good approach, cool with me

// Otherwise, the outgoing time lock should be the
// incoming timelock minus their specified delta.
delta := path[i+1].cfg.FwrdingPolicy.TimeLockDelta
totalTimelock += delta

This comment has been minimized.

@cfromknecht

cfromknecht Jun 27, 2018

Collaborator

order of operations here is a little confusing, is this equivalent to

timeLock = totalTimeLock
totalTimeLock += path[i+1].cfg.FwrdingPolicy.TimeLockDelta

?

This comment has been minimized.

@Roasbeef

Roasbeef Jun 27, 2018

Member

Yep it's the same, but unrolled in order to stress that: outgoingLock = incomingLock - delta

Roasbeef added some commits Jun 26, 2018

htlcswitch: add new TestForwardingAsymmetricTimeLockPolicies test
In this commit, we add a new test to the switch:
TestForwardingAsymmetricTimeLockPolicies. This test ensures that a link
has two channels, one of which has a greater CLTV delta than the latter,
that a payment will successfully be routed across the channels. Atm, the
test fails (including the fix to hop payload generation included in the
next commit).

Atm, due to the way that we check forwarding policies, we'll reject this
payment as we're attempting to enforce the policy of the incoming link
(cltv delta of 7), instead of that of the outgoing link (cltv delta of
6). As a result, atm, the incoming link checks if (incoming_timeout -
delta < outgoing_timeout). For the values in the test case: 112 - 7 <
106 -> 105 < 106, this check fails. The payload is proper, but the check
itself should be applied at the outgoing hop.
htlcswitch: fix bug in generateHops, use CLTV delta of prior hop to c…
…ompute payload

In this commit, we fix a bug in the generateHops helper function. Before
this commit, it erroneously used the CLTV delta of the current hop,
rather than that of the prior hop when computing the payload. This was
incorrect, as when computing the timelock for the incoming hop, we need
to factor in the CTLV delta of the outgoing lock, not the incoming lock.
htlcswitch: extend the HtlcSatifiesPolicy to also accept timelock/hei…
…ght info

In this commit, we extend the existing HtlcSatifiesPolicy method to also
accept timelock and height information. This is required as an upcoming
commit will fix an existing bug in the forwarding logic wherein we use
the time lock policies of the incoming node rather than that of the
outgoing node.
htlcswitch: move timelock policy verification logic to HtlcSatifiesPo…
…licy

In this commit, we extract the time lock policy verification logic from
the processRemoteAdds method to the HtlcSatifiesPolicy method. With this
change, we fix a lingering bug within the link: we'll no longer verify
time lock polices within the incoming link, instead we'll verify it at
forwarding time like we should. This is a bug left over from the switch
of what the CLTV delta denotes in the channel update message we made
within the spec sometime last year.
routing: fix bug in newRoute, use time lock delta of prior hop, not c…
…urrent

In this commit, we fix an existing bug in the newRoute method. Before
this commit we would use the time lock delta of the current hop to
compute the outgoing time lock for the current hop. This is incorrect as
the time lock delta of the _outgoing_ hop should be used, as this is
what we're paying for "transit" on. This is a bug left over from when we
switched the meaning of the CLTV delta on the ChannelUpdate message
sometime last year.

The fix is simple: use the CLTV delta of the prior (later in the route)
hop.
routing: fix incorrect expiry values in spec_example.json
In this commit, we fix the incorrect expiry values in the
spec_example.json test file. Many of the time locks were incorrect which
allowed bugs within the path finding logic related to CLTV deltas to go
un-detected.

@Roasbeef Roasbeef force-pushed the Roasbeef:forwarding-timelock-fix branch from 45ba03e to 6a6c29c Jun 27, 2018

@cfromknecht

LGTM 🦔

@Roasbeef Roasbeef merged commit ec7cfc6 into lightningnetwork:master Jun 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment