trampoline: accumulate inbound trampoline htlcs#4493
trampoline: accumulate inbound trampoline htlcs#4493carlaKC wants to merge 14 commits intolightningdevkit:mainfrom
Conversation
|
👋 I see @valentinewallace was un-assigned. |
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
2 times, most recently
from
2f01cdc to
9d17783
Compare
|
I find it easier to be confident in smaller PRs, so happy to see this broken up as mentioned on the dev call! |
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
from
9d17783 to
cb5376d
Compare
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
from
cb5376d to
b36e5ab
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4493 +/- ##
==========================================
+ Coverage 87.16% 87.29% +0.12%
==========================================
Files 161 161
Lines 109251 109496 +245
Branches 109251 109496 +245
==========================================
+ Hits 95230 95580 +350
+ Misses 11547 11435 -112
- Partials 2474 2481 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Can you let me know your thoughts on the top two claude'd commits here? https://github.com/valentinewallace/rust-lightning/tree/2026-03-mpp-accumulation-wip I think I prefer not entirely repurposing the existing claimable structs for trampoline. The top commit is pretty large though, admittedly, though it's super mechanical. The nice part is that there isn't a need to add the |
Nice cleanup! Didn't think that repurposing was too bad because it's relatively contained, but def nice to not need an unused |
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
3 times, most recently
from
2a44215 to
86256af
Compare
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
5 times, most recently
from
3b411ba to
54d0f2b
Compare
| // TODO: add restriction to specification that trampoline should be consistent across | ||
| // MPP parts? Currently, we'll accept a MPP trampoline payments that specify different | ||
| // next_node_id destinations (just forwarding to the last one that arrives). |
There was a problem hiding this comment.
This TODO has security implications worth calling out: if MPP parts carry different next_hop_info (onion packet, amount, cltv, next_node_id), only the last-arriving part's values are used for fee/cltv validation and forwarding. A malicious sender could exploit this by sending one part with legitimate values (to pass initial checks) and a final part with different values.
Since forwarding isn't implemented yet, this isn't exploitable today, but when it is, this needs to be addressed — either by requiring consistency across parts or by using the first part's values.
| let (next_hop_amount, next_hop_cltv) = check_blinded_forward( | ||
| outer_hop_data.multipath_trampoline_data.as_ref().map(|f| f.total_msat).unwrap_or(msg.amount_msat), msg.cltv_expiry, &next_trampoline_hop_data.payment_relay, &next_trampoline_hop_data.payment_constraints, &next_trampoline_hop_data.features |
There was a problem hiding this comment.
This changes the input to check_blinded_forward from msg.amount_msat (single HTLC amount) to the MPP total_msat. This is significant: the fee computation in amt_to_forward_msat and the check_blinded_payment_constraints (including htlc_minimum_msat check) now operate on the total MPP amount rather than the per-HTLC amount.
For the fee computation, this is correct — the blinded relay parameters are designed to be applied to the total amount, not per-part. But check_blinded_payment_constraints at line 69 of this file calls check_blinded_payment_constraints(inbound_amt_msat, ...) which checks against htlc_minimum_msat. Using the total here means a per-HTLC amount below htlc_minimum_msat would still pass if the total is above it. Is that the intended behavior for trampoline MPP?
If multipath_trampoline_data is None, this falls back to msg.amount_msat which is the per-HTLC amount (non-MPP case) — that's correct.
|
I've completed a thorough review of every changed file and hunk in this PR. My prior review comments already cover the significant issues. I found no new bugs or security problems beyond those already flagged. Review SummaryNo new issues found beyond those already flagged in the prior review pass. The trampoline MPP accumulation logic is well-structured, correctly reuses Previously flagged issues (still applicable):
Verified areas (no issues):
|
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
from
54d0f2b to
7e428bd
Compare
Followup from prefactor PR.
We don't need to track a single trampoline secret in our HTLCSource because this is already tracked in each of our previous hops contained in the source. This field was unnecessarily added under the belief that each inner trampoline onion we receive for inbound MPP trampoline would have the same session key. It can be removed with breaking changes to persistence because we currently refuse to decode trampoline forwards, and will not read HTLCSource::Trampoline to prevent downgrades.
When we receive a trampoline forward, we need to wait for MPP parts to arrive at our node before we can forward the outgoing payment onwards. This commit threads this information through to our pending htlc struct which we'll use to validate the parts we receive.
For regular blinded forwards, it's okay to use the amount in our update_add_htlc to calculate the amount that we need to foward onwards because we're only expecting on HTLC in and one HTLC out. For blinded trampoline forwards, it's possible that we have multiple incoming HTLCs that need to accumulate at our node that make our total incoming amount from which we'll calculate the amount that we need to forward onwards to the next trampoline. This commit updates our next trampoline amount calculation to use the total intended incoming amount for the payment so we can correctly calculate our next trampoline's amount. `decode_incoming_update_add_htlc_onion` is left unchanged because the call to `check_blinded` will be removed in upcoming commits.
When we are a trampoline node receiving an incoming HTLC, we need access to our outer onion's amount_to_forward to check that we have been forwarded the correct amount. We can't use the amount in the inner onion, because that contains our fee budget - somebody could forward us less than we were intended to receive, and provided it is within the trampoline fee budget we wouldn't know. In this commit we set our outer onion values in PendingHTLCInfo to perform this validation properly. In the commit that follows, we'll start tracking our expected trampoline values in trampoline-specific routing info.
When we're forwarding a trampoline payment, we need to remember the amount and CLTV that the next trampoline is expecting.
When we receive trampoline payments, we first want to validate the values in our outer onion to ensure that we've been given the amount/ expiry that the sender was intending us to receive to make sure that forwarding nodes haven't sent us less than they should.
When we are a trampoline router, we need to accumulate incoming HTLCs (if MPP is used) before forwarding the trampoline-routed outgoing HTLC(s). This commit adds a new map in channel manager, and mimics the handling done for claimable_payments. We will rely on our pending_outbound_payments (which will contain a payment for trampoline forwards) for completing MPP claims, not want to surface `PaymentClaimable` events for trampoline, so do not need to have pending_claiming_payments like we have for MPP receives. This map is not persisted, as we're currently working on refactoring restart logic to depend on channel monitors. We should not use this accumulation map in production yet, as we can hit a force close if: - We are used as a trampoline, despite not supporting the feature - A trampoline MPP part arrives and is committed to the inbound channel and added to `awaiting_trampoline_forwards` - We restart and the MPP part is not re-added to `awaiting_trampoline_forwards` In this scenario, we will not hit our MPP timeout logic for this HTLC because we have "forgotten" about it. It will be up to our counterparty to force close the channel on us, because we're not failing it back after we hit MPP timeout. Likewise, even if other MPP parts arrive, we won't consider the inbound accumulation to be complete so we'll fail them back but forget about the HTLC that came before the restart. We currently reject trampoline HTLCs earlier in the lifecycle, so we are not at risk of producing a state that could trigger such a force close. In the commits that follow, we'll allow forwarding of trampoline HTLC for tests so that we can start to cover this code.
Add our MPP accumulation logic for trampoline payments, but reject them when they fully arrive. This allows us to test parts of our trampoline flow without fully implementing outbound dispatch. This commit keeps the same first_claimable_htlc debug_assert behavior as MPP claims, asserting that we do not fail our check_claimable_incoming_htlc merge for the first HTLC that we add to a set. This assert can only be hit if our first part exceeds the `MAX_VALUE_MSAT`, which should not be hit because we check individual amounts elsewhere in the codebase (the check exists to check that multiple parts combined don't hit this overflow).
If we're a trampoline node and received an error from downstream that we can't fully decrypt, we want to double-wrap it for the original sender. Previously not implemented because we'd only focused on receives, where there's no possibility of a downstream error. While proper error handling will be added in a followup, we add the bare minimum required here for testing.
While proper error handling will be added in a followup, we add the bare minimum required here for testing. Note that we intentionally keep the behavior of not setting `payment_failed_permanently` for local failures because we can possibly retry it because we're the sender as a trampoline forwarder. For example, a local ChannelClosed error is considered to be permanent, but we can still retry along another channel.
We can't perform proper validation because we don't know the outgoing channel id until we forward the HTLC, so we just perform a basic CLTV check. We don't yet have proper handling of trampoline forwards on restart, so we only enable this in our tests.
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
from
7e428bd to
b838480
Compare
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Still making my way through but generally looks quite good! 🥇
| } | ||
| } | ||
| RoutingInfo::Trampoline { next_trampoline, new_packet_bytes, next_hop_hmac, shared_secret, current_path_key } => { | ||
| RoutingInfo::Trampoline { next_trampoline, new_packet_bytes, next_hop_hmac, shared_secret, current_path_key, incoming_multipath_data: multipath_trampoline_data } => { |
There was a problem hiding this comment.
nit: no need to rebind the field here
| // Handles the addition of a HTLC associated with a trampoline forward that we need to accumulate | ||
| // on the incoming link before forwarding onwards. If the HTLC is failed, it returns the source | ||
| // and error that should be used to fail the HTLC(s) back. |
| // TODO: add restriction to specification that trampoline should be consistent across | ||
| // MPP parts? Currently, we'll accept a MPP trampoline payments that specify different | ||
| // next_node_id destinations (just forwarding to the last one that arrives). | ||
|
|
There was a problem hiding this comment.
Adding a restriction in the spec makes sense to me!
| let proportional_fee = (forwarding_fee_proportional_millionths as u128 | ||
| * next_hop_info.amount_msat as u128 | ||
| / 1_000_000) as u64; | ||
| let our_forwarding_fee_msat = proportional_fee + forwarding_fee_base_msat as u64; |
There was a problem hiding this comment.
Can we use router's compute_fees or compute_fees_saturating?
| let proportional_fee = (forwarding_fee_proportional_millionths as u128 | ||
| * next_hop_info.amount_msat as u128 | ||
| / 1_000_000) as u64; |
There was a problem hiding this comment.
I wasn't sure about this at first because for normal forwards we calculate the proportional fee based on the amount we're relaying to the direct next hop, whereas in this case we calculate it on the next trampoline hop's amount (i.e. there may be hops in between). It looks like this is how eclair does it too, though, and maybe it isn't possible to do any other way? Could possibly use a comment.
| for (i, path) in route.paths.iter().enumerate() { | ||
| nodes[0] | ||
| .node | ||
| .test_send_payment_along_path( |
There was a problem hiding this comment.
Generally preferred to use the public API in tests. I clauded this patch but haven't vetted it much, let me know your thoughts: valentinewallace@79a0771
| pub(super) fn decode_onion_failure<T: secp256k1::Signing, L: Logger>( | ||
| &self, secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource, | ||
| ) -> DecodedOnionFailure { | ||
| macro_rules! decoded_onion_failure { |
There was a problem hiding this comment.
Looks like this can be a closure, which avoids the somewhat awkward parenthesis around the scid option below: valentinewallace@00b24b3
4 participants
This PR handles accumulation of inbound MPP trampoline parts, including handling of timeout and MPP validation. When all parts are successfully accumulated, we'll fail the MPP set backwards as we do not yet have support for outbound dispatch.
It does not include:
HTLCSource::TrampolineForwardto prevent downgrade with trampoline in flight).