From 8291e8a170010c795cbceff0e2b80c7f9740b40f Mon Sep 17 00:00:00 2001 From: Tommy Volk Date: Thu, 21 Apr 2022 07:33:01 +0000 Subject: [PATCH 1/3] multi: add `keep-failed-payment-attempts` flag --- channeldb/db.go | 14 ++++++++------ channeldb/options.go | 12 ++++++++++++ config.go | 35 ++++++++++++++++++++-------------- config_builder.go | 1 + routing/control_tower_test.go | 36 +++++++++++++++++++++++------------ routing/payment_lifecycle.go | 4 ++-- sample-lnd.conf | 4 ++++ 7 files changed, 72 insertions(+), 34 deletions(-) diff --git a/channeldb/db.go b/channeldb/db.go index 8a6935941..ac0ba9642 100644 --- a/channeldb/db.go +++ b/channeldb/db.go @@ -247,10 +247,11 @@ type DB struct { // channelStateDB separates all DB operations on channel state. channelStateDB *ChannelStateDB - dbPath string - graph *ChannelGraph - clock clock.Clock - dryRun bool + dbPath string + graph *ChannelGraph + clock clock.Clock + dryRun bool + keepFailedPaymentAttempts bool } // Open opens or creates channeldb. Any necessary schemas migrations due @@ -303,8 +304,9 @@ func CreateWithBackend(backend kvdb.Backend, modifiers ...OptionModifier) (*DB, }, backend: backend, }, - clock: opts.clock, - dryRun: opts.dryRun, + clock: opts.clock, + dryRun: opts.dryRun, + keepFailedPaymentAttempts: opts.keepFailedPaymentAttempts, } // Set the parent pointer (only used in tests). diff --git a/channeldb/options.go b/channeldb/options.go index 068a7fa01..7e121d254 100644 --- a/channeldb/options.go +++ b/channeldb/options.go @@ -61,6 +61,10 @@ type Options struct { // dryRun will fail to commit a successful migration when opening the // database if set to true. dryRun bool + + // keepFailedPaymentAttempts determines whether failed htlc attempts + // are kept on disk or removed to save space. + keepFailedPaymentAttempts bool } // DefaultOptions returns an Options populated with default values. @@ -164,3 +168,11 @@ func OptionDryRunMigration(dryRun bool) OptionModifier { o.dryRun = dryRun } } + +// OptionKeepFailedPaymentAttempts controls whether failed payment attempts are +// kept on disk after a payment settles. +func OptionKeepFailedPaymentAttempts(keepFailedPaymentAttempts bool) OptionModifier { + return func(o *Options) { + o.keepFailedPaymentAttempts = keepFailedPaymentAttempts + } +} diff --git a/config.go b/config.go index d313033a6..71044e898 100644 --- a/config.go +++ b/config.go @@ -199,6 +199,10 @@ const ( // defaultCoinSelectionStrategy is the coin selection strategy that is // used by default to fund transactions. defaultCoinSelectionStrategy = "largest" + + // defaultKeepFailedPaymentAttempts is the default setting for whether + // to keep failed payments in the database. + defaultKeepFailedPaymentAttempts = false ) var ( @@ -362,6 +366,8 @@ type Config struct { ChannelCommitBatchSize uint32 `long:"channel-commit-batch-size" description:"The maximum number of channel state updates that is accumulated before signing a new commitment."` + KeepFailedPaymentAttempts bool `long:"keep-failed-payment-attempts" description:"Keeps persistent record of all failed payment attempts for successfully settled payments."` + DefaultRemoteMaxHtlcs uint16 `long:"default-remote-max-htlcs" description:"The default max_htlc applied when opening or accepting channels. This value limits the number of concurrent HTLCs that the remote party can add to the commitment. The maximum possible value is 483."` NumGraphSyncPeers int `long:"numgraphsyncpeers" description:"The number of peers that we should receive new graph updates from. This option can be tuned to save bandwidth for light clients or routing nodes."` @@ -611,20 +617,21 @@ func DefaultConfig() Config { Invoices: &lncfg.Invoices{ HoldExpiryDelta: lncfg.DefaultHoldInvoiceExpiryDelta, }, - MaxOutgoingCltvExpiry: htlcswitch.DefaultMaxOutgoingCltvExpiry, - MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation, - MaxCommitFeeRateAnchors: lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte, - DustThreshold: uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()), - LogWriter: build.NewRotatingLogWriter(), - DB: lncfg.DefaultDB(), - Cluster: lncfg.DefaultCluster(), - RPCMiddleware: lncfg.DefaultRPCMiddleware(), - registeredChains: chainreg.NewChainRegistry(), - ActiveNetParams: chainreg.BitcoinTestNetParams, - ChannelCommitInterval: defaultChannelCommitInterval, - PendingCommitInterval: defaultPendingCommitInterval, - ChannelCommitBatchSize: defaultChannelCommitBatchSize, - CoinSelectionStrategy: defaultCoinSelectionStrategy, + MaxOutgoingCltvExpiry: htlcswitch.DefaultMaxOutgoingCltvExpiry, + MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation, + MaxCommitFeeRateAnchors: lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte, + DustThreshold: uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()), + LogWriter: build.NewRotatingLogWriter(), + DB: lncfg.DefaultDB(), + Cluster: lncfg.DefaultCluster(), + RPCMiddleware: lncfg.DefaultRPCMiddleware(), + registeredChains: chainreg.NewChainRegistry(), + ActiveNetParams: chainreg.BitcoinTestNetParams, + ChannelCommitInterval: defaultChannelCommitInterval, + PendingCommitInterval: defaultPendingCommitInterval, + ChannelCommitBatchSize: defaultChannelCommitBatchSize, + CoinSelectionStrategy: defaultCoinSelectionStrategy, + KeepFailedPaymentAttempts: defaultKeepFailedPaymentAttempts, RemoteSigner: &lncfg.RemoteSigner{ Timeout: lncfg.DefaultRemoteSignerRPCTimeout, }, diff --git a/config_builder.go b/config_builder.go index 18e9a3f6f..05f8ab168 100644 --- a/config_builder.go +++ b/config_builder.go @@ -852,6 +852,7 @@ func (d *DefaultDatabaseBuilder) BuildDatabase( channeldb.OptionSetBatchCommitInterval(cfg.DB.BatchCommitInterval), channeldb.OptionDryRunMigration(cfg.DryRunMigration), channeldb.OptionSetUseGraphCache(!cfg.DB.NoGraphCache), + channeldb.OptionKeepFailedPaymentAttempts(cfg.KeepFailedPaymentAttempts), } // We want to pre-allocate the channel graph cache according to what we diff --git a/routing/control_tower_test.go b/routing/control_tower_test.go index e24870c87..267e60505 100644 --- a/routing/control_tower_test.go +++ b/routing/control_tower_test.go @@ -48,7 +48,7 @@ var ( func TestControlTowerSubscribeUnknown(t *testing.T) { t.Parallel() - db, err := initDB() + db, err := initDB(false) require.NoError(t, err, "unable to init db") pControl := NewControlTower(channeldb.NewPaymentControl(db)) @@ -65,7 +65,7 @@ func TestControlTowerSubscribeUnknown(t *testing.T) { func TestControlTowerSubscribeSuccess(t *testing.T) { t.Parallel() - db, err := initDB() + db, err := initDB(false) require.NoError(t, err, "unable to init db") pControl := NewControlTower(channeldb.NewPaymentControl(db)) @@ -165,16 +165,24 @@ func TestControlTowerSubscribeSuccess(t *testing.T) { func TestPaymentControlSubscribeFail(t *testing.T) { t.Parallel() - t.Run("register attempt", func(t *testing.T) { - testPaymentControlSubscribeFail(t, true) + t.Run("register attempt, keep failed payments", func(t *testing.T) { + testPaymentControlSubscribeFail(t, true, true) }) - t.Run("no register attempt", func(t *testing.T) { - testPaymentControlSubscribeFail(t, false) + t.Run("register attempt, delete failed payments", func(t *testing.T) { + testPaymentControlSubscribeFail(t, true, false) + }) + t.Run("no register attempt, keep failed payments", func(t *testing.T) { + testPaymentControlSubscribeFail(t, false, true) + }) + t.Run("no register attempt, delete failed payments", func(t *testing.T) { + testPaymentControlSubscribeFail(t, false, false) }) } -func testPaymentControlSubscribeFail(t *testing.T, registerAttempt bool) { - db, err := initDB() +func testPaymentControlSubscribeFail(t *testing.T, registerAttempt, + keepFailedPaymentAttempts bool) { + + db, err := initDB(keepFailedPaymentAttempts) require.NoError(t, err, "unable to init db") pControl := NewControlTower(channeldb.NewPaymentControl(db)) @@ -228,8 +236,8 @@ func testPaymentControlSubscribeFail(t *testing.T, registerAttempt bool) { subscriber2, err := pControl.SubscribePayment(info.PaymentIdentifier) require.NoError(t, err, "expected subscribe to succeed, but got") - // We expect all subscribers to now report the final outcome followed by - // no other events. + // We expect both subscribers to now report the final outcome followed + // by no other events. subscribers := []*ControlTowerSubscriber{ subscriber1, subscriber2, } @@ -286,13 +294,17 @@ func testPaymentControlSubscribeFail(t *testing.T, registerAttempt bool) { } } -func initDB() (*channeldb.DB, error) { +func initDB(keepFailedPaymentAttempts bool) (*channeldb.DB, error) { tempPath, err := ioutil.TempDir("", "routingdb") if err != nil { return nil, err } - db, err := channeldb.Open(tempPath) + db, err := channeldb.Open( + tempPath, channeldb.OptionKeepFailedPaymentAttempts( + keepFailedPaymentAttempts, + ), + ) if err != nil { return nil, err } diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index f8b3cd558..b2022cf2f 100644 --- a/routing/payment_lifecycle.go +++ b/routing/payment_lifecycle.go @@ -55,7 +55,7 @@ func (ps paymentState) terminated() bool { } // needWaitForShards returns a bool to specify whether we need to wait for the -// outcome of the shanrdHandler. +// outcome of the shardHandler. func (ps paymentState) needWaitForShards() bool { // If we have in flight shards and the payment is in final stage, we // need to wait for the outcomes from the shards. Or if we have no more @@ -279,7 +279,7 @@ lifecycle: continue lifecycle } - // If this route will consume the last remeining amount to send + // If this route will consume the last remaining amount to send // to the receiver, this will be our last shard (for now). lastShard := rt.ReceiverAmt() == currentState.remainingAmt diff --git a/sample-lnd.conf b/sample-lnd.conf index 114ad5c19..5685f8275 100644 --- a/sample-lnd.conf +++ b/sample-lnd.conf @@ -311,6 +311,10 @@ ; a new commitment. ; channel-commit-batch-size=10 +; Keeps persistent record of all failed payment attempts for successfully +; settled payments. +; keep-failed-payment-attempts=false + ; The default max_htlc applied when opening or accepting channels. This value ; limits the number of concurrent HTLCs that the remote party can add to the ; commitment. The maximum possible value is 483. From 2dd11ed24964a0e94815b1270ca0d63257d0e298 Mon Sep 17 00:00:00 2001 From: Tommy Volk Date: Sun, 12 Jun 2022 04:18:07 +0000 Subject: [PATCH 2/3] channeldb+routing: htlcs are pruned on settle --- channeldb/payment_control.go | 13 +++++ channeldb/payment_control_test.go | 96 ++++++++++++++++++++++++++++++- routing/control_tower.go | 11 ++++ routing/mock_test.go | 31 ++++++++++ routing/payment_lifecycle.go | 16 +++++- routing/router_test.go | 2 + 6 files changed, 164 insertions(+), 5 deletions(-) diff --git a/channeldb/payment_control.go b/channeldb/payment_control.go index 4ea0a9545..7bd00e4d4 100644 --- a/channeldb/payment_control.go +++ b/channeldb/payment_control.go @@ -223,6 +223,19 @@ func (p *PaymentControl) InitPayment(paymentHash lntypes.Hash, return updateErr } +// DeleteFailedAttempts deletes all failed htlcs for a payment if configured +// by the PaymentControl db. +func (p *PaymentControl) DeleteFailedAttempts(hash lntypes.Hash) error { + if !p.db.keepFailedPaymentAttempts { + const failedHtlcsOnly = true + err := p.db.DeletePayment(hash, failedHtlcsOnly) + if err != nil { + return err + } + } + return nil +} + // paymentIndexTypeHash is a payment index type which indicates that we have // created an index of payment sequence number to payment hash. type paymentIndexType uint8 diff --git a/channeldb/payment_control_test.go b/channeldb/payment_control_test.go index c79151cb2..0417238b7 100644 --- a/channeldb/payment_control_test.go +++ b/channeldb/payment_control_test.go @@ -308,7 +308,7 @@ func TestPaymentControlFailsWithoutInFlight(t *testing.T) { // TestPaymentControlDeleteNonInFlight checks that calling DeletePayments only // deletes payments from the database that are not in-flight. -func TestPaymentControlDeleteNonInFligt(t *testing.T) { +func TestPaymentControlDeleteNonInFlight(t *testing.T) { t.Parallel() db, cleanup, err := MakeTestDB() @@ -528,7 +528,7 @@ func TestPaymentControlDeletePayments(t *testing.T) { // Register three payments: // 1. A payment with two failed attempts. - // 2. A Payment with one failed and one settled attempt. + // 2. A payment with one failed and one settled attempt. // 3. A payment with one failed and one in-flight attempt. payments := []*payment{ {status: StatusFailed}, @@ -585,7 +585,7 @@ func TestPaymentControlDeleteSinglePayment(t *testing.T) { // according to its final status. // 1. A payment with two failed attempts. // 2. Another payment with two failed attempts. - // 3. A Payment with one failed and one settled attempt. + // 3. A payment with one failed and one settled attempt. // 4. A payment with one failed and one in-flight attempt. // Initiate payments, which is a slice of payment that is used as @@ -1003,6 +1003,96 @@ func TestPaymentControlMPPRecordValidation(t *testing.T) { } } +// TestDeleteFailedAttempts checks that DeleteFailedAttempts properly removes +// failed HTLCs from finished payments. +func TestDeleteFailedAttempts(t *testing.T) { + t.Parallel() + + t.Run("keep failed payment attempts", func(t *testing.T) { + testDeleteFailedAttempts(t, true) + }) + t.Run("remove failed payment attempts", func(t *testing.T) { + testDeleteFailedAttempts(t, false) + }) +} + +func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) { + db, cleanup, err := MakeTestDB() + defer cleanup() + + require.NoError(t, err, "unable to init db") + db.keepFailedPaymentAttempts = keepFailedPaymentAttempts + + pControl := NewPaymentControl(db) + + // Register three payments: + // All payments will have one failed HTLC attempt and one HTLC attempt + // according to its final status. + // 1. A payment with two failed attempts. + // 2. A payment with one failed and one in-flight attempt. + // 3. A payment with one failed and one settled attempt. + + // Initiate payments, which is a slice of payment that is used as + // template to create the corresponding test payments in the database. + // + // Note: The payment id and number of htlc attempts of each payment will + // be added to this slice when creating the payments below. + // This allows the slice to be used directly for testing purposes. + payments := []*payment{ + {status: StatusFailed}, + {status: StatusInFlight}, + {status: StatusSucceeded}, + } + + // Use helper function to register the test payments in the data and + // populate the data to the payments slice. + createTestPayments(t, pControl, payments) + + // Check that all payments are there as we added them. + assertPayments(t, db, payments) + + // Calling DeleteFailedAttempts on a failed payment should delete all + // HTLCs. + require.NoError(t, pControl.DeleteFailedAttempts(payments[0].id)) + + // Expect all HTLCs to be deleted if the config is set to delete them. + if !keepFailedPaymentAttempts { + payments[0].htlcs = 0 + } + assertPayments(t, db, payments) + + // Calling DeleteFailedAttempts on an in-flight payment should return + // an error. + if keepFailedPaymentAttempts { + require.NoError(t, pControl.DeleteFailedAttempts(payments[1].id)) + } else { + require.Error(t, pControl.DeleteFailedAttempts(payments[1].id)) + } + + // Since DeleteFailedAttempts returned an error, we should expect the + // payment to be unchanged. + assertPayments(t, db, payments) + + // Cleaning up a successful payment should remove failed htlcs. + require.NoError(t, pControl.DeleteFailedAttempts(payments[2].id)) + // Expect all HTLCs except for the settled one to be deleted if the + // config is set to delete them. + if !keepFailedPaymentAttempts { + payments[2].htlcs = 1 + } + assertPayments(t, db, payments) + + if keepFailedPaymentAttempts { + // DeleteFailedAttempts is ignored, even for non-existent + // payments, if the control tower is configured to keep failed + // HTLCs. + require.NoError(t, pControl.DeleteFailedAttempts(lntypes.ZeroHash)) + } else { + // Attempting to cleanup a non-existent payment returns an error. + require.Error(t, pControl.DeleteFailedAttempts(lntypes.ZeroHash)) + } +} + // assertPaymentStatus retrieves the status of the payment referred to by hash // and compares it with the expected state. func assertPaymentStatus(t *testing.T, p *PaymentControl, diff --git a/routing/control_tower.go b/routing/control_tower.go index 19b5ab504..be6e61b4e 100644 --- a/routing/control_tower.go +++ b/routing/control_tower.go @@ -19,6 +19,11 @@ type ControlTower interface { // hash. InitPayment(lntypes.Hash, *channeldb.PaymentCreationInfo) error + // DeleteFailedAttempts removes all failed HTLCs from the db. It should + // be called for a given payment whenever all inflight htlcs are + // completed, and the payment has reached a final settled state. + DeleteFailedAttempts(lntypes.Hash) error + // RegisterAttempt atomically records the provided HTLCAttemptInfo. RegisterAttempt(lntypes.Hash, *channeldb.HTLCAttemptInfo) error @@ -125,6 +130,12 @@ func (p *controlTower) InitPayment(paymentHash lntypes.Hash, return p.db.InitPayment(paymentHash, info) } +// DeleteFailedAttempts deletes all failed htlcs if the payment was +// successfully settled. +func (p *controlTower) DeleteFailedAttempts(paymentHash lntypes.Hash) error { + return p.db.DeleteFailedAttempts(paymentHash) +} + // RegisterAttempt atomically records the provided HTLCAttemptInfo to the // DB. func (p *controlTower) RegisterAttempt(paymentHash lntypes.Hash, diff --git a/routing/mock_test.go b/routing/mock_test.go index 1bc09a5a9..67f6fa432 100644 --- a/routing/mock_test.go +++ b/routing/mock_test.go @@ -309,6 +309,32 @@ func (m *mockControlTowerOld) InitPayment(phash lntypes.Hash, return nil } +func (m *mockControlTowerOld) DeleteFailedAttempts(phash lntypes.Hash) error { + p, ok := m.payments[phash] + if !ok { + return channeldb.ErrPaymentNotInitiated + } + + var inFlight bool + for _, a := range p.attempts { + if a.Settle != nil { + continue + } + + if a.Failure != nil { + continue + } + + inFlight = true + } + + if inFlight { + return channeldb.ErrPaymentInFlight + } + + return nil +} + func (m *mockControlTowerOld) RegisterAttempt(phash lntypes.Hash, a *channeldb.HTLCAttemptInfo) error { @@ -665,6 +691,11 @@ func (m *mockControlTower) InitPayment(phash lntypes.Hash, return args.Error(0) } +func (m *mockControlTower) DeleteFailedAttempts(phash lntypes.Hash) error { + args := m.Called(phash) + return args.Error(0) +} + func (m *mockControlTower) RegisterAttempt(phash lntypes.Hash, a *channeldb.HTLCAttemptInfo) error { diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index b2022cf2f..36c63eb14 100644 --- a/routing/payment_lifecycle.go +++ b/routing/payment_lifecycle.go @@ -186,9 +186,21 @@ lifecycle: // Find the first successful shard and return // the preimage and route. for _, a := range payment.HTLCs { - if a.Settle != nil { - return a.Settle.Preimage, &a.Route, nil + if a.Settle == nil { + continue } + + err := p.router.cfg.Control. + DeleteFailedAttempts( + p.identifier) + if err != nil { + log.Errorf("Error deleting failed "+ + "payment attempts for "+ + "payment %v: %v", p.identifier, + err) + } + + return a.Settle.Preimage, &a.Route, nil } // Payment failed. diff --git a/routing/router_test.go b/routing/router_test.go index 343348a02..4ffee7be7 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -3571,6 +3571,7 @@ func TestSendMPPaymentSucceed(t *testing.T) { } } }) + controlTower.On("DeleteFailedAttempts", identifier).Return(nil) // Call the actual method SendPayment on router. This is place inside a // goroutine so we can set a timeout for the whole test, in case @@ -3782,6 +3783,7 @@ func TestSendMPPaymentSucceedOnExtraShards(t *testing.T) { return } }) + controlTower.On("DeleteFailedAttempts", identifier).Return(nil) // Call the actual method SendPayment on router. This is place inside a // goroutine so we can set a timeout for the whole test, in case From 714f8a81420921e8b6861eea5b8bc22643e36986 Mon Sep 17 00:00:00 2001 From: Tommy Volk Date: Thu, 21 Apr 2022 08:02:44 +0000 Subject: [PATCH 3/3] gitrelease-notes: update release notes for 0.15.1 --- docs/release-notes/release-notes-0.15.1.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/release-notes/release-notes-0.15.1.md b/docs/release-notes/release-notes-0.15.1.md index 4c3608d51..b97848fd9 100644 --- a/docs/release-notes/release-notes-0.15.1.md +++ b/docs/release-notes/release-notes-0.15.1.md @@ -16,6 +16,11 @@ addholdinvoice`](https://github.com/lightningnetwork/lnd/pull/6577). Users now need to explicitly specify the `--private` flag. +## Database + +* [Delete failed payment attempts](https://github.com/lightningnetwork/lnd/pull/6438) + once payments are settled, unless specified with `keep-failed-payment-attempts` flag. + ## Documentation * [Add minor comment](https://github.com/lightningnetwork/lnd/pull/6559) on @@ -57,4 +62,5 @@ * Eugene Siegel * Oliver Gugger * Priyansh Rastogi +* Tommy Volk * Yong Yu