From d1076271456bdab1625ea6b52b93ca3e1bd9aed9 Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Sat, 3 Nov 2018 22:48:49 -0700 Subject: [PATCH 1/3] contractcourt: remove broadcastRedeemMultiplier The multiplier doesn't make sense because funds may be equally at risk by failing to broadcast to chain regardless of whether the HTLC is a redeem or a timeout. --- contractcourt/channel_arbitrator.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index aae25746..e313ba83 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, ) } From e6fbbaa1dc247c65a8fbee7a03d2fe2b324706da Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 26 Mar 2019 11:54:08 +0100 Subject: [PATCH 2/3] link: also check expiry grace delta for forwarded htlcs Previously there was no minimum remaining blocks requirement on forwarded htlcs, which may cause channel arbitrator to force close the channel directly after forwarding the htlc. Co-authored-by: Jim Posen --- htlcswitch/link.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 901ce171..2cdba393 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -30,14 +30,16 @@ 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. + // 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. // - // TODO(roasbeef): must be < default delta + // 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 @@ -2147,14 +2149,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-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( @@ -2181,6 +2182,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", From cd535b9401fd90bb66201d52a3ad7f823caa6f2d Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 26 Mar 2019 12:05:43 +0100 Subject: [PATCH 3/3] 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 --- htlcswitch/link.go | 32 ++++++++++++++++---------------- htlcswitch/link_test.go | 20 +++++++++++++------- htlcswitch/test_utils.go | 2 ++ lnd_test.go | 28 +++++++++++++++++++++------- peer.go | 20 ++++++++++++++++++-- server.go | 12 +++++++----- 6 files changed, 77 insertions(+), 37 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 2cdba393..a29b4fd6 100644 --- a/htlcswitch/link.go +++ b/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 - // too close to the current height. - if outgoingTimeout-expiryGraceDelta <= heightNow { + // 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[:], 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) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 8d41cdd5..4d8143fa 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 053c99cf..19a6d19c 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 6d9a2114..be04bf8a 100644 --- a/lnd_test.go +++ b/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(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 f1fe9490..16c1878c 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), @@ -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) diff --git a/server.go b/server.go index d86f340c..b4f7d765 100644 --- a/server.go +++ b/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, - // 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) @@ -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 - // 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)