Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

channeldb+cnct+invoices: track invoice htlcs #3390

Merged
merged 16 commits into from Sep 5, 2019

Conversation

@joostjager
Copy link
Collaborator

commented Aug 9, 2019

This PR modifies invoice registry so that it tracks the individual htlcs paying to an invoice. This leads to the following improvements:

  • Report the amount paid to an invoice correctly. Previously only the amount of the first HTLC was reported. With this PR, that is changed to the sum of all settled HTLCs.

  • Report channel through which an invoice was paid.

  • Consistent HTLC acceptance for hodl invoices. Previously the HTLC expiry check was skipped in certain situations which could lead to channel closure.

  • More generally, this PR adds the fine-grained HTLC control that is required for multi path payments.

NOTE: The migration of invoices in this pr cannot fill in the invoice htlcs for existing invoices. If there are invoices in the accepted state during the upgrade, the held htlcs may be released on startup. This risk can be eliminated by ensuring there are no invoices in the accepted state.

Fixes #3169

Fixes #2208

@joostjager joostjager requested review from cfromknecht and Roasbeef as code owners Aug 9, 2019
@joostjager joostjager removed request for Roasbeef and cfromknecht Aug 9, 2019
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch 3 times, most recently from 323e1a4 to 3bc173c Aug 9, 2019
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch 4 times, most recently from 1f2610f to c3ada97 Aug 9, 2019
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch 2 times, most recently from 085d358 to 152f4a3 Aug 14, 2019
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch from 152f4a3 to c62061a Aug 19, 2019
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch from c62061a to 927a4a8 Aug 19, 2019
@joostjager joostjager requested a review from cfromknecht Aug 19, 2019
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch 2 times, most recently from e005e22 to 1267758 Aug 19, 2019
@joostjager joostjager changed the title channeldb+cnct+invoices: track invoice htlcs [wip] channeldb+cnct+invoices: track invoice htlcs Aug 19, 2019
@wpaulino wpaulino added this to the 0.8.0 milestone Aug 19, 2019
channeldb/invoices.go Show resolved Hide resolved
contractcourt/channel_arbitrator.go Show resolved Hide resolved
contractcourt/channel_arbitrator.go Show resolved Hide resolved
channeldb/invoices.go Show resolved Hide resolved
channeldb/invoices.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
channeldb/invoices.go Outdated Show resolved Hide resolved
contractcourt/channel_arbitrator.go Show resolved Hide resolved
invoices/invoiceregistry.go Show resolved Hide resolved
channeldb/invoices.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch from 1267758 to db53cb1 Aug 20, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

While working on mpp and with amp in mind, I already saw the extra invoice htlc fields coming. Added a commit fixup! channeldb+invoices: add invoice htlcs that converts the serialization to a tlv stream. It is unfortunate that the format has changed twice, but probably still better than a new migration.

@joostjager joostjager force-pushed the joostjager:invoice-circuits branch 3 times, most recently from 0d6bd4d to 445d9ff Sep 3, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

Also modified old migration calls to serializeInvoice and deserializeInvoice in migrations.go to the legacy version.

Migrations that aren't fully self-contained are dangerous.

@joostjager joostjager force-pushed the joostjager:invoice-circuits branch from 445d9ff to b8d8389 Sep 4, 2019
Copy link
Collaborator

left a comment

LGTM excited to start using tlv in our database, will make any future "migrations" to invoice htlcs straightforward :D

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

(still a stray fixup commit btw)

joostjager added 16 commits Aug 12, 2019
Previously a check was made for accepted and settled invoices against
the paid amount. This opens up a probe vector where an attacker can pay
to an invoice with an amt that is higher than the invoice amount and
find out if the invoice is already paid or not.
Add logic to specifically exercise the replay behavior of invoice
registry for hodl invoices.
Currently the invoice registry cannot tell apart the htlcs that pay to
an invoice. Because htlcs may also be replayed on startup, it isn't
possible to determine the total amount paid to an invoice.

This commit is a first step towards fixing that. It reports the circuit
keys of htlcs to the invoice registry, which forms the basis for
accurate invoice accounting.
This commit adds a set of htlcs to the Invoice struct and
serializes/deserializes this set to/from disk. It is a preparation for
accurate invoice accounting across restarts of lnd.

A migration is added for the invoice htlcs.

In addition to these changes, separate final cltv delta and expiry
invoice fields are created and populated. Previously it was required
to decode this from the stored payment request. The reason to create
a combined commit is to prevent multiple migrations.
Now that the Invoice struct contains the decoded final cltv delta value,
the decoding of payment requests can be removed from the invoice
registry.
As the logic around invoice mutations gets more complex, the friction
caused by having this logic split between invoice registry and channeldb
becomes more apparent. This commit brings a clearer separation of
concerns by centralizing the accept/settle logic in the invoice
registry.

The original AcceptOrSettle method is renamed to UpdateInvoice because
the update to perform is controlled by the callback.
This commit is a continuation of the centralization of invoice state
transition logic in the invoice registry.
This commit is a continuation of the centralization of invoice state
transition logic in the invoice registry.
This commit refactors the invoice registry accept/settle logic so that
it doesn't rely anymore on a set of error values to indirectly
communicate from the update callback to the main function what action is
required on the htlc.
Needed for time control in unit tests.
Previously the invoice registry wasn't aware of replayed htlcs. This was
dealt with by keeping the invoice accept/settle logic idempotent, so
that a replay wouldn't have an effect.

This mechanism has two limitations:

1. No accurate tracking of the total amount paid to an invoice. The total
amount couldn't just be increased with every htlc received, because it
could be a replay which would lead to counting the htlc amount multiple
times. Therefore the total amount was set to the amount of the first
htlc that was received, even though there may have been multiple htlcs
paying to the invoice.

2. Impossible to check htlc expiry consistently for hodl invoices. When
an htlc is new, its expiry needs to be checked against the invoice cltv
delta. But for a replay, that check must be skipped. The htlc was
accepted in time, the invoice was moved to the accepted state and a
replay some blocks later shouldn't lead to that htlc being cancelled.
Because the invoice registry couldn't recognize replays, it stopped
checking htlc expiry heights when the invoice reached the accepted
state. This prevents hold htlcs from being cancelled after a restart.
But unfortunately this also caused additional htlcs to be accepted on an
already accepted invoice without their expiry being checked.

In this commit, the invoice registry starts to persistently track htlcs
so that replays can be recognized. For replays, an htlc resolution
action is returned early. This fixes both limitations mentioned above.
@joostjager joostjager force-pushed the joostjager:invoice-circuits branch from b8d8389 to 45cd76e Sep 4, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

fixup squashed

Copy link
Member

left a comment

LGTM 🏭

@Roasbeef Roasbeef merged commit 7eca7b0 into lightningnetwork:master Sep 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 61.172%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.