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

Improve Origin and Upstream #2872

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Improve Origin and Upstream #2872

merged 5 commits into from
Jun 27, 2024

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Jun 24, 2024

We move the Upstream trait closer to the Origin, and make it more obvious than a hot Origin is:

  • an Upstream referencing the upstream HTLCs
  • an actor requesting the outgoing payment
  • (we remove the outgoing amount for channel origin which was unnecessary and didn't make sense here)

We also improve the cold trampoline relay class to record the incoming HTLC amount, which we previously didn't bother encoding but is useful to compute the fees collected during relay. To ensure backwards-compat, it is set to 0 msat for pending HTLCs. It will only affect HTLCs that were pending during the upgrade, which is acceptable.

This is useful for cases where we relay after a delay (e.g. after waiting for on-chain funding), where we will store Upstream instances while we wait for the relay condition to be met, and then transform them into Origin.Hot when we create an actor to relay the payment.

@t-bast t-bast marked this pull request as draft June 24, 2024 15:30
@t-bast t-bast changed the title Allow cold origins to specify a replyTo Improve Origin and Upstream Jun 25, 2024
@t-bast t-bast marked this pull request as ready for review June 25, 2024 06:25
@t-bast t-bast requested a review from pm47 June 25, 2024 06:25
We move the `Upstream` trait closer to the `Origin`, and make it more
obvious than a hot `Origin` is:

- an `Upstream` referencing the upstream HTLCs
- an actor requesting the outgoing payment

We also improve the cold trampoline relay class to record the incoming
HTLC amount, which we previously didn't bother encoding but is useful to
compute the fees collected during relay. To ensure backwards-compat, it
is set to `0 msat` for pending HTLCs. It will only affect HTLCs that
were pending during the upgrade, which is acceptable.
We actually don't need to pattern match on local/channel/trampoline
for a `Hot` origin, because we instead just send back a result to
the `replyTo` actor. It's only when the origin is `Cold` that we're
interested in pattern matching on the type of payment (exclusively
done in the `PostRestartHtlcCleaner`).

The only exception is to skip local origins, but it happens in only
two places so it's probably ok to have a slightly more complex pattern
matching in that case.
And move the channel/trampoline cases to `Upstream.Cold`.
pm47
pm47 previously approved these changes Jun 27, 2024
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I redid the codecs from scratch and compared with yours: they match, so we should be safe.

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@t-bast t-bast merged commit 791edf7 into master Jun 27, 2024
1 check passed
@t-bast t-bast deleted the origin-cold-reply-to branch June 27, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants