diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index aae257463..e313ba837 100644 --- a/contractcourt/channel_arbitrator.go +++ b/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, ) } diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 6c49024f9..a7ecbe90c 100644 --- a/htlcswitch/link.go +++ b/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 - // 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 { + // 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 { 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) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 8d41cdd5c..4d8143fa3 100644 --- a/htlcswitch/link_test.go +++ b/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 - // before the current true height. + // We'll craft an HTLC packet, but set the final hop CLTV to 3 blocks + // 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 - // before the current true height. The final route will be three hops, - // so the middle hop should detect the issue. + // We'll craft an HTLC packet, but set the starting height to 3 blocks + // before the current true height. This means that the outgoing time + // 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() diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go index 053c99cfc..19a6d19c3 100644 --- a/htlcswitch/test_utils.go +++ b/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, ) diff --git a/lnd_test.go b/lnd_test.go index 80b8abb11..52809049b 100644 --- a/lnd_test.go +++ b/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, + 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") } @@ -10310,7 +10323,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) @@ -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") } @@ -10646,7 +10660,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) @@ -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(defaultBitcoinTimeLockDelta-claimDelta) - defaultCSV + numBlocks := uint32(20-defaultBroadcastDelta) - defaultCSV if _, err := net.Miner.Node.Generate(numBlocks); err != nil { t.Fatalf("unable to generate blocks") } diff --git a/peer.go b/peer.go index eb7cc4345..69b4cf082 100644 --- a/peer.go +++ b/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), @@ -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) diff --git a/server.go b/server.go index 02fd2e4d0..3bcc2e33f 100644 --- a/server.go +++ b/server.go @@ -727,9 +727,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, - // TODO(roasbeef): properly configure - // * needs to be << or specified final hop time delta + ChainHash: *activeNetParams.GenesisHash, 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 - // the set of currently active peers. + // 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. p, err := newPeer( conn, connReq, s, peerAddr, inbound, localFeatures, cfg.ChanEnableTimeout, + defaultBroadcastDelta+extraExpiryGraceDelta, ) if err != nil { srvrLog.Errorf("unable to create peer %v", err)