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: fix route fee calculation and channel capacity check #1382

Merged
merged 3 commits into from Jun 29, 2018

Conversation

Projects
None yet
4 participants
@joostjager
Collaborator

joostjager commented Jun 13, 2018

This PR fixes two problems in the routing package:

  • Fee calculation for a hop does not include the fee that needs to be paid to the next hop
  • The incoming channel capacity "sanity" check does not include the fee to be paid to the current hop

It also updates the multi hop payment integration test to cover the fee miscalculation.

@joostjager joostjager force-pushed the joostjager:newroute-bug branch from 79ef30a to aa0b439 Jun 13, 2018

createHop(10000, 1000),
},
expectedTotalAmount: 102010,
} }

This comment has been minimized.

@joostjager

joostjager Jun 13, 2018

Collaborator

This test case fails on master. It returns a total amount of 102000

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

Can you add to the commit message why this fails on master, and what you did to fix it?

createHop(1000, 100),
createHop(1000, 1000),
},
expectFailure: true,

This comment has been minimized.

@joostjager

joostjager Jun 13, 2018

Collaborator

This test case fails on master. It does not generate a failure even though the first channel has insufficient capacity.

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

Can you add to the commit message why this fails on master, and what you did to fix it?

@halseth

Another very welcome bugfix! 🙌

fee := lnwire.MilliSatoshi(0)
// If this isn't the last hop, to add enough funds to pay for
// transit over the next link.
if i != len(pathEdges)-1 {
// We'll grab the edge policy and per-hop payload of
// the prior hop so we can calculate fees properly.
prevEdge := pathEdges[i+1]
prevHop := route.Hops[i+1]

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

should really be named nextHop?

This comment has been minimized.

@joostjager

joostjager Jun 13, 2018

Collaborator

Yes. Renamed.

createHop(1000, 100),
createHop(1000, 1000),
},
expectFailure: true,

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

Maybe we should make sure that the error we get is ErrInsufficientCapacity? :)

This comment has been minimized.

@joostjager

joostjager Jun 13, 2018

Collaborator

Good addition. Also because in another PR I will check for a different error code. I chose to use two fields: expectFailure and expectedErrorCode. Any suggestion to do this more go'ish is welcome.

@@ -395,7 +384,7 @@ func newRoute(amtToSend, feeLimit lnwire.MilliSatoshi, sourceVertex Vertex,
// The total amount required for this route will be the value the
// source extends to the first hop in the route.
route.TotalAmount = runningAmt
route.TotalAmount = route.Hops[0].AmtToForward + route.Hops[0].Fee

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

Comment should be updated to explain the calculation.

This comment has been minimized.

@joostjager

joostjager Jun 13, 2018

Collaborator

Updated

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

Nice. I would also add a note that the source node (the sender of the payment) is not part of the hops here.

createHop(1000, 100),
createHop(1000, 1000),
},
expectFailure: true,

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

Can you add to the commit message why this fails on master, and what you did to fix it?

createHop(10000, 1000),
},
expectedTotalAmount: 102010,
} }

This comment has been minimized.

@halseth

halseth Jun 13, 2018

Collaborator

Can you add to the commit message why this fails on master, and what you did to fix it?

@joostjager joostjager force-pushed the joostjager:newroute-bug branch from aa0b439 to 36cf58d Jun 13, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Jun 13, 2018

@halseth Changes made.

