Merge pull request #2839 from joostjager/htlc-expiry-check

link: increase expiry grace delta for exit hop and forwarded htlcs

contractcourt/channel_arbitrator.go

@@ -16,15 +16,6 @@ import (
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
-const (
-	// broadcastRedeemMultiplier is the additional factor that we'll scale
-	// the normal broadcastDelta by when deciding whether or not to
-	// broadcast a commitment to claim an HTLC on-chain. We use a scaled
-	// value, as when redeeming we want to ensure that we have enough time
-	// to redeem the HTLC, well before it times out.
-	broadcastRedeemMultiplier = 2
-)
-
 var (
 	// errAlreadyForceClosed is an error returned when we attempt to force
 	// close a channel that's already in the process of doing so.
@@ -1101,7 +1092,6 @@ func (c *ChannelArbitrator) checkChainActions(height uint32,
 		"height=%v", c.cfg.ChanPoint, height)
 
 	actionMap := make(ChainActionMap)
-	redeemCutoff := c.cfg.BroadcastDelta * broadcastRedeemMultiplier
 
 	// First, we'll make an initial pass over the set of incoming and
 	// outgoing HTLC's to decide if we need to go on chain at all.
@@ -1134,7 +1124,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32,
 			continue
 		}
 		haveChainActions = haveChainActions || c.shouldGoOnChain(
-			htlc.RefundTimeout, redeemCutoff, height,
+			htlc.RefundTimeout, c.cfg.BroadcastDelta, height,
 		)
 	}
 

htlcswitch/link.go

@@ -30,16 +30,6 @@ func init() {
 }
 
 const (
-	// expiryGraceDelta is a grace period that the timeout of incoming
-	// HTLC's that pay directly to us (i.e we're the "exit node") must up
-	// hold. We'll reject any HTLC's who's timeout minus this value is less
-	// that or equal to the current block height. We require this in order
-	// to ensure that if the extending party goes to the chain, then we'll
-	// be able to claim the HTLC still.
-	//
-	// TODO(roasbeef): must be < default delta
-	expiryGraceDelta = 2
-
 	// maxCltvExpiry is the maximum outgoing time lock that the node accepts
 	// for forwarded payments. The value is relative to the current block
 	// height. The reason to have a maximum is to prevent funds getting
@@ -244,6 +234,18 @@ type ChannelLinkConfig struct {
 	// fee rate. A random timeout will be selected between these values.
 	MinFeeUpdateTimeout time.Duration
 	MaxFeeUpdateTimeout time.Duration
+
+	// ExpiryGraceDelta is the minimum difference between the current block
+	// height and the CLTV we require on 1) an outgoing HTLC in order to
+	// forward as an intermediary hop, or 2) an incoming HTLC to reveal the
+	// preimage as the final hop. We'll reject any HTLC's who's timeout minus
+	// this value is less than or equal to the current block height. We require
+	// this in order to ensure that we have sufficient time to claim or
+	// timeout an HTLC on chain.
+	//
+	// This MUST be greater than the maximum BroadcastDelta of the
+	// ChannelArbitrator for the outbound channel.
+	ExpiryGraceDelta uint32
 }
 
 // channelLink is the service which drives a channel's commitment update
@@ -2151,14 +2153,13 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte,
 		return failure
 	}
 
