From 0fc401de19a65fb8c9a655e38396acd778306c78 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 11 Oct 2019 15:46:10 -0400 Subject: [PATCH] routing+routerrpc: take max cltv limit into account within path finding With the introduction of the max CLTV limit parameter, nodes are able to reject HTLCs that exceed it. This should also be applied to path finding, otherwise HTLCs crafted by the same node that exceed it never left the switch. This wasn't a big deal since the previous max CLTV limit was ~5000 blocks. Once it was lowered to 1008, the issue became more apparent. Therefore, all of our path finding attempts now have a restriction of said limit in in order to properly carry out HTLCs to the network. --- lnrpc/routerrpc/router.pb.go | 5 +++-- lnrpc/routerrpc/router.proto | 5 +++-- lnrpc/routerrpc/router_backend.go | 30 ++++++++++++++++++++++++++---- lnrpc/routerrpc/router_server.go | 7 +++++-- lnrpc/rpc.pb.go | 5 +++-- lnrpc/rpc.proto | 5 +++-- lnrpc/rpc.swagger.json | 2 +- routing/pathfind.go | 6 +++--- routing/pathfind_test.go | 14 ++++++-------- routing/payment_session.go | 13 ++++--------- routing/payment_session_test.go | 4 ++-- routing/router.go | 2 +- routing/router_test.go | 2 ++ rpcserver.go | 21 +++++++++++++-------- 14 files changed, 75 insertions(+), 46 deletions(-) diff --git a/lnrpc/routerrpc/router.pb.go b/lnrpc/routerrpc/router.pb.go index b215e899..317c5717 100644 --- a/lnrpc/routerrpc/router.pb.go +++ b/lnrpc/routerrpc/router.pb.go @@ -212,8 +212,9 @@ type SendPaymentRequest struct { //any channel may be used. OutgoingChanId uint64 `protobuf:"varint,8,opt,name=outgoing_chan_id,json=outgoingChanId,proto3" json:"outgoing_chan_id,omitempty"` //* - //An optional maximum total time lock for the route. If zero, there is no - //maximum enforced. + //An optional maximum total time lock for the route. This should not exceed + //lnd's `--max-cltv-expiry` setting. If zero, then the value of + //`--max-cltv-expiry` is enforced. CltvLimit int32 `protobuf:"varint,9,opt,name=cltv_limit,json=cltvLimit,proto3" json:"cltv_limit,omitempty"` //* //Optional route hints to reach the destination through private channels. diff --git a/lnrpc/routerrpc/router.proto b/lnrpc/routerrpc/router.proto index 0e255267..7a5012bb 100644 --- a/lnrpc/routerrpc/router.proto +++ b/lnrpc/routerrpc/router.proto @@ -54,8 +54,9 @@ message SendPaymentRequest { uint64 outgoing_chan_id = 8; /** - An optional maximum total time lock for the route. If zero, there is no - maximum enforced. + An optional maximum total time lock for the route. This should not exceed + lnd's `--max-cltv-expiry` setting. If zero, then the value of + `--max-cltv-expiry` is enforced. */ int32 cltv_limit = 9; diff --git a/lnrpc/routerrpc/router_backend.go b/lnrpc/routerrpc/router_backend.go index cd384fe3..85e7b3b4 100644 --- a/lnrpc/routerrpc/router_backend.go +++ b/lnrpc/routerrpc/router_backend.go @@ -55,6 +55,10 @@ type RouterBackend struct { // Tower is the ControlTower instance that is used to track pending // payments. Tower routing.ControlTower + + // MaxTotalTimelock is the maximum total time lock a route is allowed to + // have. + MaxTotalTimelock uint32 } // MissionControl defines the mission control dependencies of routerrpc. @@ -471,11 +475,14 @@ func (r *RouterBackend) extractIntentFromSendRequest( payIntent.OutgoingChannelID = &rpcPayReq.OutgoingChanId } - // Take cltv limit from request if set. - if rpcPayReq.CltvLimit != 0 { - cltvLimit := uint32(rpcPayReq.CltvLimit) - payIntent.CltvLimit = &cltvLimit + // Take the CLTV limit from the request if set, otherwise use the max. + cltvLimit, err := ValidateCLTVLimit( + uint32(rpcPayReq.CltvLimit), r.MaxTotalTimelock, + ) + if err != nil { + return nil, err } + payIntent.CltvLimit = cltvLimit // Take fee limit from request. payIntent.FeeLimit = lnwire.NewMSatFromSatoshis( @@ -675,3 +682,18 @@ func ValidatePayReqExpiry(payReq *zpay32.Invoice) error { return nil } + +// ValidateCLTVLimit returns a valid CLTV limit given a value and a maximum. If +// the value exceeds the maximum, then an error is returned. If the value is 0, +// then the maximum is used. +func ValidateCLTVLimit(val, max uint32) (uint32, error) { + switch { + case val == 0: + return max, nil + case val > max: + return 0, fmt.Errorf("total time lock of %v exceeds max "+ + "allowed %v", val, max) + default: + return val, nil + } +} diff --git a/lnrpc/routerrpc/router_server.go b/lnrpc/routerrpc/router_server.go index c9188429..37e68474 100644 --- a/lnrpc/routerrpc/router_server.go +++ b/lnrpc/routerrpc/router_server.go @@ -248,11 +248,14 @@ func (s *Server) EstimateRouteFee(ctx context.Context, feeLimit := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) // Finally, we'll query for a route to the destination that can carry - // that target amount, we'll only request a single route. + // that target amount, we'll only request a single route. Set a + // restriction for the default CLTV limit, otherwise we can find a route + // that exceeds it and is useless to us. route, err := s.cfg.Router.FindRoute( s.cfg.RouterBackend.SelfNode, destNode, amtMsat, &routing.RestrictParams{ - FeeLimit: feeLimit, + FeeLimit: feeLimit, + CltvLimit: s.cfg.RouterBackend.MaxTotalTimelock, }, nil, ) if err != nil { diff --git a/lnrpc/rpc.pb.go b/lnrpc/rpc.pb.go index 5ed5f55c..09017648 100644 --- a/lnrpc/rpc.pb.go +++ b/lnrpc/rpc.pb.go @@ -1052,8 +1052,9 @@ type SendRequest struct { //any channel may be used. OutgoingChanId uint64 `protobuf:"varint,9,opt,name=outgoing_chan_id,json=outgoingChanId,proto3" json:"outgoing_chan_id,omitempty"` //* - //An optional maximum total time lock for the route. If zero, there is no - //maximum enforced. + //An optional maximum total time lock for the route. This should not exceed + //lnd's `--max-cltv-expiry` setting. If zero, then the value of + //`--max-cltv-expiry` is enforced. CltvLimit uint32 `protobuf:"varint,10,opt,name=cltv_limit,json=cltvLimit,proto3" json:"cltv_limit,omitempty"` //* //An optional field that can be used to pass an arbitrary set of TLV records diff --git a/lnrpc/rpc.proto b/lnrpc/rpc.proto index 1530652f..b7f56a87 100644 --- a/lnrpc/rpc.proto +++ b/lnrpc/rpc.proto @@ -888,8 +888,9 @@ message SendRequest { uint64 outgoing_chan_id = 9; /** - An optional maximum total time lock for the route. If zero, there is no - maximum enforced. + An optional maximum total time lock for the route. This should not exceed + lnd's `--max-cltv-expiry` setting. If zero, then the value of + `--max-cltv-expiry` is enforced. */ uint32 cltv_limit = 10; diff --git a/lnrpc/rpc.swagger.json b/lnrpc/rpc.swagger.json index 2163ddd7..36abef9d 100644 --- a/lnrpc/rpc.swagger.json +++ b/lnrpc/rpc.swagger.json @@ -3437,7 +3437,7 @@ "cltv_limit": { "type": "integer", "format": "int64", - "description": "* \nAn optional maximum total time lock for the route. If zero, there is no\nmaximum enforced." + "description": "* \nAn optional maximum total time lock for the route. This should not exceed\nlnd's `--max-cltv-expiry` setting. If zero, then the value of\n`--max-cltv-expiry` is enforced." }, "dest_tlv": { "type": "object", diff --git a/routing/pathfind.go b/routing/pathfind.go index 9967d05b..33c452f0 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -260,7 +260,7 @@ type RestrictParams struct { // CltvLimit is the maximum time lock of the route excluding the final // ctlv. After path finding is complete, the caller needs to increase // all cltv expiry heights with the required final cltv delta. - CltvLimit *uint32 + CltvLimit uint32 // DestPayloadTLV should be set to true if we need to drop off a TLV // payload at the final hop in order to properly complete this payment @@ -495,8 +495,8 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, incomingCltv := toNodeDist.incomingCltv + uint32(timeLockDelta) - // Check that we have cltv limit and that we are within it. - if r.CltvLimit != nil && incomingCltv > *r.CltvLimit { + // Check that we are within our CLTV limit. + if incomingCltv > r.CltvLimit { return } diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 4e7e9dc1..d5e35ee4 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -54,6 +54,7 @@ var ( noRestrictions = &RestrictParams{ FeeLimit: noFeeLimit, ProbabilitySource: noProbabilitySource, + CltvLimit: math.MaxUint32, } testPathFindingConfig = &PathFindingConfig{} @@ -789,6 +790,7 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc &RestrictParams{ FeeLimit: test.feeLimit, ProbabilitySource: noProbabilitySource, + CltvLimit: math.MaxUint32, }, testPathFindingConfig, sourceNode.PubKeyBytes, target, paymentAmt, @@ -1916,6 +1918,7 @@ func TestRestrictOutgoingChannel(t *testing.T) { FeeLimit: noFeeLimit, OutgoingChannelID: &outgoingChannelID, ProbabilitySource: noProbabilitySource, + CltvLimit: math.MaxUint32, }, testPathFindingConfig, sourceVertex, target, paymentAmt, @@ -1942,7 +1945,7 @@ func TestRestrictOutgoingChannel(t *testing.T) { // TestCltvLimit asserts that a cltv limit is obeyed by the path finding // algorithm. func TestCltvLimit(t *testing.T) { - t.Run("no limit", func(t *testing.T) { testCltvLimit(t, 0, 1) }) + t.Run("no limit", func(t *testing.T) { testCltvLimit(t, 2016, 1) }) t.Run("no path", func(t *testing.T) { testCltvLimit(t, 50, 0) }) t.Run("force high cost", func(t *testing.T) { testCltvLimit(t, 80, 3) }) } @@ -1998,19 +2001,13 @@ func testCltvLimit(t *testing.T, limit uint32, expectedChannel uint64) { paymentAmt := lnwire.NewMSatFromSatoshis(100) target := testGraphInstance.aliasMap["target"] - // Find the best path given the cltv limit. - var cltvLimit *uint32 - if limit != 0 { - cltvLimit = &limit - } - path, err := findPath( &graphParams{ graph: testGraphInstance.graph, }, &RestrictParams{ FeeLimit: noFeeLimit, - CltvLimit: cltvLimit, + CltvLimit: limit, ProbabilitySource: noProbabilitySource, }, testPathFindingConfig, @@ -2195,6 +2192,7 @@ func testProbabilityRouting(t *testing.T, p10, p11, p20, minProbability float64, &RestrictParams{ FeeLimit: noFeeLimit, ProbabilitySource: probabilitySource, + CltvLimit: math.MaxUint32, }, &PathFindingConfig{ PaymentAttemptPenalty: lnwire.NewMSatFromSatoshis(10), diff --git a/routing/payment_session.go b/routing/payment_session.go index 95197d48..370cc87a 100644 --- a/routing/payment_session.go +++ b/routing/payment_session.go @@ -73,15 +73,10 @@ func (p *paymentSession) RequestRoute(payment *LightningPayment, // does not reject the HTLC if some blocks are mined while it's in-flight. finalCltvDelta += BlockPadding - // If a route cltv limit was specified, we need to subtract the final - // delta before passing it into path finding. The optimal path is - // independent of the final cltv delta and the path finding algorithm is - // unaware of this value. - var cltvLimit *uint32 - if payment.CltvLimit != nil { - limit := *payment.CltvLimit - uint32(finalCltvDelta) - cltvLimit = &limit - } + // We need to subtract the final delta before passing it into path + // finding. The optimal path is independent of the final cltv delta and + // the path finding algorithm is unaware of this value. + cltvLimit := payment.CltvLimit - uint32(finalCltvDelta) // TODO(roasbeef): sync logic amongst dist sys diff --git a/routing/payment_session_test.go b/routing/payment_session_test.go index 67b6c04b..14f98449 100644 --- a/routing/payment_session_test.go +++ b/routing/payment_session_test.go @@ -20,7 +20,7 @@ func TestRequestRoute(t *testing.T) { // We expect find path to receive a cltv limit excluding the // final cltv delta (including the block padding). - if *r.CltvLimit != 22-uint32(BlockPadding) { + if r.CltvLimit != 22-uint32(BlockPadding) { t.Fatal("wrong cltv limit") } @@ -58,7 +58,7 @@ func TestRequestRoute(t *testing.T) { finalCltvDelta := uint16(8) payment := &LightningPayment{ - CltvLimit: &cltvLimit, + CltvLimit: cltvLimit, FinalCLTVDelta: finalCltvDelta, } diff --git a/routing/router.go b/routing/router.go index cd048d32..3908a93d 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1560,7 +1560,7 @@ type LightningPayment struct { // CltvLimit is the maximum time lock that is allowed for attempts to // complete this payment. - CltvLimit *uint32 + CltvLimit uint32 // PaymentHash is the r-hash value to use within the HTLC extended to // the first hop. diff --git a/routing/router_test.go b/routing/router_test.go index fba79ac7..0aae52ca 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "image/color" + "math" "math/rand" "strings" "sync/atomic" @@ -219,6 +220,7 @@ func TestFindRoutesWithFeeLimit(t *testing.T) { restrictions := &RestrictParams{ FeeLimit: lnwire.NewMSatFromSatoshis(10), ProbabilitySource: noProbabilitySource, + CltvLimit: math.MaxUint32, } route, err := ctx.router.FindRoute( diff --git a/rpcserver.go b/rpcserver.go index e4dc4a63..437735db 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -501,10 +501,11 @@ func newRPCServer(s *server, macService *macaroons.Service, return info.NodeKey1Bytes, info.NodeKey2Bytes, nil }, - FindRoute: s.chanRouter.FindRoute, - MissionControl: s.missionControl, - ActiveNetParams: activeNetParams.Params, - Tower: s.controlTower, + FindRoute: s.chanRouter.FindRoute, + MissionControl: s.missionControl, + ActiveNetParams: activeNetParams.Params, + Tower: s.controlTower, + MaxTotalTimelock: cfg.MaxOutgoingCltvExpiry, } var ( @@ -2940,7 +2941,7 @@ func (r *rpcServer) unmarshallSendToRouteRequest( type rpcPaymentIntent struct { msat lnwire.MilliSatoshi feeLimit lnwire.MilliSatoshi - cltvLimit *uint32 + cltvLimit uint32 dest route.Vertex rHash [32]byte cltvDelta uint16 @@ -2987,10 +2988,14 @@ func extractPaymentIntent(rpcPayReq *rpcPaymentRequest) (rpcPaymentIntent, error payIntent.outgoingChannelID = &rpcPayReq.OutgoingChanId } - // Take cltv limit from request if set. - if rpcPayReq.CltvLimit != 0 { - payIntent.cltvLimit = &rpcPayReq.CltvLimit + // Take the CLTV limit from the request if set, otherwise use the max. + cltvLimit, err := routerrpc.ValidateCLTVLimit( + rpcPayReq.CltvLimit, cfg.MaxOutgoingCltvExpiry, + ) + if err != nil { + return payIntent, err } + payIntent.cltvLimit = cltvLimit if len(rpcPayReq.DestTlv) != 0 { var err error