Relay Trampoline payments #1220
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1220 +/- ##
==========================================
+ Coverage 76.66% 77.07% +0.41%
==========================================
Files 140 142 +2
Lines 9710 9880 +170
Branches 390 402 +12
==========================================
+ Hits 7444 7615 +171
+ Misses 2266 2265 -1
|
t-bast
force-pushed
the
trampoline-routing
branch
from
acced8d to
1c08704
Compare
Recovery after a restart isn't implemented yet. A lot of polish is still needed to send the right errors and emit the correct events.
The previous logic has been moved from Switchboard to payments. There are two cases to consider: * HTLCs not relayed at all: we can fail instantly * HTLCs partially relayed: we need to wait for downstream responses
t-bast
force-pushed
the
trampoline-routing
branch
from
1c08704 to
770d32f
Compare
pm47
reviewed
Nov 29, 2019
eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/NodeRelayer.scala
Outdated
Show resolved
Hide resolved
| def main(pendingIncoming: Map[ByteVector32, PendingRelay], pendingOutgoing: Map[UUID, Upstream.TrampolineRelayed]): Receive = { | ||
| // We make sure we receive all payment parts before forwarding to the next trampoline node. | ||
| case IncomingPacket.NodeRelayPacket(add, outer, inner, next) => outer.paymentSecret match { | ||
| case None => rejectHtlc(add.id, add.channelId, add.amountMsat) |
There was a problem hiding this comment.
payment secret is mandatory for trampoline?
There was a problem hiding this comment.
Yes, this is something we need to do whenever we can for security against probing / fee stealing
| case MultiPartPaymentFSM.MultiPartHtlcSucceeded(paymentHash, parts) => pendingIncoming.get(paymentHash) match { | ||
| case Some(PendingRelay(htlcs, _, nextPayload, nextPacket, handler)) => | ||
| val upstream = Upstream.TrampolineRelayed(htlcs) | ||
| handler ! PoisonPill |
There was a problem hiding this comment.
Still not convinced about this pattern of not immediately shutting down the multipart payment fsm once the payment has succeeded/failed. The race condition with extraneous htlcs arriving is present in both cases. I think we should experiment with a deadletter handler instead.
There was a problem hiding this comment.
It's worth investigating, but it's not really a trampoline issue - this is rather something we could change in the MultiPartPaymentFSM (independently of this PR)
There was a problem hiding this comment.
I added that to my post-MVP todo-list
This prevents a conflict between channels being reestablished and post-restart logic.
This is ugly but works (except in one corner-case). This is a temporary measure: the right way to fix this is to update the DB schema to correctly account for Trampoline data. Since DB schema changes are painful, we will aggregate all of them in one change later. When we do that we will make this more elegant.
This event is stored in the AuditDb and linked to payment metrics. We're currently loosing some information by collapsing it to a legacy relayed event. We will update the DB schema in a later change to store more information.
pm47
reviewed
Dec 18, 2019
| }) | ||
|
|
||
| override def mdc(currentMessage: Any): MDC = { | ||
| val paymentHash_opt = currentMessage match { | ||
| case IncomingPacket.NodeRelayPacket(add, _, _, _) => Some(add.paymentHash) |
There was a problem hiding this comment.
nit: using extractors is nice but it will break everytime the signature of any of the classes changes
pm47
approved these changes
Dec 18, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO in this PR (MVP, can be merged on master):
To limit the number of changes to review, I'll stop there. At this point (once we've properly reviewed this code) merging to master shouldn't have any negative impact (no DB schema update, trampoline disabled, post-restart should work, etc).
The trampoline UX will be somewhat bad because many improvements/polish are missing.
Some shortcuts were taken, a few hacks here and there need to be fixed, but nothing too scary.
Those improvements will be done in separate PRs to allow a better code review experience.
TODO in subsequent PRs (post-MVP, should be done before release):