Simplify outgoing payment state machine#692
Merged
Merged
Conversation
t-bast
force-pushed
the
simplify-outgoing-payment-fsm
branch
from
c070ae3 to
d3b37a9
Compare
t-bast
force-pushed
the
simplify-outgoing-payment-fsm
branch
from
d3b37a9 to
23c1dc7
Compare
t-bast
force-pushed
the
simplify-outgoing-payment-fsm
branch
from
23c1dc7 to
59a4a1d
Compare
t-bast
force-pushed
the
simplify-outgoing-payment-fsm
branch
from
59a4a1d to
956ae55
Compare
Contributor
thomash-acinq
left a comment
thomash-acinq
left a comment
There was a problem hiding this comment.
If we drop the support for multiple channels, we should modify Peer.kt to only have a single channel instead of the current channels: Map<ByteVector32, ChannelState>
| removeFromState(payment.request.paymentId) | ||
| Failure(payment.request, OutgoingPaymentFailure(finalError, payment.failures + failure)) | ||
| } else { | ||
| // The trampoline node is asking us to retry the payment with more fees or a larger expiry delta. |
Contributor
There was a problem hiding this comment.
It feels like we should should be able to share code with sendPayment instead of duplicating it.
Member
Author
There was a problem hiding this comment.
Can you detail how exactly you'd do that? What we do in each case here is slightly different from what we do when sending the first attempt in sendPayment, I haven't found a way to share more code that doesn't sacrifice clarity...
Member
Author
There was a problem hiding this comment.
Thanks, added with a small refactoring.
We previously supported having multiple channels with our peer, because we didn't yet support splicing. Now that we support splicing, we always have at most one active channel with our peer. This lets us simplify greatly the outgoing payment state machine: payments are always made with a single outgoing HTLC instead of potentially multiple HTLCs (MPP). We don't need any kind of path-finding: we simply need to check the balance of our active channel, if any. We may introduce support for connecting to multiple peers in the future. When that happens, we will still have a single active channel per peer, but we may allow splitting outgoing payments across our peers. We will need to re-work the outgoing payment state machine when this happens, but it is too early to support this now anyway. This refactoring makes it easier to create payment onion, by creating the trampoline onion *and* the outer onion in the same function call. This will make it simpler to migrate to the version of trampoline that is currently specified in lightning/bolts#836 where some fields will be included in the payment onion instead of the trampoline onion.
Each outgoing HTLC will have its own ID in the payments DB. Since we support retries, we have a 1-to-N mapping from a payment ID to its child IDs.
t-bast
force-pushed
the
simplify-outgoing-payment-fsm
branch
from
956ae55 to
baf8ce2
Compare
And rename those methods for clarify.
And correctly handle other payment details types, such as blinded payments.
thomash-acinq
approved these changes
Oct 18, 2024
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
We previously supported having multiple channels with our peer, because we didn't yet support splicing. Now that we support splicing, we always have at most one active channel with our peer. This lets us simplify greatly the outgoing payment state machine: payments are always made with a single outgoing HTLC instead of potentially multiple HTLCs (MPP).
We don't need any kind of path-finding: we simply need to check the balance of our active channel, if any.
We may introduce support for connecting to multiple peers in the future. When that happens, we will still have a single active channel per peer, but we may allow splitting outgoing payments across our peers. We will need to re-work the outgoing payment state machine when this happens, but it is too early to support this now anyway.
This refactoring makes it easier to create payment onion, by creating the trampoline onion and the outer onion in the same function call. This will make it simpler to migrate to the version of trampoline that is currently specified in lightning/bolts#836.