diff --git a/channeldb/invoice_test.go b/channeldb/invoice_test.go index f624350e6..61e0b5d0f 100644 --- a/channeldb/invoice_test.go +++ b/channeldb/invoice_test.go @@ -99,7 +99,10 @@ func TestInvoiceWorkflow(t *testing.T) { // now have the settled bit toggle to true and a non-default // SettledDate payAmt := fakeInvoice.Terms.Value * 2 - if _, err := db.AcceptOrSettleInvoice(paymentHash, payAmt); err != nil { + _, err = db.AcceptOrSettleInvoice( + paymentHash, payAmt, checkHtlcParameters, + ) + if err != nil { t.Fatalf("unable to settle invoice: %v", err) } dbInvoice2, err := db.LookupInvoice(paymentHash) @@ -261,7 +264,9 @@ func TestInvoiceAddTimeSeries(t *testing.T) { paymentHash := invoice.Terms.PaymentPreimage.Hash() - _, err := db.AcceptOrSettleInvoice(paymentHash, 0) + _, err := db.AcceptOrSettleInvoice( + paymentHash, 0, checkHtlcParameters, + ) if err != nil { t.Fatalf("unable to settle invoice: %v", err) } @@ -342,7 +347,9 @@ func TestDuplicateSettleInvoice(t *testing.T) { } // With the invoice in the DB, we'll now attempt to settle the invoice. - dbInvoice, err := db.AcceptOrSettleInvoice(payHash, amt) + dbInvoice, err := db.AcceptOrSettleInvoice( + payHash, amt, checkHtlcParameters, + ) if err != nil { t.Fatalf("unable to settle invoice: %v", err) } @@ -362,7 +369,9 @@ func TestDuplicateSettleInvoice(t *testing.T) { // If we try to settle the invoice again, then we should get the very // same invoice back, but with an error this time. - dbInvoice, err = db.AcceptOrSettleInvoice(payHash, amt) + dbInvoice, err = db.AcceptOrSettleInvoice( + payHash, amt, checkHtlcParameters, + ) if err != ErrInvoiceAlreadySettled { t.Fatalf("expected ErrInvoiceAlreadySettled") } @@ -407,7 +416,10 @@ func TestQueryInvoices(t *testing.T) { // We'll only settle half of all invoices created. if i%2 == 0 { - if _, err := db.AcceptOrSettleInvoice(paymentHash, i); err != nil { + _, err := db.AcceptOrSettleInvoice( + paymentHash, i, checkHtlcParameters, + ) + if err != nil { t.Fatalf("unable to settle invoice: %v", err) } } @@ -648,3 +660,7 @@ func TestQueryInvoices(t *testing.T) { } } } + +func checkHtlcParameters(invoice *Invoice) error { + return nil +} diff --git a/channeldb/invoices.go b/channeldb/invoices.go index 47a8e1990..95e7c9fb2 100644 --- a/channeldb/invoices.go +++ b/channeldb/invoices.go @@ -631,8 +631,12 @@ func (d *DB) QueryInvoices(q InvoiceQuery) (InvoiceSlice, error) { // // When the preimage for the invoice is unknown (hold invoice), the invoice is // marked as accepted. +// +// TODO: Store invoice cltv as separate field in database so that it doesn't +// need to be decoded from the payment request. func (d *DB) AcceptOrSettleInvoice(paymentHash [32]byte, - amtPaid lnwire.MilliSatoshi) (*Invoice, error) { + amtPaid lnwire.MilliSatoshi, + checkHtlcParameters func(invoice *Invoice) error) (*Invoice, error) { var settledInvoice *Invoice err := d.Update(func(tx *bbolt.Tx) error { @@ -662,6 +666,7 @@ func (d *DB) AcceptOrSettleInvoice(paymentHash [32]byte, settledInvoice, err = acceptOrSettleInvoice( invoices, settleIndex, invoiceNum, amtPaid, + checkHtlcParameters, ) return err @@ -988,8 +993,10 @@ func deserializeInvoice(r io.Reader) (Invoice, error) { return invoice, nil } -func acceptOrSettleInvoice(invoices, settleIndex *bbolt.Bucket, invoiceNum []byte, - amtPaid lnwire.MilliSatoshi) (*Invoice, error) { +func acceptOrSettleInvoice(invoices, settleIndex *bbolt.Bucket, + invoiceNum []byte, amtPaid lnwire.MilliSatoshi, + checkHtlcParameters func(invoice *Invoice) error) ( + *Invoice, error) { invoice, err := fetchInvoice(invoiceNum, invoices) if err != nil { @@ -1007,6 +1014,13 @@ func acceptOrSettleInvoice(invoices, settleIndex *bbolt.Bucket, invoiceNum []byt return &invoice, ErrInvoiceAlreadyCanceled } + // If the invoice is still open, check the htlc parameters. + if err := checkHtlcParameters(&invoice); err != nil { + return &invoice, err + } + + // Check to see if we can settle or this is an hold invoice and we need + // to wait for the preimage. holdInvoice := invoice.Terms.PaymentPreimage == UnknownPreimage if holdInvoice { invoice.Terms.State = ContractAccepted diff --git a/contractcourt/htlc_incoming_contest_resolver.go b/contractcourt/htlc_incoming_contest_resolver.go index c1933e9e3..9af694d68 100644 --- a/contractcourt/htlc_incoming_contest_resolver.go +++ b/contractcourt/htlc_incoming_contest_resolver.go @@ -135,28 +135,52 @@ func (h *htlcIncomingContestResolver) Resolve() (ContractResolver, error) { preimageSubscription := h.PreimageDB.SubscribeUpdates() defer preimageSubscription.CancelSubscription() - // Create a buffered hodl chan to prevent deadlock. - hodlChan := make(chan interface{}, 1) + // Define closure to process hodl events either direct or triggered by + // later notifcation. + processHodlEvent := func(e invoices.HodlEvent) (ContractResolver, + error) { - // Notify registry that we are potentially settling as exit hop - // on-chain, so that we will get a hodl event when a corresponding hodl - // invoice is settled. - event, err := h.Registry.NotifyExitHopHtlc(h.payHash, h.htlcAmt, hodlChan) - if err != nil && err != channeldb.ErrInvoiceNotFound { - return nil, err - } - defer h.Registry.HodlUnsubscribeAll(hodlChan) + if e.Preimage == nil { + log.Infof("%T(%v): Exit hop HTLC canceled "+ + "(expiry=%v, height=%v), abandoning", h, + h.htlcResolution.ClaimOutpoint, + h.htlcExpiry, currentHeight) - // If the htlc can be settled directly, we can progress to the inner - // resolver immediately. - if event != nil && event.Preimage != nil { - if err := applyPreimage(*event.Preimage); err != nil { + h.resolved = true + return nil, h.Checkpoint(h) + } + + if err := applyPreimage(*e.Preimage); err != nil { return nil, err } return &h.htlcSuccessResolver, nil } + // Create a buffered hodl chan to prevent deadlock. + hodlChan := make(chan interface{}, 1) + + // Notify registry that we are potentially resolving as an exit hop + // on-chain. If this HTLC indeed pays to an existing invoice, the + // invoice registry will tell us what to do with the HTLC. This is + // identical to HTLC resolution in the link. + event, err := h.Registry.NotifyExitHopHtlc( + h.payHash, h.htlcAmt, h.htlcExpiry, currentHeight, + hodlChan, + ) + switch err { + case channeldb.ErrInvoiceNotFound: + case nil: + defer h.Registry.HodlUnsubscribeAll(hodlChan) + + // Resolve the htlc directly if possible. + if event != nil { + return processHodlEvent(*event) + } + default: + return nil, err + } + // With the epochs and preimage subscriptions initialized, we'll query // to see if we already know the preimage. preimage, ok := h.PreimageDB.LookupPreimage(h.payHash) @@ -193,16 +217,7 @@ func (h *htlcIncomingContestResolver) Resolve() (ContractResolver, error) { case hodlItem := <-hodlChan: hodlEvent := hodlItem.(invoices.HodlEvent) - // Only process settle events. - if hodlEvent.Preimage == nil { - continue - } - - if err := applyPreimage(*event.Preimage); err != nil { - return nil, err - } - - return &h.htlcSuccessResolver, nil + return processHodlEvent(hodlEvent) case newBlock, ok := <-blockEpochs.Epochs: if !ok { @@ -211,8 +226,7 @@ func (h *htlcIncomingContestResolver) Resolve() (ContractResolver, error) { // If this new height expires the HTLC, then this means // we never found out the preimage, so we can mark - // resolved and - // exit. + // resolved and exit. newHeight := uint32(newBlock.Height) if newHeight >= h.htlcExpiry { log.Infof("%T(%v): HTLC has timed out "+ diff --git a/contractcourt/htlc_success_resolver.go b/contractcourt/htlc_success_resolver.go index ce3ec056a..f4005525d 100644 --- a/contractcourt/htlc_success_resolver.go +++ b/contractcourt/htlc_success_resolver.go @@ -7,7 +7,6 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/davecgh/go-spew/spew" - "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnwallet" @@ -78,9 +77,11 @@ func (h *htlcSuccessResolver) ResolverKey() []byte { } // Resolve attempts to resolve an unresolved incoming HTLC that we know the -// preimage to. If the HTLC is on the commitment of the remote party, then -// we'll simply sweep it directly. Otherwise, we'll hand this off to the utxo -// nursery to do its duty. +// preimage to. If the HTLC is on the commitment of the remote party, then we'll +// simply sweep it directly. Otherwise, we'll hand this off to the utxo nursery +// to do its duty. There is no need to make a call to the invoice registry +// anymore. Every HTLC has already passed through the incoming contest resolver +// and in there the invoice was already marked as settled. // // TODO(roasbeef): create multi to batch // @@ -177,19 +178,6 @@ func (h *htlcSuccessResolver) Resolve() (ContractResolver, error) { return nil, fmt.Errorf("quitting") } - // With the HTLC claimed, we can attempt to settle its - // corresponding invoice if we were the original destination. As - // the htlc is already settled at this point, we don't need to - // read on the hodl channel. - hodlChan := make(chan interface{}, 1) - _, err = h.Registry.NotifyExitHopHtlc( - h.payHash, h.htlcAmt, hodlChan, - ) - if err != nil && err != channeldb.ErrInvoiceNotFound { - log.Errorf("Unable to settle invoice with payment "+ - "hash %x: %v", h.payHash, err) - } - // Once the transaction has received a sufficient number of // confirmations, we'll mark ourselves as fully resolved and exit. h.resolved = true @@ -255,17 +243,6 @@ func (h *htlcSuccessResolver) Resolve() (ContractResolver, error) { return nil, fmt.Errorf("quitting") } - // With the HTLC claimed, we can attempt to settle its corresponding - // invoice if we were the original destination. As the htlc is already - // settled at this point, we don't need to read on the hodl - // channel. - hodlChan := make(chan interface{}, 1) - _, err = h.Registry.NotifyExitHopHtlc(h.payHash, h.htlcAmt, hodlChan) - if err != nil && err != channeldb.ErrInvoiceNotFound { - log.Errorf("Unable to settle invoice with payment "+ - "hash %x: %v", h.payHash, err) - } - h.resolved = true return nil, h.Checkpoint(h) } diff --git a/contractcourt/interfaces.go b/contractcourt/interfaces.go index fa702e9f9..a18ee3d6c 100644 --- a/contractcourt/interfaces.go +++ b/contractcourt/interfaces.go @@ -21,6 +21,7 @@ type Registry interface { // htlc should be resolved. If the htlc cannot be resolved immediately, // the resolution is sent on the passed in hodlChan later. NotifyExitHopHtlc(payHash lntypes.Hash, paidAmount lnwire.MilliSatoshi, + expiry uint32, currentHeight int32, hodlChan chan<- interface{}) (*invoices.HodlEvent, error) // HodlUnsubscribeAll unsubscribes from all hodl events. diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index a9dc2bc56..68350f62f 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -24,6 +24,7 @@ type InvoiceDatabase interface { // htlc should be resolved. If the htlc cannot be resolved immediately, // the resolution is sent on the passed in hodlChan later. NotifyExitHopHtlc(payHash lntypes.Hash, paidAmount lnwire.MilliSatoshi, + expiry uint32, currentHeight int32, hodlChan chan<- interface{}) (*invoices.HodlEvent, error) // CancelInvoice attempts to cancel the invoice corresponding to the diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 1c7cb11f7..dc50b4fd8 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -235,13 +235,6 @@ type ChannelLinkConfig struct { MinFeeUpdateTimeout time.Duration MaxFeeUpdateTimeout time.Duration - // 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 @@ -1173,15 +1166,12 @@ func (l *channelLink) processHodlEvent(hodlEvent invoices.HodlEvent, htlcs ...hodlHtlc) error { hash := hodlEvent.Hash - if hodlEvent.Preimage == nil { - l.debugf("Received hodl cancel event for %v", hash) - } else { - l.debugf("Received hodl settle event for %v", hash) - } // Determine required action for the resolution. var hodlAction func(htlc hodlHtlc) error if hodlEvent.Preimage != nil { + l.debugf("Received hodl settle event for %v", hash) + hodlAction = func(htlc hodlHtlc) error { return l.settleHTLC( *hodlEvent.Preimage, htlc.pd.HtlcIndex, @@ -1189,10 +1179,30 @@ func (l *channelLink) processHodlEvent(hodlEvent invoices.HodlEvent, ) } } else { + l.debugf("Received hodl cancel event for %v, reason=%v", + hash, hodlEvent.CancelReason) + hodlAction = func(htlc hodlHtlc) error { - failure := lnwire.NewFailUnknownPaymentHash( - htlc.pd.Amount, - ) + var failure lnwire.FailureMessage + switch hodlEvent.CancelReason { + + case invoices.CancelAmountTooLow: + fallthrough + case invoices.CancelInvoiceUnknown: + fallthrough + case invoices.CancelInvoiceCanceled: + failure = lnwire.NewFailUnknownPaymentHash( + htlc.pd.Amount, + ) + + case invoices.CancelExpiryTooSoon: + failure = lnwire.FailFinalExpiryTooSoon{} + + default: + return fmt.Errorf("unknown cancel reason: %v", + hodlEvent.CancelReason) + } + l.sendHTLCError( htlc.pd.HtlcIndex, failure, htlc.obfuscator, htlc.pd.SourceRef, @@ -2739,94 +2749,14 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor, return false, nil } - // 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 <= 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) - - failure := lnwire.NewFinalExpiryTooSoon() - l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef) - - return true, nil - } - - // We're the designated payment destination. Therefore we attempt to - // see if we have an invoice locally which'll allow us to settle this - // htlc. - // - // Only the immutable data from LookupInvoice is used, because otherwise - // a race condition may be created with concurrent writes to the invoice - // registry. For example: cancelation of an invoice. - invoiceHash := lntypes.Hash(pd.RHash) - invoice, minCltvDelta, err := l.cfg.Registry.LookupInvoice(invoiceHash) - if err != nil { - log.Errorf("unable to query invoice registry: %v", err) - failure := lnwire.NewFailUnknownPaymentHash(pd.Amount) - l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef) - - return true, nil - } - - // If the invoice is already settled, we choose to accept the payment to - // simplify failure recovery. - // - // NOTE: Though our recovery and forwarding logic is predominately - // batched, settling invoices happens iteratively. We may reject one of - // two payments for the same rhash at first, but then restart and reject - // both after seeing that the invoice has been settled. Without any - // record of which one settles first, it is ambiguous as to which one - // actually settled the invoice. Thus, by accepting all payments, we - // eliminate the race condition that can lead to this inconsistency. - // - // TODO(conner): track ownership of settlements to properly recover from - // failures? or add batch invoice settlement - // - // TODO(joostjager): The log statement below is not always accurate, as - // the invoice may have been canceled after the LookupInvoice call. - // Leaving it as is for now, because fixing this would involve changing - // the signature of InvoiceRegistry.SettleInvoice just because of this - // log statement. - if invoice.Terms.State == channeldb.ContractSettled { - log.Warnf("Accepting duplicate payment for hash=%x", - pd.RHash[:]) - } - - // If we're not currently in debug mode, and the extended htlc doesn't - // meet the value requested, then we'll fail the htlc. Otherwise, we - // settle this htlc within our local state update log, then send the - // update entry to the remote party. - // - // NOTE: We make an exception when the value requested by the invoice is - // zero. This means the invoice allows the payee to specify the amount - // of satoshis they wish to send. So since we expect the htlc to have a - // different amount, we should not fail. - if !l.cfg.DebugHTLC && invoice.Terms.Value > 0 && - pd.Amount < invoice.Terms.Value { - - log.Errorf("rejecting htlc due to incorrect amount: expected "+ - "%v, received %v", invoice.Terms.Value, pd.Amount) - - failure := lnwire.NewFailUnknownPaymentHash(pd.Amount) - l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef) - - return true, nil - } - // As we're the exit hop, we'll double check the hop-payload included in // the HTLC to ensure that it was crafted correctly by the sender and // matches the HTLC we were extended. - // - // NOTE: We make an exception when the value requested by the invoice is - // zero. This means the invoice allows the payee to specify the amount - // of satoshis they wish to send. So since we expect the htlc to have a - // different amount, we should not fail. - if !l.cfg.DebugHTLC && invoice.Terms.Value > 0 && - fwdInfo.AmountToForward < invoice.Terms.Value { + if !l.cfg.DebugHTLC && pd.Amount != fwdInfo.AmountToForward { log.Errorf("Onion payload of incoming htlc(%x) has incorrect "+ "value: expected %v, got %v", pd.RHash, - invoice.Terms.Value, fwdInfo.AmountToForward) + pd.Amount, fwdInfo.AmountToForward) failure := lnwire.NewFailUnknownPaymentHash(pd.Amount) l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef) @@ -2835,27 +2765,11 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor, } // We'll also ensure that our time-lock value has been computed - // correctly. Only check the final cltv expiry for invoices when the - // invoice has not yet moved to the accepted state. Otherwise hodl htlcs - // would be canceled after a restart. - expectedHeight := heightNow + minCltvDelta - switch { - case !l.cfg.DebugHTLC && - invoice.Terms.State == channeldb.ContractOpen && - pd.Timeout < expectedHeight: - - log.Errorf("Incoming htlc(%x) has an expiration that is too "+ - "soon: expected at least %v, got %v", - pd.RHash[:], expectedHeight, pd.Timeout) - - failure := lnwire.FailFinalExpiryTooSoon{} - l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef) - - return true, nil - - case !l.cfg.DebugHTLC && pd.Timeout != fwdInfo.OutgoingCTLV: - log.Errorf("HTLC(%x) has incorrect time-lock: expected %v, "+ - "got %v", pd.RHash[:], pd.Timeout, fwdInfo.OutgoingCTLV) + // correctly. + if !l.cfg.DebugHTLC && pd.Timeout != fwdInfo.OutgoingCTLV { + log.Errorf("Onion payload of incoming htlc(%x) has incorrect "+ + "time-lock: expected %v, got %v", + pd.RHash[:], pd.Timeout, fwdInfo.OutgoingCTLV) failure := lnwire.NewFinalIncorrectCltvExpiry( fwdInfo.OutgoingCTLV, @@ -2868,10 +2782,27 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor, // Notify the invoiceRegistry of the exit hop htlc. If we crash right // after this, this code will be re-executed after restart. We will // receive back a resolution event. + invoiceHash := lntypes.Hash(pd.RHash) + event, err := l.cfg.Registry.NotifyExitHopHtlc( - invoiceHash, pd.Amount, l.hodlQueue.ChanIn(), + invoiceHash, pd.Amount, pd.Timeout, int32(heightNow), + l.hodlQueue.ChanIn(), ) - if err != nil { + + switch err { + + // Cancel htlc if we don't have an invoice for it. + case channeldb.ErrInvoiceNotFound: + failure := lnwire.NewFailUnknownPaymentHash(pd.Amount) + l.sendHTLCError(pd.HtlcIndex, failure, obfuscator, pd.SourceRef) + + return true, nil + + // No error. + case nil: + + // Pass error to caller. + default: return false, err } diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index 9f62f6c4d..be9c39943 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -766,7 +766,9 @@ func newMockRegistry(minDelta uint32) *mockInvoiceRegistry { return testInvoiceCltvExpiry, nil } - registry := invoices.NewRegistry(cdb, decodeExpiry) + finalCltvRejectDelta := int32(5) + + registry := invoices.NewRegistry(cdb, decodeExpiry, finalCltvRejectDelta) registry.Start() return &mockInvoiceRegistry{ @@ -784,10 +786,12 @@ func (i *mockInvoiceRegistry) SettleHodlInvoice(preimage lntypes.Preimage) error } func (i *mockInvoiceRegistry) NotifyExitHopHtlc(rhash lntypes.Hash, - amt lnwire.MilliSatoshi, hodlChan chan<- interface{}) ( - *invoices.HodlEvent, error) { + amt lnwire.MilliSatoshi, expiry uint32, currentHeight int32, + hodlChan chan<- interface{}) (*invoices.HodlEvent, error) { - event, err := i.registry.NotifyExitHopHtlc(rhash, amt, hodlChan) + event, err := i.registry.NotifyExitHopHtlc( + rhash, amt, expiry, currentHeight, hodlChan, + ) if err != nil { return nil, err } diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go index bc1035b3b..d749eaa2e 100644 --- a/htlcswitch/test_utils.go +++ b/htlcswitch/test_utils.go @@ -1056,7 +1056,6 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer, MinFeeUpdateTimeout: minFeeUpdateTimeout, MaxFeeUpdateTimeout: maxFeeUpdateTimeout, OnChannelFailure: func(lnwire.ChannelID, lnwire.ShortChannelID, LinkFailureError) {}, - FinalCltvRejectDelta: 5, OutgoingCltvRejectDelta: 3, }, channel, diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 172eb075d..5fe17d04d 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -2,6 +2,7 @@ package invoices import ( "bytes" + "errors" "fmt" "sync" "sync/atomic" @@ -27,12 +28,64 @@ var ( DebugHash = DebugPre.Hash() ) +// HtlcCancelReason defines reasons for which htlcs can be canceled. +type HtlcCancelReason uint8 + +const ( + // CancelInvoiceUnknown is returned if the preimage is unknown. + CancelInvoiceUnknown HtlcCancelReason = iota + + // CancelExpiryTooSoon is returned when the timelock of the htlc + // does not satisfy the invoice cltv expiry requirement. + CancelExpiryTooSoon + + // CancelInvoiceCanceled is returned when the invoice is already + // canceled and can't be paid to anymore. + CancelInvoiceCanceled + + // CancelAmountTooLow is returned when the amount paid is too low. + CancelAmountTooLow +) + +// String returns a human readable identifier for the cancel reason. +func (r HtlcCancelReason) String() string { + switch r { + case CancelInvoiceUnknown: + return "InvoiceUnknown" + case CancelExpiryTooSoon: + return "ExpiryTooSoon" + case CancelInvoiceCanceled: + return "InvoiceCanceled" + case CancelAmountTooLow: + return "CancelAmountTooLow" + default: + return "Unknown" + } +} + +var ( + // ErrInvoiceExpiryTooSoon is returned when an invoice is attempted to be + // accepted or settled with not enough blocks remaining. + ErrInvoiceExpiryTooSoon = errors.New("invoice expiry too soon") + + // ErrInvoiceAmountTooLow is returned when an invoice is attempted to be + // accepted or settled with an amount that is too low. + ErrInvoiceAmountTooLow = errors.New("paid amount less than invoice amount") +) + // HodlEvent describes how an htlc should be resolved. If HodlEvent.Preimage is // set, the event indicates a settle event. If Preimage is nil, it is a cancel // event. type HodlEvent struct { + // Preimage is the htlc preimage. Its value is nil in case of a cancel. Preimage *lntypes.Preimage - Hash lntypes.Hash + + // Hash is the htlc hash. + Hash lntypes.Hash + + // CancelReason specifies the reason why invoice registry decided to + // cancel the htlc. + CancelReason HtlcCancelReason } // InvoiceRegistry is a central registry of all the outstanding invoices @@ -70,6 +123,13 @@ type InvoiceRegistry struct { // is used to unsubscribe from all hashes efficiently. hodlReverseSubscriptions map[chan<- interface{}]map[lntypes.Hash]struct{} + // 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 int32 + wg sync.WaitGroup quit chan struct{} } @@ -79,7 +139,7 @@ type InvoiceRegistry struct { // layer. The in-memory layer is in place such that debug invoices can be added // which are volatile yet available system wide within the daemon. func NewRegistry(cdb *channeldb.DB, decodeFinalCltvExpiry func(invoice string) ( - uint32, error)) *InvoiceRegistry { + uint32, error), finalCltvRejectDelta int32) *InvoiceRegistry { return &InvoiceRegistry{ cdb: cdb, @@ -93,6 +153,7 @@ func NewRegistry(cdb *channeldb.DB, decodeFinalCltvExpiry func(invoice string) ( hodlSubscriptions: make(map[lntypes.Hash]map[chan<- interface{}]struct{}), hodlReverseSubscriptions: make(map[chan<- interface{}]map[lntypes.Hash]struct{}), decodeFinalCltvExpiry: decodeFinalCltvExpiry, + finalCltvRejectDelta: finalCltvRejectDelta, quit: make(chan struct{}), } } @@ -460,6 +521,35 @@ func (i *InvoiceRegistry) LookupInvoice(rHash lntypes.Hash) (channeldb.Invoice, return invoice, expiry, nil } +// checkHtlcParameters is a callback used inside invoice db transactions to +// atomically check-and-update an invoice. +func (i *InvoiceRegistry) checkHtlcParameters(invoice *channeldb.Invoice, + amtPaid lnwire.MilliSatoshi, htlcExpiry uint32, currentHeight int32) error { + + expiry, err := i.decodeFinalCltvExpiry(string(invoice.PaymentRequest)) + if err != nil { + return err + } + + if htlcExpiry < uint32(currentHeight+i.finalCltvRejectDelta) { + return ErrInvoiceExpiryTooSoon + } + + if htlcExpiry < uint32(currentHeight)+expiry { + return ErrInvoiceExpiryTooSoon + } + + // If an invoice amount is specified, check that enough is paid. This + // check is only performed for open invoices. Once a sufficiently large + // payment has been made and the invoice is in the accepted or settled + // state, any amount will be accepted on top of that. + if invoice.Terms.Value > 0 && amtPaid < invoice.Terms.Value { + return ErrInvoiceAmountTooLow + } + + return nil +} + // NotifyExitHopHtlc attempts to mark an invoice as settled. If the invoice is a // debug invoice, then this method is a noop as debug invoices are never fully // settled. The return value describes how the htlc should be resolved. @@ -472,59 +562,112 @@ func (i *InvoiceRegistry) LookupInvoice(rHash lntypes.Hash) (channeldb.Invoice, // the channel is either buffered or received on from another goroutine to // prevent deadlock. func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, - amtPaid lnwire.MilliSatoshi, hodlChan chan<- interface{}) ( - *HodlEvent, error) { + amtPaid lnwire.MilliSatoshi, expiry uint32, currentHeight int32, + hodlChan chan<- interface{}) (*HodlEvent, error) { i.Lock() defer i.Unlock() - log.Debugf("Invoice(%x): htlc accepted", rHash[:]) - - createEvent := func(preimage *lntypes.Preimage) *HodlEvent { - return &HodlEvent{ - Hash: rHash, - Preimage: preimage, - } + debugLog := func(s string) { + log.Debugf("Invoice(%x): %v, amt=%v, expiry=%v", + rHash[:], s, amtPaid, expiry) } - // First check the in-memory debug invoice index to see if this is an // existing invoice added for debugging. if invoice, ok := i.debugInvoices[rHash]; ok { + debugLog("payment to debug invoice accepted") + // Debug invoices are never fully settled, so we just settle the // htlc in this case. - return createEvent(&invoice.Terms.PaymentPreimage), nil + return &HodlEvent{ + Hash: rHash, + Preimage: &invoice.Terms.PaymentPreimage, + }, nil } // If this isn't a debug invoice, then we'll attempt to settle an // invoice matching this rHash on disk (if one exists). - invoice, err := i.cdb.AcceptOrSettleInvoice(rHash, amtPaid) + invoice, err := i.cdb.AcceptOrSettleInvoice( + rHash, amtPaid, + func(inv *channeldb.Invoice) error { + return i.checkHtlcParameters( + inv, amtPaid, expiry, currentHeight, + ) + }, + ) switch err { // If invoice is already settled, settle htlc. This means we accept more // payments to the same invoice hash. + // + // NOTE: Though our recovery and forwarding logic is predominately + // batched, settling invoices happens iteratively. We may reject one of + // two payments for the same rhash at first, but then restart and reject + // both after seeing that the invoice has been settled. Without any + // record of which one settles first, it is ambiguous as to which one + // actually settled the invoice. Thus, by accepting all payments, we + // eliminate the race condition that can lead to this inconsistency. + // + // TODO(conner): track ownership of settlements to properly recover from + // failures? or add batch invoice settlement case channeldb.ErrInvoiceAlreadySettled: - return createEvent(&invoice.Terms.PaymentPreimage), nil + debugLog("accepting duplicate payment to settled invoice") + + return &HodlEvent{ + Hash: rHash, + Preimage: &invoice.Terms.PaymentPreimage, + }, nil // If invoice is already canceled, cancel htlc. case channeldb.ErrInvoiceAlreadyCanceled: - return createEvent(nil), nil + debugLog("invoice already canceled") + + return &HodlEvent{ + Hash: rHash, + CancelReason: CancelInvoiceCanceled, + }, nil // If invoice is already accepted, add this htlc to the list of // subscribers. case channeldb.ErrInvoiceAlreadyAccepted: + debugLog("accepting duplicate payment to accepted invoice") + i.hodlSubscribe(hodlChan, rHash) return nil, nil + // If there are not enough blocks left, cancel the htlc. + case ErrInvoiceExpiryTooSoon: + debugLog("expiry too soon") + + return &HodlEvent{ + Hash: rHash, + CancelReason: CancelExpiryTooSoon, + }, nil + + // If there are not enough blocks left, cancel the htlc. + case ErrInvoiceAmountTooLow: + debugLog("amount too low") + + return &HodlEvent{ + Hash: rHash, + CancelReason: CancelAmountTooLow, + }, nil + // If this call settled the invoice, settle the htlc. Otherwise // subscribe for a future hodl event. case nil: i.notifyClients(rHash, invoice, invoice.Terms.State) switch invoice.Terms.State { case channeldb.ContractSettled: - log.Debugf("Invoice(%x): settled", rHash[:]) + debugLog("settled") - return createEvent(&invoice.Terms.PaymentPreimage), nil + return &HodlEvent{ + Hash: rHash, + Preimage: &invoice.Terms.PaymentPreimage, + }, nil case channeldb.ContractAccepted: + debugLog("accepted") + // Subscribe to updates to this invoice. i.hodlSubscribe(hodlChan, rHash) return nil, nil @@ -534,6 +677,8 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, } default: + debugLog(err.Error()) + return nil, err } } @@ -584,7 +729,8 @@ func (i *InvoiceRegistry) CancelInvoice(payHash lntypes.Hash) error { log.Debugf("Invoice(%v): canceled", payHash) i.notifyHodlSubscribers(HodlEvent{ - Hash: payHash, + Hash: payHash, + CancelReason: CancelInvoiceCanceled, }) i.notifyClients(payHash, invoice, channeldb.ContractCanceled) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 8ffc780c3..cb554faa6 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -6,11 +6,9 @@ import ( "testing" "time" - "github.com/btcsuite/btcd/chaincfg" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnwire" - "github.com/lightningnetwork/lnd/zpay32" ) var ( @@ -23,18 +21,15 @@ var ( hash = preimage.Hash() - // testPayReq is a dummy payment request that does parse properly. It - // has no relation with the real invoice parameters and isn't asserted - // on in this test. LookupInvoice requires this to have a valid value. - testPayReq = "lnbc500u1pwywxzwpp5nd2u9xzq02t0tuf2654as7vma42lwkcjptx4yzfq0umq4swpa7cqdqqcqzysmlpc9ewnydr8rr8dnltyxphdyf6mcqrsd6dml8zajtyhwe6a45d807kxtmzayuf0hh2d9tn478ecxkecdg7c5g85pntupug5kakm7xcpn63zqk" + testInvoiceExpiry = uint32(3) + + testCurrentHeight = int32(0) + + testFinalCltvRejectDelta = int32(3) ) func decodeExpiry(payReq string) (uint32, error) { - invoice, err := zpay32.Decode(payReq, &chaincfg.MainNetParams) - if err != nil { - return 0, err - } - return uint32(invoice.MinFinalCLTVExpiry()), nil + return uint32(testInvoiceExpiry), nil } var ( @@ -43,7 +38,6 @@ var ( PaymentPreimage: preimage, Value: lnwire.MilliSatoshi(100000), }, - PaymentRequest: []byte(testPayReq), } ) @@ -54,7 +48,7 @@ func newTestContext(t *testing.T) (*InvoiceRegistry, func()) { } // Instantiate and start the invoice registry. - registry := NewRegistry(cdb, decodeExpiry) + registry := NewRegistry(cdb, decodeExpiry, testFinalCltvRejectDelta) err = registry.Start() if err != nil { @@ -121,7 +115,9 @@ func TestSettleInvoice(t *testing.T) { // Settle invoice with a slightly higher amount. amtPaid := lnwire.MilliSatoshi(100500) - _, err = registry.NotifyExitHopHtlc(hash, amtPaid, hodlChan) + _, err = registry.NotifyExitHopHtlc( + hash, amtPaid, testInvoiceExpiry, 0, hodlChan, + ) if err != nil { t.Fatal(err) } @@ -153,13 +149,18 @@ func TestSettleInvoice(t *testing.T) { } // Try to settle again. - _, err = registry.NotifyExitHopHtlc(hash, amtPaid, hodlChan) + _, err = registry.NotifyExitHopHtlc( + hash, amtPaid, testInvoiceExpiry, testCurrentHeight, hodlChan, + ) if err != nil { t.Fatal("expected duplicate settle to succeed") } // Try to settle again with a different amount. - _, err = registry.NotifyExitHopHtlc(hash, amtPaid+600, hodlChan) + _, err = registry.NotifyExitHopHtlc( + hash, amtPaid+600, testInvoiceExpiry, testCurrentHeight, + hodlChan, + ) if err != nil { t.Fatal("expected duplicate settle to succeed") } @@ -274,7 +275,9 @@ func TestCancelInvoice(t *testing.T) { // Notify arrival of a new htlc paying to this invoice. This should // succeed. hodlChan := make(chan interface{}) - event, err := registry.NotifyExitHopHtlc(hash, amt, hodlChan) + event, err := registry.NotifyExitHopHtlc( + hash, amt, testInvoiceExpiry, testCurrentHeight, hodlChan, + ) if err != nil { t.Fatal("expected settlement of a canceled invoice to succeed") } @@ -292,7 +295,7 @@ func TestHoldInvoice(t *testing.T) { defer cleanup() // Instantiate and start the invoice registry. - registry := NewRegistry(cdb, decodeExpiry) + registry := NewRegistry(cdb, decodeExpiry, testFinalCltvRejectDelta) err = registry.Start() if err != nil { @@ -345,7 +348,9 @@ func TestHoldInvoice(t *testing.T) { // NotifyExitHopHtlc without a preimage present in the invoice registry // should be possible. - event, err := registry.NotifyExitHopHtlc(hash, amtPaid, hodlChan) + event, err := registry.NotifyExitHopHtlc( + hash, amtPaid, testInvoiceExpiry, testCurrentHeight, hodlChan, + ) if err != nil { t.Fatalf("expected settle to succeed but got %v", err) } @@ -354,7 +359,9 @@ func TestHoldInvoice(t *testing.T) { } // Test idempotency. - event, err = registry.NotifyExitHopHtlc(hash, amtPaid, hodlChan) + event, err = registry.NotifyExitHopHtlc( + hash, amtPaid, testInvoiceExpiry, testCurrentHeight, hodlChan, + ) if err != nil { t.Fatalf("expected settle to succeed but got %v", err) } @@ -434,3 +441,24 @@ func newDB() (*channeldb.DB, func(), error) { return cdb, cleanUp, nil } + +// TestUnknownInvoice tests that invoice registry returns an error when the +// invoice is unknown. This is to guard against returning a cancel hodl event +// for forwarded htlcs. In the link, NotifyExitHopHtlc is only called if we are +// the exit hop, but in htlcIncomingContestResolver it is called with forwarded +// htlc hashes as well. +func TestUnknownInvoice(t *testing.T) { + registry, cleanup := newTestContext(t) + defer cleanup() + + // Notify arrival of a new htlc paying to this invoice. This should + // succeed. + hodlChan := make(chan interface{}) + amt := lnwire.MilliSatoshi(100000) + _, err := registry.NotifyExitHopHtlc( + hash, amt, testInvoiceExpiry, testCurrentHeight, hodlChan, + ) + if err != channeldb.ErrInvoiceNotFound { + t.Fatal("expected invoice not found error") + } +} diff --git a/lnd_multi-hop_htlc_local_chain_claim_test.go b/lnd_multi-hop_htlc_local_chain_claim_test.go index b3453a035..dfea87688 100644 --- a/lnd_multi-hop_htlc_local_chain_claim_test.go +++ b/lnd_multi-hop_htlc_local_chain_claim_test.go @@ -10,7 +10,9 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/lnrpc" + "github.com/lightningnetwork/lnd/lnrpc/invoicesrpc" "github.com/lightningnetwork/lnd/lntest" + "github.com/lightningnetwork/lnd/lntypes" ) // testMultiHopHtlcLocalChainClaim tests that in a multi-hop HTLC scenario, if @@ -24,21 +26,29 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) // Carol refusing to actually settle or directly cancel any HTLC's // self. aliceChanPoint, bobChanPoint, carol := createThreeHopNetwork( - t, net, true, + t, net, false, ) // Clean up carol's node when the test finishes. defer shutdownAndAssert(net, t, carol) - // With the network active, we'll now add a new invoice at Carol's end. - invoiceReq := &lnrpc.Invoice{ - Value: 100000, + // With the network active, we'll now add a new hodl 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 + preimage := lntypes.Preimage{1, 2, 3} + payHash := preimage.Hash() + invoiceReq := &invoicesrpc.AddHoldInvoiceRequest{ + Value: invoiceAmt, CltvExpiry: 40, + Hash: payHash[:], } - ctxt, _ := context.WithTimeout(ctxb, defaultTimeout) - carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq) + ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout) + defer cancel() + carolInvoice, err := carol.AddHoldInvoice(ctxt, invoiceReq) if err != nil { - t.Fatalf("unable to generate carol invoice: %v", err) + t.Fatalf("unable to add invoice: %v", err) } // Now that we've created the invoice, we'll send a single payment from @@ -58,12 +68,12 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) t.Fatalf("unable to send payment: %v", err) } - // We'll now wait until all 3 nodes have the HTLC as just sent fully - // locked in. + // At this point, all 3 nodes should now have an active channel with + // the created HTLC pending on all of them. var predErr error nodes := []*lntest.HarnessNode{net.Alice, net.Bob, carol} err = lntest.WaitPredicate(func() bool { - predErr = assertActiveHtlcs(nodes, carolInvoice.RHash) + predErr = assertActiveHtlcs(nodes, payHash[:]) if predErr != nil { return false } @@ -71,9 +81,14 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) return true }, time.Second*15) if err != nil { - t.Fatalf("htlc mismatch: %v", err) + t.Fatalf("htlc mismatch: %v", predErr) } + // Wait for carol to mark invoice as accepted. There is a small gap to + // bridge between adding the htlc to the channel and executing the exit + // hop logic. + waitForInvoiceAccepted(t, carol, payHash) + // At this point, Bob decides that he wants to exit the channel // immediately, so he force closes his commitment transaction. ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout) @@ -87,6 +102,26 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) err) } + // Suspend Bob to force Carol to go to chain. + restartBob, err := net.SuspendNode(net.Bob) + if err != nil { + t.Fatalf("unable to suspend bob: %v", err) + } + + // Settle invoice. This will just mark the invoice as settled, as there + // is no link anymore to remove the htlc from the commitment tx. For + // this test, it is important to actually settle and not leave the + // invoice in the accepted state, because without a known preimage, the + // channel arbitrator won't go to chain. + ctx, cancel = context.WithTimeout(ctxb, defaultTimeout) + defer cancel() + _, err = carol.SettleInvoice(ctx, &invoicesrpc.SettleInvoiceMsg{ + Preimage: preimage[:], + }) + if err != nil { + t.Fatalf("settle invoice: %v", err) + } + // 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(invoiceReq.CltvExpiry - @@ -129,6 +164,11 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) } assertTxInBlock(t, block, commitHash) + // Restart bob again. + if err := restartBob(); err != nil { + t.Fatalf("unable to restart bob: %v", err) + } + // After the force close transacion is mined, Carol should broadcast // her second level HTLC transacion. Bob will broadcast a sweep tx to // sweep his output in the channel with Carol. He can do this @@ -352,3 +392,30 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest) t.Fatalf(predErr.Error()) } } + +// waitForInvoiceAccepted waits until the specified invoice moved to the +// accepted state by the node. +func waitForInvoiceAccepted(t *harnessTest, node *lntest.HarnessNode, + payHash lntypes.Hash) { + + ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout) + defer cancel() + invoiceUpdates, err := node.SubscribeSingleInvoice(ctx, + &invoicesrpc.SubscribeSingleInvoiceRequest{ + RHash: payHash[:], + }, + ) + if err != nil { + t.Fatalf("subscribe single invoice: %v", err) + } + + for { + update, err := invoiceUpdates.Recv() + if err != nil { + t.Fatalf("invoice update err: %v", err) + } + if update.State == lnrpc.Invoice_ACCEPTED { + break + } + } +} diff --git a/lnd_multi-hop_htlc_receiver_chain_claim_test.go b/lnd_multi-hop_htlc_receiver_chain_claim_test.go index f629be63c..8bdd5ed4d 100644 --- a/lnd_multi-hop_htlc_receiver_chain_claim_test.go +++ b/lnd_multi-hop_htlc_receiver_chain_claim_test.go @@ -11,7 +11,9 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/lnrpc" + "github.com/lightningnetwork/lnd/lnrpc/invoicesrpc" "github.com/lightningnetwork/lnd/lntest" + "github.com/lightningnetwork/lnd/lntypes" ) // testMultiHopReceiverChainClaim tests that in the multi-hop setting, if the @@ -26,24 +28,29 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest) // Carol refusing to actually settle or directly cancel any HTLC's // self. aliceChanPoint, bobChanPoint, carol := createThreeHopNetwork( - t, net, true, + t, net, false, ) // Clean up carol's node when the test finishes. 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. + // With the network active, we'll now add a new hodl 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{ + preimage := lntypes.Preimage{1, 2, 4} + payHash := preimage.Hash() + invoiceReq := &invoicesrpc.AddHoldInvoiceRequest{ Value: invoiceAmt, CltvExpiry: 40, + Hash: payHash[:], } - ctxt, _ := context.WithTimeout(ctxb, defaultTimeout) - carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq) + ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout) + defer cancel() + carolInvoice, err := carol.AddHoldInvoice(ctxt, invoiceReq) if err != nil { - t.Fatalf("unable to generate carol invoice: %v", err) + t.Fatalf("unable to add invoice: %v", err) } // Now that we've created the invoice, we'll send a single payment from @@ -68,7 +75,7 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest) var predErr error nodes := []*lntest.HarnessNode{net.Alice, net.Bob, carol} err = lntest.WaitPredicate(func() bool { - predErr = assertActiveHtlcs(nodes, carolInvoice.RHash) + predErr = assertActiveHtlcs(nodes, payHash[:]) if predErr != nil { return false } @@ -79,6 +86,30 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest) t.Fatalf("htlc mismatch: %v", predErr) } + // Wait for carol to mark invoice as accepted. There is a small gap to + // bridge between adding the htlc to the channel and executing the exit + // hop logic. + waitForInvoiceAccepted(t, carol, payHash) + + restartBob, err := net.SuspendNode(net.Bob) + if err != nil { + t.Fatalf("unable to suspend bob: %v", err) + } + + // Settle invoice. This will just mark the invoice as settled, as there + // is no link anymore to remove the htlc from the commitment tx. For + // this test, it is important to actually settle and not leave the + // invoice in the accepted state, because without a known preimage, the + // channel arbitrator won't go to chain. + ctx, cancel = context.WithTimeout(ctxb, defaultTimeout) + defer cancel() + _, err = carol.SettleInvoice(ctx, &invoicesrpc.SettleInvoiceMsg{ + Preimage: preimage[:], + }) + if err != nil { + t.Fatalf("settle invoice: %v", err) + } + // 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 @@ -123,6 +154,11 @@ func testMultiHopReceiverChainClaim(net *lntest.NetworkHarness, t *harnessTest) // Confirm the commitment. mineBlocks(t, net, 1, 1) + // Restart bob again. + if err := restartBob(); err != nil { + t.Fatalf("unable to restart bob: %v", err) + } + // After the force close transaction is mined, Carol should broadcast // her second level HTLC transaction. Bob will broadcast a sweep tx to // sweep his output in the channel with Carol. When Bob notices Carol's diff --git a/lnd_multi-hop_htlc_remote_chain_claim_test.go b/lnd_multi-hop_htlc_remote_chain_claim_test.go index 7493ead47..cb5bb97f4 100644 --- a/lnd_multi-hop_htlc_remote_chain_claim_test.go +++ b/lnd_multi-hop_htlc_remote_chain_claim_test.go @@ -10,7 +10,9 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/lnrpc" + "github.com/lightningnetwork/lnd/lnrpc/invoicesrpc" "github.com/lightningnetwork/lnd/lntest" + "github.com/lightningnetwork/lnd/lntypes" ) // testMultiHopHtlcRemoteChainClaim tests that in the multi-hop HTLC scenario, @@ -24,22 +26,28 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest // Carol refusing to actually settle or directly cancel any HTLC's // self. aliceChanPoint, bobChanPoint, carol := createThreeHopNetwork( - t, net, true, + t, net, false, ) // Clean up carol's node when the test finishes. defer shutdownAndAssert(net, t, carol) - // With the network active, we'll now add a new invoice at Carol's end. + // With the network active, we'll now add a new hodl 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{ + preimage := lntypes.Preimage{1, 2, 5} + payHash := preimage.Hash() + invoiceReq := &invoicesrpc.AddHoldInvoiceRequest{ Value: invoiceAmt, CltvExpiry: 40, + Hash: payHash[:], } - ctxt, _ := context.WithTimeout(ctxb, defaultTimeout) - carolInvoice, err := carol.AddInvoice(ctxt, invoiceReq) + ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout) + defer cancel() + carolInvoice, err := carol.AddHoldInvoice(ctxt, invoiceReq) if err != nil { - t.Fatalf("unable to generate carol invoice: %v", err) + t.Fatalf("unable to add invoice: %v", err) } // Now that we've created the invoice, we'll send a single payment from @@ -59,12 +67,12 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest t.Fatalf("unable to send payment: %v", err) } - // We'll now wait until all 3 nodes have the HTLC as just sent fully - // locked in. + // At this point, all 3 nodes should now have an active channel with + // the created HTLC pending on all of them. var predErr error nodes := []*lntest.HarnessNode{net.Alice, net.Bob, carol} err = lntest.WaitPredicate(func() bool { - predErr = assertActiveHtlcs(nodes, carolInvoice.RHash) + predErr = assertActiveHtlcs(nodes, payHash[:]) if predErr != nil { return false } @@ -72,9 +80,14 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest return true }, time.Second*15) if err != nil { - t.Fatalf("htlc mismatch: %v", err) + t.Fatalf("htlc mismatch: %v", predErr) } + // Wait for carol to mark invoice as accepted. There is a small gap to + // bridge between adding the htlc to the channel and executing the exit + // hop logic. + waitForInvoiceAccepted(t, carol, payHash) + // Next, Alice decides that she wants to exit the channel, so she'll // immediately force close the channel by broadcast her commitment // transaction. @@ -102,6 +115,26 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest t.Fatalf("unable to find sweeping tx in mempool: %v", err) } + // Suspend bob, so Carol is forced to go on chain. + restartBob, err := net.SuspendNode(net.Bob) + if err != nil { + t.Fatalf("unable to suspend bob: %v", err) + } + + // Settle invoice. This will just mark the invoice as settled, as there + // is no link anymore to remove the htlc from the commitment tx. For + // this test, it is important to actually settle and not leave the + // invoice in the accepted state, because without a known preimage, the + // channel arbitrator won't go to chain. + ctx, cancel = context.WithTimeout(ctxb, defaultTimeout) + defer cancel() + _, err = carol.SettleInvoice(ctx, &invoicesrpc.SettleInvoiceMsg{ + Preimage: preimage[:], + }) + if err != nil { + t.Fatalf("settle invoice: %v", err) + } + // 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(invoiceReq.CltvExpiry- @@ -144,6 +177,11 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest } assertTxInBlock(t, block, commitHash) + // Restart bob again. + if err := restartBob(); err != nil { + t.Fatalf("unable to restart bob: %v", err) + } + // After the force close transacion is mined, Carol should broadcast // her second level HTLC transacion. Bob will broadcast a sweep tx to // sweep his output in the channel with Carol. He can do this diff --git a/peer.go b/peer.go index 4c65290ff..0b36f63a4 100644 --- a/peer.go +++ b/peer.go @@ -196,10 +196,6 @@ type peer struct { // remote node. localFeatures *lnwire.RawFeatureVector - // 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 @@ -242,7 +238,7 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server, addr *lnwire.NetAddress, inbound bool, localFeatures *lnwire.RawFeatureVector, chanActiveTimeout time.Duration, - finalCltvRejectDelta, outgoingCltvRejectDelta uint32) ( + outgoingCltvRejectDelta uint32) ( *peer, error) { nodePub := addr.IdentityKey @@ -258,7 +254,6 @@ func newPeer(conn net.Conn, connReq *connmgr.ConnReq, server *server, localFeatures: localFeatures, - finalCltvRejectDelta: finalCltvRejectDelta, outgoingCltvRejectDelta: outgoingCltvRejectDelta, sendQueue: make(chan outgoingMsg), @@ -598,7 +593,6 @@ func (p *peer) addLink(chanPoint *wire.OutPoint, UnsafeReplay: cfg.UnsafeReplay, MinFeeUpdateTimeout: htlcswitch.DefaultMinLinkFeeUpdateTimeout, MaxFeeUpdateTimeout: htlcswitch.DefaultMaxLinkFeeUpdateTimeout, - FinalCltvRejectDelta: p.finalCltvRejectDelta, OutgoingCltvRejectDelta: p.outgoingCltvRejectDelta, } diff --git a/server.go b/server.go index 29a6b5415..72cbb1e3f 100644 --- a/server.go +++ b/server.go @@ -342,7 +342,10 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, cc *chainControl, readPool: readPool, chansToRestore: chansToRestore, - invoices: invoices.NewRegistry(chanDB, decodeFinalCltvExpiry), + invoices: invoices.NewRegistry( + chanDB, decodeFinalCltvExpiry, + defaultFinalCltvRejectDelta, + ), channelNotifier: channelnotifier.New(chanDB), @@ -2556,7 +2559,6 @@ func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq, p, err := newPeer( conn, connReq, s, peerAddr, inbound, localFeatures, cfg.ChanEnableTimeout, - defaultFinalCltvRejectDelta, defaultOutgoingCltvRejectDelta, ) if err != nil {