diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index d1afa568..e5cf4cd8 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -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, + amtToForward lnwire.MilliSatoshi, + incomingTimeout, outgoingTimeout uint32, + heightNow uint32) lnwire.FailureMessage // Bandwidth returns the amount of milli-satoshis which current link // might pass through channel link. The value returned from this method diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 2aeb9707..3da07399 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1747,7 +1747,9 @@ func (l *channelLink) UpdateForwardingPolicy(newPolicy ForwardingPolicy) { // // NOTE: Part of the ChannelLink interface. func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte, - incomingHtlcAmt, amtToForward lnwire.MilliSatoshi) lnwire.FailureMessage { + incomingHtlcAmt, amtToForward lnwire.MilliSatoshi, + incomingTimeout, outgoingTimeout uint32, + heightNow uint32) lnwire.FailureMessage { l.RLock() policy := l.cfg.FwrdingPolicy @@ -1788,10 +1790,8 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte, // any case, we'll cancel this HTLC. actualFee := incomingHtlcAmt - amtToForward if incomingHtlcAmt < amtToForward || actualFee < expectedFee { - l.errorf("outgoing htlc(%x) has insufficient "+ - "fee: expected %v, got %v", payHash[:], - int64(expectedFee), - int64(actualFee)) + l.errorf("outgoing htlc(%x) has insufficient fee: expected %v, "+ + "got %v", payHash[:], int64(expectedFee), int64(actualFee)) // As part of the returned error, we'll send our latest routing // policy so the sending node obtains the most up to date data. @@ -1808,6 +1808,54 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte, return failure } + // We want to avoid accepting an HTLC which will expire in the near + // future, so we'll reject an HTLC if its expiration time is too close + // to the current height. + timeDelta := policy.TimeLockDelta + if incomingTimeout-timeDelta <= heightNow { + l.errorf("htlc(%x) has an expiry that's too soon: "+ + "outgoing_expiry=%v, best_height=%v", payHash[:], + incomingTimeout-timeDelta, heightNow) + + var failure lnwire.FailureMessage + update, err := l.cfg.FetchLastChannelUpdate( + l.ShortChanID(), + ) + if err != nil { + failure = lnwire.NewTemporaryChannelFailure(update) + } else { + failure = lnwire.NewExpiryTooSoon(*update) + } + + return failure + } + + // Finally, we'll ensure that the time-lock on the outgoing HTLC meets + // the following constraint: the incoming time-lock minus our time-lock + // 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 { + l.errorf("Incoming htlc(%x) has incorrect time-lock value: "+ + "expected at least %v block delta, got %v block delta", + payHash[:], timeDelta, incomingTimeout-outgoingTimeout) + + // Grab the latest routing policy so the sending node is up to + // date with our current policy. + var failure lnwire.FailureMessage + update, err := l.cfg.FetchLastChannelUpdate( + l.ShortChanID(), + ) + if err != nil { + failure = lnwire.NewTemporaryChannelFailure(update) + } else { + failure = lnwire.NewIncorrectCltvExpiry( + incomingTimeout, *update, + ) + } + + return failure + } + return nil } @@ -2289,8 +2337,7 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg, needUpdate = true // There are additional channels left within this route. So - // we'll verify that our forwarding constraints have been - // properly met by this incoming HTLC. + // we'll simply do some forwarding package book-keeping. default: // If hodl.AddIncoming is requested, we will not // validate the forwarded ADD, nor will we send the @@ -2332,14 +2379,16 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg, chanIterator.EncodeNextHop(buf) updatePacket := &htlcPacket{ - incomingChanID: l.ShortChanID(), - incomingHTLCID: pd.HtlcIndex, - outgoingChanID: fwdInfo.NextHop, - sourceRef: pd.SourceRef, - incomingAmount: pd.Amount, - amount: addMsg.Amount, - htlc: addMsg, - obfuscator: obfuscator, + incomingChanID: l.ShortChanID(), + incomingHTLCID: pd.HtlcIndex, + outgoingChanID: fwdInfo.NextHop, + sourceRef: pd.SourceRef, + incomingAmount: pd.Amount, + amount: addMsg.Amount, + htlc: addMsg, + obfuscator: obfuscator, + incomingTimeout: pd.Timeout, + outgoingTimeout: fwdInfo.OutgoingCTLV, } switchPackets = append( switchPackets, updatePacket, @@ -2348,80 +2397,8 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg, continue } - // We'll consult the forwarding policy for this link - // when checking time locked related constraints. - hopPolicy := l.cfg.FwrdingPolicy - - // We want to avoid forwarding an HTLC which will - // expire in the near future, so we'll reject an HTLC - // if its expiration time is too close to the current - // height. - timeDelta := hopPolicy.TimeLockDelta - if pd.Timeout-timeDelta <= heightNow { - log.Errorf("htlc(%x) has an expiry "+ - "that's too soon: outgoing_expiry=%v, "+ - "best_height=%v", pd.RHash[:], - pd.Timeout-timeDelta, heightNow) - - var failure lnwire.FailureMessage - update, err := l.cfg.FetchLastChannelUpdate( - l.ShortChanID(), - ) - if err != nil { - failure = lnwire.NewTemporaryChannelFailure( - update, - ) - } else { - failure = lnwire.NewExpiryTooSoon(*update) - } - - l.sendHTLCError( - pd.HtlcIndex, failure, obfuscator, - pd.SourceRef, - ) - needUpdate = true - continue - } - - // Finally, we'll ensure that the time-lock on the - // outgoing HTLC meets the following constraint: the - // incoming time-lock minus our time-lock delta should - // equal the outgoing time lock. Otherwise, whether the - // sender messed up, or an intermediate node tampered - // with the HTLC. - if pd.Timeout-timeDelta < fwdInfo.OutgoingCTLV { - log.Errorf("Incoming htlc(%x) has incorrect "+ - "time-lock value: expected at least "+ - "%v block delta, got %v block delta", - pd.RHash[:], timeDelta, - pd.Timeout-fwdInfo.OutgoingCTLV) - - // Grab the latest routing policy so the - // sending node is up to date with our current - // policy. - update, err := l.cfg.FetchLastChannelUpdate( - l.ShortChanID(), - ) - if err != nil { - l.fail(LinkFailureError{ - code: ErrInternalError}, - "unable to create channel "+ - "update while handling "+ - "the error: %v", err) - return false - } - - failure := lnwire.NewIncorrectCltvExpiry( - pd.Timeout, *update) - l.sendHTLCError( - pd.HtlcIndex, failure, obfuscator, pd.SourceRef, - ) - - needUpdate = true - continue - } - - // TODO(roasbeef): also add max timeout value + // TODO(roasbeef): ensure don't accept outrageous + // timeout for htlc // With all our forwarding constraints met, we'll // create the outgoing HTLC using the parameters as @@ -2470,14 +2447,16 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg, // section. if fwdPkg.State == channeldb.FwdStateLockedIn { updatePacket := &htlcPacket{ - incomingChanID: l.ShortChanID(), - incomingHTLCID: pd.HtlcIndex, - outgoingChanID: fwdInfo.NextHop, - sourceRef: pd.SourceRef, - incomingAmount: pd.Amount, - amount: addMsg.Amount, - htlc: addMsg, - obfuscator: obfuscator, + incomingChanID: l.ShortChanID(), + incomingHTLCID: pd.HtlcIndex, + outgoingChanID: fwdInfo.NextHop, + sourceRef: pd.SourceRef, + incomingAmount: pd.Amount, + amount: addMsg.Amount, + htlc: addMsg, + obfuscator: obfuscator, + incomingTimeout: pd.Timeout, + outgoingTimeout: fwdInfo.OutgoingCTLV, } fwdPkg.FwdFilter.Set(idx) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 8c56b1e4..2cfba17b 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -618,6 +618,7 @@ func TestLinkForwardTimelockPolicyMismatch(t *testing.T) { _, err = n.makePayment(n.aliceServer, n.carolServer, n.bobServer.PubKey(), hops, amount, htlcAmt, htlcExpiry).Wait(30 * time.Second) + // We should get an error, and that error should indicate that the HTLC // should be rejected due to a policy violation. if err == nil { @@ -637,9 +638,9 @@ func TestLinkForwardTimelockPolicyMismatch(t *testing.T) { } } -// TestLinkForwardTimelockPolicyMismatch tests that if a node is an -// intermediate node in a multi-hop payment and receives an HTLC that violates -// its current fee policy, then the HTLC is rejected with the proper error. +// TestLinkForwardFeePolicyMismatch tests that if a node is an intermediate +// node in a multi-hop payment and receives an HTLC that violates its current +// fee policy, then the HTLC is rejected with the proper error. func TestLinkForwardFeePolicyMismatch(t *testing.T) { t.Parallel() @@ -4528,3 +4529,55 @@ func TestExpectedFee(t *testing.T) { } } } + +// TestForwardingAsymmetricTimeLockPolicies tests that each link is able to +// properly handle forwarding HTLCs when their outgoing channels have +// asymmetric policies w.r.t what they require for time locks. +func TestForwardingAsymmetricTimeLockPolicies(t *testing.T) { + t.Parallel() + + // First, we'll create our traditional three hop network. Bob + // interacting with and asserting the state of two of the end points + // for this test. + channels, cleanUp, _, err := createClusterChannels( + btcutil.SatoshiPerBitcoin*3, + btcutil.SatoshiPerBitcoin*5, + ) + if err != nil { + t.Fatalf("unable to create channel: %v", err) + } + defer cleanUp() + + n := newThreeHopNetwork( + t, channels.aliceToBob, channels.bobToAlice, channels.bobToCarol, + channels.carolToBob, testStartingHeight, + ) + if err := n.start(); err != nil { + t.Fatalf("unable to start three hop network: %v", err) + } + defer n.stop() + + // Now that each of the links are up, we'll modify the link from Alice + // -> Bob to have a greater time lock delta than that of the link of + // Bob -> Carol. + n.firstBobChannelLink.UpdateForwardingPolicy(ForwardingPolicy{ + TimeLockDelta: 7, + }) + + // Now that the Alice -> Bob link has been updated, we'll craft and + // send a payment from Alice -> Carol. This should succeed as normal, + // even though Bob has asymmetric time lock policies. + amount := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) + htlcAmt, totalTimelock, hops := generateHops( + amount, testStartingHeight, n.firstBobChannelLink, + n.carolChannelLink, + ) + + _, err = n.makePayment( + n.aliceServer, n.carolServer, n.bobServer.PubKey(), hops, + amount, htlcAmt, totalTimelock, + ).Wait(30 * time.Second) + if err != nil { + t.Fatalf("unable to send payment: %v", err) + } +} diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index dc47730f..b96fa1e4 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -613,7 +613,7 @@ func (f *mockChannelLink) HandleChannelUpdate(lnwire.Message) { func (f *mockChannelLink) UpdateForwardingPolicy(_ ForwardingPolicy) { } func (f *mockChannelLink) HtlcSatifiesPolicy([32]byte, lnwire.MilliSatoshi, - lnwire.MilliSatoshi) lnwire.FailureMessage { + lnwire.MilliSatoshi, uint32, uint32, uint32) lnwire.FailureMessage { return nil } diff --git a/htlcswitch/packet.go b/htlcswitch/packet.go index 01e50d27..b6bff471 100644 --- a/htlcswitch/packet.go +++ b/htlcswitch/packet.go @@ -80,6 +80,15 @@ type htlcPacket struct { // circuit holds a reference to an Add's circuit which is persisted in // the switch during successful forwarding. circuit *PaymentCircuit + + // incomingTimeout is the timeout that the incoming HTLC carried. This + // is the timeout of the HTLC applied to the incoming link. + incomingTimeout uint32 + + // outgoingTimeout is the timeout of the proposed outgoing HTLC. This + // will be extraced from the hop payload recevived by the incoming + // link. + outgoingTimeout uint32 } // inKey returns the circuit key used to identify the incoming htlc. diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 1d649d73..12240238 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -964,9 +964,11 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { // Before we check the link's bandwidth, we'll ensure // that the HTLC satisfies the current forwarding // policy of this target link. + currentHeight := atomic.LoadUint32(&s.bestHeight) err := link.HtlcSatifiesPolicy( htlc.PaymentHash, packet.incomingAmount, - packet.amount, + packet.amount, packet.incomingTimeout, + packet.outgoingTimeout, currentHeight, ) if err != nil { linkErrs[link.ShortChanID()] = err @@ -1404,6 +1406,7 @@ out: } atomic.StoreUint32(&s.bestHeight, uint32(blockEpoch.Height)) + // A local close request has arrived, we'll forward this to the // relevant link (if it exists) so the channel can be // cooperatively closed (if possible). diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go index 505b87c7..8da96ece 100644 --- a/htlcswitch/test_utils.go +++ b/htlcswitch/test_utils.go @@ -601,15 +601,17 @@ func generateHops(payAmt lnwire.MilliSatoshi, startingHeight uint32, nextHop = path[i+1].channel.ShortChanID() } + var timeLock uint32 // If this is the last, hop, then the time lock will be their // specified delta policy plus our starting height. - totalTimelock += lastHop.cfg.FwrdingPolicy.TimeLockDelta - timeLock := totalTimelock - - // Otherwise, the outgoing time lock should be the incoming - // timelock minus their specified delta. - if i != len(path)-1 { - delta := path[i].cfg.FwrdingPolicy.TimeLockDelta + if i == len(path)-1 { + totalTimelock += lastHop.cfg.FwrdingPolicy.TimeLockDelta + timeLock = totalTimelock + } else { + // Otherwise, the outgoing time lock should be the + // incoming timelock minus their specified delta. + delta := path[i+1].cfg.FwrdingPolicy.TimeLockDelta + totalTimelock += delta timeLock = totalTimelock - delta } diff --git a/routing/pathfind.go b/routing/pathfind.go index b85b149f..5aa2298b 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -373,14 +373,14 @@ func newRoute(amtToSend, feeLimit lnwire.MilliSatoshi, sourceVertex Vertex, // route such that each hops time lock increases as we // walk backwards in the route, using the delta of the // previous hop. - route.TotalTimeLock += uint32(edge.TimeLockDelta) + delta := uint32(pathEdges[i+1].TimeLockDelta) + route.TotalTimeLock += delta // Otherwise, the value of the outgoing time-lock will // be the value of the time-lock for the _outgoing_ // HTLC, so we factor in their specified grace period // (time lock delta). - nextHop.OutgoingTimeLock = route.TotalTimeLock - - uint32(edge.TimeLockDelta) + nextHop.OutgoingTimeLock = route.TotalTimeLock - delta } route.Hops[i] = nextHop diff --git a/routing/testdata/spec_example.json b/routing/testdata/spec_example.json index a0cd0ffe..892d99b7 100644 --- a/routing/testdata/spec_example.json +++ b/routing/testdata/spec_example.json @@ -30,7 +30,7 @@ "channel_id": 12345, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 1, - "expiry": 20, + "expiry": 10, "min_htlc": 1, "fee_base_msat": 100, "fee_rate": 1000, @@ -43,7 +43,7 @@ "channel_id": 12345, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 0, - "expiry": 10, + "expiry": 20, "min_htlc": 1, "fee_base_msat": 200, "fee_rate": 2000, @@ -56,7 +56,7 @@ "channel_id": 12345839, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 1, - "expiry": 40, + "expiry": 10, "min_htlc": 1, "fee_base_msat": 100, "fee_rate": 1000, @@ -69,7 +69,7 @@ "channel_id": 12345839, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 0, - "expiry": 10, + "expiry": 40, "min_htlc": 1, "fee_base_msat": 400, "fee_rate": 4000, @@ -95,7 +95,7 @@ "channel_id": 1234583, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 1, - "expiry": 40, + "expiry": 30, "min_htlc": 1, "fee_base_msat": 300, "fee_rate": 3000, @@ -108,7 +108,7 @@ "channel_id": 1234589, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 1, - "expiry": 20, + "expiry": 30, "min_htlc": 1, "fee_base_msat": 300, "fee_rate": 3000, @@ -121,7 +121,7 @@ "channel_id": 1234589, "channel_point": "89dc56859c6a082d15ba1a7f6cb6be3fea62e1746e2cb8497b1189155c21a233:0", "flags": 0, - "expiry": 30, + "expiry": 20, "min_htlc": 1, "fee_base_msat": 200, "fee_rate": 2000,