From 34de5922edaa6eb37137c143b2612a57ad7e985c Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 11 May 2021 08:45:29 +0200 Subject: [PATCH 1/5] multi: add height-based invoice expiry This commit adds height-based invoice expiry for hodl invoices that have active htlcs. This allows us to cancel our intentionally held htlcs before channels are force closed. We only add this for hodl invoices because we expect regular invoices to automatically be resolved. We still keep hodl invoices in the time-based expiry queue, because we want to expire open invoices that reach their timeout before any htlcs are added. Since htlcs are added after the invoice is created, we add new htlcs as they arrive in the invoice registry. In this commit, we allow adding of duplicate entries for an invoice to be added to the expiry queue as each htlc arrives to keep implementation simple. Our cancellation logic can already handle the case where an entry is already canceled, so this is ok. --- config.go | 17 ++ htlcswitch/mock.go | 19 ++- invoices/invoice_expiry_watcher.go | 209 ++++++++++++++++++++++-- invoices/invoice_expiry_watcher_test.go | 35 +++- invoices/invoiceregistry.go | 10 ++ invoices/invoiceregistry_test.go | 19 ++- invoices/test_utils_test.go | 4 +- lncfg/invoices.go | 12 ++ sample-lnd.conf | 20 +++ server.go | 15 +- 10 files changed, 328 insertions(+), 32 deletions(-) create mode 100644 lncfg/invoices.go diff --git a/config.go b/config.go index 0a23839c..2bfef99f 100644 --- a/config.go +++ b/config.go @@ -344,6 +344,8 @@ type Config struct { GcCanceledInvoicesOnTheFly bool `long:"gc-canceled-invoices-on-the-fly" description:"If true, we'll delete newly canceled invoices on the fly."` + Invoices *lncfg.Invoices `group:"invoices" namespace:"invoices"` + Routing *lncfg.Routing `group:"routing" namespace:"routing"` Gossip *lncfg.Gossip `group:"gossip" namespace:"gossip"` @@ -529,6 +531,9 @@ func DefaultConfig() Config { MaxChannelUpdateBurst: discovery.DefaultMaxChannelUpdateBurst, ChannelUpdateInterval: discovery.DefaultChannelUpdateInterval, }, + Invoices: &lncfg.Invoices{ + HoldExpiryDelta: lncfg.DefaultHoldInvoiceExpiryDelta, + }, MaxOutgoingCltvExpiry: htlcswitch.DefaultMaxOutgoingCltvExpiry, MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation, MaxCommitFeeRateAnchors: lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte, @@ -1369,6 +1374,18 @@ func ValidateConfig(cfg Config, usageMessage string, return nil, err } + // Log a warning if our expiry delta is not greater than our incoming + // broadcast delta. We do not fail here because this value may be set + // to zero to intentionally keep lnd's behavior unchanged from when we + // didn't auto-cancel these invoices. + if cfg.Invoices.HoldExpiryDelta <= lncfg.DefaultIncomingBroadcastDelta { + ltndLog.Warnf("Invoice hold expiry delta: %v <= incoming "+ + "delta: %v, accepted hold invoices will force close "+ + "channels if they are not canceled manually", + cfg.Invoices.HoldExpiryDelta, + lncfg.DefaultIncomingBroadcastDelta) + } + // Validate the subconfigs for workers, caches, and the tower client. err = lncfg.Validate( cfg.Workers, diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index d3e9167e..13872a45 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -797,6 +797,20 @@ type mockInvoiceRegistry struct { cleanup func() } +type mockChainNotifier struct { + chainntnfs.ChainNotifier +} + +// RegisterBlockEpochNtfn mocks a successful call to register block +// notifications. +func (m *mockChainNotifier) RegisterBlockEpochNtfn(*chainntnfs.BlockEpoch) ( + *chainntnfs.BlockEpochEvent, error) { + + return &chainntnfs.BlockEpochEvent{ + Cancel: func() {}, + }, nil +} + func newMockRegistry(minDelta uint32) *mockInvoiceRegistry { cdb, cleanup, err := newDB() if err != nil { @@ -805,7 +819,10 @@ func newMockRegistry(minDelta uint32) *mockInvoiceRegistry { registry := invoices.NewRegistry( cdb, - invoices.NewInvoiceExpiryWatcher(clock.NewDefaultClock()), + invoices.NewInvoiceExpiryWatcher( + clock.NewDefaultClock(), 0, 0, nil, + &mockChainNotifier{}, + ), &invoices.RegistryConfig{ FinalCltvRejectDelta: 5, }, diff --git a/invoices/invoice_expiry_watcher.go b/invoices/invoice_expiry_watcher.go index 14257581..70d73608 100644 --- a/invoices/invoice_expiry_watcher.go +++ b/invoices/invoice_expiry_watcher.go @@ -5,6 +5,8 @@ import ( "sync" "time" + "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/clock" "github.com/lightningnetwork/lnd/lntypes" @@ -34,6 +36,28 @@ func (e invoiceExpiryTs) Less(other queue.PriorityQueueItem) bool { return e.Expiry.Before(other.(*invoiceExpiryTs).Expiry) } +// Compile time assertion that invoiceExpiryHeight implements invoiceExpiry. +var _ invoiceExpiry = (*invoiceExpiryHeight)(nil) + +// invoiceExpiryHeight holds information about an invoice which can be used to +// cancel it based on its expiry height. +type invoiceExpiryHeight struct { + paymentHash lntypes.Hash + expiryHeight uint32 +} + +// Less implements PriorityQueueItem.Less such that the top item in the +// priority queue is the lowest block height. +func (b invoiceExpiryHeight) Less(other queue.PriorityQueueItem) bool { + return b.expiryHeight < other.(*invoiceExpiryHeight).expiryHeight +} + +// expired returns a boolean that indicates whether this entry has expired, +// taking our expiry delta into account. +func (b invoiceExpiryHeight) expired(currentHeight, delta uint32) bool { + return currentHeight+delta >= b.expiryHeight +} + // InvoiceExpiryWatcher handles automatic invoice cancellation of expried // invoices. Upon start InvoiceExpiryWatcher will retrieve all pending (not yet // settled or canceled) invoices invoices to its watcing queue. When a new @@ -49,6 +73,21 @@ type InvoiceExpiryWatcher struct { // It is useful for testing. clock clock.Clock + // notifier provides us with block height updates. + notifier chainntnfs.ChainNotifier + + // blockExpiryDelta is the number of blocks before a htlc's expiry that + // we expire the invoice based on expiry height. We use a delta because + // we will go to some delta before our expiry, so we want to cancel + // before this to prevent force closes. + blockExpiryDelta uint32 + + // currentHeight is the current block height. + currentHeight uint32 + + // currentHash is the block hash for our current height. + currentHash *chainhash.Hash + // cancelInvoice is a template method that cancels an expired invoice. cancelInvoice func(lntypes.Hash, bool) error @@ -56,6 +95,15 @@ type InvoiceExpiryWatcher struct { // the next invoice to expire. timestampExpiryQueue queue.PriorityQueue + // blockExpiryQueue holds blockExpiry items and is used to find the + // next invoice to expire based on block height. Only hold invoices + // with active htlcs are added to this queue, because they require + // manual cancellation when the hltc is going to time out. Items in + // this queue may already be in the timestampExpiryQueue, this is ok + // because they will not be expired based on timestamp if they have + // active htlcs. + blockExpiryQueue queue.PriorityQueue + // newInvoices channel is used to wake up the main loop when a new // invoices is added. newInvoices chan []invoiceExpiry @@ -67,11 +115,18 @@ type InvoiceExpiryWatcher struct { } // NewInvoiceExpiryWatcher creates a new InvoiceExpiryWatcher instance. -func NewInvoiceExpiryWatcher(clock clock.Clock) *InvoiceExpiryWatcher { +func NewInvoiceExpiryWatcher(clock clock.Clock, + expiryDelta, startHeight uint32, startHash *chainhash.Hash, + notifier chainntnfs.ChainNotifier) *InvoiceExpiryWatcher { + return &InvoiceExpiryWatcher{ - clock: clock, - newInvoices: make(chan []invoiceExpiry), - quit: make(chan struct{}), + clock: clock, + notifier: notifier, + blockExpiryDelta: expiryDelta, + currentHeight: startHeight, + currentHash: startHash, + newInvoices: make(chan []invoiceExpiry), + quit: make(chan struct{}), } } @@ -91,8 +146,17 @@ func (ew *InvoiceExpiryWatcher) Start( ew.started = true ew.cancelInvoice = cancelInvoice + + ntfn, err := ew.notifier.RegisterBlockEpochNtfn(&chainntnfs.BlockEpoch{ + Height: int32(ew.currentHeight), + Hash: ew.currentHash, + }) + if err != nil { + return err + } + ew.wg.Add(1) - go ew.mainLoop() + go ew.mainLoop(ntfn) return nil } @@ -122,6 +186,32 @@ func makeInvoiceExpiry(paymentHash lntypes.Hash, case channeldb.ContractOpen: return makeTimestampExpiry(paymentHash, invoice) + // If an invoice has active htlcs, we want to expire it based on block + // height. We only do this for hodl invoices, since regular invoices + // should resolve themselves automatically. + case channeldb.ContractAccepted: + if !invoice.HodlInvoice { + log.Debugf("Invoice in accepted state not added to "+ + "expiry watcher: %v", paymentHash) + + return nil + } + + var minHeight uint32 + for _, htlc := range invoice.Htlcs { + // We only care about accepted htlcs, since they will + // trigger force-closes. + if htlc.State != channeldb.HtlcStateAccepted { + continue + } + + if minHeight == 0 || htlc.Expiry < minHeight { + minHeight = htlc.Expiry + } + } + + return makeHeightExpiry(paymentHash, minHeight) + default: log.Debugf("Invoice not added to expiry watcher: %v", paymentHash) @@ -151,18 +241,36 @@ func makeTimestampExpiry(paymentHash lntypes.Hash, } } +// makeHeightExpiry creates height-based expiry for an invoice based on its +// lowest htlc expiry height. +func makeHeightExpiry(paymentHash lntypes.Hash, + minHeight uint32) *invoiceExpiryHeight { + + if minHeight == 0 { + log.Warnf("make height expiry called with 0 height") + return nil + } + + return &invoiceExpiryHeight{ + paymentHash: paymentHash, + expiryHeight: minHeight, + } +} + // AddInvoices adds invoices to the InvoiceExpiryWatcher. func (ew *InvoiceExpiryWatcher) AddInvoices(invoices ...invoiceExpiry) { - if len(invoices) > 0 { - select { - case ew.newInvoices <- invoices: - log.Debugf("Added %d invoices to the expiry watcher", - len(invoices)) + if len(invoices) == 0 { + return + } - // Select on quit too so that callers won't get blocked in case - // of concurrent shutdown. - case <-ew.quit: - } + select { + case ew.newInvoices <- invoices: + log.Debugf("Added %d invoices to the expiry watcher", + len(invoices)) + + // Select on quit too so that callers won't get blocked in case + // of concurrent shutdown. + case <-ew.quit: } } @@ -178,6 +286,23 @@ func (ew *InvoiceExpiryWatcher) nextTimestampExpiry() <-chan time.Time { return nil } +// nextHeightExpiry returns a channel that will immediately be read from if +// the top item on our queue has expired. +func (ew *InvoiceExpiryWatcher) nextHeightExpiry() <-chan uint32 { + if ew.blockExpiryQueue.Empty() { + return nil + } + + top := ew.blockExpiryQueue.Top().(*invoiceExpiryHeight) + if !top.expired(ew.currentHeight, ew.blockExpiryDelta) { + return nil + } + + blockChan := make(chan uint32, 1) + blockChan <- top.expiryHeight + return blockChan +} + // cancelNextExpiredInvoice will cancel the next expired invoice and removes // it from the expiry queue. func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() { @@ -198,6 +323,25 @@ func (ew *InvoiceExpiryWatcher) cancelNextExpiredInvoice() { } } +// cancelNextHeightExpiredInvoice looks at our height based queue and expires +// the next invoice if we have reached its expiry block. +func (ew *InvoiceExpiryWatcher) cancelNextHeightExpiredInvoice() { + if ew.blockExpiryQueue.Empty() { + return + } + + top := ew.blockExpiryQueue.Top().(*invoiceExpiryHeight) + if !top.expired(ew.currentHeight, ew.blockExpiryDelta) { + return + } + + // We always force-cancel block-based expiry so that we can + // cancel invoices that have been accepted but not yet resolved. + // This helps us avoid force closes. + ew.expireInvoice(top.paymentHash, true) + ew.blockExpiryQueue.Pop() +} + // expireInvoice attempts to expire an invoice and logs an error if we get an // unexpected error. func (ew *InvoiceExpiryWatcher) expireInvoice(hash lntypes.Hash, force bool) { @@ -226,6 +370,11 @@ func (ew *InvoiceExpiryWatcher) pushInvoices(invoices []invoiceExpiry) { ew.timestampExpiryQueue.Push(expiry) } + case *invoiceExpiryHeight: + if expiry != nil { + ew.blockExpiryQueue.Push(expiry) + } + default: log.Errorf("unexpected queue item: %T", inv) } @@ -234,12 +383,20 @@ func (ew *InvoiceExpiryWatcher) pushInvoices(invoices []invoiceExpiry) { // mainLoop is a goroutine that receives new invoices and handles cancellation // of expired invoices. -func (ew *InvoiceExpiryWatcher) mainLoop() { - defer ew.wg.Done() +func (ew *InvoiceExpiryWatcher) mainLoop(blockNtfns *chainntnfs.BlockEpochEvent) { + defer func() { + blockNtfns.Cancel() + ew.wg.Done() + }() + + // We have two different queues, so we use a different cancel method + // depending on which expiry condition we have hit. Starting with time + // based expiry is an arbitrary choice to start off. + cancelNext := ew.cancelNextExpiredInvoice for { // Cancel any invoices that may have expired. - ew.cancelNextExpiredInvoice() + cancelNext() select { @@ -252,13 +409,29 @@ func (ew *InvoiceExpiryWatcher) mainLoop() { default: select { + // Wait until the next invoice expires. case <-ew.nextTimestampExpiry(): - // Wait until the next invoice expires. + cancelNext = ew.cancelNextExpiredInvoice + continue + + case <-ew.nextHeightExpiry(): + cancelNext = ew.cancelNextHeightExpiredInvoice continue case newInvoices := <-ew.newInvoices: ew.pushInvoices(newInvoices) + // Consume new blocks. + case block, ok := <-blockNtfns.Epochs: + if !ok { + log.Debugf("block notifications " + + "canceled") + return + } + + ew.currentHeight = uint32(block.Height) + ew.currentHash = block.Hash + case <-ew.quit: return } diff --git a/invoices/invoice_expiry_watcher_test.go b/invoices/invoice_expiry_watcher_test.go index e2c7ea82..99a99dfc 100644 --- a/invoices/invoice_expiry_watcher_test.go +++ b/invoices/invoice_expiry_watcher_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/clock" "github.com/lightningnetwork/lnd/lntypes" ) @@ -19,13 +20,40 @@ type invoiceExpiryWatcherTest struct { canceledInvoices []lntypes.Hash } +type mockChainNotifier struct { + chainntnfs.ChainNotifier + + blockChan chan *chainntnfs.BlockEpoch +} + +func newMockNotifier() *mockChainNotifier { + return &mockChainNotifier{ + blockChan: make(chan *chainntnfs.BlockEpoch), + } +} + +// RegisterBlockEpochNtfn mocks a block epoch notification, using the mock's +// block channel to deliver blocks to the client. +func (m *mockChainNotifier) RegisterBlockEpochNtfn(*chainntnfs.BlockEpoch) ( + *chainntnfs.BlockEpochEvent, error) { + + return &chainntnfs.BlockEpochEvent{ + Epochs: m.blockChan, + Cancel: func() {}, + }, nil +} + // newInvoiceExpiryWatcherTest creates a new InvoiceExpiryWatcher test fixture // and sets up the test environment. func newInvoiceExpiryWatcherTest(t *testing.T, now time.Time, numExpiredInvoices, numPendingInvoices int) *invoiceExpiryWatcherTest { + mockNotifier := newMockNotifier() test := &invoiceExpiryWatcherTest{ - watcher: NewInvoiceExpiryWatcher(clock.NewTestClock(testTime)), + watcher: NewInvoiceExpiryWatcher( + clock.NewTestClock(testTime), 0, + uint32(testCurrentHeight), nil, mockNotifier, + ), testData: generateInvoiceExpiryTestData( t, now, 0, numExpiredInvoices, numPendingInvoices, ), @@ -84,7 +112,10 @@ func (t *invoiceExpiryWatcherTest) checkExpectations() { // Tests that InvoiceExpiryWatcher can be started and stopped. func TestInvoiceExpiryWatcherStartStop(t *testing.T) { - watcher := NewInvoiceExpiryWatcher(clock.NewTestClock(testTime)) + watcher := NewInvoiceExpiryWatcher( + clock.NewTestClock(testTime), 0, uint32(testCurrentHeight), nil, + newMockNotifier(), + ) cancel := func(lntypes.Hash, bool) error { t.Fatalf("unexpected call") return nil diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index c79e3464..9f6c2ca7 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -1101,6 +1101,16 @@ func (i *InvoiceRegistry) notifyExitHopHtlcLocked( } + // If we have fully accepted the set of htlcs for this invoice, + // we can now add it to our invoice expiry watcher. We do not + // add invoices before they are fully accepted, because it is + // possible that we MppTimeout the htlcs, and then our relevant + // expiry height could change. + if res.outcome == resultAccepted { + expiry := makeInvoiceExpiry(ctx.hash, invoice) + i.expiryWatcher.AddInvoices(expiry) + } + i.hodlSubscribe(hodlChan, ctx.circuitKey) default: diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 799d5395..0e0f3fcb 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -352,7 +352,11 @@ func TestSettleHoldInvoice(t *testing.T) { FinalCltvRejectDelta: testFinalCltvRejectDelta, Clock: clock.NewTestClock(testTime), } - registry := NewRegistry(cdb, NewInvoiceExpiryWatcher(cfg.Clock), &cfg) + + expiryWatcher := NewInvoiceExpiryWatcher( + cfg.Clock, 0, uint32(testCurrentHeight), nil, newMockNotifier(), + ) + registry := NewRegistry(cdb, expiryWatcher, &cfg) err = registry.Start() if err != nil { @@ -521,7 +525,10 @@ func TestCancelHoldInvoice(t *testing.T) { FinalCltvRejectDelta: testFinalCltvRejectDelta, Clock: clock.NewTestClock(testTime), } - registry := NewRegistry(cdb, NewInvoiceExpiryWatcher(cfg.Clock), &cfg) + expiryWatcher := NewInvoiceExpiryWatcher( + cfg.Clock, 0, uint32(testCurrentHeight), nil, newMockNotifier(), + ) + registry := NewRegistry(cdb, expiryWatcher, &cfg) err = registry.Start() if err != nil { @@ -946,7 +953,9 @@ func TestInvoiceExpiryWithRegistry(t *testing.T) { Clock: testClock, } - expiryWatcher := NewInvoiceExpiryWatcher(cfg.Clock) + expiryWatcher := NewInvoiceExpiryWatcher( + cfg.Clock, 0, uint32(testCurrentHeight), nil, newMockNotifier(), + ) registry := NewRegistry(cdb, expiryWatcher, &cfg) // First prefill the Channel DB with some pre-existing invoices, @@ -1049,7 +1058,9 @@ func TestOldInvoiceRemovalOnStart(t *testing.T) { GcCanceledInvoicesOnStartup: true, } - expiryWatcher := NewInvoiceExpiryWatcher(cfg.Clock) + expiryWatcher := NewInvoiceExpiryWatcher( + cfg.Clock, 0, uint32(testCurrentHeight), nil, newMockNotifier(), + ) registry := NewRegistry(cdb, expiryWatcher, &cfg) // First prefill the Channel DB with some pre-existing expired invoices. diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index 3e49a957..b78c06aa 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -193,7 +193,9 @@ func newTestContext(t *testing.T) *testContext { t.Fatal(err) } - expiryWatcher := NewInvoiceExpiryWatcher(clock) + expiryWatcher := NewInvoiceExpiryWatcher( + clock, 0, uint32(testCurrentHeight), nil, newMockNotifier(), + ) // Instantiate and start the invoice ctx.registry. cfg := RegistryConfig{ diff --git a/lncfg/invoices.go b/lncfg/invoices.go new file mode 100644 index 00000000..16a52d88 --- /dev/null +++ b/lncfg/invoices.go @@ -0,0 +1,12 @@ +package lncfg + +// DefaultHoldInvoiceExpiryDelta defines the number of blocks before the expiry +// height of a hold invoice's htlc that lnd will automatically cancel the +// invoice to prevent the channel from force closing. This value *must* be +// greater than DefaultIncomingBroadcastDelta to prevent force closes. +const DefaultHoldInvoiceExpiryDelta = DefaultIncomingBroadcastDelta + 2 + +// Invoices holds the configuration options for invoices. +type Invoices struct { + HoldExpiryDelta uint32 `long:"holdexpirydelta" description:"The number of blocks before a hold invoice's htlc expires that the invoice should be canceled to prevent a force close. Force closes will not be prevented if this value is not greater than DefaultIncomingBroadcastDelta."` +} diff --git a/sample-lnd.conf b/sample-lnd.conf index 2d63c619..24a0f4fe 100644 --- a/sample-lnd.conf +++ b/sample-lnd.conf @@ -1120,3 +1120,23 @@ litecoin.node=ltcd ; will accept over the channel update interval. ; gossip.max-channel-update-burst=10 ; gossip.channel-update-interval=1m + +[invoices] +; If a hold invoice has accepted htlcs that reach their expiry height and are +; not timed out, the channel holding the htlc is force closed to resolve the +; invoice's htlcs. To prevent force closes, lnd automatically cancels these +; invoices before they reach their expiry height. +; +; Hold expiry delta describes the number of blocks before expiry that these +; invoices should be canceled. Setting this value to 0 will ensure that hold +; invoices can be settled right up until their expiry height, but will result +; in the channel they are on being force closed if they are not resolved before +; expiry. +; +; Lnd goes to chain before the expiry for a htlc is reached so that there is +; time to resolve it on chain. This value needs to be greater than the +; DefaultIncomingBroadcastDelta set by lnd, otherwise the channel will be force +; closed anyway. A warning will be logged on startup if this value is not large +; enough to prevent force closes. +; +; invoices.holdexpirydelta=15 diff --git a/server.go b/server.go index eddec220..72629791 100644 --- a/server.go +++ b/server.go @@ -442,11 +442,6 @@ func newServer(cfg *Config, listenAddrs []net.Addr, readPool: readPool, chansToRestore: chansToRestore, - invoices: invoices.NewRegistry( - remoteChanDB, invoices.NewInvoiceExpiryWatcher(clock.NewDefaultClock()), - ®istryConfig, - ), - channelNotifier: channelnotifier.New(remoteChanDB), identityECDH: nodeKeyECDH, @@ -483,11 +478,19 @@ func newServer(cfg *Config, listenAddrs []net.Addr, subscribers: make(map[uint64]*preimageSubscriber), } - _, currentHeight, err := s.cc.ChainIO.GetBestBlock() + currentHash, currentHeight, err := s.cc.ChainIO.GetBestBlock() if err != nil { return nil, err } + expiryWatcher := invoices.NewInvoiceExpiryWatcher( + clock.NewDefaultClock(), cfg.Invoices.HoldExpiryDelta, + uint32(currentHeight), currentHash, cc.ChainNotifier, + ) + s.invoices = invoices.NewRegistry( + remoteChanDB, expiryWatcher, ®istryConfig, + ) + s.htlcNotifier = htlcswitch.NewHtlcNotifier(time.Now) s.htlcSwitch, err = htlcswitch.New(htlcswitch.Config{ From 85e56dbfb736afeb4f4d6651fbdb74ba0e5a531e Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 11 May 2021 08:45:30 +0200 Subject: [PATCH 2/5] invoices/test: add test for hodl invoices --- invoices/invoice_expiry_watcher_test.go | 113 ++++++++++++++++++++++++ invoices/test_utils_test.go | 110 +++++++++++++++++++++++ 2 files changed, 223 insertions(+) diff --git a/invoices/invoice_expiry_watcher_test.go b/invoices/invoice_expiry_watcher_test.go index 99a99dfc..63ddfb92 100644 --- a/invoices/invoice_expiry_watcher_test.go +++ b/invoices/invoice_expiry_watcher_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/lightningnetwork/lnd/chainntnfs" + "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/clock" "github.com/lightningnetwork/lnd/lntypes" ) @@ -203,3 +204,115 @@ func TestInvoiceExpiryWhenAddingMultipleInvoices(t *testing.T) { test.watcher.Stop() test.checkExpectations() } + +// TestExpiredHodlInv tests expiration of an already-expired hodl invoice +// which has no htlcs. +func TestExpiredHodlInv(t *testing.T) { + t.Parallel() + + creationDate := testTime.Add(time.Hour * -24) + expiry := time.Hour + + test := setupHodlExpiry( + t, creationDate, expiry, 0, channeldb.ContractOpen, nil, + ) + + test.assertCanceled(t, test.hash) + test.watcher.Stop() +} + +// TestAcceptedHodlNotExpired tests that hodl invoices which are in an accepted +// state are not expired once their time-based expiry elapses, using a regular +// invoice that expires at the same time as a control to ensure that invoices +// with that timestamp would otherwise be expired. +func TestAcceptedHodlNotExpired(t *testing.T) { + t.Parallel() + + creationDate := testTime + expiry := time.Hour + + test := setupHodlExpiry( + t, creationDate, expiry, 0, channeldb.ContractAccepted, nil, + ) + defer test.watcher.Stop() + + // Add another invoice that will expire at our expiry time as a control + // value. + tsExpires := &invoiceExpiryTs{ + PaymentHash: lntypes.Hash{1, 2, 3}, + Expiry: creationDate.Add(expiry), + Keysend: true, + } + test.watcher.AddInvoices(tsExpires) + + test.mockClock.SetTime(creationDate.Add(expiry + 1)) + + // Assert that only the ts expiry invoice is expired. + test.assertCanceled(t, tsExpires.PaymentHash) +} + +// TestHeightAlreadyExpired tests the case where we add an invoice with htlcs +// that have already expired to the expiry watcher. +func TestHeightAlreadyExpired(t *testing.T) { + t.Parallel() + + expiredHtlc := []*channeldb.InvoiceHTLC{ + { + State: channeldb.HtlcStateAccepted, + Expiry: uint32(testCurrentHeight), + }, + } + + test := setupHodlExpiry( + t, testTime, time.Hour, 0, channeldb.ContractAccepted, + expiredHtlc, + ) + defer test.watcher.Stop() + + test.assertCanceled(t, test.hash) +} + +// TestExpiryHeightArrives tests the case where we add a hodl invoice to the +// expiry watcher when it has no htlcs, htlcs are added and then they finally +// expire. We use a non-zero delta for this test to check that we expire with +// sufficient buffer. +func TestExpiryHeightArrives(t *testing.T) { + var ( + creationDate = testTime + expiry = time.Hour * 2 + delta uint32 = 1 + ) + + // Start out with a hodl invoice that is open, and has no htlcs. + test := setupHodlExpiry( + t, creationDate, expiry, delta, channeldb.ContractOpen, nil, + ) + defer test.watcher.Stop() + + htlc1 := uint32(testCurrentHeight + 10) + expiry1 := makeHeightExpiry(test.hash, htlc1) + + // Add htlcs to our invoice and progress its state to accepted. + test.watcher.AddInvoices(expiry1) + test.setState(channeldb.ContractAccepted) + + // Progress time so that our expiry has elapsed. We no longer expect + // this invoice to be canceled because it has been accepted. + test.mockClock.SetTime(creationDate.Add(expiry)) + + // Tick our mock block subscription with the next block, we don't + // expect anything to happen. + currentHeight := uint32(testCurrentHeight + 1) + test.announceBlock(t, currentHeight) + + // Now, we add another htlc to the invoice. This one has a lower expiry + // height than our current ones. + htlc2 := currentHeight + 5 + expiry2 := makeHeightExpiry(test.hash, htlc2) + test.watcher.AddInvoices(expiry2) + + // Announce our lowest htlc expiry block minus our delta, the invoice + // should be expired now. + test.announceBlock(t, htlc2-delta) + test.assertCanceled(t, test.hash) +} diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index b78c06aa..51f41fc9 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -8,11 +8,13 @@ import ( "io/ioutil" "os" "runtime/pprof" + "sync" "testing" "time" "github.com/btcsuite/btcd/btcec" "github.com/btcsuite/btcd/chaincfg" + "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/clock" "github.com/lightningnetwork/lnd/lntypes" @@ -367,3 +369,111 @@ func checkFailResolution(t *testing.T, res HtlcResolution, return failResolution } + +type hodlExpiryTest struct { + hash lntypes.Hash + state channeldb.ContractState + stateLock sync.Mutex + mockNotifier *mockChainNotifier + mockClock *clock.TestClock + cancelChan chan lntypes.Hash + watcher *InvoiceExpiryWatcher +} + +func (h *hodlExpiryTest) setState(state channeldb.ContractState) { + h.stateLock.Lock() + defer h.stateLock.Unlock() + + h.state = state +} + +func (h *hodlExpiryTest) announceBlock(t *testing.T, height uint32) { + select { + case h.mockNotifier.blockChan <- &chainntnfs.BlockEpoch{ + Height: int32(height), + }: + + case <-time.After(testTimeout): + t.Fatalf("block %v not consumed", height) + } +} + +func (h *hodlExpiryTest) assertCanceled(t *testing.T, expected lntypes.Hash) { + select { + case actual := <-h.cancelChan: + require.Equal(t, expected, actual) + + case <-time.After(testTimeout): + t.Fatalf("invoice: %v not canceled", h.hash) + } +} + +// setupHodlExpiry creates a hodl invoice in our expiry watcher and runs an +// arbitrary update function which advances the invoices's state. +func setupHodlExpiry(t *testing.T, creationDate time.Time, + expiry time.Duration, heightDelta uint32, + startState channeldb.ContractState, + startHtlcs []*channeldb.InvoiceHTLC) *hodlExpiryTest { + + mockNotifier := newMockNotifier() + mockClock := clock.NewTestClock(testTime) + + test := &hodlExpiryTest{ + state: startState, + watcher: NewInvoiceExpiryWatcher( + mockClock, heightDelta, uint32(testCurrentHeight), nil, + mockNotifier, + ), + cancelChan: make(chan lntypes.Hash), + mockNotifier: mockNotifier, + mockClock: mockClock, + } + + // Use an unbuffered channel to block on cancel calls so that the test + // does not exit before we've processed all the invoices we expect. + cancelImpl := func(paymentHash lntypes.Hash, force bool) error { + test.stateLock.Lock() + currentState := test.state + test.stateLock.Unlock() + + if currentState != channeldb.ContractOpen && !force { + return nil + } + + select { + case test.cancelChan <- paymentHash: + case <-time.After(testTimeout): + } + + return nil + } + + require.NoError(t, test.watcher.Start(cancelImpl)) + + // We set preimage and hash so that we can use our existing test + // helpers. In practice we would only have the hash, but this does not + // affect what we're testing at all. + preimage := lntypes.Preimage{1} + test.hash = preimage.Hash() + + invoice := newTestInvoice(t, preimage, creationDate, expiry) + invoice.State = startState + invoice.HodlInvoice = true + invoice.Htlcs = make(map[channeldb.CircuitKey]*channeldb.InvoiceHTLC) + + // If we have any htlcs, add them with unique circult keys. + for i, htlc := range startHtlcs { + key := channeldb.CircuitKey{ + HtlcID: uint64(i), + } + + invoice.Htlcs[key] = htlc + } + + // Create an expiry entry for our invoice in its starting state. This + // mimics adding invoices to the watcher on start. + entry := makeInvoiceExpiry(test.hash, invoice) + test.watcher.AddInvoices(entry) + + return test +} From b7d1ed0cbb31fc0436525e87a8083285640cc9e9 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 11 May 2021 08:45:31 +0200 Subject: [PATCH 3/5] itest/test: test hold invoice cancel before force close --- lntest/itest/lnd_hold_invoice_force_test.go | 72 +++++++++++---------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/lntest/itest/lnd_hold_invoice_force_test.go b/lntest/itest/lnd_hold_invoice_force_test.go index 00831000..7a71ac74 100644 --- a/lntest/itest/lnd_hold_invoice_force_test.go +++ b/lntest/itest/lnd_hold_invoice_force_test.go @@ -14,9 +14,8 @@ import ( "github.com/stretchr/testify/require" ) -// testHoldInvoiceForceClose demonstrates that recipients of hold invoices -// will not release active htlcs for their own invoices when they expire, -// resulting in a force close of their channel. +// testHoldInvoiceForceClose tests cancelation of accepted hold invoices which +// would otherwise trigger force closes when they expire. func testHoldInvoiceForceClose(net *lntest.NetworkHarness, t *harnessTest) { ctxb, cancel := context.WithCancel(context.Background()) defer cancel() @@ -94,38 +93,43 @@ func testHoldInvoiceForceClose(net *lntest.NetworkHarness, t *harnessTest) { require.NoError(t.t, net.Alice.WaitForBlockchainSync(ctxb)) require.NoError(t.t, net.Bob.WaitForBlockchainSync(ctxb)) - // Alice should have a waiting-close channel because she has force - // closed to time out the htlc. - assertNumPendingChannels(t, net.Alice, 1, 0) - - // We should have our force close tx in the mempool. - mineBlocks(t, net, 1, 1) - - // Ensure alice and bob are synced to chain after we've mined our force - // close. - require.NoError(t.t, net.Alice.WaitForBlockchainSync(ctxb)) - require.NoError(t.t, net.Bob.WaitForBlockchainSync(ctxb)) - - // At this point, Bob's channel should be resolved because his htlc is - // expired, so no further action is required. Alice will still have a - // pending force close channel because she needs to resolve the htlc. - assertNumPendingChannels(t, net.Alice, 0, 1) - assertNumPendingChannels(t, net.Bob, 0, 0) - + // Our channel should not have been force closed, instead we expect our + // channel to still be open and our invoice to have been canceled before + // expiry. ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - err = waitForNumChannelPendingForceClose(ctxt, net.Alice, 1, - func(channel *lnrpcForceCloseChannel) error { - numHtlcs := len(channel.PendingHtlcs) - if numHtlcs != 1 { - return fmt.Errorf("expected 1 htlc, got: "+ - "%v", numHtlcs) - } - - return nil - }, - ) + chanInfo, err := getChanInfo(ctxt, net.Alice) require.NoError(t.t, err) - // Cleanup Alice's force close. - cleanupForceClose(t, net, net.Alice, chanPoint) + fundingTxID, err := lnrpc.GetChanPointFundingTxid(chanPoint) + require.NoError(t.t, err) + chanStr := fmt.Sprintf("%v:%v", fundingTxID, chanPoint.OutputIndex) + require.Equal(t.t, chanStr, chanInfo.ChannelPoint) + + err = wait.NoError(func() error { + inv, err := net.Bob.LookupInvoice(ctxt, &lnrpc.PaymentHash{ + RHash: payHash[:], + }) + if err != nil { + return err + } + + if inv.State != lnrpc.Invoice_CANCELED { + return fmt.Errorf("expected canceled invoice, got: %v", + inv.State) + } + + for _, htlc := range inv.Htlcs { + if htlc.State != lnrpc.InvoiceHTLCState_CANCELED { + return fmt.Errorf("expected htlc canceled, "+ + "got: %v", htlc.State) + } + } + + return nil + }, defaultTimeout) + require.NoError(t.t, err, "expected canceled invoice") + + // Clean up the channel. + ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout) + closeChannelAndAssert(ctxt, t, net, net.Alice, chanPoint, false) } From 8066ff7047fecf4b36718e6d7718e3a70200a4a5 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 11 May 2021 08:45:32 +0200 Subject: [PATCH 4/5] itest/test: assert expired hold invoice updated correctly --- ..._force_close_on_chain_htlc_timeout_test.go | 28 +++++-------------- lntest/itest/log_error_whitelist.txt | 2 ++ 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go b/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go index 68547328..cf237c89 100644 --- a/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go +++ b/lntest/itest/lnd_multi-hop_remote_force_close_on_chain_htlc_timeout_test.go @@ -178,8 +178,8 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness, err = waitForNumChannelPendingForceClose(ctxt, bob, 0, nil) require.NoError(t.t, err) - // While we're here, we demonstrate some bugs in our handling of - // invoices that timeout on chain. + // While we're here, we assert that our expired invoice's state is + // correctly updated, and can no longer be settled. assertOnChainInvoiceState(ctxb, t, carol, preimage) // We'll close out the test by closing the channel from Alice to Bob, @@ -191,12 +191,8 @@ func testMultiHopRemoteForceCloseOnChainHtlcTimeout(net *lntest.NetworkHarness, ) } -// assertOnChainInvoiceState asserts that we have some bugs with how we handle -// hold invoices that are expired on-chain. -// - htlcs accepted: despite being timed out, our htlcs are still in accepted -// state -// - can settle: our invoice that has expired on-chain can still be settled -// even though we don't claim any htlcs. +// assertOnChainInvoiceState asserts that we have the correct state for a hold +// invoice that has expired on chain, and that it can't be settled. func assertOnChainInvoiceState(ctx context.Context, t *harnessTest, node *lntest.HarnessNode, preimage lntypes.Preimage) { @@ -207,22 +203,12 @@ func assertOnChainInvoiceState(ctx context.Context, t *harnessTest, require.NoError(t.t, err) for _, htlc := range inv.Htlcs { - require.Equal(t.t, lnrpc.InvoiceHTLCState_ACCEPTED, htlc.State) + require.Equal(t.t, lnrpc.InvoiceHTLCState_CANCELED, htlc.State) } + require.Equal(t.t, lnrpc.Invoice_CANCELED, inv.State) _, err = node.SettleInvoice(ctx, &invoicesrpc.SettleInvoiceMsg{ Preimage: preimage[:], }) - require.NoError(t.t, err, "expected erroneous invoice settle") - - inv, err = node.LookupInvoice(ctx, &lnrpc.PaymentHash{ - RHash: hash[:], - }) - require.NoError(t.t, err) - - require.True(t.t, inv.Settled, "expected erroneously settled invoice") // nolint:staticcheck - for _, htlc := range inv.Htlcs { - require.Equal(t.t, lnrpc.InvoiceHTLCState_SETTLED, htlc.State, - "expected htlcs to be erroneously settled") - } + require.Error(t.t, err, "should not be able to settle invoice") } diff --git a/lntest/itest/log_error_whitelist.txt b/lntest/itest/log_error_whitelist.txt index 665b3266..31610f3d 100644 --- a/lntest/itest/log_error_whitelist.txt +++ b/lntest/itest/log_error_whitelist.txt @@ -279,3 +279,5 @@