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

Trampoline/MPP DB changes #1287

Merged
merged 11 commits into from Jan 29, 2020
Merged

Trampoline/MPP DB changes #1287

merged 11 commits into from Jan 29, 2020

Conversation

@t-bast
Copy link
Member

t-bast commented Jan 22, 2020

This PR refactors payment structures and DB schema to correctly handle Trampoline and MPP.
A notable change is the distinction between a target node and the final recipient.
To correctly report fees, we need to store information about the recipient, even though
we're sending a payment to the first trampoline node (our target node).

It can quickly become confusing because there are multiple amounts that appear during the payment:

  • the final amount (amount from the invoice, what the final recipient should receive)
  • to which we add trampoline fees to find the amount the first trampoline node needs to receive
  • from which we can compute the amount of the first HTLC once we've found a route to the first trampoline node (and know the associated routing fees)

I tried to make the wording uniform across the codebase to make sure we don't get those mixed up.
Those changes allow us to clean multiple temporary hacks made to support Trampoline/MPP in the first version of Phoenix.

There were also multiple places where we weren't consistent when setting parentId vs id in payment events: this is now fixed.

If you think there are other columns that can be added/changed in the PaymentsDb or AuditDb, I think it's worth adding them now to minimize migrations.

This is a big PR, but probably one of the most important ones for MPP/Trampoline support, so please take your time to review and challenge.
Even comments such as: "this part is a bit messy, please rework" are useful :).

In a follow-up PR, I will update the API to make it easier to work with MPP and Trampoline from the CLI.

E2E tests:

  • Test migrating the Endurance DB
  • Setup nodes, make various types of payments, then upgrade their DB (will be done once the PR is approved, just before merging)
t-bast added 2 commits Dec 31, 2019
With MPP and Trampoline (and particularly the combination of the two),
we need to keep track of multiple amounts, recipients and fees.
There's a trampoline fee and a fee to reach the first trampoline node.
The trampoline nodes must appear in the route, but not as payment recipients.

Adding new fields to payment events and DB structs lets us distinguish those.
Add new colums to better handle Trampoline and MPP.
Add DB migrations.
@t-bast t-bast requested a review from pm47 Jan 22, 2020
There's a race condition, we need to wait for the AuditDb to handle the
payment events.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 22, 2020

Codecov Report

Merging #1287 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1287      +/-   ##
==========================================
+ Coverage    77.4%   77.46%   +0.05%     
==========================================
  Files         143      143              
  Lines       10079    10082       +3     
  Branches      416      410       -6     
==========================================
+ Hits         7802     7810       +8     
+ Misses       2277     2272       -5
Impacted Files Coverage Δ
...src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala 80% <ø> (ø) ⬆️
...cala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala 99.06% <100%> (+0.44%) ⬆️
...la/fr/acinq/eclair/transactions/Transactions.scala 96.77% <100%> (+0.02%) ⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala 93.03% <100%> (+0.74%) ⬆️
...a/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala 98.26% <100%> (ø) ⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 90.32% <100%> (ø) ⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala 74.19% <0%> (-1.08%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 76.57% <0%> (-0.29%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 83.65% <0%> (+0.09%) ⬆️
... and 1 more
The requirement to include `var_onion_optin` in invoice feature bits
was added after the first Phoenix release.

Phoenix users will thus have non spec-compliant invoices in their
payment history.
However it doesn't hurt as long as the new invoices generated are compliant.
We allow reading from the DB non-compliant invoices for backwards-compat.
Copy link
Member

pm47 left a comment

Incomplete review, requesting feedback on my comments

t-bast added 4 commits Jan 28, 2020
* Rename finalAmount -> recipientAmount
* Apply suggested code comments from PR
Instead of adding weird hooks to allow invalid invoices,
we make a specific case for `var_onion_optin`.

We accept invoices that don't set this field; this is a harmless
spec violation (as long as we set it in new invoices).
Add migration to set it to "Standard" by default.
@t-bast t-bast requested a review from pm47 Jan 28, 2020
t-bast added 2 commits Jan 28, 2020
@pm47
pm47 approved these changes Jan 29, 2020
@t-bast t-bast merged commit 453a7c6 into master Jan 29, 2020
4 checks passed
4 checks passed
ci/semaphoreci/pr: Build & Test The build passed on Semaphore 2.0.
Details
ci/semaphoreci/push: Build & Test The build passed on Semaphore 2.0.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@t-bast t-bast deleted the trampoline-db-changes branch Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.