-
Notifications
You must be signed in to change notification settings - Fork 390
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
Handle receiving payments via Trampoline #3670
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
e7f6810
to
52497a2
Compare
lightning/src/ln/onion_payment.rs
Outdated
{ | ||
log_info!(logger, "Failed to accept/forward incoming HTLC: {}", $msg); | ||
let (sha256_of_onion, failure_code) = if msg.blinding_point.is_some() { | ||
let (sha256_of_onion, failure_code) = if $force_blinding_error || msg.blinding_point.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to check the failure code for INVALID_ONION_BLINDING
instead of having the extra argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, provided we never ever want to use that error code with non-zeroed data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we always want to use it with zeroed data, why would the sha256 field for INVALID_ONION_BLINDING then be in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, hence my question :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll only ever want to use zeroed data with this error code, which I think is just a special exception for update_malformeds for blinded HTLCs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified it to infer the onion hash from the error code instead of adding an extra field.
lightning/src/ln/onion_utils.rs
Outdated
Ok((_, None)) => Err(OnionDecodeErr::Malformed { | ||
err_msg: "Non-final Trampoline onion data provided to us as last hop", | ||
// todo: find more suitable error code | ||
err_code: 0x4000 | 22, | ||
err_code: INVALID_ONION_BLINDING, | ||
trampoline_onion_blinding: true, | ||
}), | ||
Ok((_, Some(_))) => Err(OnionDecodeErr::Malformed { | ||
err_msg: "Final Trampoline onion data provided to us as intermediate hop", | ||
// todo: find more suitable error code | ||
err_code: 0x4000 | 22, | ||
err_code: INVALID_ONION_BLINDING, | ||
trampoline_onion_blinding: true, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat annoying, but if we want to be accurate to the spec I believe we shouldn't be returning INVALID_ONION_BLINDING
for non-blinded trampoline forward/receives, or for blinded receives where the intro blinding point is set. Apologies if I misunderstood some context on this when we discussed offline. I think if we matched on the specific variants instead of _
this change should be doable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was wondering about that lol. Trivial fix, no worries
@@ -2080,7 +2080,7 @@ mod fuzzy_internal_msgs { | |||
pub struct InboundTrampolineEntrypointPayload { | |||
pub amt_to_forward: u64, | |||
pub outgoing_cltv_value: u32, | |||
pub multipath_trampoline_data: FinalOnionHopData, | |||
pub multipath_trampoline_data: Option<FinalOnionHopData>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the discussion you're referring to in the commit message? lightning/bolts#836 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, exactly! @t-bast and I discussed it a bit offline, and I believe they're assessing Eclair code viability for not mandating it outside of MPP scenarios. Alternatively, I can also make it mandatory on the sending side (right now we have an internal mismatch), but I think this approach makes more sense for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think that for simplicity, it may be better to just always set this field? One code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd agree, although it's all cfg-gated for now anyway I suppose... @arik-so was the reason for doing this now for ease of testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially. The other option would require us deciding on a mechanism to convert the prng_seed into a pseudo-random payment secret in the outgoing onion construction method, that's about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But generating that secret is something to be looked at when implementing the forwarding part isn't it? Isn't it best for now to stick with the proposed spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec did actually get updated this morning to make it optional for the outer onion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should update the commit message then?
52497a2
to
b7f927e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been gathering more background knowledge on blinded paths and trampoline and did my best to review this PR. I feel I am still missing quite a few pieces, so wouldn't count me as a full reviewer.
@@ -2080,7 +2080,7 @@ mod fuzzy_internal_msgs { | |||
pub struct InboundTrampolineEntrypointPayload { | |||
pub amt_to_forward: u64, | |||
pub outgoing_cltv_value: u32, | |||
pub multipath_trampoline_data: FinalOnionHopData, | |||
pub multipath_trampoline_data: Option<FinalOnionHopData>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think that for simplicity, it may be better to just always set this field? One code path.
lightning/src/ln/onion_payment.rs
Outdated
{ | ||
log_info!(logger, "Failed to accept/forward incoming HTLC: {}", $msg); | ||
let (sha256_of_onion, failure_code) = if msg.blinding_point.is_some() { | ||
let (sha256_of_onion, failure_code) = if $force_blinding_error || msg.blinding_point.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we always want to use it with zeroed data, why would the sha256 field for INVALID_ONION_BLINDING then be in the spec?
@@ -514,6 +514,24 @@ impl Writeable for ForwardTlvs { | |||
} | |||
} | |||
|
|||
#[cfg(trampoline)] | |||
impl Writeable for TrampolineForwardTlvs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to commit msg why we need to serialize blinded trampoline fwds in the future and not yet now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I meant explain why this code is needed now, besides unit tests? I got that it encodes tlv data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only for the unit test, maybe squash with the test? Or just leave as is, non blocking.
lightning/src/ln/onion_payment.rs
Outdated
InboundHTLCErr { | ||
err_code: INVALID_ONION_BLINDING, | ||
err_data: vec![0; 32], | ||
msg: "Amount or cltv_expiry violated blinded payment constraints", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention trampoline in the msg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the message to mention Trampoline.
3574d7d
to
8e70d5e
Compare
4e07aff
to
6df45ce
Compare
lightning/src/ln/onion_utils.rs
Outdated
@@ -1828,15 +1831,34 @@ where | |||
trampoline_shared_secret, | |||
), | |||
}), | |||
Ok((_, None)) => Err(OnionDecodeErr::Malformed { | |||
Ok((msgs::InboundTrampolinePayload::BlindedForward(_), None)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much correct error handling actually matters here, but to be accurate to the spec we'd want to return Relay
if the intro node blinding point is set IIUC. Looks pretty easy though:
diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs
index 45a9e7a09..eeb513319 100644
--- a/lightning/src/ln/onion_utils.rs
+++ b/lightning/src/ln/onion_utils.rs
@@ -1831,13 +1831,33 @@ where
trampoline_shared_secret,
),
}),
- Ok((msgs::InboundTrampolinePayload::BlindedForward(_), None)) => {
+ Ok((msgs::InboundTrampolinePayload::BlindedForward(hop_data), None)) => {
+ if hop_data.intro_node_blinding_point.is_some() {
+ return Err(OnionDecodeErr::Relay {
+ err_msg: "Non-final intro node Trampoline onion data provided to us as last hop",
+ err_code: INVALID_ONION_BLINDING,
+ shared_secret,
+ trampoline_shared_secret: Some(SharedSecret::from_bytes(
+ trampoline_shared_secret,
+ )),
+ })
+ }
Err(OnionDecodeErr::Malformed {
err_msg: "Non-final Trampoline onion data provided to us as last hop",
err_code: INVALID_ONION_BLINDING,
})
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; I think in this scenario it should also be an invalid payload, so I adjusted it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah since we're the intro node to the blinded path I believe we should still blind the error but just do Relay
instead of Malformed
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjusted the error code for being the intro node to a multi-hop blinded path
lightning/src/ln/onion_utils.rs
Outdated
err_code: INVALID_ONION_BLINDING, | ||
}) | ||
}, | ||
Ok((msgs::InboundTrampolinePayload::BlindedReceive(_), Some(_))) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat similarly here -- if the intro node blinding point is set we want to return an unblinded error, because if we're a receiver and the intro node, the blinded path only had 1 hop to begin with so no need to blind the error:
diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs
index 45a9e7a09..a6a28ff3d 100644
--- a/lightning/src/ln/onion_utils.rs
+++ b/lightning/src/ln/onion_utils.rs
@@ -1837,7 +1837,17 @@ where
err_code: INVALID_ONION_BLINDING,
})
},
- Ok((msgs::InboundTrampolinePayload::BlindedReceive(_), Some(_))) => {
+ Ok((msgs::InboundTrampolinePayload::BlindedReceive(hop_data), Some(_))) => {
+ if hop_data.intro_node_blinding_point.is_some() {
+ return Err(OnionDecodeErr::Relay {
+ err_msg: "Final Trampoline intro node onion data provided to us as intermediate hop",
+ err_code: 0x4000 | 22,
+ shared_secret,
+ trampoline_shared_secret: Some(SharedSecret::from_bytes(
+ trampoline_shared_secret,
+ )),
+ })
+ }
Err(OnionDecodeErr::Malformed {
err_msg:
"Final Trampoline onion data provided to us as intermediate hop",
).unwrap() | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra newline
nodes[0].node.send_payment_with_route(route.clone(), payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0)).unwrap(); | ||
|
||
let replacement_onion = { | ||
// create a substitute onion where the last Trampoline hop is a forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is outdated now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified comment to apply to both the invalid onion and the unblinded receive
// pop the last dummy hop | ||
trampoline_payloads.pop(); | ||
|
||
trampoline_payloads.push(msgs::OutboundTrampolinePayload::Receive { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe note that we don't support sending to unblinded trampolines which is why we need to do all this manual stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a comment at the top of the replacement_onion block
LGTM after last feedback is addressed. Fine with a squash |
6df45ce
to
6132b1b
Compare
I addressed your feedback, @valentinewallace, and squashed most things, but left two fixup commits where I suspect you might have further thoughts. Also a bit unsure about the last commit, which combines all the unit tests into one, but happy either way. |
lightning/src/ln/channelmanager.rs
Outdated
@@ -5984,7 +5984,7 @@ where | |||
onion_packet.hmac, payment_hash, None, &*self.node_signer | |||
) { | |||
Ok(res) => res, | |||
Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => { | |||
Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code, .. }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer necessary change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, removed
lightning/src/ln/onion_utils.rs
Outdated
err_code: INVALID_ONION_BLINDING, | ||
}) | ||
}, | ||
Ok((_, None)) => Err(OnionDecodeErr::Relay { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably more clear to be explicit with these final two arms?
Ok((msgs::InboundTrampolinePayload::Forward(_) | msgs::InboundTrampolinePayload::Receive(_), None)) => Err(OnionDecodeErr::Relay {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean like this?
Ok((
msgs::InboundTrampolinePayload::Forward(_)
| msgs::InboundTrampolinePayload::BlindedForward(_),
None,
)) => Err(OnionDecodeErr::Relay {
err_msg: "Non-final Trampoline onion data provided to us as last hop",
err_code: 0x4000 | 22,
shared_secret,
trampoline_shared_secret: Some(SharedSecret::from_bytes(
trampoline_shared_secret,
)),
}),
Ok((
msgs::InboundTrampolinePayload::Receive(_)
| msgs::InboundTrampolinePayload::BlindedReceive(_),
Some(_),
)) => Err(OnionDecodeErr::Relay {
err_msg: "Final Trampoline onion data provided to us as intermediate hop",
err_code: 0x4000 | 22,
shared_secret,
trampoline_shared_secret: Some(SharedSecret::from_bytes(
trampoline_shared_secret,
)),
}),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I was wondering what cases I was looking at, what was left. I think explicit makes it easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -127,7 +130,7 @@ pub fn fail_blinded_htlc_backwards( | |||
let unblinded_node_updates = get_htlc_update_msgs!(nodes[i], nodes[i-1].node.get_our_node_id()); | |||
assert_eq!(unblinded_node_updates.update_fail_htlcs.len(), 1); | |||
nodes[i-1].node.handle_update_fail_htlc( | |||
nodes[i].node.get_our_node_id(), &unblinded_node_updates.update_fail_htlcs[i-1] | |||
nodes[i].node.get_our_node_id(), &unblinded_node_updates.update_fail_htlcs[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks like a bug fix, or is it part of the new functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a bug fix I noticed in passing, shall I move it into a separate commit? I think since I stopped using this method for the test, I can remove it from the PR entirely and open a separate one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is independent, I think it is helpful to isolate in a commit or pr with some explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the change from this PR
@@ -2081,29 +2088,44 @@ fn do_test_trampoline_single_hop_receive(success: bool) { | |||
|
|||
check_added_monitors!(&nodes[0], 1); | |||
|
|||
if success { | |||
if matches!(scenario, TrampolineSingleHopReceiveCheck::SuccessfulBlindedReceive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes the scenario
value seems to push this to the point where it becomes hard to read. I'd say: just acceptable? There are probably other ways to compose a test from reusable building blocks, but maybe not worth it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah perhaps I should just drop the final commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped the scenario commit.
// Simulate a payment failure of A (0) -> B (1) -> C(Trampoline (blinded forward)) (2) | ||
do_test_trampoline_single_hop_receive(false); | ||
} | ||
do_test_trampoline_single_hop_receive(TrampolineSingleHopReceiveCheck::SuccessfulBlindedReceive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there currently a test covering the forwarding operation of trampoline node? It's not yet implemented, but to assert that nothing weird happens if we receive a forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially without the cfg flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there isn't, but then again, what should happen if we receive it prior to supporting it? I think this is actually a good argument to simply remove it in the subsequent PR instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test ensuring sane behavior when receiving a Trampoline forward hop.
6132b1b
to
07d7640
Compare
40c7e03
to
32a35a1
Compare
32a35a1
to
65bdb4e
Compare
Add something as a PR description - or maybe the title is self-explanatory? |
Hm, I need to think of what description would be more than just a rephrasing of the title, but I can probably come up with something. |
Perhaps something about what is and isn't supported yet and a link to a follow up issue or the parent issue? That's something at least... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think? I mean nothing stands out.
@@ -2080,7 +2080,7 @@ mod fuzzy_internal_msgs { | |||
pub struct InboundTrampolineEntrypointPayload { | |||
pub amt_to_forward: u64, | |||
pub outgoing_cltv_value: u32, | |||
pub multipath_trampoline_data: FinalOnionHopData, | |||
pub multipath_trampoline_data: Option<FinalOnionHopData>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should update the commit message then?
65bdb4e
to
7f5cd2b
Compare
@@ -134,8 +134,6 @@ RUSTFLAGS="--cfg=taproot" cargo test --verbose --color always -p lightning | |||
[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean | |||
RUSTFLAGS="--cfg=splicing" cargo test --verbose --color always -p lightning | |||
[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean | |||
RUSTFLAGS="--cfg=trampoline" cargo test --verbose --color always -p lightning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following the commit message here in 908c6bf since we don't support trampoline forwarding yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, it ensures that receiving forwarding onions results in a rejection, but not a panic or some undefined behavior. I'll rephrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Joost explained that we test that we don't support forwarding basically
} | ||
{ | ||
let payment_failed_conditions = PaymentFailedConditions::new() | ||
.expected_htlc_error_data(0x4000 | 10, &[0; 0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment what this error code is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment
7f5cd2b
to
a827324
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking :)
lightning/src/ln/channelmanager.rs
Outdated
#[cfg(trampoline)] | ||
onion_utils::Hop::TrampolineReceive { .. } | onion_utils::Hop::TrampolineBlindedReceive { .. } => { | ||
// OUR PAYMENT! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we DRY this now with the other OUR PAYMENT!
case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. CI's far from finished anyway, so I pushed the squashed fixup
@@ -3391,7 +3361,6 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, NS)> for InboundOnionPayload wh | |||
} | |||
} | |||
|
|||
#[cfg(trampoline)] | |||
impl<NS: Deref> ReadableArgs<(Option<PublicKey>, NS)> for InboundTrampolinePayload where NS::Target: NodeSigner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of a shame to keep this duplicated with InboundOnionPayload
since the code is complex and ~identical, but might be best to save this consideration for follow-up so we can get this in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, that one saw a lot of back and forth, and I agree, but the alternative is clunky, too, and loses the benefit of semantic distinction
err_code: INVALID_ONION_BLINDING, | ||
}) | ||
}, | ||
Ok((msgs::InboundTrampolinePayload::Forward(_), None)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that these variants are spelled out explicitly now, reads more obviously correct 👍
|
||
check_added_monitors!(&nodes[0], 1); | ||
|
||
// Check that we've queued the HTLCs of the async keysend payment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think this is a keysend payment?
a827324
to
2b6a6d7
Compare
In this PR, we add end-to-end tests for Trampoline receive and forwarding behavior (specifically rejection of payments for the latter), ensuring sanity, no unexpected errors, and obviating the cfg-gating.
Per the resolution of the spec discussion regarding the requirement of this field outside MPP scenarios, we are now marking this field as optional.
Trivial fix where the Trampoline hop TLV inside blinded contexts differs from un-blinded ones.
It does not need to be a macro, so we convert it into a simple lambda.
When a blinded hop fails to decode, we send a special malformed error. However, we previously simply checked the presence of a blinding point within the `UpdateAddHTLC` message, which is not necessarily applicable to Trampoline, so now we additionally return a flag if the error stemmed from an inner onion's blinded hop decoding failure.
We will need the ability to serialize blinded Trampoline forwards to construct their encrypted TLV data from its unencrypted state using `construct_blinded_hops`, which initially we only do in unit tests.
Handle payment receives with payment details located inside an inner onion.
In the next PR, we will be adding support for forwarding payments between Trampoline hops. Until we do that, forwarding payments need to be rejected, which we test in this commit.
2b6a6d7
to
3970f1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the remark that my mental model of LDK still isn't complete enough to be fully confident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Matt's LGTM we should be good to land when CI passes!
This PR implements handling payment receives from Trampoline onions, but does not yet implement forwarding payments through Trampoline. It also adds tests for end-to-end payment scenarios, including both for single-hop Trampoline receives, as well as for multi-hop Trampoline forwards, thus also providing sufficient coverage to remove the cfg gating.
The overall set of Trampoline components can be tracked here.