Control Tower: Sender-side checks for duplicate payments #2 by cfromknecht · Pull Request #1719 · lightningnetwork/lnd

cfromknecht · 2018-08-10T21:54:19Z

This PR is an extension of #1070, mostly just touching up documentation. The only thing that is new in the PR is the reordering of logic w/in handleLocalDispatch, which should result in better clean up of circuits and fwd references. This was going to be done in a separate PR, but the changes were overlapping enough that I made them here.

FIxes #973.

cfromknecht · 2018-08-14T02:20:03Z

@vapopov rebased w/ split commits

cfromknecht · 2018-08-14T02:42:12Z

I've also added a strict flag to the current ControlTower implementation. This is meant to be more tolerant of payments that could already have duplicate payments in flight. The stricter state machine would cause issues if enabled immediately, because there could be db's in such a state with duplicate payments. It also means that we don't need to inspect the circuit map during migration, which would require a bit of a code move to be able to parse and find the payment hashes of already in flight payments.

In the future, we can default to the stricter state machine once we are confident that old payment states have been resolved/cleanedup.

cfromknecht · 2018-08-15T20:21:11Z

Correcting what I said earlier, I did end up making changes that inspect the circuit map and mark all locally sourced payment hashes as inflight

cfromknecht · 2018-08-21T04:29:39Z

Noticed that the performance hit of doing serial state transitions caused the async_payment_benchmark to timeout. In response, three commits have been added, which:

reduces the number of db calls from 2 to 1 for each state transition
removes the ControlTower's exclusive lock, so that we can utilize bolt's Batch txns
made all handling of local response asynchronous, so that db transactions don't affect the critical path of the switch's primary event loop

Roasbeef

Moving onto testing this as is locally (the latest set of the diff). Will do one final pass over the logic in the switch to ensure things are being updated properly in a few edge cases.

Roasbeef · 2018-08-21T04:47:28Z

Nice! I forget at times that we have the infra to test the pre and post migration state.

@vapopov's idea :)

Roasbeef · 2018-08-21T04:49:06Z

Could use a comment here explaining what "strict" mode entails.

for Settle/Fail responses.

by reading the payment hash from the circuit map.

in the circuit map are marked StatusInFlight. We also check that hashes contained in forwarded circuits are not updated.

This commit splits FetchPaymentStatus and UpdatePaymentStatus, such that they each invoke helper methods that can be composed into different db txns. This enables us to improve performance on send/receive, as we can remove the exclusive lock from the control tower, and allow concurrent calls to utilize Batch more effectively.

Composes the new payment status helper methods such that we only require one db txn per state transition. This also allows us to remove the exclusive lock from the control tower, and enable more concurrent requests.

This commit moves the logic handling responses to locally-initiated payments to be asynchronous. The reordering of operations into handleLocalDispatch brings a serious performance burden to the switch's main event loop. However, the at-most once semantics of circuit map and idempotency of cleanup methods allows concurrent operations to run in parallel. Prior to this commit, the async_payments_benchmark would timeout due to the forcibly serial nature of the prior design. With this change, there is no perceptible difference in the benchmark OMM, even though we've added two extra db calls.

Roasbeef

LGTM 🎩

Tested on both mainnet and testnet!

Roasbeef · 2018-08-23T04:18:40Z

+	// If the provided payment is nil, we have discarded the error decryptor
+	// due to a restart. We'll return a fixed error and signal a temporary
+	// channel failure to the router.
+	case payment == nil:


cfromknecht · 2018-08-23T04:33:42Z

Great work on this @vapopov 🎉

cfromknecht mentioned this pull request Aug 11, 2018

payments+switch: add sender-side check to not pay same preimage twice #1070

Closed

cfromknecht force-pushed the duplicate-payments branch from c03cd00 to f8b8f65 Compare August 14, 2018 02:11

cfromknecht force-pushed the duplicate-payments branch from 8bc06ae to 9e0ac3c Compare August 14, 2018 03:56

Roasbeef added enhancement Improvements to existing features / behaviour payments Related to invoices/payments htlcswitch P2 should be fixed if one has time bug fix labels Aug 14, 2018

Roasbeef added this to the 0.5 milestone Aug 15, 2018

cfromknecht force-pushed the duplicate-payments branch 4 times, most recently from 0f00102 to a1619cd Compare August 21, 2018 04:27

cfromknecht force-pushed the duplicate-payments branch from a1619cd to c86cc22 Compare August 21, 2018 04:45

Roasbeef reviewed Aug 21, 2018

View reviewed changes

cfromknecht force-pushed the duplicate-payments branch 3 times, most recently from e2d9c36 to c945002 Compare August 22, 2018 02:12

