From 5d2de91241bfce7330d5b3ac4038df955cbc1224 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 4 Apr 2019 14:48:55 +0200 Subject: [PATCH] config: update broadcast delta to reduce risk of channel force close --- config.go | 47 +++++++++++++++++++++++++++++++++++++++++++---- lnd_test.go | 17 +++++------------ 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/config.go b/config.go index 6e83a888..f0329435 100644 --- a/config.go +++ b/config.go @@ -68,11 +68,50 @@ const ( defaultTorV2PrivateKeyFilename = "v2_onion_private_key" defaultTorV3PrivateKeyFilename = "v3_onion_private_key" - defaultIncomingBroadcastDelta = 20 - defaultFinalCltvRejectDelta = 2 + // defaultIncomingBroadcastDelta defines the number of blocks before the + // expiry of an incoming htlc at which we force close the channel. We + // only go to chain if we also have the preimage to actually pull in the + // htlc. BOLT #2 suggests 7 blocks. We use a few more for extra safety. + // Within this window we need to get our sweep or 2nd level success tx + // confirmed, because after that the remote party is also able to claim + // the htlc using the timeout path. + defaultIncomingBroadcastDelta = 10 - defaultOutgoingBroadcastDelta = 10 - defaultOutgoingCltvRejectDelta = 0 + // defaultFinalCltvRejectDelta defines the number of blocks before the + // expiry of an incoming exit hop htlc at which we cancel it back + // immediately. It is an extra safety measure over the final cltv + // requirement as it is defined in the invoice. It ensures that we + // cancel back htlcs that, when held on to, may cause us to force close + // the channel because we enter the incoming broadcast window. Bolt #11 + // suggests 9 blocks here. We use a few more for additional safety. + // + // There is still a small gap that remains between receiving the + // RevokeAndAck and canceling back. If a new block arrives within that + // window, we may still force close the channel. There is currently no + // way to reject an UpdateAddHtlc of which we already know that it will + // push us in the broadcast window. + defaultFinalCltvRejectDelta = defaultIncomingBroadcastDelta + 3 + + // defaultOutgoingBroadcastDelta defines the number of blocks before the + // expiry of an outgoing htlc at which we force close the channel. We + // are not in a hurry to force close, because there is nothing to claim + // for us. We do need to time the htlc out, because there may be an + // incoming htlc that will time out too (albeit later). Bolt #2 suggests + // a value of -1 here, but we allow one block less to prevent potential + // confusion around the negative value. It means we force close the + // channel at exactly the htlc expiry height. + defaultOutgoingBroadcastDelta = 0 + + // defaultOutgoingCltvRejectDelta defines the number of blocks before + // the expiry of an outgoing htlc at which we don't want to offer it to + // the next peer anymore. If that happens, we cancel back the incoming + // htlc. This is to prevent the situation where we have an outstanding + // htlc that brings or will soon bring us inside the outgoing broadcast + // window and trigger us to force close the channel. Bolt #2 suggests a + // value of 0. We pad it a bit, to prevent a slow round trip to the next + // peer and a block arriving during that round trip to trigger force + // closure. + defaultOutgoingCltvRejectDelta = defaultOutgoingBroadcastDelta + 3 // minTimeLockDelta is the minimum timelock we require for incoming // HTLCs on our channels. diff --git a/lnd_test.go b/lnd_test.go index 51fe50ed..bf49d3ef 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -9466,7 +9466,8 @@ 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. + // timeout. With the default outgoing broadcast delta of zero, this will + // be the same height as the htlc expiry height. numBlocks := uint32(finalCltvDelta - defaultOutgoingBroadcastDelta) if _, err := net.Miner.Node.Generate(numBlocks); err != nil { t.Fatalf("unable to generate blocks: %v", err) @@ -9503,8 +9504,9 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf("htlc mismatch: %v", predErr) } - // We'll mine defaultCSV blocks in order to generate the sweep transaction - // of Bob's funding output. + // We'll mine defaultCSV blocks in order to generate the sweep + // transaction of Bob's funding output. This will also bring us to the + // maturity height of the htlc tx output. if _, err := net.Miner.Node.Generate(defaultCSV); err != nil { t.Fatalf("unable to generate blocks: %v", err) } @@ -9514,15 +9516,6 @@ func testMultiHopHtlcLocalTimeout(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf("unable to find bob's funding output sweep tx: %v", err) } - // We'll now mine the remaining blocks to cause the HTLC itself to - // timeout. - _, err = net.Miner.Node.Generate( - defaultOutgoingBroadcastDelta - defaultCSV, - ) - if err != nil { - t.Fatalf("unable to generate blocks: %v", err) - } - // The second layer HTLC timeout transaction should now have been // broadcast on-chain. secondLayerHash, err := waitForTxInMempool(net.Miner.Node, minerMempoolTimeout)