cnct: define separate broadcast delta for outgoing htlcs

This commits exposes the various parameters around going to chain and
accepting htlcs in a clear way.

In addition to this, it reverts those parameters to what they were
before the merge of commit d1076271456bdab1625ea6b52b93ca3e1bd9aed9.

config.go

@@ -67,7 +67,11 @@ const (
 	defaultTorV2PrivateKeyFilename = "v2_onion_private_key"
 	defaultTorV3PrivateKeyFilename = "v3_onion_private_key"
 
-	defaultBroadcastDelta = 10
+	defaultIncomingBroadcastDelta = 20
+	defaultFinalCltvRejectDelta   = 2
+
+	defaultOutgoingBroadcastDelta  = 10
+	defaultOutgoingCltvRejectDelta = 0
 
 	// minTimeLockDelta is the minimum timelock we require for incoming
 	// HTLCs on our channels.

contractcourt/chain_arbitrator.go

@@ -53,13 +53,20 @@ type ChainArbitratorConfig struct {
 	// ChainHash is the chain that this arbitrator is to operate within.
 	ChainHash chainhash.Hash
 
-	// BroadcastDelta is the delta that we'll use to decide when to
-	// broadcast our commitment transaction.  This value should be set
-	// based on our current fee estimation of the commitment transaction.
-	// We use this to determine when we should broadcast instead of the
-	// just the HTLC timeout, as we want to ensure that the commitment
-	// transaction is already confirmed, by the time the HTLC expires.
-	BroadcastDelta uint32
+	// IncomingBroadcastDelta is the delta that we'll use to decide when to
+	// broadcast our commitment transaction if we have incoming htlcs. This
+	// value should be set based on our current fee estimation of the
+	// commitment transaction. We use this to determine when we should
+	// broadcast instead of the just the HTLC timeout, as we want to ensure
+	// that the commitment transaction is already confirmed, by the time the
+	// HTLC expires. Otherwise we may end up not settling the htlc on-chain
+	// because the other party managed to time it out.
+	IncomingBroadcastDelta uint32
+
+	// OutgoingBroadcastDelta is the delta that we'll use to decide when to
+	// broadcast our commitment transaction if there are active outgoing
+	// htlcs. This value can be lower than the incoming broadcast delta.
+	OutgoingBroadcastDelta uint32
 
 	// NewSweepAddr is a function that returns a new address under control
 	// by the wallet. We'll use this to sweep any no-delay outputs as a

contractcourt/channel_arbitrator.go

@@ -1106,7 +1106,8 @@ func (c *ChannelArbitrator) checkChainActions(height uint32,
 		// We'll need to go on-chain for an outgoing HTLC if it was
 		// never resolved downstream, and it's "close" to timing out.
 		haveChainActions = haveChainActions || c.shouldGoOnChain(
-			htlc.RefundTimeout, c.cfg.BroadcastDelta, height,
+			htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta,
+			height,
 		)
 	}
 	for _, htlc := range c.activeHTLCs.incomingHTLCs {
@@ -1124,7 +1125,8 @@ func (c *ChannelArbitrator) checkChainActions(height uint32,
 			continue
 		}
 		haveChainActions = haveChainActions || c.shouldGoOnChain(
-			htlc.RefundTimeout, c.cfg.BroadcastDelta, height,
+			htlc.RefundTimeout, c.cfg.IncomingBroadcastDelta,
+			height,
 		)
 	}
 
@@ -1162,7 +1164,8 @@ func (c *ChannelArbitrator) checkChainActions(height uint32,
 		// until the HTLC times out to see if we can also redeem it
 		// on-chain.
 		case !c.shouldGoOnChain(
-			htlc.RefundTimeout, c.cfg.BroadcastDelta, height,
+			htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta,
+			height,
 		):
 			// TODO(roasbeef): also need to be able to query
 			// circuit map to see if HTLC hasn't been fully

contractcourt/channel_arbitrator_test.go

@@ -165,7 +165,8 @@ func createTestChannelArbitrator(log ArbitratorLog) (*ChannelArbitrator,
 		DeliverResolutionMsg: func(...ResolutionMsg) error {
 			return nil
 		},
-		BroadcastDelta: 5,
+		OutgoingBroadcastDelta: 5,
+		IncomingBroadcastDelta: 5,
 		Notifier: &mockNotifier{
 			epochChan: make(chan *chainntnfs.BlockEpoch),
 			spendChan: make(chan *chainntnfs.SpendDetail),

htlcswitch/link.go

@@ -235,17 +235,18 @@ type ChannelLinkConfig struct {
 	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
+	// FinalCltvRejectDelta defines the number of blocks before the expiry
+	// of the htlc where we no longer settle it as an exit hop and instead
+	// cancel it back. Normally this value should be lower than the cltv
+	// expiry of any invoice we create and the code effectuating this should
+	// not be hit.
+	FinalCltvRejectDelta uint32
+
+	// OutgoingCltvRejectDelta defines the number of blocks before expiry of
+	// an htlc where we don't offer an htlc anymore. This should be at least
+	// the outgoing broadcast delta, because in any case we don't want to
+	// risk offering an htlc that triggers channel closure.
+	OutgoingCltvRejectDelta uint32
 }
 
 // channelLink is the service which drives a channel's commitment update
@@ -2156,7 +2157,7 @@ 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 too
 	// close to the current height.
-	if outgoingTimeout-l.cfg.ExpiryGraceDelta <= heightNow {
+	if outgoingTimeout-l.cfg.OutgoingCltvRejectDelta <= heightNow {
 		l.errorf("htlc(%x) has an expiry that's too soon: "+
 			"outgoing_expiry=%v, best_height=%v", payHash[:],
 			outgoingTimeout, heightNow)
@@ -2679,7 +2680,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-l.cfg.ExpiryGraceDelta <= heightNow {
+	if pd.Timeout-l.cfg.FinalCltvRejectDelta <= heightNow {
 		log.Errorf("htlc(%x) has an expiry that's too soon: expiry=%v"+
 			", best_height=%v", pd.RHash[:], pd.Timeout, heightNow)
 

htlcswitch/test_utils.go

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

lnd_test.go

@@ -9420,7 +9420,7 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest) {
 	// We'll now mine enough blocks to trigger Bob's broadcast of his
 	// commitment transaction due to the fact that the HTLC is about to
 	// timeout.
-	numBlocks := uint32(finalCltvDelta - defaultBroadcastDelta)
+	numBlocks := uint32(finalCltvDelta - defaultOutgoingBroadcastDelta)
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks: %v", err)
 	}
@@ -9469,7 +9469,10 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest) {
 
 	// We'll now mine the remaining blocks to cause the HTLC itself to
 	// timeout.
-	if _, err := net.Miner.Node.Generate(defaultBroadcastDelta - defaultCSV); err != nil {
+	_, err = net.Miner.Node.Generate(
+		defaultOutgoingBroadcastDelta - defaultCSV,
+	)
+	if err != nil {
 		t.Fatalf("unable to generate blocks: %v", err)
 	}
 
@@ -9600,7 +9603,7 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 	const invoiceAmt = 100000
 	invoiceReq := &lnrpc.Invoice{
 		Value:      invoiceAmt,
-		CltvExpiry: 20,
+		CltvExpiry: 40,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -9644,7 +9647,9 @@ 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(invoiceReq.CltvExpiry - defaultBroadcastDelta)
+	numBlocks := uint32(
+		invoiceReq.CltvExpiry - defaultIncomingBroadcastDelta,
+	)
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks")
 	}
@@ -10324,7 +10329,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,
+		CltvExpiry: 40,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -10380,7 +10385,9 @@ 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(20 - defaultBroadcastDelta)
+	numBlocks := uint32(invoiceReq.CltvExpiry -
+		defaultIncomingBroadcastDelta)
+
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks")
 	}
@@ -10661,7 +10668,7 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 	const invoiceAmt = 100000
 	invoiceReq := &lnrpc.Invoice{
 		Value:      invoiceAmt,
-		CltvExpiry: 20,
+		CltvExpiry: 40,
 	}
 	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
 	carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq)
@@ -10731,7 +10738,9 @@ 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.
-	numBlocks := uint32(20-defaultBroadcastDelta) - defaultCSV
+	numBlocks := uint32(invoiceReq.CltvExpiry-
+		defaultIncomingBroadcastDelta) - defaultCSV
+
 	if _, err := net.Miner.Node.Generate(numBlocks); err != nil {
 		t.Fatalf("unable to generate blocks")
 	}

peer.go

@@ -58,13 +58,6 @@ 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
@@ -206,11 +199,13 @@ 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
+	// finalCltvRejectDelta defines the number of blocks before the expiry
+	// of the htlc where we no longer settle it as an exit hop.
+	finalCltvRejectDelta uint32
+
+	// outgoingCltvRejectDelta defines the number of blocks before expiry of
+	// an htlc where we don't offer an htlc anymore.
+	outgoingCltvRejectDelta uint32
 
 	// remoteLocalFeatures is the local feature vector received from the
 	// peer during the connection handshake.
@@ -249,7 +244,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, expiryGraceDelta uint32) (
+	chanActiveTimeout time.Duration,
+	finalCltvRejectDelta, outgoingCltvRejectDelta uint32) (
 	*peer, error) {
 
 	nodePub := addr.IdentityKey
@@ -264,7 +260,9 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server,
 		server: server,
 
 		localFeatures: localFeatures,
-		expiryGraceDelta: expiryGraceDelta,
+
+		finalCltvRejectDelta:    finalCltvRejectDelta,
+		outgoingCltvRejectDelta: outgoingCltvRejectDelta,
 
 		sendQueue:     make(chan outgoingMsg),
 		outgoingQueue: make(chan outgoingMsg),
@@ -604,7 +602,8 @@ func (p *peer) addLink(chanPoint *wire.OutPoint,
 		UnsafeReplay:            cfg.UnsafeReplay,
 		MinFeeUpdateTimeout:     htlcswitch.DefaultMinLinkFeeUpdateTimeout,
 		MaxFeeUpdateTimeout:     htlcswitch.DefaultMaxLinkFeeUpdateTimeout,
-		ExpiryGraceDelta:    p.expiryGraceDelta,
+		FinalCltvRejectDelta:    p.finalCltvRejectDelta,
+		OutgoingCltvRejectDelta: p.outgoingCltvRejectDelta,
 	}
 
 	link := htlcswitch.NewChannelLink(linkCfg, lnChan)

server.go

@@ -729,7 +729,8 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, cc *chainControl,
 
 	s.chainArb = contractcourt.NewChainArbitrator(contractcourt.ChainArbitratorConfig{
 		ChainHash:              *activeNetParams.GenesisHash,
-		BroadcastDelta: defaultBroadcastDelta,
+		IncomingBroadcastDelta: defaultIncomingBroadcastDelta,
+		OutgoingBroadcastDelta: defaultOutgoingBroadcastDelta,
 		NewSweepAddr: func() ([]byte, error) {
 			return newSweepPkScript(cc.wallet)
 		},
@@ -2475,14 +2476,16 @@ func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq,
 	localFeatures.Set(lnwire.GossipQueriesOptional)
 
 	// Now that we've established a connection, create a peer, and it to the
-	// 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.
+	// set of currently active peers. Configure the peer with the incoming
+	// and outgoing broadcast deltas to prevent htlcs from being accepted or
+	// offered that would trigger channel closure. In case of outgoing
+	// htlcs, an extra block is added to prevent the channel from being
+	// closed when the htlc is outstanding and a new block comes in.
 	p, err := newPeer(
 		conn, connReq, s, peerAddr, inbound, localFeatures,
 		cfg.ChanEnableTimeout,
-		defaultBroadcastDelta+extraExpiryGraceDelta,
+		defaultFinalCltvRejectDelta,
+		defaultOutgoingCltvRejectDelta,
 	)
 	if err != nil {
 		srvrLog.Errorf("unable to create peer %v", err)