From ab4da0f53d8a4f2af60d5a7a23cfd40ca32dc590 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Wed, 3 Apr 2019 12:18:19 +0200 Subject: [PATCH 1/5] 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 | 6 ++- contractcourt/chain_arbitrator.go | 21 +++++++---- contractcourt/channel_arbitrator.go | 9 +++-- contractcourt/channel_arbitrator_test.go | 3 +- htlcswitch/link.go | 27 +++++++------- htlcswitch/test_utils.go | 20 +++++----- lnd_test.go | 25 +++++++++---- peer.go | 47 ++++++++++++------------ server.go | 17 +++++---- 9 files changed, 101 insertions(+), 74 deletions(-) diff --git a/config.go b/config.go index 167743ed..56f3689c 100644 --- a/config.go +++ b/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. diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index 297e6990..e21ba7ca 100644 --- a/contractcourt/chain_arbitrator.go +++ b/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 diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index e313ba83..87e6221e 100644 --- a/contractcourt/channel_arbitrator.go +++ b/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 diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index a072773a..758fd4cd 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/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), diff --git a/htlcswitch/link.go b/htlcswitch/link.go index a7ecbe90..93199cf7 100644 --- a/htlcswitch/link.go +++ b/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) diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go index 19a6d19c..960ac08e 100644 --- a/htlcswitch/test_utils.go +++ b/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( @@ -1041,15 +1040,16 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer, UpdateContractSignals: func(*contractcourt.ContractSignals) error { return nil }, - ChainEvents: &contractcourt.ChainEventSubscription{}, - SyncStates: true, - BatchSize: 10, - BatchTicker: ticker.NewForce(batchTimeout), - FwdPkgGCTicker: ticker.NewForce(fwdPkgTimeout), - MinFeeUpdateTimeout: minFeeUpdateTimeout, - MaxFeeUpdateTimeout: maxFeeUpdateTimeout, - OnChannelFailure: func(lnwire.ChannelID, lnwire.ShortChannelID, LinkFailureError) {}, - ExpiryGraceDelta: expiryGraceDelta, + ChainEvents: &contractcourt.ChainEventSubscription{}, + SyncStates: true, + BatchSize: 10, + BatchTicker: ticker.NewForce(batchTimeout), + FwdPkgGCTicker: ticker.NewForce(fwdPkgTimeout), + MinFeeUpdateTimeout: minFeeUpdateTimeout, + MaxFeeUpdateTimeout: maxFeeUpdateTimeout, + OnChannelFailure: func(lnwire.ChannelID, lnwire.ShortChannelID, LinkFailureError) {}, + FinalCltvRejectDelta: 3, + OutgoingCltvRejectDelta: 3, }, channel, ) diff --git a/lnd_test.go b/lnd_test.go index 487d02fc..55a3f792 100644 --- a/lnd_test.go +++ b/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") } diff --git a/peer.go b/peer.go index 97bf5541..e227d141 100644 --- a/peer.go +++ b/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 @@ -263,8 +259,10 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server, server: server, - localFeatures: localFeatures, - expiryGraceDelta: expiryGraceDelta, + localFeatures: localFeatures, + + finalCltvRejectDelta: finalCltvRejectDelta, + outgoingCltvRejectDelta: outgoingCltvRejectDelta, sendQueue: make(chan outgoingMsg), outgoingQueue: make(chan outgoingMsg), @@ -596,15 +594,16 @@ func (p *peer) addLink(chanPoint *wire.OutPoint, *chanPoint, signals, ) }, - OnChannelFailure: onChannelFailure, - SyncStates: syncStates, - BatchTicker: ticker.New(50 * time.Millisecond), - FwdPkgGCTicker: ticker.New(time.Minute), - BatchSize: 10, - UnsafeReplay: cfg.UnsafeReplay, - MinFeeUpdateTimeout: htlcswitch.DefaultMinLinkFeeUpdateTimeout, - MaxFeeUpdateTimeout: htlcswitch.DefaultMaxLinkFeeUpdateTimeout, - ExpiryGraceDelta: p.expiryGraceDelta, + OnChannelFailure: onChannelFailure, + SyncStates: syncStates, + BatchTicker: ticker.New(50 * time.Millisecond), + FwdPkgGCTicker: ticker.New(time.Minute), + BatchSize: 10, + UnsafeReplay: cfg.UnsafeReplay, + MinFeeUpdateTimeout: htlcswitch.DefaultMinLinkFeeUpdateTimeout, + MaxFeeUpdateTimeout: htlcswitch.DefaultMaxLinkFeeUpdateTimeout, + FinalCltvRejectDelta: p.finalCltvRejectDelta, + OutgoingCltvRejectDelta: p.outgoingCltvRejectDelta, } link := htlcswitch.NewChannelLink(linkCfg, lnChan) diff --git a/server.go b/server.go index 5a8f6d6a..955c782f 100644 --- a/server.go +++ b/server.go @@ -728,8 +728,9 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, cc *chainControl, contractBreaches := make(chan *ContractBreachEvent, 1) s.chainArb = contractcourt.NewChainArbitrator(contractcourt.ChainArbitratorConfig{ - ChainHash: *activeNetParams.GenesisHash, - BroadcastDelta: defaultBroadcastDelta, + ChainHash: *activeNetParams.GenesisHash, + 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) From 037913fd286b73e6d3c54c46ae1096135082b9bd Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 5 Apr 2019 10:50:00 +0200 Subject: [PATCH 2/5] link: rewrite height comparisons without subtraction Prevent the case where a uint32 wrap around could happen. --- htlcswitch/link.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 93199cf7..41725f16 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -2155,9 +2155,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-l.cfg.OutgoingCltvRejectDelta <= heightNow { + // future, so we'll reject an HTLC if the outgoing expiration time is + // too close to the current height. + if outgoingTimeout <= heightNow+l.cfg.OutgoingCltvRejectDelta { l.errorf("htlc(%x) has an expiry that's too soon: "+ "outgoing_expiry=%v, best_height=%v", payHash[:], outgoingTimeout, heightNow) @@ -2175,7 +2175,8 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte, return failure } - if outgoingTimeout-heightNow > maxCltvExpiry { + // Check absolute max delta. + if outgoingTimeout > maxCltvExpiry+heightNow { l.errorf("outgoing htlc(%x) has a time lock too far in the "+ "future: got %v, but maximum is %v", payHash[:], outgoingTimeout-heightNow, maxCltvExpiry) @@ -2188,7 +2189,7 @@ func (l *channelLink) HtlcSatifiesPolicy(payHash [32]byte, // 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 { + if incomingTimeout < outgoingTimeout+timeDelta { l.errorf("Incoming htlc(%x) has incorrect time-lock value: "+ "expected at least %v block delta, got %v block delta", payHash[:], timeDelta, incomingTimeout-outgoingTimeout) @@ -2680,7 +2681,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.FinalCltvRejectDelta <= heightNow { + if pd.Timeout <= heightNow+l.cfg.FinalCltvRejectDelta { log.Errorf("htlc(%x) has an expiry that's too soon: expiry=%v"+ ", best_height=%v", pd.RHash[:], pd.Timeout, heightNow) From af7d0e5ff5587812b779bf64ec8493213a65bd48 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 5 Apr 2019 10:29:56 +0200 Subject: [PATCH 3/5] htlcswitch/test: convert TestChannelLinkSingleHopPayment to two hop network --- htlcswitch/link_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 4d8143fa..950a5ddf 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -179,7 +179,8 @@ func createInterceptorFunc(prefix, receiver string, messages []expectedMessage, func TestChannelLinkSingleHopPayment(t *testing.T) { t.Parallel() - channels, cleanUp, _, err := createClusterChannels( + // Setup a alice-bob network. + aliceChannel, bobChannel, cleanUp, err := createTwoClusterChannels( btcutil.SatoshiPerBitcoin*3, btcutil.SatoshiPerBitcoin*5) if err != nil { @@ -187,15 +188,14 @@ func TestChannelLinkSingleHopPayment(t *testing.T) { } defer cleanUp() - n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice, - channels.bobToCarol, channels.carolToBob, testStartingHeight) + n := newTwoHopNetwork(t, aliceChannel, bobChannel, testStartingHeight) if err := n.start(); err != nil { t.Fatal(err) } defer n.stop() aliceBandwidthBefore := n.aliceChannelLink.Bandwidth() - bobBandwidthBefore := n.firstBobChannelLink.Bandwidth() + bobBandwidthBefore := n.bobChannelLink.Bandwidth() debug := false if debug { @@ -205,12 +205,12 @@ func TestChannelLinkSingleHopPayment(t *testing.T) { // Log message that bob receives. n.bobServer.intersect(createLogFunc("bob", - n.firstBobChannelLink.ChanID())) + n.bobChannelLink.ChanID())) } amount := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) htlcAmt, totalTimelock, hops := generateHops(amount, testStartingHeight, - n.firstBobChannelLink) + n.bobChannelLink) // Wait for: // * HTLC add request to be sent to bob. @@ -219,7 +219,7 @@ func TestChannelLinkSingleHopPayment(t *testing.T) { // * alice<->bob commitment state to be updated. // * user notification to be sent. receiver := n.bobServer - firstHop := n.firstBobChannelLink.ShortChanID() + firstHop := n.bobChannelLink.ShortChanID() rhash, err := makePayment( n.aliceServer, receiver, firstHop, hops, amount, htlcAmt, totalTimelock, @@ -248,10 +248,10 @@ func TestChannelLinkSingleHopPayment(t *testing.T) { "amount") } - if bobBandwidthBefore+amount != n.firstBobChannelLink.Bandwidth() { + if bobBandwidthBefore+amount != n.bobChannelLink.Bandwidth() { t.Fatalf("bob bandwidth isn't match: expected %v, got %v", bobBandwidthBefore+amount, - n.firstBobChannelLink.Bandwidth()) + n.bobChannelLink.Bandwidth()) } } From 1b2816006f8ed8477e92d14837c828b6a5fc5ada Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 5 Apr 2019 11:13:52 +0200 Subject: [PATCH 4/5] htlcswitch/test: align test invoice cltv expiry --- htlcswitch/mock.go | 4 +++- htlcswitch/test_utils.go | 10 +++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index a72dd897..a4d8f1bb 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -728,6 +728,8 @@ func newDB() (*channeldb.DB, func(), error) { return cdb, cleanUp, nil } +const testInvoiceCltvExpiry = 4 + type mockInvoiceRegistry struct { settleChan chan lntypes.Hash @@ -743,7 +745,7 @@ func newMockRegistry(minDelta uint32) *mockInvoiceRegistry { } decodeExpiry := func(invoice string) (uint32, error) { - return 3, nil + return testInvoiceCltvExpiry, nil } registry := invoices.NewRegistry(cdb, decodeExpiry) diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go index 960ac08e..89762d75 100644 --- a/htlcswitch/test_utils.go +++ b/htlcswitch/test_utils.go @@ -527,6 +527,12 @@ func generatePaymentWithPreimage(invoiceAmt, htlcAmt lnwire.MilliSatoshi, preimage, rhash [32]byte) (*channeldb.Invoice, *lnwire.UpdateAddHTLC, error) { + // Create the db invoice. Normally the payment requests needs to be set, + // because it is decoded in InvoiceRegistry to obtain the cltv expiry. + // But because the mock registry used in tests is mocking the decode + // step and always returning the value of testInvoiceCltvExpiry, we + // don't need to bother here with creating and signing a payment + // request. invoice := &channeldb.Invoice{ CreationDate: time.Now(), Terms: channeldb.ContractTerm{ @@ -602,8 +608,6 @@ type threeHopNetwork struct { func generateHops(payAmt lnwire.MilliSatoshi, startingHeight uint32, path ...*channelLink) (lnwire.MilliSatoshi, uint32, []ForwardingInfo) { - lastHop := path[len(path)-1] - totalTimelock := startingHeight runningAmt := payAmt @@ -620,7 +624,7 @@ func generateHops(payAmt lnwire.MilliSatoshi, startingHeight uint32, // If this is the last, hop, then the time lock will be their // specified delta policy plus our starting height. if i == len(path)-1 { - totalTimelock += lastHop.cfg.FwrdingPolicy.TimeLockDelta + totalTimelock += testInvoiceCltvExpiry timeLock = totalTimelock } else { // Otherwise, the outgoing time lock should be the From 206d93d8568f93c818577468019479156d41aaab Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 5 Apr 2019 11:33:48 +0200 Subject: [PATCH 5/5] htlcswitch/test: test zero value for outbound cltv reject delta --- htlcswitch/link_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 950a5ddf..3964efea 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -394,7 +394,29 @@ func TestChannelLinkBidirectionalOneHopPayments(t *testing.T) { // hops. In this test we send the payment from Carol to Alice over Bob peer. // (Carol -> Bob -> Alice) and checking that HTLC was settled properly and // balances were changed in two channels. +// +// The test is executed with two different OutgoingCltvRejectDelta values for +// bob. In addition to a normal positive value, we also test the zero case +// because this is currently the configured value in lnd +// (defaultOutgoingCltvRejectDelta). func TestChannelLinkMultiHopPayment(t *testing.T) { + t.Run( + "bobOutgoingCltvRejectDelta 3", + func(t *testing.T) { + testChannelLinkMultiHopPayment(t, 3) + }, + ) + t.Run( + "bobOutgoingCltvRejectDelta 0", + func(t *testing.T) { + testChannelLinkMultiHopPayment(t, 0) + }, + ) +} + +func testChannelLinkMultiHopPayment(t *testing.T, + bobOutgoingCltvRejectDelta uint32) { + t.Parallel() channels, cleanUp, _, err := createClusterChannels( @@ -407,6 +429,13 @@ func TestChannelLinkMultiHopPayment(t *testing.T) { n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice, channels.bobToCarol, channels.carolToBob, testStartingHeight) + + n.firstBobChannelLink.cfg.OutgoingCltvRejectDelta = + bobOutgoingCltvRejectDelta + + n.secondBobChannelLink.cfg.OutgoingCltvRejectDelta = + bobOutgoingCltvRejectDelta + if err := n.start(); err != nil { t.Fatal(err) }