link: increase expiry grace delta

This commit increase the expiry grace delta to a value above the
broadcast delta. This prevents htlcs from being accepted that would
immediately trigger a channel force close.

A correct delta is generated in server.go where there is access to
the broadcast delta and passed via the peer to the links.

Co-authored-by: Jim Posen <jim.posen@gmail.com>

htlcswitch/link.go

@@ -30,18 +30,6 @@ func init() {
 }
 
 const (
-	// 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 = 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
@@ -246,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
@@ -2150,9 +2150,9 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte,
 	}
 
 	// We want to avoid offering an HTLC which will expire in the near
-	// future, so we'll reject an HTLC if the outgoing expiration time is
+	// future, so we'll reject an HTLC if the outgoing expiration time is too
-	// too close to the current height.
+	// close to the current height.
-	if outgoingTimeout-expiryGraceDelta <= heightNow {
+	if outgoingTimeout-l.cfg.ExpiryGraceDelta <= heightNow {
 		l.errorf("htlc(%x) has an expiry that's too soon: "+
 			"outgoing_expiry=%v, best_height=%v", payHash[:],
 			outgoingTimeout, heightNow)
@@ -2675,7 +2675,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

@@ -8164,6 +8164,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)
@@ -8180,6 +8188,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 {
@@ -8210,6 +8219,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)
@@ -9652,9 +9662,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,
+		Value:      invoiceAmt,
+		CltvExpiry: 20,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -9698,7 +9711,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")
 	}
@@ -10382,7 +10395,8 @@ 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,
+		Value:      100000,
+		CltvExpiry: 20,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -10438,7 +10452,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")
 	}
@@ -10722,7 +10736,8 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 	// With the network active, we'll now add a new invoice at Carol's end.
 	const invoiceAmt = 100000
 	invoiceReq := &lnrpc.Invoice{
-		Value: invoiceAmt,
+		Value:      invoiceAmt,
+		CltvExpiry: 20,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -10792,8 +10807,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
 
@@ -249,7 +263,8 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server,
 
 		server: server,
 
-		localFeatures: localFeatures,
+		localFeatures:    localFeatures,
+		expiryGraceDelta: expiryGraceDelta,
 
 		sendQueue:     make(chan outgoingMsg),
 		outgoingQueue: make(chan outgoingMsg),
@@ -587,6 +602,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

@@ -711,9 +711,7 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, cc *chainControl,
 	contractBreaches := make(chan *ContractBreachEvent, 1)
 
 	s.chainArb = contractcourt.NewChainArbitrator(contractcourt.ChainArbitratorConfig{
-		ChainHash: *activeNetParams.GenesisHash,
+		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)
@@ -2373,11 +2371,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)