Introduce Realistic constraints for Dummy Hops#4501
Conversation
|
👋 Thanks for assigning @TheBlueMatt 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.
|
No new issues found. The PR has been substantially refactored since my prior review. All five issues I flagged previously are now resolved:
I traced the full new flow and found it sound:
Non-blocking design observation (not posted inline): |
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:
|
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
✅ Added second reviewer: @wpaulino |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Updated .01 → .02 Changes:
|
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
Dummy hops reuse blinded forwarding validation, but an amount or CLTV underflow at this stage occurs while processing a locally peeled dummy hop. Report that context instead of describing the failure as a generic blinded forward so logs identify the failing layer accurately.
Exact forwarding fees in blinded payment paths can reveal unusually specific local policy. Round base and proportional fees upward into common network buckets while retaining the existing CLTV buckets and rejecting unsupported CLTV deltas. Rounding upward preserves sufficient forwarding fees without exposing precise settings. Update the asynchronous payment test to use a bucket-aligned fee so its deterministic route ordering and fee expectations remain stable.
Receive-side accounting needs to distinguish value retained by locally peeled dummy hops from the amount delivered to the final payment payload. Add `dummy_skimmed_fees_msat` to `PaymentClaimable` and `PaymentClaimed`, then thread the value through pending receive state and claimed HTLC bookkeeping. Persist the new fields as optional TLVs with zero defaults so old serialized events and payment state remain readable. Later commits populate the value while peeling dummy hops.
Inbound HTLCs may peel a dummy hop and then defer decoding the rewritten `update_add_htlc` through `ChannelManager`. Storing only the rewritten message loses the fee retained by each dummy hop before the final receive payload is decoded. Store accumulated dummy-hop fees alongside pending update-add state and carry them into receive routing. Use a new serialized field for the extended state while converting the legacy representation with no accumulated fee, preserving restart compatibility.
Onion decoding may peel multiple local dummy hops recursively without passing through deferred `ChannelManager` state. Track the amount removed at each peel and attach the accumulated value when the final receive payload is reached. Use checked accumulation and fail with an onion-blinding error on overflow so receive-side events report the full locally retained fee without weakening validation.
Payment claim helpers previously returned only the amount delivered to the final receive payload. Include `dummy_skimmed_fees_msat` from `PaymentClaimed` so route tests compare the sender's forwarded amount against both the delivered value and fees retained by local dummy hops.
Zero-fee dummy hops are distinguishable from realistic forwarding hops. Derive their fee and CLTV requirements from a nearby forwarding TLV when available, then cap each value at common network defaults to avoid reproducing outlier policies too precisely. Use the same nonzero defaults when no nearby forwarding hop is available. The advertised HTLC maximum bounds the amount entering the payee, including fees retained by its dummy hops. Apply the payee maximum before processing dummy hops and subtract each dummy-hop fee so `compute_payinfo` exposes the largest final amount that can traverse the path. Update asynchronous, blinded-payment, and offer tests for the nonzero dummy-hop fee and CLTV defaults. Account explicitly for conservative fee aggregation and the resulting sender overpayment in claim assertions. AI-assisted: Error Findings Co-Authored-By: OpenAI Codex <codex@openai.com>
Callers building blinded payment paths may need to supply explicit dummy-hop relay parameters for path-length privacy. Make `BlindedPaymentPath::new_with_dummy_hops` public now that fee aggregation, CLTV deltas, and HTLC bounds are covered by the preceding implementation and tests. Remove the stale TODOs for adding dummy hops and validating their end-to-end behavior.
|
Updated .02 → .03 Thanks, @TheBlueMatt — Changes:
Note: |
4 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.