// enough capacity to carry the required amount which
// includes the fee dictated at each hop. Make the comparison
// in msat to prevent rounding errors.
if nextHop.AmtToForward + fee > lnwire.NewMSatFromSatoshis(

This comment has been minimized.

@Roasbeef

Roasbeef Jun 13, 2018

Member

This is actually the outgoing channel though. Also at this point w/ the old code, the amtToForward already factors in the fee added at the prior hop. The fee is the difference between the incoming and outgoing amount to a target node.

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

I thiiink this is correct now.

//
// * amt_in - fee >= amt_to_forward
amtToForward = runningAmt - fee
nextHop := route.Hops[i+1]

This comment has been minimized.

@Roasbeef

Roasbeef Jun 13, 2018

Member

This is actually the previous hop. We're traversing backwards right now along the route.

This comment has been minimized.

@joostjager

joostjager Jun 14, 2018

Collaborator

It gets confusing with different definitions of prev and next. Is it next loop-wise or next in the payment route. It would be good to adhere to one definition. Discussed with @halseth and opted for "next = next in route".

This comment has been minimized.

@joostjager

joostjager Jun 14, 2018

Collaborator

Maybe I should rename it to that. nextHopInRoute.

This comment has been minimized.

@halseth

halseth Jun 14, 2018

Collaborator

Ah, yeah I was thinking "next = next in route". It is definitely confusing using prev when we are traversing backwards. Looking at the comments, lastHop is also being used, so I think sticking to "first = first in route, last = last in route, next = next in route" makes sense. Comments will need to be updated to reflect this though.

@@ -75,7 +75,7 @@ type ChannelHop struct {
// included within the Sphinx packet.
type Hop struct {
// Channel is the active payment channel edge that this hop will travel
// along.
// along. This is the _incoming_ channel to this hop.

This comment has been minimized.

@Roasbeef

Roasbeef Jun 13, 2018

Member

This is the outgoing channel. The HTLC will flow on this path.

This comment has been minimized.

@joostjager

joostjager Jun 14, 2018

Collaborator

As I understood it, a hop is an intermediate or final node (from rfc).

The result of findPath is a list of channels that the htlc should flow along. First element in this list contains the first channel. From source to first intermediate hop. So if the first element in the list is the first hop, than this Channel property must be the incoming channel to the first hop?

This comment has been minimized.

@halseth

halseth Jun 14, 2018

Collaborator

As I understood it Hop is analogous to an "edge" here, so saying it is incoming/outgoing doesn't make that much sense. However, the ChannelHop contains a ChannelEdgePolicy that houses the NodeID of the node this edge leads to. So something about that can be added to the comment.

This comment has been minimized.

@joostjager

joostjager Jun 14, 2018

Collaborator

Yes, it is in the language. According to the lightning rfc, a hop is a node. The Hop type is really what it should be. But ChannelHop might be better renamed to something else. ChannelEdge then probably. Not too far off, it's a policy, a target and a bandwidth estimate.

// The amount that this hop needs to forward is based
// on how much the prior hop carried plus the fee
// that needs to be paid to the prior hop
amtToForward = nextHop.AmtToForward + nextHop.Fee

This comment has been minimized.

@Roasbeef

Roasbeef Jun 13, 2018

Member

The fee has already been accumulated into what the next hop pays. The fee is the difference between hop_{i+1}.amount and hop_{i}.amount.

This comment has been minimized.

@halseth

halseth Jun 14, 2018

Collaborator

I think what is confusing here is how the Hop.Fee relates to the Hop.AmountToForward. From the godoc on those fields, it looks like the end node of this Hop (=edge) will receive AmountToFoward, meaning the start node subtracted Fee. But here we set the "last hop" fee to zero, which doesn't work with that definition. So I think we should make the documentation match the usage here (by either modifying docs or use).

This comment has been minimized.

@joostjager

joostjager Jun 16, 2018

Collaborator

A hop is a node according to lightning-rfc. I think we should stick to that definition in the code?

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

I think it makes sense to name hop=node with the current comments :)

@joostjager joostjager force-pushed the joostjager:newroute-bug branch from 36cf58d to aa7d37d Jun 14, 2018

// following inequality most hold true:
//
// * amt_in - fee >= amt_to_forward
amtToForward = runningAmt - fee

This comment has been minimized.

@halseth

halseth Jun 14, 2018

Collaborator

This is where the current implementation is incorrect in my understanding. The second to last hop will here always have amtToForward equal to the last hop.

If this is correct, how did this ever work? 🤔

This comment has been minimized.

@Roasbeef

Roasbeef Jun 18, 2018

Member

That's correct, it should forward this exact amount to the last hop. Note that this is was goes into the hop payload for the node.

createHop(10, 1000),
createHop(10, 1000),
},
expectedTotalAmount: 100002,

This comment has been minimized.

@halseth

halseth Jun 14, 2018

Collaborator

maybe add a per hop expected fees as well. Will also make it easier to see how the total is calculated :)

This comment has been minimized.

@joostjager

joostjager Jun 14, 2018

Collaborator

Indeed, maybe better to assert the full result. But I'd rather postpone that until #1321 is merged. Then I can properly separate the newRoute impl+test from the findPath impl+test. Different responsibilties, separate files.

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

I don't see why we would have to postpone that. Looks like that could be added to these testcases without any dependency on #1321.

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Jun 16, 2018

Integration test added. It fails on master with "unable to route payment to destination: FeeInsufficient(fee=1101000 mSAT, update=(lnwire.ChannelUpdate)"

@joostjager joostjager force-pushed the joostjager:newroute-bug branch 3 times, most recently from 1e3d558 to 97bdf61 Jun 16, 2018

@halseth

This PR is starting to get into a rally good shape! The code makes more sense now, and it is IMO much easier to follow after your recent comment updates 👍

I also emailed Travis to unflag your account such that we can get the tests running.

lnd_test.go Outdated
updateFeeReq := &lnrpc.PolicyUpdateRequest{
BaseFeeMsat: baseFee,
FeeRate: float64(feeRate) / feeBase,

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

Is this a global feeBase?

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

That was the idea. But I see that I did not reference the global one defined in rpcserver everywhere. And on a second thought, it does not look like such a good idea. Someone could accidentally change feeBase in rpcserver, breaking the external interface, but not failing the test. Replaced all occurences in lnd_test with a different value testFeeBase.

lnd_test.go Outdated
chanPoint,
)
time.Sleep(time.Millisecond * 50)

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

not needed?

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

No, removed.

