trampoline: accumulate inbound trampoline htlcs#4493
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% 86.27% -0.90%
==========================================
Files 161 157 -4
Lines 109251 109194 -57
Branches 109251 109194 -57
==========================================
- Hits 95230 94204 -1026
- Misses 11547 12370 +823
- Partials 2474 2620 +146
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.
|
After a thorough review of every file and hunk in this PR, I've confirmed that my prior review comments cover all significant issues. I found no new bugs, security problems, or logic errors. Review SummaryNo new issues found beyond those already flagged in the prior review pass. Previously flagged issues (still applicable):
Previously flagged issues now resolved in current code:
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.
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.
There was a problem hiding this comment.
Will add a comment 👍
and maybe it isn't possible to do any other way?
Technically we could do this any way we like, since how a trampoline picks (and advertises) its fees isn't specified. This is probably good enough for now IMO since we're only losing out on the proportional fee on the total fees between us and the trampoline, which is really minimal.
| 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
Review comment from valentinewallace on lightning/src/ln/onion_payment.rs:239: "nit: no need to rebind the field here" The field name `incoming_multipath_data` is fine on its own; no rename was needed for the destructure or the construction site below.
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.
Review comment from ldk-claude-review-bot on lightning/src/ln/onion_payment.rs:193: > This changes the input to check_blinded_forward from msg.amount_msat > (single HTLC amount) to the MPP total_msat. ... 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? Yes - the blinded path payinfo applies to the aggregate amount that the trampoline relay will forward onward, not to individual MPP parts arriving on the inbound. Document this inline to be clear about why we do this.
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).
Review comment from valentinewallace on lightning/src/ln/channelmanager.rs:8455: "nit: doc comment" Switch the leading // comments above handle_trampoline_htlc to /// so they become proper rustdoc.
Addresses two related review comments: ldk-claude-review-bot on lightning/src/ln/channelmanager.rs:8576: > The as u64 truncation of the u128 result could silently lose data if > forwarding_fee_proportional_millionths is very large. ... the as u64 > would silently truncate, resulting in an *understated* fee, which lets > the attacker get their payment forwarded at below the node's required > fee. valentinewallace on lightning/src/ln/channelmanager.rs:8577: > Can we use router's compute_fees or compute_fees_saturating? valentinewallace on lightning/src/ln/channelmanager.rs:8576: > 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. Replace the hand-rolled `as u64` proportional-fee calculation with router::compute_fees, which uses checked_mul/checked_add internally and returns None on overflow. Chain it into the existing checked_add / checked_sub overflow handling so the trampoline failure path is taken when any step would overflow, rather than silently truncating to an understated fee. Add a comment at the call site explaining why the fee is computed against the next trampoline hop's amount.
remove accumulator entry on MPP completion regardless of failure mode The fee/CLTV underflow paths previously returned without removing the entry from awaiting_trampoline_forwards, leaving a stale accumulator that the MPP-timeout sweep would re-fail. Restructure the match so remove() runs unconditionally on Ok(true), and consume the owned TrampolinePayment for the rest of the function. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Review comment from valentinewallace on lightning/src/ln/onion_utils.rs:2146: > Looks like this can be a closure, which avoids the somewhat awkward > parenthesis around the scid option below Convert the `decoded_onion_failure!` macro inside `decode_onion_failure` into a closure so call sites can pass `Some(scid)` and an `&[u8]` data slice directly without the macro-style parenthesization. Match the patch suggested in valentinewallace/00b24b3c: take `data: &[u8]` and `to_vec()` into the optional `onion_error_data`, drop the now-unneeded `#[allow(unused)]` on the Reason arm.
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.
Review comment from valentinewallace on lightning/src/ln/blinded_payment_tests.rs:2861: > 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 Adopt the patch's core idea: * Extend `create_trampoline_forward_blinded_tail` to also return the underlying `BlindedPaymentPath` so callers can register it in `PaymentParameters`. Update the existing single-path caller to use `.0` on the new tuple. * Build `PaymentParameters::blinded(vec![path_bob, path_barry])` from the two paths and dispatch the MPP via `send_payment_with_route` instead of `test_add_new_pending_payment` + `test_send_payment_along_path`. The registered blinded paths are required, not decorative: `insert_previously_failed_blinded_path` (outbound_payment.rs:2508) is invoked on the trampoline rejection / timeout failures this test exercises, and `debug_assert!`s that the failing tail is registered in payment_params. Also bump Carol's outer-hop `cltv_expiry_delta` from `trampoline_cltv + excess_final_cltv` (112) to `carol_relay.cltv_ expiry_delta + trampoline_cltv + excess_final_cltv` (184). Required by `FixedRouter` (router.rs:765-773), which validates that the last outer- hop's `cltv_expiry_delta` equals `sum(trampoline_hops[*].cltv_expiry_ delta)`. That sum is `blinded_path.payinfo.cltv_expiry_delta + excess_final_cltv_delta`, and the payinfo aggregates the intermediate trampoline relay's `cltv_expiry_delta` (= `carol_relay.cltv_expiry_ delta` from `fwd_tail`) plus `min_final_cltv_expiry_delta` (= `trampoline_cltv`). The `test_send_payment_along_path` helpers bypassed this validation, so 112 worked there; the public API does not. Reverting to 112 panics with "Path had a total trampoline CLTV of 184, which is not equal to the total last-hop CLTV delta of 112". Comment the derivation at the call site so the overloaded "relay" naming doesn't trip up future readers. Return `last_hop_cltv_delta` from `send_trampoline_mpp_payment` so the on-chain timeout branch can compute Carol's incoming HTLC CLTV from the same value the route is built with, rather than re-stating the magic constant 184 in two places.
carlaKC
force-pushed
the
2299-mpp-accumulation
branch
from
b838480 to
e48b8f8
Compare
+ one code move in |
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).