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 Payload Construction Method #3386

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Oct 28, 2024

Introduce a method for constructing Trampoline payloads.

This will allow the non-Trampoline method to be modified to generate a Trampoline onion and finish its own regular payload construction.

/// amended to match the features present in the invoice this node generated.
pub node_features: NodeFeatures,
/// The channel_announcement features of the channel that should be used from the previous hop
/// to reach this node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really confused, there's no explicit way that I see in OutboundOnionPayload::BlindedForward to specify the channel used. The previous trampoline hop can pick any channel they want, AFAIU...

@@ -454,6 +454,9 @@ impl_writeable_tlv_based!(BlindedTail, {
pub struct Path {
/// The list of unblinded hops in this [`Path`]. Must be at least length one.
pub hops: Vec<RouteHop>,
/// The list of unblinded Trampoline hops. If present, must be at least one.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Vacuous statement is vacuous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"If present, must be at least one".

@@ -454,6 +454,9 @@ impl_writeable_tlv_based!(BlindedTail, {
pub struct Path {
/// The list of unblinded hops in this [`Path`]. Must be at least length one.
pub hops: Vec<RouteHop>,
/// The list of unblinded Trampoline hops. If present, must be at least one.
/// The public key of the first Trampoline hop must match the public key of the last regular hop.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why this needs to be true. Can we just have each trampoline hop be next hops starting after the last unblinded node.

@@ -454,6 +454,9 @@ impl_writeable_tlv_based!(BlindedTail, {
pub struct Path {
/// The list of unblinded hops in this [`Path`]. Must be at least length one.
pub hops: Vec<RouteHop>,
/// The list of unblinded Trampoline hops. If present, must be at least one.
/// The public key of the first Trampoline hop must match the public key of the last regular hop.
pub trampoline_hops: Vec<TrampolineHop>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems weird to add this to the public API in a PR that doesn't use it. Can you go ahead and wire up build_trampoline_onion_payloads to build the valid onion in the same PR?

@arik-so arik-so force-pushed the arik/trampoline/payload-construction branch 2 times, most recently from 7cad33e to 856ce97 Compare November 19, 2024 23:04
@arik-so arik-so force-pushed the arik/trampoline/payload-construction branch from 856ce97 to 5ea3ada Compare November 21, 2024 17:15
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 39.03743% with 114 lines in your changes missing coverage. Please review.

Project coverage is 90.36%. Comparing base (8da30df) to head (5ea3ada).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/onion_utils.rs 13.38% 110 Missing ⚠️
lightning/src/events/mod.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3386      +/-   ##
==========================================
+ Coverage   89.61%   90.36%   +0.75%     
==========================================
  Files         129      130       +1     
  Lines      105506   113544    +8038     
  Branches   105506   113544    +8038     
==========================================
+ Hits        94544   102604    +8060     
- Misses       8208     8214       +6     
+ Partials     2754     2726      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Ah I guess my previous comments still need to be addressed.

@@ -404,13 +404,16 @@ pub struct BlindedTail {
pub excess_final_cltv_expiry_delta: u32,
/// The total amount paid on this [`Path`], excluding the fees.
pub final_value_msat: u64,
/// Used for determining the type of Trampoline path to construct
pub final_hop_supports_trampoline: bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we instead consider storing a *Features of some form?

@@ -454,6 +454,9 @@ impl_writeable_tlv_based!(BlindedTail, {
pub struct Path {
/// The list of unblinded hops in this [`Path`]. Must be at least length one.
pub hops: Vec<RouteHop>,
/// The list of unblinded Trampoline hops. If present, must be at least one.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"If present, must be at least one".

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.

2 participants