-	// We want to avoid accepting an HTLC which will expire in the near
+	// We want to avoid offering an HTLC which will expire in the near
-	// future, so we'll reject an HTLC if its expiration time is too close
+	// future, so we'll reject an HTLC if the outgoing expiration time is too
-	// to the current height.
+	// close to the current height.
-	timeDelta := policy.TimeLockDelta
+	if outgoingTimeout-l.cfg.ExpiryGraceDelta <= heightNow {
-	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)
+			outgoingTimeout, heightNow)
 
 		var failure lnwire.FailureMessage
 		update, err := l.cfg.FetchLastChannelUpdate(
@@ -2185,6 +2186,7 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte,
 	// 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.
+	timeDelta := policy.TimeLockDelta
 	if incomingTimeout-timeDelta < outgoingTimeout {
 		l.errorf("Incoming htlc(%x) has incorrect time-lock value: "+
 			"expected at least %v block delta, got %v block delta",
@@ -2677,7 +2679,7 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor,
 
 	// First, we'll check the expiry of the HTLC itself against, the current
 	// block height. If the timeout is too soon, then we'll reject the HTLC.
-	if pd.Timeout-expiryGraceDelta <= heightNow {
+	if pd.Timeout-l.cfg.ExpiryGraceDelta <= heightNow {
 		log.Errorf("htlc(%x) has an expiry that's too soon: expiry=%v"+
 			", best_height=%v", pd.RHash[:], pd.Timeout, heightNow)
 

htlcswitch/link_test.go

@@ -1336,10 +1336,13 @@ func TestChannelLinkExpiryTooSoonExitNode(t *testing.T) {
 
 	amount := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin)
 
-	// We'll craft an HTLC packet, but set the starting height to 10 blocks
+	// We'll craft an HTLC packet, but set the final hop CLTV to 3 blocks
-	// before the current true height.
+	// after the current true height. This is less or equal to the expiry
+	// grace delta of 3, so we expect the incoming htlc to be failed by the
+	// exit hop.
+	lastHopDelta := n.firstBobChannelLink.cfg.FwrdingPolicy.TimeLockDelta
 	htlcAmt, totalTimelock, hops := generateHops(amount,
-		startingHeight-10, n.firstBobChannelLink)
+		startingHeight+3-lastHopDelta, n.firstBobChannelLink)
 
 	// Now we'll send out the payment from Alice to Bob.
 	firstHop := n.firstBobChannelLink.ShortChanID()
@@ -1395,11 +1398,14 @@ func TestChannelLinkExpiryTooSoonMidNode(t *testing.T) {
 
 	amount := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin)
 
-	// We'll craft an HTLC packet, but set the starting height to 10 blocks
+	// We'll craft an HTLC packet, but set the starting height to 3 blocks
-	// before the current true height. The final route will be three hops,
+	// before the current true height. This means that the outgoing time
-	// so the middle hop should detect the issue.
+	// lock of the middle hop will be at starting height + 3 blocks (channel
+	// policy time lock delta is 6 blocks). There is an expiry grace delta
+	// of 3 blocks relative to the current height, meaning that htlc will
+	// not be sent out by the middle hop.
 	htlcAmt, totalTimelock, hops := generateHops(amount,
-		startingHeight-10, n.firstBobChannelLink, n.carolChannelLink)
+		startingHeight-3, n.firstBobChannelLink, n.carolChannelLink)
 
 	// Now we'll send out the payment from Alice to Bob.
 	firstHop := n.firstBobChannelLink.ShortChanID()

htlcswitch/test_utils.go

@@ -1019,6 +1019,7 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer,
 		fwdPkgTimeout       = 15 * time.Second
 		minFeeUpdateTimeout = 30 * time.Minute
 		maxFeeUpdateTimeout = 40 * time.Minute
+		expiryGraceDelta    = 3
 	)
 
 	link := NewChannelLink(
@@ -1048,6 +1049,7 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer,
 			MinFeeUpdateTimeout: minFeeUpdateTimeout,
 			MaxFeeUpdateTimeout: maxFeeUpdateTimeout,
 			OnChannelFailure:    func(lnwire.ChannelID, lnwire.ShortChannelID, LinkFailureError) {},
+			ExpiryGraceDelta:    expiryGraceDelta,
 		},
 		channel,
 	)

lnd_test.go

@@ -8104,6 +8104,14 @@ out:
 		t.Fatalf("unable to generate carol invoice: %v", err)
 	}
 
+	carolPayReq, err := carol.DecodePayReq(ctxb,
+		&lnrpc.PayReqString{
+			PayReq: carolInvoice.PaymentRequest,
+		})
+	if err != nil {
+		t.Fatalf("unable to decode generated payment request: %v", err)
+	}
+
 	// Before we send the payment, ensure that the announcement of the new
 	// channel has been processed by Alice.
 	ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
@@ -8120,6 +8128,7 @@ out:
 		PaymentHashString: hex.EncodeToString(makeFakePayHash(t)),
 		DestString:        hex.EncodeToString(carol.PubKey[:]),
 		Amt:               payAmt,
+		FinalCltvDelta:    int32(carolPayReq.CltvExpiry),
 	}
 	resp, err := net.Alice.SendPaymentSync(ctxt, sendReq)
 	if err != nil {
@@ -8150,6 +8159,7 @@ out:
 		PaymentHashString: hex.EncodeToString(carolInvoice.RHash),
 		DestString:        hex.EncodeToString(carol.PubKey[:]),
 		Amt:               int64(htlcAmt.ToSatoshis()), // 10k satoshis are expected.
+		FinalCltvDelta:    int32(carolPayReq.CltvExpiry),
 	}
 	ctxt, _ = context.WithTimeout(ctxb, defaultTimeout)
 	resp, err = net.Alice.SendPaymentSync(ctxt, sendReq)
@@ -9585,9 +9595,12 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 	defer shutdownAndAssert(net, t, carol)
 
 	// With the network active, we'll now add a new invoice at Carol's end.
+	// Make sure the cltv expiry delta is large enough, otherwise Bob won't
+	// send out the outgoing htlc.
 	const invoiceAmt = 100000
 	invoiceReq := &lnrpc.Invoice{
 		Value:      invoiceAmt,
+		CltvExpiry: 20,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -9631,7 +9644,7 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 	// Now we'll mine enough blocks to prompt carol to actually go to the
 	// chain in order to sweep her HTLC since the value is high enough.
 	// TODO(roasbeef): modify once go to chain policy changes
-	numBlocks := uint32(defaultBitcoinTimeLockDelta - (2 * defaultBroadcastDelta))
+	numBlocks := uint32(invoiceReq.CltvExpiry - defaultBroadcastDelta)
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks")
 	}
@@ -10311,6 +10324,7 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 	// With the network active, we'll now add a new invoice at Carol's end.
 	invoiceReq := &lnrpc.Invoice{
 		Value:      100000,
+		CltvExpiry: 20,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -10366,7 +10380,7 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 
 	// We'll now mine enough blocks so Carol decides that she needs to go
 	// on-chain to claim the HTLC as Bob has been inactive.
-	numBlocks := uint32(defaultBitcoinTimeLockDelta - (2 * defaultBroadcastDelta))
+	numBlocks := uint32(20 - defaultBroadcastDelta)
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks")
 	}
@@ -10647,6 +10661,7 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 	const invoiceAmt = 100000
 	invoiceReq := &lnrpc.Invoice{
 		Value:      invoiceAmt,
+		CltvExpiry: 20,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -10716,8 +10731,7 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 
 	// We'll now mine enough blocks so Carol decides that she needs to go
 	// on-chain to claim the HTLC as Bob has been inactive.
-	claimDelta := uint32(2 * defaultBroadcastDelta)
+	numBlocks := uint32(20-defaultBroadcastDelta) - defaultCSV
-	numBlocks := uint32(defaultBitcoinTimeLockDelta-claimDelta) - defaultCSV
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks")
 	}

peer.go

@@ -58,6 +58,13 @@ const (
 	// messages to be sent across the wire, requested by objects outside
 	// this struct.
 	outgoingQueueLen = 50
+
+	// extraExpiryGraceDelta is the additional number of blocks required by
+	// the ExpiryGraceDelta of the forwarding policy beyond the maximum
+	// broadcast delta. This is the minimum number of blocks between the
+	// expiry on an accepted or offered HTLC and the block height at which
+	// we will go to chain.
+	extraExpiryGraceDelta = 3
 )
 
 // outgoingMsg packages an lnwire.Message to be sent out on the wire, along with
@@ -199,6 +206,12 @@ type peer struct {
 	// remote node.
 	localFeatures *lnwire.RawFeatureVector
 
+	// expiryGraceDelta is the block time allowance for HTLCs offered and
+	// received on channels with this peer. The parameter is used to
+	// configure links with the peer. See ExpiryGraceDelta on
+	// ChannelLinkConfig for more information.
+	expiryGraceDelta uint32
+
 	// remoteLocalFeatures is the local feature vector received from the
 	// peer during the connection handshake.
 	remoteLocalFeatures *lnwire.FeatureVector
@@ -236,7 +249,8 @@ var _ lnpeer.Peer = (*peer)(nil)
 func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server,
 	addr *lnwire.NetAddress, inbound bool,
 	localFeatures *lnwire.RawFeatureVector,
-	chanActiveTimeout time.Duration) (*peer, error) {
+	chanActiveTimeout time.Duration, expiryGraceDelta uint32) (
+	*peer, error) {
 
 	nodePub := addr.IdentityKey
 
@@ -250,6 +264,7 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server,
 		server: server,
 
 		localFeatures:    localFeatures,
+		expiryGraceDelta: expiryGraceDelta,
 
 		sendQueue:     make(chan outgoingMsg),
 		outgoingQueue: make(chan outgoingMsg),
@@ -592,6 +607,7 @@ func (p *peer) addLink(chanPoint *wire.OutPoint,
 		UnsafeReplay:        cfg.UnsafeReplay,
 		MinFeeUpdateTimeout: htlcswitch.DefaultMinLinkFeeUpdateTimeout,
 		MaxFeeUpdateTimeout: htlcswitch.DefaultMaxLinkFeeUpdateTimeout,
+		ExpiryGraceDelta:    p.expiryGraceDelta,
 	}
 
 	link := htlcswitch.NewChannelLink(linkCfg, lnChan)

server.go

@@ -728,8 +728,6 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, cc *chainControl,
 
 	s.chainArb = contractcourt.NewChainArbitrator(contractcourt.ChainArbitratorConfig{
 		ChainHash:      *activeNetParams.GenesisHash,
-		// TODO(roasbeef): properly configure
-		//  * needs to be << or specified final hop time delta
 		BroadcastDelta: defaultBroadcastDelta,
 		NewSweepAddr: func() ([]byte, error) {
 			return newSweepPkScript(cc.wallet)
@@ -2475,11 +2473,15 @@ func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq,
 	localFeatures.Set(lnwire.DataLossProtectRequired)
 	localFeatures.Set(lnwire.GossipQueriesOptional)
 
-	// Now that we've established a connection, create a peer, and it to
+	// Now that we've established a connection, create a peer, and it to the
-	// the set of currently active peers.
+	// set of currently active peers. Configure the peer with a expiry grace
+	// delta greater than the broadcast delta, to prevent links from
+	// accepting htlcs that may trigger channel arbitrator force close the
+	// channel immediately.
 	p, err := newPeer(
 		conn, connReq, s, peerAddr, inbound, localFeatures,
 		cfg.ChanEnableTimeout,
+		defaultBroadcastDelta+extraExpiryGraceDelta,
 	)
 	if err != nil {
 		srvrLog.Errorf("unable to create peer %v", err)