vapopov added 8 commits August 21, 2018 19:23

channeldb: add payment statuses: ground, in flight, completed

7296cfb

channeldb: test of payment statuses transitions

7f6fbd4

channeldb: add migration for payment statuses for completed payments

f405376

channeldb: payment statuses migration test

24e3310

channeldb: applying payment statuses migration to current database

41e31e0

htlcswitch: control tower implementation

a3d32be

htlcswitch: control tower test of payment statuses transitions

3502877

htlcswitch: integration of control tower to htlc switch

21fc7aa

vapopov and others added 20 commits August 21, 2018 19:23

lnd: add integration test checks unavailability to pay same preimage

76dbe67

channeldb/db: add comment describing migration

79a4203

channeldb/db_test: move migration helper back to meta_test

090e97c

channeldb/meta_test: restore migration helper + godocs

9f46727

channeldb/migrations: touch up documentation

93e5f9a

channeldb/migrations_test: touch up docs

3a579f3

channeldb/payments: touch up docs

e7c0f4c

htlcswitch/switch: reorder persistent calls and app ntfn...

d6083e0

for Settle/Fail responses.

htlcswitch/mock: remove new line to satisfy linter

9b52c51

htlcswitch/switch_control: expand godocs and add ErrUnknownPaymentStatus

8751285

htlcswitch/switch_control_test: extend tests + godocs

3f0dfd4

htlcswitch/control_tower: move from switch_control

8b2237f

htlcswitch/control_tower_test: move from switch_control_test

971ae3c

htlcswitch/control_tower: add strict mode toggling

98d2ffb

htlcswitch/control_tower_test: test strict and non-strict ctrltwrs

2027444

htlcswitch/switch: use non-strict PaymentControl

2dd8f07

channeldb/migrations: mark locally-sourced payments as InFlight...

11bb568

by reading the payment hash from the circuit map.

channeldb/migrations_test: assert locally-sourced circuits...

0865ac7

in the circuit map are marked StatusInFlight. We also check that hashes contained in forwarded circuits are not updated.

htlcswitch/control_tower: use one db txn for transitions

5dc2a4a

Composes the new payment status helper methods such that we only require one db txn per state transition. This also allows us to remove the exclusive lock from the control tower, and enable more concurrent requests.

cfromknecht force-pushed the duplicate-payments branch from c945002 to 0b6f8bb Compare August 22, 2018 02:58

cfromknecht added 3 commits August 22, 2018 10:26

lnd_test: satisfy linter alignment

80814cf

htlcswitch/switch: prevent panic for unknown error decryptor

9c5c1d0

cfromknecht force-pushed the duplicate-payments branch from 0b6f8bb to 9c5c1d0 Compare August 22, 2018 17:26

Roasbeef approved these changes Aug 23, 2018

View reviewed changes

Roasbeef merged commit 4f20905 into lightningnetwork:master Aug 23, 2018

cfromknecht mentioned this pull request Aug 23, 2018

lnd should detect and refuse double payment / invoice spending #1353

Closed

bitromortac mentioned this pull request Mar 10, 2020

channeldb+lnrpc+lncli: listpayments pagination support #3960

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control Tower: Sender-side checks for duplicate payments #2#1719

Control Tower: Sender-side checks for duplicate payments #2#1719
Roasbeef merged 32 commits into
lightningnetwork:masterfrom
cfromknecht:duplicate-payments

cfromknecht commented Aug 10, 2018 •

edited by Roasbeef

Loading

Uh oh!

cfromknecht commented Aug 14, 2018

Uh oh!

cfromknecht commented Aug 14, 2018

Uh oh!

cfromknecht commented Aug 15, 2018

Uh oh!

cfromknecht commented Aug 21, 2018

Uh oh!

Roasbeef left a comment

Uh oh!

Roasbeef Aug 21, 2018

Uh oh!

cfromknecht Aug 21, 2018

Uh oh!

Roasbeef Aug 21, 2018

Uh oh!

cfromknecht Aug 21, 2018

Uh oh!

Roasbeef left a comment

Uh oh!

Roasbeef Aug 23, 2018

Uh oh!

cfromknecht commented Aug 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cfromknecht commented Aug 10, 2018 • edited by Roasbeef Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfromknecht commented Aug 14, 2018

Uh oh!

cfromknecht commented Aug 14, 2018

Uh oh!

cfromknecht commented Aug 15, 2018

Uh oh!

cfromknecht commented Aug 21, 2018

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

cfromknecht Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

Roasbeef Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

cfromknecht Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

cfromknecht commented Aug 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cfromknecht commented Aug 10, 2018 •

edited by Roasbeef

Loading