Introduce Realistic constraints for Dummy Hops#4501
Introduce Realistic constraints for Dummy Hops#4501shaavan wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
When amount or CLTV validation underflows on a dummy hop, return a dummy-hop-specific error. This avoids misleading logs and makes blinded-path failures easier to debug.
Propagate dummy-hop skimmed fees through HTLC peeling, channel persistence, and receive-side payment tracking. Expose the accumulated value on `PaymentClaimable` and `PaymentClaimed`. This keeps `amount_msat` focused on the amount delivered to the final receive TLVs, while still surfacing the additional revenue from dummy hops. Receivers can account for these fees without changing the meaning of existing skimmed-fee fields.
Update functional test helpers and their callers to derive per-path claim amounts and expected forwarded values when dummy hops are present, including cumulative fees across multiple dummy hops. Without this, tests claiming blinded payments with dummy hops treat the payee's `amount_msat` as the amount forwarded through the route, which no longer matches the wire amounts once dummy-hop fees are applied.
Replace zeroed dummy-hop relay parameters with non-trivial default fee, CLTV, and HTLC minimum values, and propagate these defaults into blinded payinfo construction and routing tests. Dummy hops only preserve their privacy value if they behave like plausible forwarding hops. Realistic defaults ensure hidden hops affect fees and CLTV like real relays, instead of standing out as zero-cost padding.
Expand dummy-hop blinded payment tests to cover advertised payinfo behavior end to end. Introduce an underpayment case that strips dummy-hop relay fees from the sender's payinfo, and assert that receive-path construction exposes the expected aggregated base fee and CLTV delta for non-default dummy hops. Also verify that under-advertising the blinded path HTLC minimum causes the receiver to reject the payment while processing hidden dummy hops. Co-Authored-By: OpenAI Codex <codex@openai.com>
Allow external callers to construct blinded receive paths with dummy hops via `new_with_dummy_hops`.
|
I've assigned @wpaulino as a reviewer! |
| }, { | ||
| (0, blinding_point, option), | ||
| (65537, skimmed_fee_msat, option), | ||
| (65539, dummy_hops_skimmed_fee_msat, option), |
There was a problem hiding this comment.
Bug: dummy_hops_skimmed_fee_msat is serialized in the wire update_add_htlc message, but it should be an internal-only field. The field is only meaningful for the receiver's local dummy-hop peeling, never transmitted between peers.
Because it appears in the wire format, a malicious channel peer can inject an arbitrary value. That injected value then flows — unsanitized — into PaymentClaimable and PaymentClaimed events (via channelmanager.rs:5194 and onion_utils.rs:2578), misleading wallet software about dummy-hop fee earnings.
The receiver should either:
- Strip/ignore
dummy_hops_skimmed_fee_msatfrom the deserialized wire message before processing (e.g., force it toNone), or - Remove it from the wire serialization entirely and only carry it in an internal struct used during local dummy-hop peeling.
As-is, any value a peer sets in TLV 65539 gets added to the real accumulated dummy-hop fees.
| create_recv_pending_htlc_info(decoded_hop, shared_secret, msg.payment_hash, | ||
| msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat, | ||
| msg.amount_msat, msg.cltv_expiry, None, allow_underpay, | ||
| msg.dummy_hops_skimmed_fee_msat, msg.skimmed_fee_msat, | ||
| msg.accountable.unwrap_or(false), current_height) |
There was a problem hiding this comment.
Bug: msg.dummy_hops_skimmed_fee_msat comes directly from the wire-deserialized UpdateAddHTLC sent by our channel peer. A malicious peer can set this to any value, and it will propagate into the PendingHTLCInfo, then into the ClaimableHTLC, and finally into the PaymentClaimable / PaymentClaimed events — corrupting the reported dummy-hop fee for the application layer.
This should be forced to None here (or earlier, at deserialization) since the real value is only determined by local dummy-hop peeling in process_pending_update_add_htlcs.
| let this_dummy_fee = msg.amount_msat.saturating_sub(outgoing_amt_msat); | ||
| let dummy_hops_skimmed_fee_msat = | ||
| msg.dummy_hops_skimmed_fee_msat.unwrap_or(0).saturating_add(this_dummy_fee); |
There was a problem hiding this comment.
Related to the wire-injection issue: msg.dummy_hops_skimmed_fee_msat originates from the deserialized wire message. If a malicious peer injects a large value, it gets accumulated here with the real dummy-hop fee. The initial value should always be 0, not the untrusted wire value.
Consider changing line 2578 to always start accumulation from 0 for the first dummy hop, or ensure the field is stripped from the wire message before this function is called.
| payment_id: PaymentId([42; 32]), | ||
| bolt12_invoice: None, | ||
| }, | ||
| dummy_hops_skimmed_fee_msat: None, | ||
| skimmed_fee_msat: None, | ||
| blinding_point: None, |
There was a problem hiding this comment.
Broken indentation: payment_id and bolt12_invoice (fields of the inner HTLCSource::OutboundRoute) are under-indented by one tab level, and dummy_hops_skimmed_fee_msat, skimmed_fee_msat, blinding_point (fields of the outer OutboundHTLCOutput) are also under-indented. While this compiles, it makes the struct nesting misleading. Running cargo fmt should fix this.
| state: OutboundHTLCState, | ||
| source: HTLCSource, | ||
| blinding_point: Option<PublicKey>, | ||
| dummy_hops_skimmed_fee_msat: Option<u64>, |
There was a problem hiding this comment.
Compilation error: This new required field is missing from many OutboundHTLCOutput construction sites in the BOLT spec test vectors later in this file (e.g., lines ~16869, ~16885, ~17301, ~17556, ~17700, ~17935, and others). There are at least 12 construction sites that don't include dummy_hops_skimmed_fee_msat, which will cause a compilation failure.
Each of these sites needs dummy_hops_skimmed_fee_msat: None, added.
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4501 +/- ##
==========================================
+ Coverage 86.05% 86.20% +0.14%
==========================================
Files 159 160 +1
Lines 105759 107721 +1962
Branches 105759 107721 +1962
==========================================
+ Hits 91013 92857 +1844
- Misses 12227 12237 +10
- Partials 2519 2627 +108
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:
|
3 participants
Resolves #4326. Follow-up to #4152.
Overview
This PR makes dummy hops in blinded payment paths behave like real forwarding relays across construction, routing, receive handling, and testing.
It introduces realistic relay parameters, propagates dummy-hop fees end to end, and ensures that hidden-hop constraints are enforced consistently. It also exposes a public constructor for building blinded paths with dummy hops, backed by end-to-end test coverage.
This change is driven by a core requirement: dummy hops only provide privacy if they are indistinguishable from real relays.
Previously, dummy hops used zeroed parameters and incomplete fee handling, which could make them stand out and lead to inconsistencies between advertised payinfo and actual enforcement. This PR removes those gaps and aligns dummy-hop behavior with real routing semantics.
Key Changes
Align dummy-hop behavior with real relay semantics: Introduce realistic relay parameters (fees, CLTV delta, HTLC minimum), enforce hidden-hop constraints during receive handling, and return errors for accurate failure signaling.
Ensure end-to-end correctness and observability: Propagate dummy-hop fees through receive handling and expose them via
PaymentClaimableandPaymentClaimed, update test helpers and add end-to-end coverage (payinfo, underpayment, HTLC minimum), and makeBlindedPaymentPath::new_with_dummy_hopspublic with validated behavior.