lnd_test.go Outdated
@@ -2953,19 +3005,26 @@ func testMultiHopPayments(net *lntest.NetworkHarness, t *harnessTest) {
// increasing of time is needed to embed the HTLC in commitment
// transaction, in channel Carol->David->Alice->Bob, order is Bob,
// Alice, David, Carol.
const amountPaid = int64(5000)
amountPaid := int64(5000)
const expectedFeeAlice = 505

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

These calculations should be explained.

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

Comments added

lnd_test.go Outdated
assertAmountPaid(t, ctxb, "Alice(local) => Bob(remote)", net.Bob,
aliceFundPoint, int64(0), amountPaid)
assertAmountPaid(t, ctxb, "Alice(local) => Bob(remote)", net.Alice,
aliceFundPoint, amountPaid, int64(0))
amountPaid += expectedFeeAlice

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

Same, should be explained.

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

Comments added and variables clarified.

lnd_test.go Outdated
const exectedFees = 5
if feeReport.DayFeeSum != exectedFees {
if feeReport.DayFeeSum != uint64(expectedFeeDave) {

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

Comments above should be updated.

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

Fee amount updated

return &ChannelHop {
ChannelEdgePolicy: &channeldb.ChannelEdgePolicy {
Node: &channeldb.LightningNode{},

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

I think we should set the baseFee here for clarity. Either as a parameter, or just BaseFee: 0. I think it makes sense as a parameter, as that lets us also test hops with non-zero base fee.

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

Added

// A three hop payment where the first and second hop
// will both charge 1 msat. The fee for the first hop
// is actually slightly higher than 1, because the amount
// t0 fwd also includes the fee for the second hop. This

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

"to forward"

// will both charge 1 msat. The fee for the first hop
// is actually slightly higher than 1, because the amount
// t0 fwd also includes the fee for the second hop. This
// gets rounded down to 1.

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

Nice test case! Also add a similar test with the feerate exactly high enough to not get rounded down?

This comment has been minimized.

@joostjager

joostjager Jun 19, 2018

Collaborator

Added

createHop(10, 1000),
createHop(10, 1000),
},
expectedTotalAmount: 100002,

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

I don't see why we would have to postpone that. Looks like that could be added to these testcases without any dependency on #1321.

expectedCode := testCase.expectedErrorCode
if err == nil || !IsError(err, expectedCode) {
t.Fatalf("expected newRoute to fail " +
"with error code %v",

This comment has been minimized.

@halseth

halseth Jun 18, 2018

Collaborator

add "instead got error %v"

@joostjager joostjager force-pushed the joostjager:newroute-bug branch from 4574fbd to 9059219 Jun 19, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Jun 19, 2018

@halseth Your comments have been addressed.

@joostjager joostjager force-pushed the joostjager:newroute-bug branch from 9059219 to fa388d5 Jun 27, 2018

@Roasbeef

Excellent work! One worthy follow up I may add as an issue is to take advantage of the testing.Quick package to allow us to generate a set of randomized "well formed" routes and assert certain properties against them based on path finding attempts. If we had this type of testing in place, we may have caught this bug much earlier on.

I left one minor comment w.r.t adding a comment within some fields on the newly added testing fixture just to elaborate on the meaning of a particular attribute. Other than that, LGTM ⚡️

Once the fixup commits have been rebased in, we can get this one merged, then proceed with the other related PR's.

hops []*ChannelHop
paymentAmount lnwire.MilliSatoshi
expectedFees []lnwire.MilliSatoshi
expectedTimeLocks []uint32

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

What exactly is the expected time lock here? I'm having a hard time following this usage, but everything else is pristine. It may help to add a set of comments to all the fields in this struct.

This comment has been minimized.

@joostjager

joostjager Jun 28, 2018

Collaborator

Comments added

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

joostjager added some commits Jun 12, 2018

routing: fix route fee calculation and channel capacity check
This commit fixes the logic inside the newRoute function to
address the following problems:

- Fee calculation for a hop does not include the fee that needs
  to be paid to the next hop.

- The incoming channel capacity "sanity" check does not include
  the fee to be paid to the current hop.
lnd_test: use different fee policy in multi hop payment test
This change makes the test more sensitive to bugs than a route
with nodes that all enforce the same fee policy.

In addition to that, the fee has been increased to a level
at which potential problems with improper fee calculation
become detectable (and not disappear in rounding).
routing: add time lock asserts to NewRoute test
This comment extends the unit tests for NewRoute with checks
on the total time lock for a route as well as the expected time
lock values for every hop along the route.

@joostjager joostjager force-pushed the joostjager:newroute-bug branch from cf484a6 to 416f368 Jun 28, 2018

@Roasbeef

LGTM 🚀

@Roasbeef Roasbeef merged commit 64a0734 into lightningnetwork:master Jun 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.006%) to 54.731%
Details

@halseth halseth referenced this pull request Jul 12, 2018

Closed

FeeInsufficient #1549

@joostjager joostjager deleted the joostjager:newroute-bug branch Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment