Review Branch: Dummy Hop support for Blinded Payment Path #7
Conversation
WalkthroughAdds configurable dummy-hop support to blinded payment paths: new Dummy TLV type, path constructors supporting dummy hop counts and ReceiveAuthKey, updated TLV (de)serialization and onion payload handling for Dummy hops, router defaults for dummy hops, refactors in onion forwarding, and test/tooling updates to exercise dummy-hop behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender
participant Intro as Intro Node
participant Dummy as Dummy Hop(s)
participant Final as Final Node
Sender->>Intro: HTLC with encrypted onion (blinded)
Intro->>Intro: Decrypt TLV -> Forward
Intro->>Dummy: Forward to dummy hop (Dummy TLV)
rect rgb(230,240,255)
Note over Dummy: repeated Dummy hops
Dummy->>Dummy: Decrypt Dummy TLV, peel, update blinding point
Dummy->>Dummy: Forward to next (may be another Dummy)
end
Dummy->>Final: Forward final (Receive TLV)
Final->>Final: Decrypt Receive TLV, extract pay info
Final->>Sender: Claim / fulfill payment
sequenceDiagram
participant Builder
participant Path as BlindedPaymentPath
participant Auth as ReceiveAuthKey
Builder->>Auth: obtain receive auth key
Builder->>Path: new_with_dummy_hops(intermediates, payee, dummy_count, auth_key, payee_tlvs, ...)
rect rgb(230,240,255)
Note over Path: construction
Path->>Path: create forward hops, sign one_hop with auth key
Path->>Path: clamp dummy_count to MAX_DUMMY_HOPS_COUNT
Path->>Path: insert Dummy TLVs and pad stream
Path->>Path: append final Receive TLV
end
Path->>Builder: return constructed blinded path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
lightning/src/ln/onion_utils.rs (2)
1683-1690: Additions to LocalHTLCFailureReason are soundNew UnauthenticatedPayload variant and docs look good. Consider explicitly mentioning ReceiveAuthKey-based auth in the doc for clarity.
1731-1731: Wire code mapping aligns with intentMapping UnauthenticatedPayload to the same BADONION | PERM | 24 as InvalidOnionBlinding is correct. Note: From will decode to InvalidOnionBlinding for this code; if intentional, add a brief comment to prevent future confusion.
lightning/src/ln/onion_payment.rs (1)
248-267: New auth gate for blinded receives; verify external error mappingGood addition: early reject unauthenticated receive payloads. Two follow-ups:
- Verify how LocalHTLCFailureReason::UnauthenticatedPayload is surfaced on the wire. If you intend to avoid leaking auth status, consider mapping it to InvalidOnionPayload at the boundary.
- Optional: downgrade log_warn to log_info/log_debug to reduce noise from unauthenticated probes.
lightning/src/ln/msgs.rs (1)
3773-3773: Minor: scope of receive_auth_key.Defined once per decode; OK. If you want to avoid potential “unused” warnings in future refactors, define it inside the blinded-branch.
lightning/src/ln/channelmanager.rs (1)
18746-18750: Avoid redundant shadowing of current_height.If current_height was (or will be) declared earlier as suggested, remove this duplicate let to reduce shadowing.
Apply if earlier declaration exists:
- let current_height: u32 = node[0].node.best_block.read().unwrap().height;lightning/src/blinded_path/payment.rs (2)
697-699: Avoid referencing a temporary&PaymentDummyTlvin the iteratorUsing
&PaymentDummyTlvinsidemaptakes a reference to a temporary. While ZSTs are often const-promoted, it’s clearer and safer to bind once and reuse.Apply:
- let tlvs = intermediate_nodes - .iter() - .map(|node| BlindedPaymentTlvsRef::Forward(&node.tlvs)) - .chain((0..dummy_count).map(|_| BlindedPaymentTlvsRef::Dummy(&PaymentDummyTlv))) - .chain(core::iter::once(BlindedPaymentTlvsRef::Receive(&payee_tlvs))); + let dummy_tlv = PaymentDummyTlv; + let dummy_iter = core::iter::repeat(BlindedPaymentTlvsRef::Dummy(&dummy_tlv)) + .take(dummy_count); + let tlvs = intermediate_nodes + .iter() + .map(|node| BlindedPaymentTlvsRef::Forward(&node.tlvs)) + .chain(dummy_iter) + .chain(core::iter::once(BlindedPaymentTlvsRef::Receive(&payee_tlvs)));Also applies to: 683-705
679-681: Align payment padding round-off with message padding (if intended)
PAYMENT_PADDING_ROUND_OFF = 30differs from message padding. Consider sharing a common constant or documenting why they differ for onion size uniformity.lightning/src/offers/flow.rs (1)
320-356: Routing now threadsReceiveAuthKeycorrectly; consider HTLC min source
- Passing
receive_auth_keyintoRouter::create_blinded_payment_pathsis correct.- Consider deriving
htlc_minimum_msatfrom channel mins or policy instead of hardcoding1.Also applies to: 342-346
fuzz/src/invoice_request_deser.rs (1)
86-87: Derive ReceiveAuthKey instead of using a constant to improve fuzz coverageUse entropy to vary the key so more paths are explored.
- let receive_auth_key = ReceiveAuthKey([41; 32]); + let receive_auth_key = ReceiveAuthKey(entropy_source.get_secure_random_bytes());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(21 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(12 hunks)lightning/src/ln/channelmanager.rs(10 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(13 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(4 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
lightning/src/ln/onion_payment.rs (2)
lightning/src/ln/onion_utils.rs (7)
shared_secret(2283-2294)new(783-783)new(2168-2168)new(2172-2174)new(2178-2180)new(2735-2737)new(3953-3955)lightning/src/ln/channel.rs (6)
new(1091-1093)new(1205-1214)new(12825-12876)new(13190-13241)None(11074-11074)None(14118-14118)
lightning/src/ln/max_payment_path_len_tests.rs (2)
lightning/src/blinded_path/payment.rs (1)
new(120-139)lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/ln/blinded_payment_tests.rs (4)
lightning/src/blinded_path/payment.rs (1)
new(120-139)lightning/src/offers/flow.rs (1)
get_receive_auth_key(192-194)lightning/src/util/test_utils.rs (2)
get_receive_auth_key(1730-1732)get_receive_auth_key(1822-1824)lightning/src/sign/mod.rs (3)
get_receive_auth_key(880-880)get_receive_auth_key(2220-2222)get_receive_auth_key(2393-2395)
lightning/src/ln/msgs.rs (2)
lightning/src/blinded_path/payment.rs (1)
new(120-139)lightning/src/ln/channelmanager.rs (2)
new(3847-3925)new(16114-16135)
lightning/src/routing/router.rs (4)
lightning/src/offers/flow.rs (1)
create_blinded_payment_paths(322-356)fuzz/src/chanmon_consistency.rs (1)
create_blinded_payment_paths(130-136)fuzz/src/full_stack.rs (1)
create_blinded_payment_paths(158-164)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)
lightning/src/blinded_path/payment.rs (2)
lightning/src/blinded_path/message.rs (4)
new_with_dummy_hops(91-119)new(68-85)blinded_hops(171-173)blinded_hops(693-733)lightning/src/ln/msgs.rs (8)
new(2510-2512)new(2545-2547)read(1147-1171)read(1175-1181)read(3345-3356)read(3405-3409)read(3592-3763)read(3770-3912)
lightning/src/offers/flow.rs (3)
lightning/src/routing/router.rs (4)
create_blinded_payment_paths(129-222)create_blinded_payment_paths(247-255)create_blinded_payment_paths(288-292)a(3619-3619)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)lightning/src/ln/channelmanager.rs (2)
usable_channels(5743-5743)test_create_blinded_payment_paths(12930-12942)
lightning/src/util/test_utils.rs (1)
lightning/src/routing/router.rs (14)
first_hops(156-207)first_hops(7886-7886)first_hops(7910-7910)first_hops(8313-8313)first_hops(8323-8323)first_hops(8382-8382)first_hops(8542-8542)first_hops(8622-8622)first_hops(8687-8687)first_hops(8741-8741)first_hops(8861-8861)first_hops(8877-8877)first_hops(8889-8889)first_hops(9033-9033)
🪛 GitHub Actions: Continuous Integration Checks
lightning/src/ln/msgs.rs
[error] 2351-2351: field payment_tlvs_authenticated is never read
[error] 2351-2351: could not compile lightning (lib) due to 1 previous error
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: check_release
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: semver-checks
- GitHub Check: ext-test
🔇 Additional comments (41)
fuzz/src/full_stack.rs (1)
159-161: Router signature update looks correctFuzzRouter now matches the trait’s new receive_auth_key parameter; unused params are fine here. Nothing else to change.
Please confirm all other Router impls and call sites were updated consistently (build should catch mismatches).
lightning/src/ln/onion_utils.rs (2)
1891-1892: Serialization IDs appended safelyAdding (42, ChannelBalanceOverdrawn) and (43, UnauthenticatedPayload) at the end maintains backward compatibility of the TLV enum. Looks good.
Double‑check no prior release used these indices for different variants.
2053-2054: Debug assertions updated appropriatelyExtending the data-length assertion to UnauthenticatedPayload (32 bytes) matches InvalidOnionBlinding behavior. LGTM.
fuzz/src/refund_deser.rs (1)
71-81: LGTM: fuzz path updated to new auth modelSwitched to ReceiveTlvs and passing ReceiveAuthKey into BlindedPaymentPath::new correctly. No issues.
Also applies to: 99-109
lightning/src/ln/max_payment_path_len_tests.rs (1)
215-235: LGTM: blinded path construction updatedReceiveTlvs and receive_auth_key are correctly threaded into BlindedPaymentPath::new. Path-length/packet-size assertions remain valid.
lightning/src/ln/onion_payment.rs (1)
460-504: Logger deref threading is consistentPassing logger.deref() into decode/create helpers matches the generic L: Deref<Target=Logger> pattern and avoids cloning contexts. Looks good.
lightning/src/ln/msgs.rs (8)
59-59: ChaChaDualPolyReadAdapter import looks correct.Adapter change aligns with adding AAD-based auth; no issues spotted.
3661-3666: Correct: thread receive_auth_key into decrypt; derive rho from ECDH.The switch to ChaChaDualPolyReadAdapter with (rho, receive_auth_key) is sound.
Ensure node_signer.get_receive_auth_key() is stable and non-blocking; if it can fail or rotate, confirm caller expectations.
3676-3683: Good validation: forbid AAD on forward hops.Rejecting used_aad for Forward prevents misrouted/forged use of authenticated TLVs on non-final hops.
3694-3705: Dummy variant decode is fine; carries auth bit forward.No logical issues; field propagation is consistent.
3710-3728: Receive-path propagation looks correct.You pass used_aad into BlindedReceive payload; constraints and bounds checks remain intact.
3826-3844: Trampoline: same forward-hop AAD gating — good.Mirrors non-trampoline path and rejects unexpected authenticated TLVs.
3859-3877: Trampoline receive-path propagation — consistent.Auth flag plumbed through; looks good.
2342-2343: Revert dead_code allowance on payment_tlvs_authenticated
The flag is read in msgs.rs (decode paths) and in onion_payment.rs; no dead_code lint will fire. Remove the proposed#[cfg_attr(not(fuzzing), allow(dead_code))]annotations.Likely an incorrect or invalid review comment.
lightning/src/ln/channelmanager.rs (9)
35-41: Imports align with the payment-context refactor.No issues spotted.
4881-4885: Correctly propagating height and logger.Passing current_height and &*self.logger to create_recv_pending_htlc_info matches the updated signature.
6992-6996: Consistent logger propagation.Forwarding &*self.logger here is consistent with the new API.
14814-14816: Router injected into invoice-request flow looks good.This aligns with creating blinded payment paths in the flow.
17879-17880: Test utilities import is appropriate.TestLogger is used below; no concerns.
18701-18707: Test logger setup LGTM.Creation and later passing of &logger are correct.
18723-18727: Verify current_height declaration order in test.current_height is used here; ensure it’s declared before this call (and not only later at Line 18746). If it wasn’t previously declared, move the declaration above this use; if it was, avoid re-declaring it later.
Suggested adjustment if needed (insert before this call):
+ let current_height: u32 = node[0].node.best_block.read().unwrap().height;
18759-18762: Minor test setup change LGTM.Logger instantiation is fine.
18772-18775: Updated call signature handled correctly.Passing current_height and &logger matches the new API.
lightning/src/routing/router.rs (6)
31-31: Import of ReceiveAuthKey looks goodRequired for the new authenticated blinded paths API.
183-186: Use of saturating_add for max_cltv_expiry is correctPrevents u32 overflow when composing per-hop deltas into the payee constraints. Good safety improvement.
201-205: Passing receive_auth_key into BlindedPaymentPath::newThreading the auth key into blinded path construction aligns with the new authenticated flow; call ordering looks correct in both multi-hop and dummy-hop branches.
Also applies to: 213-216
248-251: FixedRouter signature updateSignature matches the trait change; unreachable/Err behavior preserved.
285-293: Trait docs updated to reflect authentication requirementDoc and signature changes are consistent with the new model.
132-134: Public API change:Router::create_blinded_payment_pathsaddsreceive_auth_key
All internalRouterimpls (DefaultRouter, test utils, fuzz harness) have been updated to include the newreceive_auth_keyparameter. Verify any external Rust or FFI bindings for compatibility. Consider accepting&ReceiveAuthKeyto avoid unnecessary copies.lightning/src/blinded_path/payment.rs (3)
549-556: Reusing TLV type 65539 for Dummy: confirm wire-compat expectationsYou encode Dummy as
(65539, ())and decode withis_dummy. Given the comment about removing prior auth TLV, confirm:
- Older peers won’t misinterpret 65539 in payment contexts.
- We never receive legacy payloads requiring the old field.
Also applies to: 558-563, 595-600
141-150: API for dummy hops looks good; dummy count is bounded
new_with_dummy_hops+ capping viaMAX_DUMMY_HOPS_COUNTis clean and prevents overflow of padding.Also applies to: 688-694
762-767:compute_payinfochange to&ReceiveTlvsis consistentUsing payee constraints from
ReceiveTlvsintegrates with the new auth flow without behavioral regressions.Also applies to: 936-944
lightning/src/offers/flow.rs (4)
942-956: Invoice response flow updated to use new router signature: LGTMSwitch to router-based blinded path creation with
ReceiveTlvsintegrates with the new auth model.Also applies to: 959-967
812-867: Static invoice builder flow updated: LGTMDropping
entropy_sourcehere and relying on router and storedReceiveAuthKeyis coherent.Also applies to: 1642-1661
1298-1402: Async refresh plumbing changes look consistentRouter generics and calls align after the signature changes. No functional regressions spotted.
Also applies to: 1396-1456
312-318: ReceiveAuthKey is Copy, by-value use is safe
ReceiveAuthKey derives Copy/Clone, so returning it from &self simply copies it without moving ownership.lightning/src/util/test_utils.rs (1)
290-304: ReceiveAuthKey threading in TestRouter looks correctSignature and forwarding to inner router align with the updated API. No issues spotted.
lightning/src/ln/async_payments_tests.rs (1)
861-863: Updated TLV access is consistentUsing tlvs.payment_constraints.max_cltv_expiry under Receive variant looks right.
lightning/src/offers/signer.rs (1)
39-51: Reserved HMAC inputs documentedGood defensive note reserving [5;16], [7;16], [8;16]. No behavior change.
fuzz/src/invoice_request_deser.rs (1)
134-143: BlindedPaymentPath::new call matches updated signatureArgument order (recipient, receive_auth_key, payee_tlvs, …) is correct. LGTM.
fuzz/src/chanmon_consistency.rs (1)
130-135: Router stub updated to new signatureSignature matches the new API; no functional impact in fuzz stub.
| payment_secret, | ||
| payment_constraints: PaymentConstraints { | ||
| max_cltv_expiry: u32::max_value(), | ||
| htlc_minimum_msat: chan_upd.htlc_minimum_msat, | ||
| }, | ||
| payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}), | ||
| }; | ||
| let nonce = Nonce([43u8; 16]); | ||
| let mut payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key); | ||
| payee_tlvs.authentication.1 = Nonce([0u8; 16]); | ||
| // Use a mismatched ReceiveAuthKey to force auth failure: | ||
| let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Align test expectation with new unauthenticated failure reason
create_recv_pending_htlc_info now returns UnauthenticatedPayload on auth failure. This test still expects InvalidOnionPayload. Update the expectation to UnauthenticatedPayload.
- expect_payment_failed_conditions(
- &nodes[0], payment_hash, true,
- PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[]),
- );
+ expect_payment_failed_conditions(
+ &nodes[0], payment_hash, true,
+ PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::UnauthenticatedPayload, &[]),
+ );If you intend to deliberately coalesce UnauthenticatedPayload to InvalidOnionPayload for external exposure, please confirm the mapping and keep the test as-is; otherwise, apply the diff above. To check usage sites:
Also applies to: 1452-1456
🏁 Script executed:
#!/bin/bash
rg -nP 'UnauthenticatedPayload|InvalidOnionPayload|failure_code\(\)' -C3Length of output: 35327
Replace InvalidOnionPayload with UnauthenticatedPayload in tests
In lightning/src/ln/blinded_payment_tests.rs update both expectations:
- PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[])
+ PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::UnauthenticatedPayload, &[])Apply this change at lines 1410–1419 and again at 1452–1456.
🤖 Prompt for AI Agents
In lightning/src/ln/blinded_payment_tests.rs around lines 1410–1419 and again
around 1452–1456, the tests assert the wrong onion error variant; replace
expectations of InvalidOnionPayload with UnauthenticatedPayload in both places
so the test checks for the UnauthenticatedPayload enum variant instead of
InvalidOnionPayload, updating any pattern matches or assert_eq! calls to use
UnauthenticatedPayload.
There was a problem hiding this comment.
@coderabbitai See carefully, the LocalHTLCFailureReason::UnauthenticatedPayload variant doesn't exist anymore
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for LocalHTLCFailureReason enum definition
ast-grep --pattern 'pub enum LocalHTLCFailureReason {
$$$
}'
Length of output: 12900
🏁 Script executed:
#!/bin/bash
# Also search with rg for the enum definition
rg -A 30 "enum LocalHTLCFailureReason" --type rust
Length of output: 2458
🏁 Script executed:
#!/bin/bash
# Search for authentication failure handling in the codebase
rg "authentication.*fail|auth.*fail" --type rust -C 3 | head -100
Length of output: 3367
🏁 Script executed:
#!/bin/bash
# Check what failure reason is used in create_recv_pending_htlc_info for auth failures
rg -A 10 "create_recv_pending_htlc_info" lightning/src/ln/onion_payment.rs | head -50
Length of output: 934
🏁 Script executed:
#!/bin/bash
# Search for how ReceiveAuthKey validation/authentication is handled
rg "ReceiveAuthKey|verify_for_context|authenticate" lightning/src/ln/onion_payment.rs -C 5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for authentication validation in blinded path payment code
rg "authenticate|verify.*auth" lightning/src/blinded_path/payment.rs -C 5
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Look for where authentication failures result in specific error codes
rg "InvalidOnionPayload|InvalidOnionBlinding" lightning/src/ln/onion_payment.rs -B 3 -A 3
Length of output: 4773
🏁 Script executed:
#!/bin/bash
# Search for where receive TLVs are decoded and authenticated
rg -A 20 "BlindedReceive|decrypt.*payload" lightning/src/ln/onion_payment.rs | head -80
Length of output: 3158
🏁 Script executed:
#!/bin/bash
# Check what happens when decoding blinded receive payloads fails
rg "BlindedPaymentTlvs::Receive" lightning/src/ln/msgs.rs -B 5 -A 10
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Look for the actual authentication/decryption in the msgs module
rg "decrypt|authenticate" lightning/src/ln/msgs.rs -C 5 | head -100
Length of output: 2062
🏁 Script executed:
#!/bin/bash
# Search for ChaChaDualPolyReadAdapter and how it handles authentication failures
rg "ChaChaDualPolyReadAdapter" lightning/src/ln/msgs.rs -B 10 -A 15 | head -150
Length of output: 4839
🏁 Script executed:
#!/bin/bash
# Look for DecodeError::InvalidValue in the context of authentication
rg "DecodeError::InvalidValue" lightning/src/ln/msgs.rs -B 3 -A 3 | head -80
Length of output: 2815
🏁 Script executed:
#!/bin/bash
# Check what happens when used_aad is false (authentication fails)
rg "used_aad" lightning/src/ln/msgs.rs -C 8
Length of output: 2544
🏁 Script executed:
#!/bin/bash
# Check how DecodeError is mapped to LocalHTLCFailureReason
rg "DecodeError.*InvalidValue" lightning/src/ln/onion_payment.rs -B 5 -A 5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where decode errors become HTLC failures
rg "decode.*onion|DecodeError" lightning/src/ln/onion_payment.rs -C 5 | head -100
Length of output: 3157
🏁 Script executed:
#!/bin/bash
# Check the decode_incoming function to see error mapping
rg -A 30 "pub.*fn decode_incoming_update_add_htlc_onion" lightning/src/ln/onion_payment.rs | head -60
Length of output: 1578
🏁 Script executed:
#!/bin/bash
# Find where InboundOnionPayload is decoded and how errors are mapped
rg "InboundOnionPayload::read" lightning/src/ln/onion_utils.rs -B 5 -A 15
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how the decode error flows in decode_incoming_update_add_htlc_onion
rg -A 50 "let hop = InboundOnionPayload::read" lightning/src/ln/onion_payment.rs
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the complete flow from decode to error
rg "peel_onion|decode_next_untagged_hop" lightning/src/ln/onion_payment.rs -A 20 | head -100
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for how onion decode errors map to failure codes when blinding is involved
rg "DecodeError" lightning/src/ln/onion_utils.rs -C 5 | head -100
Length of output: 2731
🏁 Script executed:
#!/bin/bash
# Check what happens in the test - look at the actual failure path
rg -A 20 "fails_receive_tlvs_authentication" lightning/src/ln/blinded_payment_tests.rs | head -80
Length of output: 937
🏁 Script executed:
#!/bin/bash
# Look for how blinded path decode failures are handled
rg "blinding.*InvalidOnion|InvalidOnion.*blinding" --type rust -C 3
Length of output: 701
@shaavan You're absolutely correct that LocalHTLCFailureReason::UnauthenticatedPayload doesn't exist. Looking at the code flow, here's what actually happens:
When authentication fails in a blinded path:
- Authentication fails →
ChaChaDualPolyReadAdapterreturnsDecodeError::InvalidValuewhen!used_aad - DecodeError mapped →
InvalidValueinitially maps toLocalHTLCFailureReason::InvalidOnionPayload - Blinded path override → In
decode_incoming_update_add_htlc_onion, there's special logic:if msg.blinding_point.is_some() || failure_reason == LocalHTLCFailureReason::InvalidOnionBlinding { ([0; 32], LocalHTLCFailureReason::InvalidOnionBlinding) }
Since the message has a blinding_point set (it's a blinded path), any failure reason gets converted to InvalidOnionBlinding to avoid leaking information about the failure.
Therefore, the test expectation should be LocalHTLCFailureReason::InvalidOnionBlinding, not InvalidOnionPayload:
expect_payment_failed_conditions(
&nodes[0], payment_hash, true,
- PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[]),
+ PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionBlinding, &[0; 32]),
);Note: The error data should also be &[0; 32] (32 zero bytes) for blinded path failures.
🧬 Code Graph Analysis Results
[relevant_code_snippets]
-
lightning/src/ln/onion_payment.rs
-
Summary (documenting failure variants and where UnauthenticatedPayload would appear)
- Path: 531-652
- Description: This snippet defines the decode_incoming_update_add_htlc_onion function which handles onion decoding for UpdateAddHTLC messages. It includes handling for various failure cases, notably including InvalidOnionPayload and InvalidOnionBlinding. There is no reference to a variant named UnauthenticatedPayload in LocalHTLCFailureReason; the code paths shown demonstrate the failure variants that are actually supported (e.g., InvalidOnionPayload, InvalidOnionBlinding) and how they are encoded into UpdateFailMalformedHTLC or related structures.
- Key items:
- Failure handling branches for different onion decode outcomes (Malformed vs Relay paths).
- Use of LocalHTLCFailureReason values such as InvalidOnionPayload and InvalidOnionBlinding.
- Construction of the corresponding HTLC failure messages from those reasons.
-
File/lines:
- File: lightning/src/ln/onion_payment.rs
- Lines: 531-652
-
-
lightning/src/ln/onion_payment.rs
- Contextual reference (usage in tests indicating existing variants)
- This area also demonstrates that tests refer to LocalHTLCFailureReason variants like InvalidOnionPayload and InvalidOnionBlinding, further evidencing that UnauthenticatedPayload is not present as a variant in this enum.
- File: lightning/src/ln/onion_payment.rs
- Lines: surrounding the 531-652 block (test usage shown in related test code)
- Contextual reference (usage in tests indicating existing variants)
PaymentDummyTlv is an empty TLV inserted immediately before the actual ReceiveTlvs in a blinded path. Receivers treat these dummy hops as real hops, which prevents timing-based attacks. Allowing arbitrary dummy hops before the final ReceiveTlvs obscures the recipient's true position in the route and makes it harder for an onlooker to infer the destination, strengthening recipient privacy.
Adds a new constructor for blinded paths that allows specifying the number of dummy hops. This enables users to insert arbitrary hops before the real destination, enhancing privacy by making it harder to infer the sender–receiver distance or identify the final destination. Lays the groundwork for future use of dummy hops in blinded path construction.
shaavan
force-pushed
the
pay-dummy-review
branch
from
61f0ead to
923ece5
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lightning/src/blinded_path/payment.rs (1)
277-282: AAD enforcement now explicit (resolves prior concern)Forward requires no AAD; Dummy and Receive require AAD. This fixes the earlier gap around discarding used_aad.
🧹 Nitpick comments (4)
lightning/src/ln/offers_tests.rs (1)
1413-1447: Inline claim viaClaimAlongRouteArgsis correct; consider avoiding the magic1000msatThe new block correctly:
- Pulls
PaymentClaimablefromalice, validates thePaymentContext, and extracts the preimage.- Uses
ClaimAlongRouteArgs::new(bob, &[&[alice]], payment_preimage)pluswith_expected_extra_total_fees_msat(1000)soclaim_payment_along_routecan account for dummy-hop-induced overpay.- Asserts the claimed invoice matches the original.
Only minor nit:
1000msat is a magic constant. To reduce brittleness if fee settings change, you could either document why this exact value is expected here or derive it from the observed fee delta (e.g., from events/HTLCs) or a helper, instead of hard-coding it. Otherwise this change looks good.lightning/src/ln/functional_test_utils.rs (1)
3555-3562: Remove debug println in hot test loopAvoid noisy stdout in test utils. Drop the println! or gate behind the existing TestLogger.
Apply:
- for _ in 0..dummy_count { - println!("This ran"); - node.node.process_pending_htlc_forwards(); - } + for _ in 0..dummy_count { + node.node.process_pending_htlc_forwards(); + }lightning/src/blinded_path/payment.rs (1)
568-576: Clarify TLV assignment in docsCode uses TLV 65539 for Dummy, while the comment mentions reusing a removed “authentication” field. Update the comment to explicitly state 65539 is used for Dummy to avoid confusion.
Also applies to: 577-591
lightning/src/ln/channelmanager.rs (1)
6919-6951: Consider matchingforward_info: Nonedirectly for clarity.Lines 6927-6931 bind
forward_infoand then assert it'sNone. Since dummy hops always haveforward_info: None(peronion_payment.rslines 630-635), consider matching this directly:( onion_utils::Hop::Dummy { intro_node_blinding_point, next_hop_hmac, new_packet_bytes, .. }, - Some(NextPacketDetails { next_packet_pubkey, forward_info }), + Some(NextPacketDetails { next_packet_pubkey, forward_info: None }), ) => { - debug_assert!( - forward_info.is_none(), - "Dummy hops must not contain any forward info, since they are not actually forwarded." - ); let new_update_add_htlc = create_new_update_add_htlc(This provides compile-time verification rather than runtime assertion, making the code's intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lightning/src/blinded_path/payment.rs(12 hunks)lightning/src/ln/async_payments_tests.rs(2 hunks)lightning/src/ln/blinded_payment_tests.rs(5 hunks)lightning/src/ln/channelmanager.rs(11 hunks)lightning/src/ln/functional_test_utils.rs(11 hunks)lightning/src/ln/msgs.rs(2 hunks)lightning/src/ln/offers_tests.rs(1 hunks)lightning/src/ln/onion_payment.rs(7 hunks)lightning/src/ln/onion_utils.rs(3 hunks)lightning/src/routing/router.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/blinded_payment_tests.rs
- lightning/src/ln/msgs.rs
- lightning/src/routing/router.rs
🧰 Additional context used
🧬 Code graph analysis (5)
lightning/src/ln/offers_tests.rs (1)
lightning/src/ln/functional_test_utils.rs (5)
new(3238-3247)new(3455-3475)new(3837-3852)new(5155-5175)claim_payment_along_route(4122-4136)
lightning/src/ln/channelmanager.rs (1)
lightning/src/ln/onion_payment.rs (6)
check_incoming_htlc_cltv(680-708)create_fwd_pending_htlc_info(94-253)create_new_update_add_htlc(654-678)create_recv_pending_htlc_info(256-439)decode_incoming_update_add_htlc_onion(531-652)invalid_payment_err_data(43-48)
lightning/src/ln/async_payments_tests.rs (2)
lightning/src/routing/router.rs (1)
blinded_path(1796-1803)lightning/src/offers/test_utils.rs (1)
blinded_path(130-139)
lightning/src/ln/onion_payment.rs (2)
lightning/src/ln/onion_utils.rs (8)
new(783-783)new(2142-2142)new(2146-2148)new(2152-2154)new(2726-2728)new(3940-3942)next_hop_pubkey(116-127)shared_secret(2266-2278)lightning/src/ln/msgs.rs (2)
new(2517-2519)new(2552-2554)
lightning/src/blinded_path/payment.rs (2)
lightning/src/blinded_path/message.rs (1)
new_with_dummy_hops(91-119)lightning/src/ln/onion_utils.rs (1)
next_hop_pubkey(116-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: benchmark
- GitHub Check: linting
- GitHub Check: check_release
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, 1.75.0)
🔇 Additional comments (12)
lightning/src/ln/async_payments_tests.rs (2)
58-58: Import ofDEFAULT_PAYMENT_DUMMY_HOPSis appropriateThe new constant is used locally in this test to adapt to dummy hops in blinded payment paths; the import is minimal and correctly scoped.
1856-1872: Advancing the blinded payment path across dummy hops before reading constraintsAdvancing
blinded_pathonce at the intermediate node and thenDEFAULT_PAYMENT_DUMMY_HOPStimes with the recipient’s keys before callingdecrypt_intro_payloadensures the test readsmax_cltv_expiryat the actual receive hop in the presence of dummy hops. This keeps the expiry-calculation logic consistent with the new path layout.lightning/src/ln/onion_payment.rs (1)
470-507: Dummy hop peeling is broken
decode_incoming_update_add_htlc_onioncan now returnHop::Dummy, butpeel_payment_oniononly treatsForward/BlindedForwardas forwarding cases. ADummyhop therefore drops into the_branch, callscreate_recv_pending_htlc_info, and blows up withInvalidOnionBlinding. As soon as a path inserts one of the new dummy hops, we will reject the payment instead of peeling the dummy layer, so the headline feature can’t work.Please add explicit handling for
Hop::Dummyhere (and avoid routing/receiving fallbacks) so that we strip the dummy layer and continue decoding to the real next hop before producingPendingHTLCInfo. raise_critical_issuelightning/src/ln/functional_test_utils.rs (1)
3799-3822: Fee adjustment hook looks good; confirm single point of applicationThe new expected_extra_total_fees_msat is only added in claim_payment_along_route. Confirm tests never compare total fees directly from do_claim_payment_along_route; otherwise, callers may miss the adjustment.
Also applies to: 4128-4135
lightning/src/blinded_path/payment.rs (2)
120-151: Good split: delegate to new_with_dummy_hops(0)Refactor keeps existing behavior while exposing configurability.
242-254: Verify introduction-node update for Dummy hopsFor Dummy, you set introduction_node to our own node_id and advance the blinding point. That’s correct if only the final recipient peels dummy hops. If callers might advance across Dummy outside the payee, consider leaving introduction_node unchanged while removing the hop (behaviorally equivalent when we are already the intro node) to avoid accidental node-id drift.
lightning/src/ln/channelmanager.rs (6)
74-77: LGTM: Import additions support ForwardInfo refactoring.The new imports (
ForwardInfo,HopConnector,NextPacketDetails,invalid_payment_err_data) align with the refactoring to consolidate forward information into a structured type and support dummy hop processing.
4891-4891: LGTM: ForwardInfo refactoring improves code organization.The refactoring consolidates
outgoing_amt_msat,outgoing_cltv_value, andoutgoing_connectorinto aForwardInfostruct. This reduces parameter count and groups related data logically, making the code easier to maintain.Also applies to: 4902-4905, 4925-4928
5099-5106: LGTM: Defensive error handling for dummy hops.The match arm for
Hop::Dummyreturns an error withdebug_assert!(false)because dummy hops should be handled earlier in the decode logic (Hunk 9) before reachingconstruct_pending_htlc_status. This defensive check will catch bugs during development if the assumption is violated.
6961-6966: LGTM: Safe extraction of outgoing SCID through forward_info.The refactored code safely extracts
outgoing_scid_optfrom the nestedOptiontypes usingas_ref().and_then(...). For dummy hops whereforward_infoisNone, this correctly producesNonefor the SCID.
4960-4963: Verification complete: Dummy hops safely bypass thecan_forward_htlcvalidation.The code confirms your concern is already properly handled. At line 6921-6948, the
Hop::Dummymatch arm has a debug assertion verifying thatforward_info.is_none()for dummy hops, and critically, executes acontinuestatement at line 6946. This early exit means dummy hops never reach thecan_forward_htlccheck at line 7028. Only the non-Dummy cases (matched by the_ =>arm at line 6950) proceed to that validation point. Therefore, theforward_inforequirement incan_forward_htlc(lines 4960-4963) cannot be violated by dummy hops.
7109-7112: Dummy HTLCs are correctly processed in subsequent iterations; termination is guaranteed.After verification, the mechanism works as intended:
Re-invocation loop: After
process_pending_update_add_htlcs()processes a batch and swaps dummy HTLCs back into the decode queue,needs_pending_htlc_processing()detects the non-empty queue and returns true. The background processor or caller then re-invokesprocess_pending_htlc_forwards(), triggering another iteration.Termination guarantee: Each iteration peels one layer of Dummy hops from the onion. Since blinded paths have finite length (deterministically bounded by cryptographic constraints), and each peel produces a valid, shorter onion, the iterative process terminates when all Dummy layers are exhausted. When the decode queue becomes empty,
needs_pending_htlc_processing()returns false and the loop stops.No infinite loop risk: Circular Dummy hops are cryptographically impossible—each hop's HMAC validation ensures onions can only progress forward (one layer shorter), never cycle.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
lightning/src/ln/msgs.rs (1)
2358-2359: Document the new Dummy hop variant and its constraints.Great addition. Please add a brief doc comment clarifying when/why this variant appears and which fields are disallowed (amt/cltv/total_msat/metadata/keysend/invoice_request) and that AAD must be set.
pub enum InboundOnionPayload { Forward(InboundOnionForwardPayload), TrampolineEntrypoint(InboundTrampolineEntrypointPayload), Receive(InboundOnionReceivePayload), BlindedForward(InboundOnionBlindedForwardPayload), BlindedReceive(InboundOnionBlindedReceivePayload), - Dummy { intro_node_blinding_point: Option<PublicKey> }, + /// A dummy/padding hop inside a blinded path. + /// + /// - Carries no amt/cltv/total_msat/metadata/keysend/invoice_request. + /// - Must be authenticated with AAD in the encrypted TLVs. + /// - Optionally propagates the introduction node's blinding point when present. + Dummy { intro_node_blinding_point: Option<PublicKey> }, }lightning/src/routing/router.rs (2)
204-207: Avoid all-or-nothing collection; keep successful blinded pathsCollecting into Result<Vec<_>, _> means a single failed path build (more likely with dummy hops) discards all successful ones. Prefer filtering out failures and proceeding with any valid paths found.
- .map(|forward_node| { - BlindedPaymentPath::new_with_dummy_hops( - &[forward_node], recipient, DEFAULT_PAYMENT_DUMMY_HOPS, local_node_receive_key, tlvs.clone(), u64::MAX, - MIN_FINAL_CLTV_EXPIRY_DELTA, &*self.entropy_source, secp_ctx - ) - }) - .take(MAX_PAYMENT_PATHS) - .collect::<Result<Vec<_>, _>>(); + .filter_map(|forward_node| { + match BlindedPaymentPath::new_with_dummy_hops( + &[forward_node], recipient, DEFAULT_PAYMENT_DUMMY_HOPS, local_node_receive_key, tlvs.clone(), u64::MAX, + MIN_FINAL_CLTV_EXPIRY_DELTA, &*self.entropy_source, secp_ctx + ) { + Ok(p) => Some(p), + Err(()) => { + log_trace!(self.logger, "Skipping blinded path for peer {} due to dummy-hop build failure", + forward_node.node_id); + None + } + } + }) + .take(MAX_PAYMENT_PATHS) + .collect::<Vec<_>>();And below, adjust the match to check emptiness rather than Result:
- match paths { - Ok(paths) if !paths.is_empty() => Ok(paths), + match paths.len() { + n if n > 0 => Ok(paths), _ => {
77-79: Make the default configurable and document rationalePublic default is fine; consider threading it via user config (e.g., RouteParametersConfig/PaymentParameters) or feature-gating for tests. At minimum, document privacy/performance trade-offs (onion size, CLTV/fees) and when to tune it.
lightning/src/ln/blinded_payment_tests.rs (1)
193-244: Good focused coverage for one‑hop blinded path with dummy hopsThis test mirrors the existing one‑hop blinded path flow while exercising
BlindedPaymentPath::new_with_dummy_hopsandpass_along_path_with_dummy, which is what you want to validate dummy-hop peeling end‑to‑end. Consider factoring the common setup withdo_one_hop_blinded_pathinto a small helper to reduce duplication, but it’s optional.lightning/src/ln/onion_payment.rs (1)
472-485: Consider more specific error message.The error message "Failed to decode update add htlc onion" is generic. Since this error occurs specifically when
forward_infois missing or invalid for a forward hop, consider a more descriptive message like "Missing forward routing information for intermediate hop" to aid debugging.lightning/src/ln/channelmanager.rs (4)
75-80: Imports updated for new dummy/forwarding helpers look consistentThe added
onion_paymentsymbols (check_incoming_htlc_cltv,create_new_update_add_htlc,ForwardInfo,HopConnector,InboundHTLCErr,NextPacketDetails) are all exercised in the new logic below, so the import surface looks coherent. The duplicateonion_utilsimports (lines 79–80) are harmless but could be collapsed into a singleuse crate::ln::onion_utils::{self, ...};for clarity if you touch this area again.
4891-4903: Forwarding checks now rely onForwardInfo; logic looks sound, but relies onforward_infoinvariants
- Using
ForwardInfohere to:
- enforce SCID-privacy (
supports_scid_privacy+outgoing_scid != outbound_scid_alias()), and- validate the outgoing amount/CLTV via
outgoing_amt_msat/outgoing_cltv_value
matches the intendedoption_scid_aliassemantics and makes the checks clearly “about the next hop” rather than the incoming HTLC.- The extra
outgoing_amt_msat < get_counterparty_htlc_minimum_msat()guard seems like the right place to enforce the counterparty’s advertised minimum, on top of whateverhtlc_satisfies_configalready does with our own config.Two things worth double‑checking:
forward_infopresence:can_forward_htlcnow treatsnext_packet_details.forward_info.is_none()asInvalidOnionPayload. That’s fine as long as all call sites only invoke it for true forwards whereforward_infomust beSome. If there are paths whereNextPacketDetailsexists butforward_infois deliberatelyNone(e.g., some blinded/introduction edge cases), this will tighten behaviour and surface as onion failures rather than a more specific error.Redundant
HopConnectorpattern:can_forward_htlcalready rejects non‑ShortChannelIdconnectors before callingcan_forward_htlc_to_outgoing_channel, so theif let HopConnector::ShortChannelId(..)incan_forward_htlc_to_outgoing_channelis effectively defensive. That’s totally fine and may help if the helper is ever called from other sites, but if you want to keep the surface minimal you could instead pass the resolvedoutgoing_scidintocan_forward_htlc_to_outgoing_channeland avoid re‑matching onHopConnector.No blockers here; just worth validating the
forward_infoinvariants at callers.Also applies to: 4925-4929, 4960-4974, 4990-4990
5099-5106: TreatingHop::Dummyhere as an invariant violation is reasonable; consider making the intent more explicitThis arm makes it clear that dummy hops should have been peeled earlier and that hitting
Hop::Dummyon this code path is a bug: youdebug_assert!(false)and surface anInvalidOnionPayloadto the counterparty.That behaviour is fine, but two small tweaks could make the invariant clearer in production builds:
- Replace the bare
debug_assert!(false, ...)with something like a comment or an internal log that explains which invariant was violated (e.g., “dummy hops must be handled inprocess_pending_update_add_htlcsbefore reaching standard forwarding”).- If you ever want more structured failure data here, wiring this through
invalid_payment_err_datainstead of an emptyerr_datawould keep error handling consistent with the other onion failures.Not urgent, but would help future readers understand why this arm exists.
6855-6856: Dummy‑hop peeling viadummy_update_add_htlcslooks correct; consider tightening edge cases and persistence semanticsThe new dummy‑hop flow in
process_pending_update_add_htlcsis a nice separation of concerns:
- When
decode_incoming_update_add_htlc_onionreturns aHop::DummywithSome(NextPacketDetails { next_packet_pubkey, forward_info }), you:
- assert
forward_info.is_none()(since dummy hops aren’t actually forwarded),- build a fresh
UpdateAddHTLCviacreate_new_update_add_htlcusing the intro node blinding point and next packet data, and- stash it in
dummy_update_add_htlcs[incoming_scid_alias]beforecontinue-ing, so the original HTLC isn’t processed further on this pass.- At the end, you swap
self.decode_update_add_htlcswithdummy_update_add_htlcs, causing only the peeled HTLCs to be revisited on the next call.A couple of follow‑ups that might be worth tightening:
(Hop::Dummy, None) handling:
Currently,(Hop::Dummy, None)falls through the_ => decoded_onionarm and will be treated like any other decoded hop. If that combination is truly impossible by construction, it may be clearer (and safer) to add an explicit arm that returns anInboundHTLCErrfor(Hop::Dummy, None)rather than silently treating it as a normal hop.
forward_infoon dummy hops:
Youdebug_assert!(forward_info.is_none())but otherwise ignoreforward_infoin release builds. If there is any chance this ends upSome, you might prefer to enforce it with an explicit error (similar to point 1) so the invariant isn’t only checked in debug builds.Persistence of the peeled queue:
The new peeledUpdateAddHTLCs now live inself.decode_update_add_htlcsafter the finalmem::swap. Depending on howdecode_update_add_htlcsis serialized, you may want to ensureshould_persistis flipped totruewheneverdummy_update_add_htlcsis non‑empty (or when you perform the swap), so a crash between peeling and reprocessing doesn’t lose that intermediate state if it’s meant to be durable.Micro‑nit:
If you care about avoiding an extra lock when there are no dummy hops, you could guard the final swap withif !dummy_update_add_htlcs.is_empty()—purely an optimization, not necessary.Overall the structure looks good; the main ask is to make the “Dummy always comes with
Some(NextPacketDetails)and noforward_info” assumption explicit in the control flow.Also applies to: 6919-6951, 6961-6966, 7109-7111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lightning/src/blinded_path/payment.rs(12 hunks)lightning/src/ln/async_payments_tests.rs(2 hunks)lightning/src/ln/blinded_payment_tests.rs(5 hunks)lightning/src/ln/channelmanager.rs(11 hunks)lightning/src/ln/functional_test_utils.rs(11 hunks)lightning/src/ln/msgs.rs(2 hunks)lightning/src/ln/offers_tests.rs(1 hunks)lightning/src/ln/onion_payment.rs(7 hunks)lightning/src/ln/onion_utils.rs(3 hunks)lightning/src/routing/router.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, stable)
🔇 Additional comments (30)
lightning/src/blinded_path/payment.rs (11)
141-183: LGTM with a note on payinfo calculationThe
new_with_dummy_hopsconstructor correctly passes the dummy hop count toblinded_hopsfor path construction. Note thatcompute_payinfodoesn't account for dummy hops in fee/CLTV calculations, which appears correct since dummy hops are peeled by the receiving node and don't add routing fees or delays.
120-139: LGTM - Backward compatible delegationThe existing
newmethod correctly delegates tonew_with_dummy_hopswithdummy_hop_count = 0, maintaining backward compatibility.
242-254: LGTM - Dummy hop advancement logic is correctThe dummy hop handling correctly advances the blinding point and routes back to the same node (via
get_node_id(Recipient::Node)), which aligns with dummy hops being self-referential hops that the receiving node peels recursively.
277-282: LGTM - Proper AAD authentication for Dummy TLVsThe authentication logic correctly requires
used_aad == trueforDummyTLVs, preventing unauthenticated control TLVs from being accepted. This is critical for security as it ensures only the legitimate receiver can peel dummy hops.
372-380: LGTM - Well-documented dummy TLV typeThe
PaymentDummyTlvunit struct withpub(crate)visibility is appropriate as an internal marker type. The documentation clearly explains its privacy-enhancing purpose.
396-420: LGTM - Consistent enum variantsThe
Dummyvariants added toBlindedPaymentTlvsandBlindedPaymentTlvsRefare consistent with the existingForwardandReceivevariants.
593-602: LGTM - Correct write delegationThe
Dummycase inBlindedPaymentTlvsRef::writecorrectly delegates toPaymentDummyTlv::write.
604-660: LGTM - Robust TLV decoding with mutual exclusionThe
Readableimplementation correctly handles theDummyvariant by requiring only theis_dummyfield (TLV 65539) to be present while all other fields must beNone. This ensures proper mutual exclusion between Forward, Dummy, and Receive TLVs.
702-724: LGTM - Correct dummy hop insertion for privacyThe
blinded_hopsfunction correctly inserts dummy hops between intermediate forwarding nodes and the final receive hop, which enhances privacy by obscuring the receiver's position in the route. The use ofreceive_auth_keyfor dummy hops ensures proper authentication. The clamping toMAX_DUMMY_HOPS_COUNTat line 707 prevents abuse.
15-15: No issues foundThe import is correct. MAX_DUMMY_HOPS_COUNT is properly defined as a public constant with value 10 and includes documentation explaining it prevents paths from becoming too long. The constant is actively used in payment.rs at line 707 in a bounds check and documented at line 144.
568-575: No issues found with TLV type 65539TLV type 65539 is correctly used as a discriminator marker for the PaymentDummyTlv variant. The implementation is sound:
- PaymentDummyTlv exclusively writes 65539 as a unit marker
- ForwardTlvs and ReceiveTlvs never write 65539
- The unified read stream uses 65539's presence/absence to distinguish variants
- No conflicts exist as these are mutually exclusive message types
lightning/src/ln/async_payments_tests.rs (2)
862-866: LGTM - Correct dummy hop advancement in testThe loop correctly advances the blinded path by
DEFAULT_PAYMENT_DUMMY_HOPSiterations using the recipient's (nodes[2]) keys, which is appropriate since dummy hops are self-referential hops at the receiving node. This ensures the test properly extracts themax_cltv_expiryfrom the final receive TLVs after peeling through all dummy hops.
58-58: DEFAULT_PAYMENT_DUMMY_HOPS is properly defined and the import is valid.The constant is defined as
pub const DEFAULT_PAYMENT_DUMMY_HOPS: usize = 3;inlightning/src/routing/router.rsat line 78, which confirms the import at line 58 oflightning/src/ln/async_payments_tests.rsis correct and the value is reasonable for test expectations.lightning/src/ln/msgs.rs (1)
3698-3711: Verification complete: Dummy variant is properly handled in all match sites.All matches on InboundOnionPayload correctly handle the new Dummy variant:
- onion_utils.rs:2338: Explicitly handles
Dummyin the intermediary node packet processing path- channelmanager.rs:6919: Explicitly handles
Hop::Dummywith proper logic for dummy hops- msgs.rs:3698-3711: Decode validation correctly enforces constraints (requires
used_aad, disallowsamt/cltv/total_msat/keysend/invoice_request)The decode guardrails are sound. The test recommendations (valid Dummy with AAD, rejection when
used_aad=false, rejection when extra fields present) remain valuable for coverage but are optional.lightning/src/routing/router.rs (1)
216-219: Suggestion is based on a flawed premise—dummy hop count doesn't cause the failure.The code shows
new_with_dummy_hopsfails duringcompute_payinfo, which processes onlyintermediate_nodes. Failures occur from invalid features or CLTV overflow on those nodes—not from thedummy_hop_countparameter itself. Theblinded_hopsfunction acceptsdummy_hop_count = 0without issue (line 707 clamps it, line 711 usestake(0)which yields empty iterators). Retrying with progressively smaller hop counts won't recover from the actual failure modes.Verify the actual context where this path fails in
lightning/src/routing/router.rsbefore applying a retry strategy.Likely an incorrect or invalid review comment.
lightning/src/ln/onion_utils.rs (3)
2158-2215: Dummy hop variant wiring looks consistent with existing Hop variantsThe new
Hop::Dummyvariant mirrorsForward/BlindedForward(shared secret + HMAC + next packet bytes) and carries theintro_node_blinding_pointneeded for dummy-hop semantics. This keeps downstream handling uniform and should integrate cleanly with existing pattern matches.
2266-2277: Including Dummy inshared_secret()avoids downstream special casesExtending
Hop::shared_secretto return the dummy hop’sshared_secretensures all non-final variants expose the correct key through the same interface, which is important for callers that don’t distinguishDummyfrom other forwards.
2338-2343: Decoding Dummy inbound payload intoHop::Dummyis correctMapping
InboundOnionPayload::Dummy { intro_node_blinding_point }intoHop::Dummy { intro_node_blinding_point, shared_secret, next_hop_hmac, new_packet_bytes }reuses the standard HMAC/packet plumbing while surfacing the extra metadata needed to peel dummy hops at the receiver.lightning/src/ln/blinded_payment_tests.rs (2)
1718-1723: Usingforward_infofromNextPacketDetailskeeps spec-vector tests aligned with onion logicSwitching the Bob→Carol, Carol→Dave, and Dave→Eve
UpdateAddHTLCconstructions to usenext_packet_details.forward_info.{outgoing_amt_msat,outgoing_cltv_value}rather than re‑encoding these values manually makes the route‑blinding spec test strictly follow what the decoder computed. This reduces the risk of the test getting out of sync with the forwarding arithmetic.Also applies to: 1753-1756, 1788-1791
2021-2022: Trampoline inbound decoding now correctly reusesforward_infoUsing
carol_packet_details.forward_infoto fill Carol’sUpdateAddHTLCamount and CLTV mirrors the change above and keeps the trampoline inbound test driven by the same NextPacketDetails source of truth instead of duplicating the routing math.lightning/src/ln/onion_payment.rs (5)
18-18: LGTM: Explicit imports for new functionality.The explicit import of
OnionPacketandUpdateAddHTLCis appropriate to support the newcreate_new_update_add_htlcfunction added later in the file.
126-133: Clarify defensive programming pattern withdebug_assert!(false).The
debug_assert!(false, ...)followed by error handling is unusual. In debug builds, this will panic before the error is returned, while in release builds it returns the error. Consider either:
- Removing the
debug_assert!if this is a possible error condition- Documenting why this is expected to be unreachable (e.g., if upstream validation should prevent this)
- Using
unreachable!()if this truly should never happenThe same pattern appears at line 339 for
create_recv_pending_htlc_info.
511-528: LGTM: Well-structured encapsulation of forward hop information.The new structs properly encapsulate forwarding details, with appropriate visibility (
pub(super)) for module-internal use. TheHopConnectorenum cleanly distinguishes between SCID-based and Trampoline routing.
600-607: LGTM: Consistent population of forward routing information.All three forward hop types (Direct, BlindedForward, TrampolineForward) correctly populate the new
forward_infofield with appropriate routing details and connector types.Also applies to: 622-628, 635-640
642-647: LGTM: Correct handling of Dummy hops.Dummy hops are properly handled by computing the
next_packet_pubkeywhile settingforward_info: None, which ensures they cannot be forwarded (as intended) while still supporting the onion construction flow.lightning/src/ln/functional_test_utils.rs (5)
42-44: LGTM: Import supports new dummy hop test functionality.The import of
DEFAULT_PAYMENT_DUMMY_HOPSis appropriate for the new test helper functions that exercise dummy hop behavior.
3440-3440: LGTM: Builder pattern extension for dummy hop override.The new
dummy_hop_overridefield andwith_dummy_overridemethod properly extend the builder pattern to support testing with custom dummy hop counts.Also applies to: 3462-3462, 3510-3513
3674-3711: LGTM: Clean refactoring to support dummy hops.The refactoring of
pass_along_pathto delegate topass_along_path_with_dummymaintains backward compatibility while properly supporting the new dummy hop functionality. The conditional application ofwith_dummy_overrideis correct.
3799-3821: LGTM: Excellent documentation of fee-handling trade-off.The new
expected_extra_total_fees_msatfield is well-documented with clear explanation of:
- The existing fee-handling trade-off in LDK
- How dummy hops surface this issue in tests
- Why this field exists as a temporary workaround
The documentation provides valuable context for future developers and properly references the relevant code (
BlindedPaymentPath::advance_path_by_one).Also applies to: 3845-3845, 3861-3864
4128-4130: LGTM: Straightforward fee adjustment logic.The extraction and addition of
expected_extra_total_fees_msatis clear and correct, properly accounting for the documented fee-handling trade-off in test assertions.
| if is_last_hop { | ||
| let dummy_count = dummy_hop_override.unwrap_or(DEFAULT_PAYMENT_DUMMY_HOPS); | ||
| for _ in 0..dummy_count { | ||
| println!("This ran"); | ||
| node.node.process_pending_htlc_forwards(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove debug print statement.
Line 3558 contains println!("This ran"); which appears to be debug code that should be removed before merging.
Apply this diff:
if is_last_hop {
let dummy_count = dummy_hop_override.unwrap_or(DEFAULT_PAYMENT_DUMMY_HOPS);
for _ in 0..dummy_count {
- println!("This ran");
node.node.process_pending_htlc_forwards();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if is_last_hop { | |
| let dummy_count = dummy_hop_override.unwrap_or(DEFAULT_PAYMENT_DUMMY_HOPS); | |
| for _ in 0..dummy_count { | |
| println!("This ran"); | |
| node.node.process_pending_htlc_forwards(); | |
| } | |
| } | |
| if is_last_hop { | |
| let dummy_count = dummy_hop_override.unwrap_or(DEFAULT_PAYMENT_DUMMY_HOPS); | |
| for _ in 0..dummy_count { | |
| node.node.process_pending_htlc_forwards(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lightning/src/ln/functional_test_utils.rs around lines 3555 to 3561, remove
the leftover debug print statement println!("This ran"); inside the dummy hop
loop; simply delete that println! so the loop only iterates and calls
node.node.process_pending_htlc_forwards() for each dummy hop (respecting
dummy_hop_override or DEFAULT_PAYMENT_DUMMY_HOPS).
| // Extract PaymentClaimable, verify context, and obtain the payment preimage. | ||
| let recipient = &alice; | ||
| let payment_purpose = match get_event!(recipient, Event::PaymentClaimable) { | ||
| Event::PaymentClaimable { purpose, .. } => purpose, | ||
| _ => panic!("No Event::PaymentClaimable"), | ||
| }; | ||
|
|
||
| let payment_preimage = payment_purpose | ||
| .preimage() | ||
| .expect("No preimage in Event::PaymentClaimable"); | ||
|
|
||
| match payment_purpose { | ||
| PaymentPurpose::Bolt12OfferPayment { payment_context: ctx, .. } => { | ||
| assert_eq!(PaymentContext::Bolt12Offer(ctx), payment_context); | ||
| }, | ||
| PaymentPurpose::Bolt12RefundPayment { payment_context: ctx, .. } => { | ||
| assert_eq!(PaymentContext::Bolt12Refund(ctx), payment_context); | ||
| }, | ||
| _ => panic!("Unexpected payment purpose: {:?}", payment_purpose), | ||
| }; | ||
|
|
||
| // Build ClaimAlongRouteArgs and compensate for the expected overpayment | ||
| // caused by the payer being the introduction node of the blinded path with | ||
| // dummy hops. | ||
| let expected_paths: &[&[&Node]] = &[&[alice]]; | ||
| let args = ClaimAlongRouteArgs::new( | ||
| bob, | ||
| expected_paths, | ||
| payment_preimage, | ||
| ).with_expected_extra_total_fees_msat(1000); | ||
|
|
||
| // Execute the claim and verify the invoice was fulfilled. | ||
| let (inv, _events) = claim_payment_along_route(args); | ||
| assert_eq!(inv, Some(PaidBolt12Invoice::Bolt12Invoice(invoice.clone()))); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify the hardcoded 1000 msat extra fee adjustment and clarify the comment.
The test now manually compensates for an extra 1000 msat in fees when claiming the payment. Several concerns:
-
Magic number: The 1000 msat value appears arbitrary with no clear derivation. If this is related to dummy hop fees, it should be calculable from the number of dummy hops and per-hop fee rates rather than hardcoded.
-
Confusing comment (lines 1434-1436): The comment states "payer being the introduction node of the blinded path with dummy hops," but in this test Bob is the payer and Alice is the introduction node. This phrasing is confusing—please clarify what scenario is being described.
-
Narrow scope: Only this specific test is updated with the extra fee compensation. If dummy hops affect fee calculations, shouldn't other similar tests (e.g.,
creates_and_pays_for_offer_using_two_hop_blinded_path,creates_and_pays_for_refund_*) also require updates? -
Root cause concern: This appears to be a test-side workaround for a fee calculation mismatch rather than addressing the underlying issue. If the fee calculation in the production code doesn't properly account for dummy hops, that might be the actual bug to fix.
Please verify:
- How is the 1000 msat value derived?
- Is this value stable across different dummy hop counts and fee rates?
- Should the production fee calculation be fixed instead of compensating in tests?
You could also run this script to check if other tests might need similar adjustments:
🏁 Script executed:
#!/bin/bash
# Search for other tests using claim_bolt12_payment that might also need dummy hop fee compensation
rg -n "claim_bolt12_payment" --type rust lightning/src/ln/offers_tests.rsLength of output: 714
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the test function containing lines 1413-1447
sed -n '1350,1450p' lightning/src/ln/offers_tests.rs | head -100
Length of output: 4115
🏁 Script executed:
#!/bin/bash
# Look at the claim_bolt12_payment function definition
sed -n '185,250p' lightning/src/ln/offers_tests.rsLength of output: 3344
🏁 Script executed:
#!/bin/bash
# Search for "dummy" references to understand the feature
rg -n "dummy" --type rust lightning/src/ln/offers_tests.rsLength of output: 478
🏁 Script executed:
#!/bin/bash
# Check what tests use one_hop_blinded_path to compare
rg -n "one_hop_blinded_path" --type rust lightning/src/ln/offers_tests.rs | head -20Length of output: 130
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for NodeIdMessageRouter and MAX_DUMMY_HOPS_COUNT
rg -n "NodeIdMessageRouter|MAX_DUMMY_HOPS_COUNT" --type rust lightning/src/ln/offers_tests.rs | head -20
Length of output: 685
🏁 Script executed:
#!/bin/bash
# Look at the setup for one_hop_blinded_path test to compare
sed -n '806,875p' lightning/src/ln/offers_tests.rsLength of output: 3000
🏁 Script executed:
#!/bin/bash
# Search for where MAX_DUMMY_HOPS_COUNT is defined and its value
rg -n "MAX_DUMMY_HOPS_COUNT" --type rust -A2 lightning/src/ | head -30Length of output: 2389
🏁 Script executed:
#!/bin/bash
# Search for per-hop fee constants or fee calculation in offers context
rg -n "per.hop|fee.*hop|1000" --type rust lightning/src/ln/offers_tests.rs | grep -i "fee\|1000\|hop" | head -20Length of output: 73
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for per-hop fee constants around the test
rg -n "per.hop|100\s*msat|CLTV|fee" --type rust lightning/src/ln/offers_tests.rs | head -30
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Get more context around the comment to understand "payer being the introduction node"
sed -n '1420,1450p' lightning/src/ln/offers_tests.rsLength of output: 1105
🏁 Script executed:
#!/bin/bash
# Search for how dummy hops might affect fees
rg -n "dummy.*fee|fee.*dummy" --type rust lightning/src/ | head -20Length of output: 345
🏁 Script executed:
#!/bin/bash
# Look for any constant 100 that might be per-hop
rg -n "100" --type rust lightning/src/ln/offers_tests.rs | head -20Length of output: 73
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ClaimAlongRouteArgs and its usage
rg -n "ClaimAlongRouteArgs|with_expected_extra_total_fees_msat" --type rust lightning/src/ | head -20
Length of output: 2239
🏁 Script executed:
#!/bin/bash
# Search for "1000" in relevant modules to understand context
rg -n "1000" --type rust lightning/src/routing/router.rs lightning/src/onion_message/ | head -30Length of output: 1530
🏁 Script executed:
#!/bin/bash
# Check if there are other uses of with_expected_extra_total_fees_msat to understand typical values
rg -n "with_expected_extra_total_fees_msat" --type rust lightning/Length of output: 217
Fix the comment and document the 1000 msat extra fee derivation.
The test contains a factual error in its comment and uses an undocumented magic number:
-
Incorrect comment (lines 1434-1436): The comment states "payer being the introduction node of the blinded path with dummy hops," but this is backwards. In this test, Bob is the payer and Alice is the introduction node and recipient. The comment should clarify that Alice (the recipient/introduction node) is introducing dummy hops that increase the expected fees by 1000 msat.
-
Undocumented magic number: The 1000 msat value appears to have no derivation or documentation. If it's related to
MAX_DUMMY_HOPS_COUNT(10), explain the formula—e.g., is it 10 × 100 msat per hop? This value only appears once in the entire codebase, so it needs clear justification.
Please clarify:
- What scenario necessitates this extra fee compensation specifically for unannounced introduction nodes?
- How is 1000 msat calculated? Is it stable across different configurations?
🤖 Prompt for AI Agents
In lightning/src/ln/offers_tests.rs around lines 1413 to 1447, correct the
inaccurate comment about who is the payer/introduction node and replace the
current vague text with a short, accurate explanation that Alice is the
recipient and introduction node who is introducing dummy hops (which causes
extra routing/relay fees), and document the derivation of the 1000 msat magic
number next to the with_expected_extra_total_fees_msat(1000) call (e.g.
reference MAX_DUMMY_HOPS_COUNT and show the per-dummy-hop fee used in the test
and the multiplication used to arrive at 1000 msat — for example
"MAX_DUMMY_HOPS_COUNT (10) × X msat per dummy hop = 1000 msat"); also state
whether this value depends on config (if it is derived from constants in the
test or production config) and add a short note that this compensation is only
needed when the recipient/introduction node is unannounced and inserts dummy
hops, so reviewers/readers can see the rationale and stability of the 1000 msat
choice.
| pub(super) fn create_new_update_add_htlc<NS: Deref, T: secp256k1::Verification>( | ||
| msg: msgs::UpdateAddHTLC, node_signer: NS, secp_ctx: &Secp256k1<T>, | ||
| intro_node_blinding_point: Option<PublicKey>, | ||
| new_packet_pubkey: Result<PublicKey, secp256k1::Error>, next_hop_hmac: [u8; 32], | ||
| new_packet_bytes: [u8; 1300], | ||
| ) -> UpdateAddHTLC | ||
| where | ||
| NS::Target: NodeSigner, | ||
| { | ||
| let new_packet = OnionPacket { | ||
| version: 0, | ||
| public_key: new_packet_pubkey, | ||
| hop_data: new_packet_bytes, | ||
| hmac: next_hop_hmac, | ||
| }; | ||
|
|
||
| let next_blinding_point = | ||
| intro_node_blinding_point.or(msg.blinding_point).and_then(|blinding_point| { | ||
| let encrypted_tlvs_ss = | ||
| node_signer.ecdh(Recipient::Node, &blinding_point, None).unwrap().secret_bytes(); | ||
| onion_utils::next_hop_pubkey(&secp_ctx, blinding_point, &encrypted_tlvs_ss).ok() | ||
| }); | ||
|
|
||
| UpdateAddHTLC { onion_routing_packet: new_packet, blinding_point: next_blinding_point, ..msg } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify ECDH cannot fail in this context.
Line 673 uses .unwrap() on node_signer.ecdh(). While this is likely safe if the blinding_point has been validated upstream, consider whether this can fail in practice. If failure is possible, propagating the error would be safer than panicking.
🏁 Script executed:
#!/bin/bash
# Search for validation of blinding_point before create_new_update_add_htlc is called
rg -A5 -B5 'create_new_update_add_htlc' --type rustLength of output: 2400
I need to gather more context about the ECDH call and where the blinding_point is validated.
I need to examine the actual implementation in rust-lightning to understand ECDH error conditions.
Waiting for script results to better understand ECDH contract and error conditions.
🏁 Script executed:
#!/bin/bash
# Find NodeSigner trait definition and ecdh method signature
rg -n 'fn ecdh' --type rust -B2 -A5 -g '!test'Length of output: 7535
🏁 Script executed:
#!/bin/bash
# Get full ecdh implementation from sign/mod.rs to see all error paths
rg -n 'fn ecdh' -A30 'git/lightning/src/sign/mod.rs' --type rust | head -50Length of output: 2082
🏁 Script executed:
#!/bin/bash
# Look for other ecdh() calls to see how errors are typically handled
rg -n '\.ecdh\(' --type rust -B3 -A3 | grep -E '(ecdh|unwrap|map_err|ok\(\))' | head -40Length of output: 2430
🏁 Script executed:
#!/bin/bash
# Look at full context of the create_new_update_add_htlc call in channelmanager.rs
rg -B20 'create_new_update_add_htlc' git/lightning/src/ln/channelmanager.rs | tail -40Length of output: 1481
🏁 Script executed:
#!/bin/bash
# Check where blinding_point from msg is validated in the onion decoding
rg -n 'decode_incoming_update_add_htlc_onion' --type rust -A5 -B5 git/lightning/src/ln/onion_payment.rs | head -60Length of output: 919
🏁 Script executed:
#!/bin/bash
# Search for comments or safety documentation near the unwrap() call
rg -B10 -A2 '\.ecdh.*unwrap' git/lightning/src/ln/onion_payment.rs --type rustLength of output: 445
Replace .unwrap() with error handling to match patterns elsewhere in the codebase.
Line 673's .unwrap() on node_signer.ecdh() is inconsistent with error handling used in other parts of rust-lightning. While ECDH should succeed with Recipient::Node and valid PublicKey inputs, other code paths—such as lightning/src/ln/msgs.rs (lines 3663, 3830) and lightning/src/ln/peer_channel_encryptor.rs (lines 282, 420)—properly handle potential ECDH failures with .map_err().
Replace the .unwrap() with .map_err(|_| ()) or document the safety assumption to align with defensive error handling practices used elsewhere in the codebase.
🤖 Prompt for AI Agents
In lightning/src/ln/onion_payment.rs around lines 654 to 678, replace the
.unwrap() on node_signer.ecdh(...) inside the and_then closure with a safe
error-to-None conversion so failures yield None instead of panicking;
specifically call node_signer.ecdh(...).ok().map(|r| r.secret_bytes()) (or
equivalently use .map_err(|_| ()).ok() if you prefer the explicit map_err) and
then propagate that Option into onion_utils::next_hop_pubkey so the closure
returns None on ECDH failure rather than panicking.
Summary by CodeRabbit
New Features
Refactor
Tests