Draft
Conversation
|
👋 Hi! I see this is a draft PR. |
This was referenced Feb 12, 2026
Closed
carlaKC
force-pushed
the
2299-end-to-end
branch
from
e8ac7ab to
0bfc08a
Compare
When we receive an HTLC as a part of a claim, we validate that the CLTV on the HTLC is >= the CLTV that the sender requested we receive, but then we use the CLTV value that the sender requested we receive as the deadline to claim the HTLC anyway. This isn't generally all that interesting (they're always the same unless the previous-hop node gave us "free CLTV"), but for trampoline payments where we're both a trampoline hop and the blinded intro point and the recipient, it means we end up allowing ourselves less claim time than we actually have. Instead, here, we just use the actual HTLC CLTV deadline.
The docs for `RouteHop::cltv_expiry_delta` claim that it includes any trampoline hops, but the way we actually implemented onion building it did not. Because the docs described a simpler and more backwards-compatible API, we update the onion-building logic to match rather than updating the docs.
Now that we've cleaned up trampoline CLTV building and added `Path::total_cltv_expiry_delta`, we can use both to do some basic validation of CLTV values on blinded tails in `Route::debug_assert_route_meets_params`
Now that we are consistently using the `RouteHop::cltv_expiry_delta` as the last hop's starting CLTV rather than summing trampoline hops, `starting_htlc_offset` is a bit confusing - its actually always the current block height. Thus, here we rename it.
- This is a commit that belongs on the prefactor PR once it's rebased.
In the commits that follow, we want to be able to free the other channel without emitting an event so that we can emit a single event for trampoline payments with multiple incoming HTLCs. We still want to go through the full claim flow for each incoming HTLC (and persist the EmitEventAndFreeOtherChannel event to be picked up on restart), but do not want multiple events for the same trampoline forward. Changing from upgradable_required to upgradable_option is forwards compatible - old versions of the software will always have written this field, newer versions don't require it to be there but will be able to read it as-is. This change is not backwards compatible, because older versions of the software will expect the field to be present but newer versions may not write it. An alternative would be to add a new event type, but that would need to have an even TLV (because the event must be understood and processed on restart to claim the incoming HTLC), so that option isn't backwards compatible either.
In preparation for trampoline failures, allow multiple previous channel ids. We'll only emit a single HTLCHandlingFailed for all of our failed back HTLCs, so we want to be able to express all of them in one event.
This commit adds a SendHTLCId for trampoline forwards, identified by their session_priv. As with an OutboundRoute, we can expect our HTLC to be uniquely identified by a randomly generated session_priv. TrampolineForward could also be identified by the set of all previous outbound scid/htlc id pairs that represent its incoming HTLC(s). We choose the 32 byte session_priv to fix the size of this identifier rather than 16 byte scid/id pairs that will grow with the number of incoming htlcs.
We only have payment details for HTLCSource::TrampolineForward available once we've dispatched the payment. If we get to the stage where we need a HTLCId for the outbound payment, we expect dispatch details to be present. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>
To create the right handling type based on source, add a helper. This is mainly useful for PreviousHopData/TrampolineForward. This helper maps an OutboundRoute to a HTLCHandlingFailureType::Forward. This value isn't actually used once we reach `forward_htlc_backwards_internal`, because we don't emit `HTLCHandlingFailed` events for our own payments. This issue is pre-existing, and could be addressed with an API change to the failure function, which is left out of scope of this work.
Will need to share this code when we add trampoline forwarding. This commit exactly moves the logic as-is, in preparation for the next commit that will update to suit trampoline. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>
When we introduce trampoline forwards, we're going to want to provide two external pieces of information to create events: - When to emit an event: we only want to emit one trampoline event, even when we have multiple incoming htlcs. We need to make multiple calls to claim_funds_from_htlc_forward_hop to claim each individual htlc, which are not aware of each other, so we rely on the caller's closure to decide when to emit Some or None. - Forwarding fees: we will not be able to calculate the total fee for a trampoline forward when an individual outgoing htlcs is fulfilled, because there may be other outgoing htlcs that are not accounted for (we only get the htlc_claim_value_msat for the single htlc that was just fulfilled). In future, we'll be able to provide the total fee from the channelmanager's top level view.
Implement payment claiming for `HTLCSource::TrampolineForward` by iterating through previous hop data and claiming funds for each HTLC. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>
We'll want this extracted when we need to handle trampoline and regular forwards. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>
Implement failure propagation for `HTLCSource::TrampolineForward` by iterating through previous hop data and failing each HTLC with `TemporaryTrampolineFailure`. Note that testing should be implemented when trampoline forward is completed. Co-authored-by: Arik Sosman <git@arik.io> Co-authored-by: Maurice Poirrier <mpch@hey.com>
We're going to need to keep track of our trampoline HLTCs in the same way that we keep track of incoming MPP payment to allow them to accumulate on our incoming channel before forwarding them onwards to the outgoing channel. To do this we'll need to store the payload values we need to remember for forwarding in OnionPayload. - [ ] Readable for ClaimableHTLC is incomplete
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 commits adds a new map in channel manager, and mimics the handling done for claimable_payments. This map is not placed in claimable_payments because we'll need to be able to lock pending_outbound_payments in the commits that follow while holding a lock on our set of trampoline payments (which is not possible with claimable_payments). - [ ] Need to add persistence of trampoline payments
We're going to use the same logic for trampoline and for incoming MPP payments, so we pull this out into a separate function.
We'll only use this for non-trampoline incoming accumulated htlcs, because we want different source/failure for trampoline.
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 enabling it.
For trampoline payments, we don't want to enforce a minimum cltv delta between our incoming and outer onion outgoing CLTV because we'll calculate our delta from the inner trampoline onion's value. However, we still want to check that we get at least the CLTV that the sending node intended for us and we still want to validate our incoming value. Refactor to allow setting a zero delta, for use for trampoline payments.
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. Now that we've got rejection on inbound MPP accumulation, we relax this check to allow testing of inbound MPP trampoline processing.
Use even persistence value because we can't downgrade with a trampoline payment in flight, we'll fail to claim the appropriate incoming HTLCs. We track previous_hop_data in `TrampolineForwardInfo` so that we have it on hand in our `OutboundPayment::Retryable`to build `HTLCSource` for our retries.
When we are forwading as a trampoline within a blinded path, we need to be able to set a blinding point in the outer onion so that the next blinded trampoline can use it to decrypt its inner onion. This is only used for relaying nodes in the blinded path, because the introduction node's inner onion is encrypted using its node_id (unblinded) pubkey so it can retrieve the path key from inside its trampoline onion. Relaying nodes node_id is unknown to the original sender, so their inner onion is encrypted with their blinded identity. Relaying trampoline nodes therefore have to include the path key in the outer payload so that the inner onion can be decrypted, which in turn contains their blinded data for forwarding. This isn't used for the case where we're the sending node, because all we have to do is include the blinding point for the introduction node. For relaying nodes, we just put their encrypted data inside of their trampoline payload, relying on nodes in the blinded path to pass the blinding point along.
When we're a forwarding trampoline and we receive a final error from our route, we want to propagate that failure back to the original sender. Surface the information so that it's available to us.
- [ ] Check whether we can get away with checking path.hops[0] directly (outbound_payment should always be present?)
Similar to forwards, we need to block on the incoming channel storing our preimage to safely revoke on the outbound channel.
The blinding point that we pass in is supposed to be the "update add" blinding point equivalent, which in blinded trampoline relay is the one that we get in the outer onion.
We failed here to prevent downgrade to versions of LDK that didn't have full trampoline support. Now that we're done, we can allow reads.
To enable trampoline forwarding fully, remove the forced error introduced to prevent forwarding trampoline payments when we weren't ready.
Don't always blindly replace with a manually built test onion when we run trampoline tests (only for unblinded / failure cases where we need to mess with the onion). The we update our replacement onion logic to correctly match our internal behavior which adds one block to the current height when dispatching payments.
- [ ] Right now, we assume that the presence of a trampoline means
that we're in a blinded route. This fails when we test an
unblinded case (which we do to get coverage for forwarding).
We likely need to decouple trampoline and blinded tail to allow
this to work properly.
carlaKC
force-pushed
the
2299-end-to-end
branch
from
0bfc08a to
8002678
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains all the changes to support trampoline forwarding in LDK.
It depends on #4304 and #4402 (and thus also #4373).
This obviously needs to be broken up into parts, and could certainly use some cleaning up (specifically around replays), but opening it up early to provide some context for the decisions make in #4304.