Uh oh!
There was an error while loading. Please reload this page.
Trampoline onion construction vectors - #2906
Conversation
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File ( |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2906 +/- ##
==========================================
- Coverage 89.49% 89.47% -0.02%
==========================================
Files 117 117 Lines 95148 95189 +41 Branches 95148 95189 +41 ==========================================
+ Hits 85152 85172 +20 - Misses 7780 7794 +14 - Partials 2216 2223 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| amt_to_forward: u64, | ||
| outgoing_cltv_value: u32, | ||
| }, | ||
| // This should only be used for nested Trampoline onions |
There was a problem hiding this comment.
Wait, so we're gonna read an onion then it'll say trampoline packet then we'll read exactly this enum variant? Shouldn't we do this as a separate struct instead cause we know what we're gonna read?
There was a problem hiding this comment.
Ah, I believe I had a comment on the previous PR pointing out an alternative approach, which I will resurface
There was a problem hiding this comment.
Also, this is the external onion, where the receive hop may or may not contain an inner trampoline onion. So I'm not quite sure why we'd use a separate struct altogether.
| custom_tlvs: Vec<(u64, Vec<u8>)>, | ||
| sender_intended_htlc_amt_msat: u64, | ||
| cltv_expiry_height: u32, | ||
| trampoline_packet: Option<crate::onion_message::packet::Packet> |
There was a problem hiding this comment.
Should we go ahead and decode the inner onion here instead of storing it as the raw bytes?
There was a problem hiding this comment.
No strong opinions on my end. I think not needing to decode it until we actually support routing incoming trampolines doesn't make much of a difference in terms of utility, but saves on deserialization complexity.
But also totally see the benefit of just deserializing it.
5f8ebaa to
a401faeCompare| cltv_expiry_height: u32, | ||
| }, | ||
| #[allow(unused)] | ||
| // This should only be used for nested Trampoline onions |
There was a problem hiding this comment.
Is there a decision on if we can swap this for a wholly separate enum? It seems strange that we have invalid states here.
There was a problem hiding this comment.
Yup, I switched it out. Seems to be working.
a401fae to
45e131fCompare9493c28 to
a5e0921Comparearik-so
commented
Mar 22, 2024
This PR requires a couple allow(unused)s due to the methods being only called in tests. I hope these are few enough that it can stand on its own. |
a5e0921 to
671a009Compare| outgoing_node_id: PublicKey, | ||
| }, | ||
| #[allow(unused)] | ||
| Receive { |
There was a problem hiding this comment.
IMO we shouldn't support this and should only support the blinded path receive variant.
There was a problem hiding this comment.
This is not the legacy receive mechanism, but where the final node understands Trampoline.
There was a problem hiding this comment.
Mmm, right, I was a bit confused. I'm not sure we have a need to support this either, though. We really only care about the trampoline -> blinded path case because that's what all of BOLT12 (and async payments) will do, and less code is less code.
There was a problem hiding this comment.
I can remove this scenario, but I do need it for interop testing.
There was a problem hiding this comment.
I mean we can interop test only a subset, anyway? I imagine this variant may never make it into the BOLTs anyway, if we move towards blinded-path-recipients-everywhere.
There was a problem hiding this comment.
We can definitely interop only a subset, but having the least complex scenario for reference is helpful initially. This scenario is also the one described in the test vectors.
32044ca to
3e56abeCompare3e56abe to
52039a2Compare| use crate::io; | ||
| use crate::prelude::*; | ||
| use core::default::Default; | ||
| use bitcoin::hashes::hex::FromHex; |
There was a problem hiding this comment.
nit: import out of order
Non-polluting replacement for 2899. Would appreciate a concept ACK prior to undrafting, thanks!
If the methodology for constructing variable-length onions for Trampoline employed in the last commit of this PR is acceptable, it will inform an upstream refactor of #2756 pertaining to the Trampoline onion packet struct type.