Review: Introduce Realistic constraints for Dummy Hops#11
Open
Review: Introduce Realistic constraints for Dummy Hops#11
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request extends dummy hop support in blinded payment paths by making Changes
Sequence Diagram(s)sequenceDiagram
actor Sender
participant Router
participant ChannelMgr
participant Onion as Onion<br/>Processing
participant Event as Event<br/>Emission
Sender->>Router: create_blinded_payment_paths()
Router->>Router: Apply DEFAULT_DUMMY_HOP_* constants
Router->>Router: Compute dummy_hops_htlc_minimum_msat
Router-->>Sender: BlindedPaymentPath with dummy hops
Sender->>ChannelMgr: send_payment (with blinded path)
ChannelMgr->>ChannelMgr: Create PendingHTLCInfo<br/>(dummy_hops_skimmed_fee_msat=None)
ChannelMgr->>ChannelMgr: queue_add_htlc<br/>(dummy_hops_skimmed_fee_msat)
Note over ChannelMgr: HTLC forwarding to receiver
ChannelMgr->>Onion: process_pending_update_add_htlcs
loop For each Dummy Hop
Onion->>Onion: peel_payment_onion()
Onion->>Onion: peel_dummy_hop_update_add_htlc()<br/>→ (UpdateAddHTLC, fee_msat)
Onion->>Onion: Accumulate dummy fee
end
Onion->>Onion: create_recv_pending_htlc_info<br/>(dummy_hops_skimmed_fee_msat)
Onion->>ChannelMgr: Return PendingHTLCInfo with<br/>accumulated dummy fee
ChannelMgr->>ChannelMgr: Aggregate fees across<br/>all ClaimedHTLCs
ChannelMgr->>Event: Emit PaymentClaimed<br/>(dummy_hops_skimmed_fee_msat)
Event-->>Sender: Event with aggregated dummy fee
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🧹 Nitpick comments (7)
lightning/src/blinded_path/payment.rs (1)
396-413: Tighten comment clarity for the newDummyTlvsdefaults.Please fix the typo (
arbitrary) and consider linking the TODO rationale to a tracking issue so these default constants remain auditable.📝 Suggested comment cleanup
impl DummyTlvs { /// On Default values: /// These currently match one-to-one with the default ForwardTlvs values set by DefaultRouter - /// in test. + /// in tests. /// TODO: Figure out the reason behind these values. /// /// Zero FPM: This is set to 0, to avoid off-by-bit test failures. /// /// TODO: Encode it formally. pub(crate) fn default() -> Self { Self { payment_relay: PaymentRelay { cltv_expiry_delta: 80, fee_proportional_millionths: 0, fee_base_msat: 1000, }, payment_constraints: PaymentConstraints { - // Set to a long time in future, to prevent any payment drops due to arbritary constraints + // Set far in the future to prevent payment drops from arbitrary constraints. max_cltv_expiry: 3000, htlc_minimum_msat: 1000, }, } } }I can draft a follow-up issue template to track formalizing these defaults if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/blinded_path/payment.rs` around lines 396 - 413, The comment above DummyTlvs::default (the default() implementation returning PaymentRelay and PaymentConstraints) contains a typo and unclear TODOs; fix the typo "arbritary" to "arbitrary" and update the TODOs to reference a tracking issue or issue ID so the rationale for the chosen defaults (Zero FPM, cltv/fee values) is auditable; specifically edit the comment block that documents default() / DummyTlvs to correct spelling, add a short link or issue number for "TODO: Figure out the reason behind these values." and for "TODO: Encode it formally.", and keep the note about Zero FPM and max_cltv_expiry but clarify why max_cltv_expiry is set high (preventing arbitrary drops) for future maintainers.lightning/src/ln/msgs.rs (1)
6160-6160: Add one positive roundtrip test fordummy_hops_skimmed_fee_msat = Some(_).Current coverage only exercises the
Nonepath, so a TLV-encoding regression for type65539could slip through.✅ Minimal test addition
#[test] fn encoding_update_add_htlc() { @@ assert_eq!(encoded_value, target_value); } + +#[test] +fn encoding_update_add_htlc_with_dummy_hops_skimmed_fee() { + let mut msg = test_update_add_htlc(); + msg.dummy_hops_skimmed_fee_msat = Some(42); + + let encoded = msg.encode(); + let decoded: msgs::UpdateAddHTLC = + LengthReadable::read_from_fixed_length_buffer(&mut &encoded[..]).unwrap(); + + assert_eq!(decoded.dummy_hops_skimmed_fee_msat, Some(42)); +}Also applies to: 7060-7060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` at line 6160, Add a positive roundtrip unit test that sets the struct field dummy_hops_skimmed_fee_msat to Some(<non-zero u64>) (instead of None), performs the TLV encode→decode roundtrip for the message type that contains this field, and asserts the decoded message equals the original; this will exercise TLV type 65539 and catch regressions. Locate the message/struct that defines dummy_hops_skimmed_fee_msat and its existing None-path tests, copy the test pattern but assign a concrete Some(value), run the roundtrip (serialize then deserialize) and assert equality of the field and whole message.lightning/src/ln/async_payments_tests.rs (1)
3038-3043: Please replace the hardcoded dummy-hop CLTV delta.Line 3042 hardcodes
80and leaves a TODO. This is fragile; if dummy TLV defaults change, timeout math silently drifts.Prefer deriving this delta from the same source used when building
DummyTlvsdefaults (single source of truth).If helpful, I can draft a small helper/API shape to compute this value and remove the TODO.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 3038 - 3043, The EXTRA_CLTV_DELTA_DUE_TO_DUMMY constant should not hardcode 80; instead compute it from the DummyTlvs default so the test follows the single source of truth. Replace the literal with a computation using DEFAULT_PAYMENT_DUMMY_HOPS and the CLTV expiry delta value obtained from DummyTlvs::default() (e.g., multiply DEFAULT_PAYMENT_DUMMY_HOPS as u32 by the default dummy TLV CLTV delta field), casting types as needed; update EXTRA_CLTV_DELTA_DUE_TO_DUMMY to use that expression and remove the TODO.lightning/src/ln/blinded_payment_tests.rs (1)
328-333: Assert the propagated dummy-hop skimmed fee before handing off the HTLC.Right now this test proves the encrypted receive payload fails its
PaymentConstraints, but it still passes ifdummy_hops_skimmed_fee_msatis silently dropped from theUpdateAddHTLC. A direct assertion here would lock in the new propagation behavior.Suggested change
let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); let ev = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events); let payment_event = SendEvent::from_event(ev); +assert_eq!( + payment_event.msgs[0].dummy_hops_skimmed_fee_msat, + Some(total_dummy_fee_msat as _), +); nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 328 - 333, Assert the propagated dummy-hop skimmed fee on the UpdateAddHTLC before handing it off: after creating payment_event with SendEvent::from_event(ev) inspect payment_event.msgs[0] (the UpdateAddHTLC) and add an assertion that its dummy_hops_skimmed_fee_msat matches the expected value (or at minimum is_some()) to ensure the fee was propagated, then call nodes[1].node.handle_update_add_htlc(...). Use the existing symbols payment_event, payment_event.msgs[0], dummy_hops_skimmed_fee_msat, and handle_update_add_htlc to locate where to insert the assertion.lightning/src/ln/channelmanager.rs (3)
7903-7905: Make the ignored dummy-hop fee explicit.Matching
dummy_hops_skimmed_fee_msatto_means an unexpected non-Nonehere disappears silently. Since the new docs describe this as receive-only, I'd bind it anddebug_assert!(dummy_hops_skimmed_fee_msat.is_none())so an invariant break is obvious during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 7903 - 7905, Bind the previously ignored dummy_hops_skimmed_fee_msat instead of using `_` so its value is visible at compile time and add a development-only check: after the match arm (where skimmed_fee_msat and incoming_accountable are used) assert that dummy_hops_skimmed_fee_msat.is_none() via debug_assert! to catch unexpected non-None values during development; this keeps the receive-only invariant explicit while leaving release behavior unchanged.
430-436: Clean up the new comment blocks before this lands.
preceeding/preceeds/This feesare still in the new wording, andLDK x.0reads like a placeholder.Also applies to: 550-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 430 - 436, Fix the typos and placeholder wording in the doc comment for the struct field dummy_hops_skimmed_fee_msat: change "preceeding"/"preceeds" to "preceding"/"precedes", "This fees" to "These fees", and replace or remove the placeholder "LDK x.0" (use the intended version number or omit it). Make the language concise and consistent with surrounding docs (also apply the same corrections to the similar comment block shown at the other occurrence around lines 550-553 referencing DummyTlvs and Event::PaymentClaimable).
21267-21268: Please add one non-zero dummy-hop receive test.These edits only thread
Nonethrough the new parameter, so this file still doesn't exercise a realdummy_hops_skimmed_fee_msatvalue on the receive path.Also applies to: 21290-21290, 21316-21316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 21267 - 21268, The receive-path tests currently pass None for the dummy_hops_skimmed_fee_msat parameter (threading None through sender_intended_amt_msat, extra_fee_msat, current_height calls); add a test that supplies a non-zero dummy_hops_skimmed_fee_msat (e.g., Some(non_zero_msat)) instead of None in the receive-path call sites that reference sender_intended_amt_msat, extra_fee_msat and current_height, and assert the outcome: the final received amount is reduced by dummy_hops_skimmed_fee_msat and any relevant fee bookkeeping/state/logs reflect the skimmed fee. Locate the existing calls that currently pass None for dummy_hops_skimmed_fee_msat (the lines invoking sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, None, Some(extra_fee_msat), false, current_height) and change one to use Some(<non-zero>) and add the corresponding assertion in the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/events/mod.rs`:
- Around line 972-975: Update the doc comment for the field
dummy_hops_skimmed_fee_msat to correct typos and improve clarity: change "in
thousands of a satoshi" to "in thousandths of a satoshi", replace "receival"
with "receipt", change "receivers earns" to "receiver earns" (or "receiver's
earnings"), and add "the" where needed (e.g., "in the form of skimmed fees");
ensure the final sentence still references `amount_msat` unmodified for clarity.
- Around line 905-908: Update the doc comment for the struct/field
dummy_hops_skimmed_fee_msat: correct "thousands of a satoshi" to "thousandths of
a satoshi", change "proceeding the final receival" to "preceding the final
receipt", and adjust wording so it reads that this amount is the additional
skimmed fee (in thousandths of a satoshi) the receiver earns in addition to
amount_msat; edit the comment immediately above the dummy_hops_skimmed_fee_msat
field in mod.rs to apply these wording fixes.
In `@lightning/src/ln/channel.rs`:
- Around line 9524-9525: The code is clearing dummy_hops_skimmed_fee_msat on the
UpdateAddHTLC path which drops propagated dummy-hop fee metadata while still
copying skimmed_fee_msat from htlc; change the assignment so
dummy_hops_skimmed_fee_msat is preserved/copied from the incoming htlc (e.g. use
htlc.dummy_hops_skimmed_fee_msat or the existing struct value) instead of
setting it to None so downstream claim/accounting sees the correct dummy-hop
skimmed fee.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 4137-4139: The dummy-hop fee is being computed using a running
fwd_amt_msat across all paths which makes expected_total_fee_msat path-order
dependent; before you call dummy_hops_total_fee_msat and walk the fulfill
backwards for each path, reinitialize fwd_amt_msat to the current path's
ClaimedHTLC.value_msat (the matched Claim in your MPP/blinded payment handling)
so the dummy_hops_fee_msat is computed on that path's amount only, then add that
fee to expected_total_fee_msat and proceed with the backward walk; update
references around dummy_hops_fee_msat, dummy_hops_total_fee_msat, fwd_amt_msat
and expected_total_fee_msat accordingly to ensure per-path isolation.
- Around line 3691-3694: The assertion comparing
dummy_hops_total_fee_msat(recv_value, &dummy_tlvs) to
*dummy_hops_skimmed_fee_msat is incorrect for multipart payments because
dummy_hops_skimmed_fee_msat in channelmanager.rs is accumulated per-HTLC; change
the test to compute the expected skimmed fee by summing per-HTLC expected fees
(iterate the individual HTLC amounts from dummy_tlvs and call
dummy_hops_total_fee_msat per part or replicate its per-HTLC logic) and compare
that sum to dummy_hops_skimmed_fee_msat, or alternatively only perform the
exact-equality assertion when the payment is single-part (recv_value == single
HTLC amount).
In `@lightning/src/ln/onion_payment.rs`:
- Around line 662-663: In onion_payment.rs the two encode_relay_error calls in
the blinded-forward and dummy-hop branches have their human-readable messages
swapped; locate the encode_relay_error invocations (the one using
LocalHTLCFailureReason::InvalidOnionBlinding and shared_secret.secret_bytes()
with [0; 32]) in the blinded-forward branch and the corresponding call in the
dummy-hop branch (also repeated around the other occurrence noted), and swap the
two literal error strings so the dummy-hop branch emits "dummy hop" and the
blinded-forward branch emits "blinded forward" (ensure both occurrences at the
other location are corrected consistently).
---
Nitpick comments:
In `@lightning/src/blinded_path/payment.rs`:
- Around line 396-413: The comment above DummyTlvs::default (the default()
implementation returning PaymentRelay and PaymentConstraints) contains a typo
and unclear TODOs; fix the typo "arbritary" to "arbitrary" and update the TODOs
to reference a tracking issue or issue ID so the rationale for the chosen
defaults (Zero FPM, cltv/fee values) is auditable; specifically edit the comment
block that documents default() / DummyTlvs to correct spelling, add a short link
or issue number for "TODO: Figure out the reason behind these values." and for
"TODO: Encode it formally.", and keep the note about Zero FPM and
max_cltv_expiry but clarify why max_cltv_expiry is set high (preventing
arbitrary drops) for future maintainers.
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 3038-3043: The EXTRA_CLTV_DELTA_DUE_TO_DUMMY constant should not
hardcode 80; instead compute it from the DummyTlvs default so the test follows
the single source of truth. Replace the literal with a computation using
DEFAULT_PAYMENT_DUMMY_HOPS and the CLTV expiry delta value obtained from
DummyTlvs::default() (e.g., multiply DEFAULT_PAYMENT_DUMMY_HOPS as u32 by the
default dummy TLV CLTV delta field), casting types as needed; update
EXTRA_CLTV_DELTA_DUE_TO_DUMMY to use that expression and remove the TODO.
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 328-333: Assert the propagated dummy-hop skimmed fee on the
UpdateAddHTLC before handing it off: after creating payment_event with
SendEvent::from_event(ev) inspect payment_event.msgs[0] (the UpdateAddHTLC) and
add an assertion that its dummy_hops_skimmed_fee_msat matches the expected value
(or at minimum is_some()) to ensure the fee was propagated, then call
nodes[1].node.handle_update_add_htlc(...). Use the existing symbols
payment_event, payment_event.msgs[0], dummy_hops_skimmed_fee_msat, and
handle_update_add_htlc to locate where to insert the assertion.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 7903-7905: Bind the previously ignored dummy_hops_skimmed_fee_msat
instead of using `_` so its value is visible at compile time and add a
development-only check: after the match arm (where skimmed_fee_msat and
incoming_accountable are used) assert that dummy_hops_skimmed_fee_msat.is_none()
via debug_assert! to catch unexpected non-None values during development; this
keeps the receive-only invariant explicit while leaving release behavior
unchanged.
- Around line 430-436: Fix the typos and placeholder wording in the doc comment
for the struct field dummy_hops_skimmed_fee_msat: change "preceeding"/"preceeds"
to "preceding"/"precedes", "This fees" to "These fees", and replace or remove
the placeholder "LDK x.0" (use the intended version number or omit it). Make the
language concise and consistent with surrounding docs (also apply the same
corrections to the similar comment block shown at the other occurrence around
lines 550-553 referencing DummyTlvs and Event::PaymentClaimable).
- Around line 21267-21268: The receive-path tests currently pass None for the
dummy_hops_skimmed_fee_msat parameter (threading None through
sender_intended_amt_msat, extra_fee_msat, current_height calls); add a test that
supplies a non-zero dummy_hops_skimmed_fee_msat (e.g., Some(non_zero_msat))
instead of None in the receive-path call sites that reference
sender_intended_amt_msat, extra_fee_msat and current_height, and assert the
outcome: the final received amount is reduced by dummy_hops_skimmed_fee_msat and
any relevant fee bookkeeping/state/logs reflect the skimmed fee. Locate the
existing calls that currently pass None for dummy_hops_skimmed_fee_msat (the
lines invoking sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true,
None, Some(extra_fee_msat), false, current_height) and change one to use
Some(<non-zero>) and add the corresponding assertion in the new test.
In `@lightning/src/ln/msgs.rs`:
- Line 6160: Add a positive roundtrip unit test that sets the struct field
dummy_hops_skimmed_fee_msat to Some(<non-zero u64>) (instead of None), performs
the TLV encode→decode roundtrip for the message type that contains this field,
and asserts the decoded message equals the original; this will exercise TLV type
65539 and catch regressions. Locate the message/struct that defines
dummy_hops_skimmed_fee_msat and its existing None-path tests, copy the test
pattern but assign a concrete Some(value), run the roundtrip (serialize then
deserialize) and assert equality of the field and whole message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2013693-5704-4523-977e-e7ca7eee8f84
📒 Files selected for processing (16)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/funding.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
from
c209d06 to
e7e10f1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lightning/src/ln/blinded_payment_tests.rs (1)
346-351: Add an explicit length assertion before indexing malformed-failure updates.This keeps failures diagnostic (clear assertion) instead of panicking on out-of-bounds if behavior changes.
Suggested tweak
let mut updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()); + assert_eq!(updates.update_fail_malformed_htlcs.len(), 1); // Blinded receive failures are surfaced to the sender as malformed onion blinding errors. nodes[0].node.handle_update_fail_malformed_htlc( nodes[1].node.get_our_node_id(), &updates.update_fail_malformed_htlcs[0], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 346 - 351, Before indexing into updates.update_fail_malformed_htlcs, add an explicit assertion that the vector is non-empty (e.g., assert!(!updates.update_fail_malformed_htlcs.is_empty(), "expected at least one malformed HTLC update")); this should be placed after calling get_htlc_update_msgs and before passing updates.update_fail_malformed_htlcs[0] to nodes[0].node.handle_update_fail_malformed_htlc so the test fails with a clear diagnostic instead of an OOB panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 346-351: Before indexing into updates.update_fail_malformed_htlcs,
add an explicit assertion that the vector is non-empty (e.g.,
assert!(!updates.update_fail_malformed_htlcs.is_empty(), "expected at least one
malformed HTLC update")); this should be placed after calling
get_htlc_update_msgs and before passing updates.update_fail_malformed_htlcs[0]
to nodes[0].node.handle_update_fail_malformed_htlc so the test fails with a
clear diagnostic instead of an OOB panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 038ede50-e403-4453-960b-8649e9875e5b
📒 Files selected for processing (4)
lightning/src/blinded_path/payment.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
from
e7e10f1 to
1137288
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lightning/src/routing/router.rs (1)
153-159:⚠️ Potential issue | 🟠 MajorKeep amount eligibility aligned with the new dummy-hop HTLC minimum
On Line 182 you raise
payment_constraints.htlc_minimum_msatusing dummy TLVs, but the amount gate still checks only channel inbound minimum (Lines 158-159). Withamount_msats = Some(_), this can generate blinded paths that are immediately unusable for that amount.Suggested fix
- .filter(|details| amount_msats.unwrap_or(u64::MAX) >= details.inbound_htlc_minimum_msat.unwrap_or(0)) + .filter(|details| { + let min_htlc = cmp::max( + details.inbound_htlc_minimum_msat.unwrap_or(0), + dummy_tlvs.payment_constraints.htlc_minimum_msat, + ); + amount_msats.unwrap_or(u64::MAX) >= min_htlc + })Also applies to: 182-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/routing/router.rs` around lines 153 - 159, The amount eligibility filter currently only compares amount_msats against details.inbound_htlc_minimum_msat/inbound_htlc_maximum_msat on the first_hops iterator, but you raise the effective HTLC minimum later using dummy TLVs (payment_constraints.htlc_minimum_msat), which can allow selecting paths that will be rejected; update the filtering logic in the first_hops preparation (where first_hops.into_iter() and the amount_msats filters run) to use the effective minimum/maximum from payment_constraints (the htlc_minimum_msat set after applying DummyTlvs) when amount_msats.is_some(), i.e., compare amount_msats against payment_constraints.htlc_minimum_msat and payment_constraints.htlc_maximum_msat (falling back to channel inbound values only when payment_constraints values are None) so blinded paths are only produced when the amount satisfies the final HTLC constraints.lightning/src/ln/onion_payment.rs (1)
268-276:⚠️ Potential issue | 🟠 MajorPreserve dummy-hop skimmed fee when forwarding.
At Line 275,
dummy_hops_skimmed_fee_msatis reset toNone. This drops any accumulated dummy-hop skim value on forwarded HTLCs, so downstream claim/accounting can lose the extra fee attribution.Suggested fix
Ok(PendingHTLCInfo { routing, payment_hash: msg.payment_hash, incoming_shared_secret: shared_secret, incoming_amt_msat: Some(msg.amount_msat), outgoing_amt_msat: amt_to_forward, outgoing_cltv_value, - dummy_hops_skimmed_fee_msat: None, + dummy_hops_skimmed_fee_msat: msg.dummy_hops_skimmed_fee_msat, skimmed_fee_msat: None, incoming_accountable: msg.accountable.unwrap_or(false), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/onion_payment.rs` around lines 268 - 276, When constructing the PendingHTLCInfo in onion_payment.rs, don't overwrite the existing dummy-hop skim value with None; preserve the incoming dummy_hops_skimmed_fee_msat from the message/state so downstream claim/accounting retains the accumulated dummy-hop skim. Update the PendingHTLCInfo creation (the struct literal that sets dummy_hops_skimmed_fee_msat and skimmed_fee_msat) to assign the appropriate source (e.g., msg.dummy_hops_skimmed_fee_msat or the previously accumulated variable) instead of None, keeping skimmed_fee_msat handling consistent.
🧹 Nitpick comments (3)
lightning/src/ln/channelmanager.rs (1)
21264-21268: Add one explicit non-zero dummy-hop fee regression in this block.These call sites were updated for the new parameter, but this test block still only asserts
skimmed_fee_msathandling. A pass/fail pair withdummy_hops_skimmed_fee_msat != 0would pin down the new receive-path accounting locally.Also applies to: 21289-21290, 21316-21316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 21264 - 21268, The test block calling create_recv_pending_htlc_info needs an additional explicit non-zero dummy-hop fee regression: add a second invocation (or modify the existing assertion block) that sets up a non-zero dummy-hop skim fee and then assert that the returned skimmed_fee_msat (or dummy_hops_skimmed_fee_msat) is != 0 and is handled as expected; update the create_recv_pending_htlc_info call site(s) referenced (function name: create_recv_pending_htlc_info, error type: crate::ln::channelmanager::InboundHTLCErr) to include this non-zero dummy-hop scenario and mirror the same pass/fail assertion pattern at the other two similar locations mentioned (the blocks around the other create_recv_pending_htlc_info calls). Ensure the test exercises receive-path accounting with the non-zero dummy hop fee rather than only zero-fee cases.lightning/src/ln/blinded_payment_tests.rs (1)
346-351: Add an explicit malformed-update length assertion before indexing.Indexing directly at Line 350 can panic with less actionable output if behavior regresses. An explicit assertion makes failures clearer.
Optional test hardening
let mut updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()); + assert_eq!(updates.update_fail_malformed_htlcs.len(), 1); // Blinded receive failures are surfaced to the sender as malformed onion blinding errors. nodes[0].node.handle_update_fail_malformed_htlc( nodes[1].node.get_our_node_id(), &updates.update_fail_malformed_htlcs[0], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 346 - 351, Before indexing into updates.update_fail_malformed_htlcs in the test, add an explicit assertion that the vector is non-empty to avoid an obscure panic; locate the code around get_htlc_update_msgs(...) and the call to nodes[0].node.handle_update_fail_malformed_htlc(...), and insert an assert (e.g. assert!(!updates.update_fail_malformed_htlcs.is_empty(), "expected at least one malformed HTLC update from get_htlc_update_msgs")) immediately prior to using updates.update_fail_malformed_htlcs[0].lightning/src/ln/channel.rs (1)
14423-14424: Prefer preallocating this vector for consistency with adjacent code.
pending_outbound_dummy_hops_skimmed_feescan useVec::with_capacity(...)like the nearby vectors since the count is known.Suggested tweak
- let mut pending_outbound_dummy_hops_skimmed_fees: Vec<Option<u64>> = Vec::new(); + let mut pending_outbound_dummy_hops_skimmed_fees: Vec<Option<u64>> = + Vec::with_capacity(self.context.pending_outbound_htlcs.len());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channel.rs` around lines 14423 - 14424, Replace the empty Vec::new() for pending_outbound_dummy_hops_skimmed_fees with a preallocated Vec::with_capacity(...) using the same known count used by the adjacent pending_outbound_* vectors (i.e., match the capacity used for pending_outbound_skimmed_fees/other nearby vectors) so the vector is preallocated consistently; update the initialization of pending_outbound_dummy_hops_skimmed_fees to use Vec::with_capacity(the_known_count) where the_known_count is the same length/capacity source used by the surrounding vectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/events/mod.rs`:
- Around line 1948-1952: The optional field is being encoded/decoded using an
even TLV type (16); change it to an odd type (e.g., 17) for backward-compatible
optional TLV usage. Update the serializer where dummy_skimmed_fee_opt /
dummy_hops_skimmed_fee_msat is written (currently using type 16) and the
corresponding deserializer that reads it (same symbol) to use the new odd TLV
type, ensuring both sides match; also run/update any tests that assert TLV type
values.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 430-436: Doc comment contains typos and a placeholder: fix
spelling of "preceding"/"precedes", change "This fees" to "This fee" (singular),
and remove or replace the placeholder "LDK x.0" with a concrete wording that
won't leak into public docs (e.g., "in this LDK release" or omit version).
Update the comments around the public field dummy_hops_skimmed_fee_msat and the
references to DummyTlvs and Event::PaymentClaimable (also apply the same
corrections to the corresponding comment block near the later occurrence of the
same text) so the generated docs are grammatically correct and do not contain
placeholder version text.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 1318-1326: The per-path lookup over expected_paths currently
reuses the same claimed HTLC because each map closure calls
htlcs.iter().find(...), allowing duplicates; modify the logic to ensure each
matched ClaimedHTLC is consumed exactly once by either (a) iterating
expected_paths and for each path searching for a matching HTLC in a mutable Vec
of htlcs, removing it via its position (so subsequent searches can't reuse it),
or (b) tracking matched HTLC identities in a HashSet and skipping already-used
ones; reference the symbols expected_paths, htlcs, and claimed_htlc_matches_path
to locate and update the matching loop so each expected path gets a distinct
ClaimedHTLC.
---
Outside diff comments:
In `@lightning/src/ln/onion_payment.rs`:
- Around line 268-276: When constructing the PendingHTLCInfo in
onion_payment.rs, don't overwrite the existing dummy-hop skim value with None;
preserve the incoming dummy_hops_skimmed_fee_msat from the message/state so
downstream claim/accounting retains the accumulated dummy-hop skim. Update the
PendingHTLCInfo creation (the struct literal that sets
dummy_hops_skimmed_fee_msat and skimmed_fee_msat) to assign the appropriate
source (e.g., msg.dummy_hops_skimmed_fee_msat or the previously accumulated
variable) instead of None, keeping skimmed_fee_msat handling consistent.
In `@lightning/src/routing/router.rs`:
- Around line 153-159: The amount eligibility filter currently only compares
amount_msats against details.inbound_htlc_minimum_msat/inbound_htlc_maximum_msat
on the first_hops iterator, but you raise the effective HTLC minimum later using
dummy TLVs (payment_constraints.htlc_minimum_msat), which can allow selecting
paths that will be rejected; update the filtering logic in the first_hops
preparation (where first_hops.into_iter() and the amount_msats filters run) to
use the effective minimum/maximum from payment_constraints (the
htlc_minimum_msat set after applying DummyTlvs) when amount_msats.is_some(),
i.e., compare amount_msats against payment_constraints.htlc_minimum_msat and
payment_constraints.htlc_maximum_msat (falling back to channel inbound values
only when payment_constraints values are None) so blinded paths are only
produced when the amount satisfies the final HTLC constraints.
---
Nitpick comments:
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 346-351: Before indexing into updates.update_fail_malformed_htlcs
in the test, add an explicit assertion that the vector is non-empty to avoid an
obscure panic; locate the code around get_htlc_update_msgs(...) and the call to
nodes[0].node.handle_update_fail_malformed_htlc(...), and insert an assert (e.g.
assert!(!updates.update_fail_malformed_htlcs.is_empty(), "expected at least one
malformed HTLC update from get_htlc_update_msgs")) immediately prior to using
updates.update_fail_malformed_htlcs[0].
In `@lightning/src/ln/channel.rs`:
- Around line 14423-14424: Replace the empty Vec::new() for
pending_outbound_dummy_hops_skimmed_fees with a preallocated
Vec::with_capacity(...) using the same known count used by the adjacent
pending_outbound_* vectors (i.e., match the capacity used for
pending_outbound_skimmed_fees/other nearby vectors) so the vector is
preallocated consistently; update the initialization of
pending_outbound_dummy_hops_skimmed_fees to use
Vec::with_capacity(the_known_count) where the_known_count is the same
length/capacity source used by the surrounding vectors.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 21264-21268: The test block calling create_recv_pending_htlc_info
needs an additional explicit non-zero dummy-hop fee regression: add a second
invocation (or modify the existing assertion block) that sets up a non-zero
dummy-hop skim fee and then assert that the returned skimmed_fee_msat (or
dummy_hops_skimmed_fee_msat) is != 0 and is handled as expected; update the
create_recv_pending_htlc_info call site(s) referenced (function name:
create_recv_pending_htlc_info, error type:
crate::ln::channelmanager::InboundHTLCErr) to include this non-zero dummy-hop
scenario and mirror the same pass/fail assertion pattern at the other two
similar locations mentioned (the blocks around the other
create_recv_pending_htlc_info calls). Ensure the test exercises receive-path
accounting with the non-zero dummy hop fee rather than only zero-fee cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78016ae9-345f-4cc1-986e-6997c199c29b
📒 Files selected for processing (15)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- lightning/src/ln/offers_tests.rs
- lightning/src/ln/payment_tests.rs
- lightning/src/ln/msgs.rs
- lightning/src/ln/functional_tests.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightning/src/ln/functional_test_utils.rs (1)
4146-4170:⚠️ Potential issue | 🟡 MinorAdd explicit length checks before zipping per-path vectors.
At Line 4165, chained
ziptruncates to the shortest input. If any vector length diverges, paths/messages are silently dropped and fee expectations become incomplete.💡 Proposed fix
fn pass_claimed_payment_along_route_from_ev_with_path_amounts( per_path_claim_amt_msats: Vec<u64>, mut per_path_msgs: Vec<((msgs::UpdateFulfillHTLC, Vec<msgs::CommitmentSigned>), PublicKey)>, args: ClaimAlongRouteArgs, ) -> u64 { @@ let mut expected_total_fee_msat = 0; + assert_eq!(expected_paths.len(), per_path_claim_amt_msats.len()); + assert_eq!(expected_paths.len(), per_path_msgs.len()); for (i, ((expected_route, path_claim_amt_msat), (path_msgs, next_hop))) in expected_paths .iter()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 4146 - 4170, The chained zip in pass_claimed_payment_along_route_from_ev_with_path_amounts silently drops entries when per_path_claim_amt_msats, per_path_msgs, and expected_paths differ in length; add explicit length checks at the start of this function comparing expected_paths.len() to per_path_claim_amt_msats.len() and per_path_msgs.len() (or per_path_msgs.len() after draining) and return/panic with a clear diagnostic if they mismatch so you don't silently truncate fee expectations and path processing; reference the variables expected_paths, per_path_claim_amt_msats, and per_path_msgs and fail early with a helpful message mentioning the function name pass_claimed_payment_along_route_from_ev_with_path_amounts.
♻️ Duplicate comments (2)
lightning/src/ln/functional_test_utils.rs (1)
1318-1326:⚠️ Potential issue | 🟡 MinorAvoid reusing a single claimed HTLC across multiple expected paths.
At Line 1318, each path does a fresh
.find(...)over the fullhtlcsslice, so oneClaimedHTLCcan satisfy multiple paths and skew per-path fee expectations.💡 Proposed fix
fn claimed_htlc_value_msats_for_paths<'a, 'b, 'c>( origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], htlcs: &[ClaimedHTLC], ) -> Vec<u64> { - expected_paths - .iter() - .map(|path| { - htlcs - .iter() - .find(|htlc| claimed_htlc_matches_path(origin_node, path, htlc)) - .expect("path should have a matching claimed HTLC") - .value_msat - }) - .collect() + let mut remaining_htlcs: Vec<&ClaimedHTLC> = htlcs.iter().collect(); + expected_paths + .iter() + .map(|path| { + let idx = remaining_htlcs + .iter() + .position(|htlc| claimed_htlc_matches_path(origin_node, path, htlc)) + .expect("path should have a unique matching claimed HTLC"); + remaining_htlcs.remove(idx).value_msat + }) + .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 1318 - 1326, The current mapping over expected_paths repeatedly searches the full htlcs slice so a single ClaimedHTLC can be matched to multiple paths; instead maintain a mutable collection of remaining HTLCs (e.g., clone htlcs into a Vec) and for each path use position/find on that mutable collection with claimed_htlc_matches_path(origin_node, path, htlc) then remove the matched HTLC (swap_remove/remove) and use its value_msat so each ClaimedHTLC is consumed exactly once when computing per-path values.lightning/src/events/mod.rs (1)
1979-1979:⚠️ Potential issue | 🟠 MajorUse an odd TLV type for optional
PaymentClaimabledummy-hop fee.
dummy_hops_skimmed_fee_msatis serialized/deserialized as optional TLV type16(even). For forward/backward compatibility of optional fields, this should be odd; otherwise older decoders may reject events when the field is present.💡 Proposed fix
- (16, dummy_skimmed_fee_opt, option), + (17, dummy_skimmed_fee_opt, option),- (16, dummy_skimmed_fee_msat_opt, option), + (17, dummy_skimmed_fee_msat_opt, option),Also applies to: 2456-2456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/events/mod.rs` at line 1979, The TLV type used for the optional dummy hop fee field is currently even (16) which breaks forward/backward compatibility for optional TLVs; update the TLV type for dummy_hops_skimmed_fee_msat / dummy_skimmed_fee_opt to an odd number (e.g., 17) wherever it is registered/handled (the tuple entry (16, dummy_skimmed_fee_opt, option) and the corresponding deserialization/serialization mapping), ensuring both serialization and deserialization use the same new odd type to maintain compatibility with older decoders.
🧹 Nitpick comments (5)
lightning/src/ln/async_payments_tests.rs (1)
3039-3049: Derive this extra CLTV from the constructed blinded path instead of the current defaults.
additional_cltv_expiryis re-encoding the path builder’s default dummy-hop count and per-hop delta inside the test. If those defaults change or become path-dependent, this timeout threshold will silently drift from the invoice under test. Keeping theStaticInvoicefrombuild_async_offer_and_init_paymentand reading the blinded path’s effective delta here would make the test much less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 3039 - 3049, The test currently re-encodes defaults into additional_cltv_expiry using DEFAULT_PAYMENT_DUMMY_HOPS and DEFAULT_DUMMY_HOP_CLTV_EXPIRY_DELTA; instead, grab the constructed StaticInvoice returned by build_async_offer_and_init_payment and derive the extra CLTV directly from its blinded path (e.g., read the blinded path's effective total CLTV delta or per-hop delta/slot count exposed on StaticInvoice or its BlindedPath/PathBuilder) and use that value in the connect_blocks call (replace additional_cltv_expiry calculation with invoice-derived value), so the test follows whatever the path builder actually produced rather than hardcoded defaults.lightning/src/ln/offers_tests.rs (1)
243-248: Let this shared claim helper accept dummy TLVs instead of fixing them to the default array.Hardcoding
[DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS]here means every caller exercises the same dummy-hop shape. Taking a caller-supplied slice would make it much easier to add a focused non-default fee/CLTV case later without bypassing the helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/offers_tests.rs` around lines 243 - 248, The helper currently hardcodes DummyTlvs by calling ClaimAlongRouteArgs::new(...).with_dummy_tlvs(&[DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS]), which forces every caller to use the same dummy-hop shape; change the helper signature so it accepts a caller-provided slice or Vec of DummyTlvs and pass that into with_dummy_tlvs instead of constructing the default array inline (update callers to provide their desired dummy_tlvs or use the previous default constant when appropriate); touch the ClaimAlongRouteArgs usage, the helper function definition, and call sites that relied on the old behavior to forward the provided dummy TLVs.lightning/src/ln/msgs.rs (1)
6138-6166: Add one positive round-trip test fordummy_hops_skimmed_fee_msat.Current updates only cover
None. Please add aSome(value)encode/decode assertion so TLV65539compatibility is locked by tests.✅ Suggested test addition
+ #[test] + fn encoding_update_add_htlc_with_dummy_hops_skimmed_fee() { + let mut msg = test_update_add_htlc(); + msg.dummy_hops_skimmed_fee_msat = Some(42); + + let encoded = msg.encode(); + let decoded: msgs::UpdateAddHTLC = + LengthReadable::read_from_fixed_length_buffer(&mut &encoded[..]).unwrap(); + + assert_eq!(decoded.dummy_hops_skimmed_fee_msat, Some(42)); + }Also applies to: 7050-7068
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` around lines 6138 - 6166, The test encoding_update_add_htlc only exercises UpdateAddHTLC with dummy_hops_skimmed_fee_msat = None; add a positive round-trip case to assert encoding/decoding when dummy_hops_skimmed_fee_msat = Some(value) so TLV 65539 is covered. Modify or add a test (e.g., in encoding_update_add_htlc or a sibling test) that constructs an UpdateAddHTLC with dummy_hops_skimmed_fee_msat: Some(<u64 value>), then call .encode() and decode the bytes back and assert equality of the original UpdateAddHTLC (or that the decoded dummy_hops_skimmed_fee_msat equals Some(value)); reference the UpdateAddHTLC struct, encoding_update_add_htlc test, and the dummy_hops_skimmed_fee_msat field to locate where to add the assertion.lightning/src/ln/channel.rs (1)
12385-12390: Consider grouping fee metadata parameters to reduce positional-arg risk.The functions
queue_add_htlc(line 12385),send_htlc(line 12433), andsend_htlc_and_commit(line 12783) accept adjacentOption<u64>parameters—dummy_hops_skimmed_fee_msatfollowed byskimmed_fee_msat—that are easy to accidentally swap at call sites without compiler protection.Current callsites are correct, but introducing a struct for these fee metadata fields would eliminate this class of error:
Refactor sketch
+struct HtlcFeeMetadata { + dummy_hops_skimmed_fee_msat: Option<u64>, + skimmed_fee_msat: Option<u64>, +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channel.rs` around lines 12385 - 12390, Replace the adjacent Option<u64> positional params with a single small struct to avoid accidental swapping: define a FeeMetadata (or similar) struct with fields dummy_hops_skimmed_fee_msat: Option<u64> and skimmed_fee_msat: Option<u64> (derive Copy/Clone/Debug and Default as needed), change the signatures of queue_add_htlc, send_htlc, and send_htlc_and_commit to accept FeeMetadata instead of the two Option<u64> params, update all call sites to construct FeeMetadata { dummy_hops_skimmed_fee_msat: ..., skimmed_fee_msat: ... }, and adjust any doc comments/tests accordingly so parameter order is explicit and compiler-enforced via field names.lightning/src/ln/blinded_payment_tests.rs (1)
346-351: Consider asserting malformed-failure vector length before indexing.Indexing
[0]directly is fine when assumptions hold, but an explicit length/assert gives clearer failure diagnostics if behavior regresses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 346 - 351, The test currently indexes updates.update_fail_malformed_htlcs[0] without checking length; add an explicit assertion or guard before that access (e.g., assert!(!updates.update_fail_malformed_htlcs.is_empty()) or assert_eq!(updates.update_fail_malformed_htlcs.len(), 1)) to ensure the malformed-failure vector contains the expected entry, then call nodes[0].node.handle_update_fail_malformed_htlc(...) with updates.update_fail_malformed_htlcs[0]; reference get_htlc_update_msgs, updates.update_fail_malformed_htlcs and handle_update_fail_malformed_htlc to locate where to insert the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 4146-4170: The chained zip in
pass_claimed_payment_along_route_from_ev_with_path_amounts silently drops
entries when per_path_claim_amt_msats, per_path_msgs, and expected_paths differ
in length; add explicit length checks at the start of this function comparing
expected_paths.len() to per_path_claim_amt_msats.len() and per_path_msgs.len()
(or per_path_msgs.len() after draining) and return/panic with a clear diagnostic
if they mismatch so you don't silently truncate fee expectations and path
processing; reference the variables expected_paths, per_path_claim_amt_msats,
and per_path_msgs and fail early with a helpful message mentioning the function
name pass_claimed_payment_along_route_from_ev_with_path_amounts.
---
Duplicate comments:
In `@lightning/src/events/mod.rs`:
- Line 1979: The TLV type used for the optional dummy hop fee field is currently
even (16) which breaks forward/backward compatibility for optional TLVs; update
the TLV type for dummy_hops_skimmed_fee_msat / dummy_skimmed_fee_opt to an odd
number (e.g., 17) wherever it is registered/handled (the tuple entry (16,
dummy_skimmed_fee_opt, option) and the corresponding
deserialization/serialization mapping), ensuring both serialization and
deserialization use the same new odd type to maintain compatibility with older
decoders.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 1318-1326: The current mapping over expected_paths repeatedly
searches the full htlcs slice so a single ClaimedHTLC can be matched to multiple
paths; instead maintain a mutable collection of remaining HTLCs (e.g., clone
htlcs into a Vec) and for each path use position/find on that mutable collection
with claimed_htlc_matches_path(origin_node, path, htlc) then remove the matched
HTLC (swap_remove/remove) and use its value_msat so each ClaimedHTLC is consumed
exactly once when computing per-path values.
---
Nitpick comments:
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 3039-3049: The test currently re-encodes defaults into
additional_cltv_expiry using DEFAULT_PAYMENT_DUMMY_HOPS and
DEFAULT_DUMMY_HOP_CLTV_EXPIRY_DELTA; instead, grab the constructed StaticInvoice
returned by build_async_offer_and_init_payment and derive the extra CLTV
directly from its blinded path (e.g., read the blinded path's effective total
CLTV delta or per-hop delta/slot count exposed on StaticInvoice or its
BlindedPath/PathBuilder) and use that value in the connect_blocks call (replace
additional_cltv_expiry calculation with invoice-derived value), so the test
follows whatever the path builder actually produced rather than hardcoded
defaults.
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 346-351: The test currently indexes
updates.update_fail_malformed_htlcs[0] without checking length; add an explicit
assertion or guard before that access (e.g.,
assert!(!updates.update_fail_malformed_htlcs.is_empty()) or
assert_eq!(updates.update_fail_malformed_htlcs.len(), 1)) to ensure the
malformed-failure vector contains the expected entry, then call
nodes[0].node.handle_update_fail_malformed_htlc(...) with
updates.update_fail_malformed_htlcs[0]; reference get_htlc_update_msgs,
updates.update_fail_malformed_htlcs and handle_update_fail_malformed_htlc to
locate where to insert the check.
In `@lightning/src/ln/channel.rs`:
- Around line 12385-12390: Replace the adjacent Option<u64> positional params
with a single small struct to avoid accidental swapping: define a FeeMetadata
(or similar) struct with fields dummy_hops_skimmed_fee_msat: Option<u64> and
skimmed_fee_msat: Option<u64> (derive Copy/Clone/Debug and Default as needed),
change the signatures of queue_add_htlc, send_htlc, and send_htlc_and_commit to
accept FeeMetadata instead of the two Option<u64> params, update all call sites
to construct FeeMetadata { dummy_hops_skimmed_fee_msat: ..., skimmed_fee_msat:
... }, and adjust any doc comments/tests accordingly so parameter order is
explicit and compiler-enforced via field names.
In `@lightning/src/ln/msgs.rs`:
- Around line 6138-6166: The test encoding_update_add_htlc only exercises
UpdateAddHTLC with dummy_hops_skimmed_fee_msat = None; add a positive round-trip
case to assert encoding/decoding when dummy_hops_skimmed_fee_msat = Some(value)
so TLV 65539 is covered. Modify or add a test (e.g., in encoding_update_add_htlc
or a sibling test) that constructs an UpdateAddHTLC with
dummy_hops_skimmed_fee_msat: Some(<u64 value>), then call .encode() and decode
the bytes back and assert equality of the original UpdateAddHTLC (or that the
decoded dummy_hops_skimmed_fee_msat equals Some(value)); reference the
UpdateAddHTLC struct, encoding_update_add_htlc test, and the
dummy_hops_skimmed_fee_msat field to locate where to add the assertion.
In `@lightning/src/ln/offers_tests.rs`:
- Around line 243-248: The helper currently hardcodes DummyTlvs by calling
ClaimAlongRouteArgs::new(...).with_dummy_tlvs(&[DummyTlvs::default();
DEFAULT_PAYMENT_DUMMY_HOPS]), which forces every caller to use the same
dummy-hop shape; change the helper signature so it accepts a caller-provided
slice or Vec of DummyTlvs and pass that into with_dummy_tlvs instead of
constructing the default array inline (update callers to provide their desired
dummy_tlvs or use the previous default constant when appropriate); touch the
ClaimAlongRouteArgs usage, the helper function definition, and call sites that
relied on the old behavior to forward the provided dummy TLVs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a811f74c-3977-4851-9d70-918a3f316f01
📒 Files selected for processing (15)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
from
1137288 to
3649764
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lightning/src/ln/channelmanager.rs (1)
551-553:⚠️ Potential issue | 🟡 MinorFix remaining typo/grammar in field docs (Line 551).
preceedingis misspelled, and the sentence on Lines 552-553 reads awkwardly.✏️ Suggested doc fix
- /// The amount (in msats) skimmed off by the dummy hops preceeding the HTLC. - /// This amount is the extra amount that the final receiver earns in addition - /// to the [`Self::value`]. And is set as such in [`Event::PaymentClaimable`]. + /// The amount (in msats) skimmed off by the dummy hops preceding the HTLC. + /// This is the extra amount the final receiver earns in addition to + /// [`Self::value`], and is surfaced in [`Event::PaymentClaimable`].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 551 - 553, Fix the typo and smooth the wording in the field docs in channelmanager.rs: change "preceeding" to "preceding" and combine the second and third sentences into a single clearer sentence (e.g., "This amount is extra that the final receiver earns in addition to [`Self::value`] and is reflected in [`Event::PaymentClaimable`].") so the docs read naturally while still referencing Self::value and Event::PaymentClaimable.
🧹 Nitpick comments (2)
lightning/src/ln/msgs.rs (1)
3638-3638: Add one explicit wire round-trip test fordummy_hops_skimmed_fee_msat = Some(...).Line 3638 wires the new TLV type correctly, but this file only exercises
Noneinitializations. A focusedSome(value)encode/decode assertion would prevent silent regressions in TLV type/value mapping.✅ Suggested test addition
+ #[test] + fn encoding_update_add_htlc_with_dummy_hops_skimmed_fee() { + let mut msg = test_update_add_htlc(); + msg.dummy_hops_skimmed_fee_msat = Some(42); + + let encoded = msg.encode(); + let decoded: msgs::UpdateAddHTLC = + LengthReadable::read_from_fixed_length_buffer(&mut &encoded[..]).unwrap(); + + assert_eq!(decoded.dummy_hops_skimmed_fee_msat, Some(42)); + assert_eq!(decoded.skimmed_fee_msat, None); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` at line 3638, Add a unit test that explicitly round-trips a message containing dummy_hops_skimmed_fee_msat = Some(value) (TLV type 65539): construct the message/struct that holds the dummy_hops_skimmed_fee_msat field with a non-zero u64 value, encode it to TLV bytes using the existing encode/serialize helper (or round_trip_tlv utility if present), decode/deserialize back, and assert the decoded struct has dummy_hops_skimmed_fee_msat == Some(value); ensure the test lives alongside other TLV round-trip tests and references the dummy_hops_skimmed_fee_msat symbol and the TLV type 65539 so regressions in type/value mapping are caught.lightning/src/ln/async_payments_tests.rs (1)
987-989: Extract a shared dummy-hop test fixture.
[DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS]is now repeated across the file, whileheld_htlc_timeoutrecomputes the matching CLTV delta separately. A tiny helper for the default array and its summed CLTV delta would keep these fixtures from drifting.♻️ Suggested cleanup
+fn default_dummy_tlvs() -> [DummyTlvs; DEFAULT_PAYMENT_DUMMY_HOPS] { + [DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS] +} + +fn default_dummy_hop_cltv_expiry_delta() -> u32 { + default_dummy_tlvs() + .iter() + .map(|tlvs| tlvs.payment_relay.cltv_expiry_delta as u32) + .sum() +} + ... - let dummy_tlvs = [DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS]; + let dummy_tlvs = default_dummy_tlvs(); ... - let additional_cltv_expiry = - DEFAULT_PAYMENT_DUMMY_HOPS as u32 * DEFAULT_DUMMY_HOP_CLTV_EXPIRY_DELTA as u32; + let additional_cltv_expiry = default_dummy_hop_cltv_expiry_delta();Also applies to: 3039-3041
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 987 - 989, Tests repeat creating the default dummy-hop array and separately recomputing the matching CLTV delta; extract a small shared fixture function or helper (e.g., default_dummy_tlvs() -> [DummyTlvs; DEFAULT_PAYMENT_DUMMY_HOPS] plus a companion helper default_dummy_tlvs_cltv_delta() or a struct that returns both) and replace inline uses in PassAlongPathArgs::with_dummy_tlvs(...) and in held_htlc_timeout calculations so the array and summed CLTV delta stay consistent across tests (refer to DummyTlvs, DEFAULT_PAYMENT_DUMMY_HOPS, PassAlongPathArgs::new, with_dummy_tlvs, and held_htlc_timeout).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lightning/src/ln/channelmanager.rs`:
- Around line 551-553: Fix the typo and smooth the wording in the field docs in
channelmanager.rs: change "preceeding" to "preceding" and combine the second and
third sentences into a single clearer sentence (e.g., "This amount is extra that
the final receiver earns in addition to [`Self::value`] and is reflected in
[`Event::PaymentClaimable`].") so the docs read naturally while still
referencing Self::value and Event::PaymentClaimable.
---
Nitpick comments:
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 987-989: Tests repeat creating the default dummy-hop array and
separately recomputing the matching CLTV delta; extract a small shared fixture
function or helper (e.g., default_dummy_tlvs() -> [DummyTlvs;
DEFAULT_PAYMENT_DUMMY_HOPS] plus a companion helper
default_dummy_tlvs_cltv_delta() or a struct that returns both) and replace
inline uses in PassAlongPathArgs::with_dummy_tlvs(...) and in held_htlc_timeout
calculations so the array and summed CLTV delta stay consistent across tests
(refer to DummyTlvs, DEFAULT_PAYMENT_DUMMY_HOPS, PassAlongPathArgs::new,
with_dummy_tlvs, and held_htlc_timeout).
In `@lightning/src/ln/msgs.rs`:
- Line 3638: Add a unit test that explicitly round-trips a message containing
dummy_hops_skimmed_fee_msat = Some(value) (TLV type 65539): construct the
message/struct that holds the dummy_hops_skimmed_fee_msat field with a non-zero
u64 value, encode it to TLV bytes using the existing encode/serialize helper (or
round_trip_tlv utility if present), decode/deserialize back, and assert the
decoded struct has dummy_hops_skimmed_fee_msat == Some(value); ensure the test
lives alongside other TLV round-trip tests and references the
dummy_hops_skimmed_fee_msat symbol and the TLV type 65539 so regressions in
type/value mapping are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20fe409a-07c8-406b-9cfc-217292633517
📒 Files selected for processing (15)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
✅ Files skipped from review due to trivial changes (1)
- lightning/src/ln/channel.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- lightning/src/ln/onion_utils.rs
- lightning/src/routing/router.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/blinded_payment_tests.rs
- lightning/src/ln/htlc_reserve_unit_tests.rs
- lightning/src/blinded_path/payment.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lightning/src/ln/functional_test_utils.rs (1)
3709-3714:⚠️ Potential issue | 🟠 Major
receiving_channel_ids.len() == 1is not a reliable single-part check.Line 3709 can still be true for multipart claims arriving via the same channel, which reintroduces false assertion failures for
dummy_hops_skimmed_fee_msat.💡 Proposed fix
- if receiving_channel_ids.len() == 1 { + let is_single_part_claim = payment_event.msgs[0].amount_msat == recv_value; + if is_single_part_claim { assert_eq!( dummy_hops_total_fee_msat(recv_value, &dummy_tlvs), *dummy_hops_skimmed_fee_msat ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 3709 - 3714, The current single-part check uses receiving_channel_ids.len() == 1 which can be true for multipart claims that all arrived via the same channel; replace this with a real single-part check by asserting the number of incoming parts is 1 (e.g., check the length of the collection that tracks individual incoming part amounts/HTLCs rather than channel IDs). Specifically, in the block using receiving_channel_ids, change the condition to verify the actual parts count (look for a variable like receiving_part_amounts, incoming_htlcs, parts, or similar in the surrounding code) and only run the dummy_hops_total_fee_msat(...) == *dummy_hops_skimmed_fee_msat assertion when that parts-count equals 1, keeping the same use of recv_value and dummy_tlvs/dummy_hops_skimmed_fee_msat.
🧹 Nitpick comments (4)
lightning/src/ln/onion_utils.rs (1)
2577-2579: LGTM with optional defensive assertion suggestion.The fee calculation and accumulation logic is correct. The use of
saturating_subandsaturating_addprovides safety against overflow/underflow panics. Sincecheck_blinded_forward()(per context snippet 2) validates that the amounts are valid before reaching this code,msg.amount_msat >= outgoing_amt_msatshould always hold.Consider adding a debug_assert to catch any upstream validation failures during development:
🛡️ Optional defensive assertion
+ debug_assert!( + msg.amount_msat >= outgoing_amt_msat, + "Dummy hop fee calculation: incoming amount should be >= outgoing amount" + ); 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);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/onion_utils.rs` around lines 2577 - 2579, Add a defensive debug assertion before computing this_dummy_fee to ensure upstream validation holds: assert that msg.amount_msat >= outgoing_amt_msat (this mirrors the contract enforced by check_blinded_forward()). Then keep the existing saturating_sub/saturating_add logic that computes this_dummy_fee and updates dummy_hops_skimmed_fee_msat; reference the variables msg.amount_msat, outgoing_amt_msat, this_dummy_fee, and dummy_hops_skimmed_fee_msat so you place the debug_assert immediately above the current calculation.lightning/src/ln/msgs.rs (1)
6157-6165: Add one positive round-trip test fordummy_hops_skimmed_fee_msat = Some(_).Current coverage only exercises the
Nonepath, so regressions in TLV65539encoding/decoding could slip through.🧪 Minimal test addition
+ #[test] + fn encoding_update_add_htlc_with_dummy_hops_skimmed_fee() { + let mut msg = test_update_add_htlc(); + msg.dummy_hops_skimmed_fee_msat = Some(42); + + let encoded = msg.encode(); + let decoded: msgs::UpdateAddHTLC = + LengthReadable::read_from_fixed_length_buffer(&mut &encoded[..]).unwrap(); + + assert_eq!(decoded.dummy_hops_skimmed_fee_msat, Some(42)); + assert_eq!(decoded.skimmed_fee_msat, None); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` around lines 6157 - 6165, Add a positive round-trip test that exercises the Some(_) branch for the TLV field dummy_hops_skimmed_fee_msat: construct an UpdateAddHTLC instance (the same one used in the existing test) but set dummy_hops_skimmed_fee_msat = Some(<u64 value>), then call update_add_htlc.encode() and either assert the encoded bytes equal the expected hex for that TLV or decode the bytes back and assert the decoded UpdateAddHTLC matches the original; reference the UpdateAddHTLC type and the update_add_htlc.encode()/decode path used in the existing test to locate where to add this new assertion.lightning/src/ln/functional_test_utils.rs (1)
4168-4173: Add explicit length checks before the nestedzipto prevent silent truncation.If any vector length drifts, the current loop will silently skip paths instead of failing loudly.
♻️ Proposed hardening
fn pass_claimed_payment_along_route_from_ev_with_path_amounts( per_path_claim_amt_msats: Vec<u64>, mut per_path_msgs: Vec<((msgs::UpdateFulfillHTLC, Vec<msgs::CommitmentSigned>), PublicKey)>, args: ClaimAlongRouteArgs, ) -> u64 { let ClaimAlongRouteArgs { origin_node, expected_paths, dummy_tlvs, expected_extra_fees, expected_min_htlc_overpay, skip_last, payment_preimage: our_payment_preimage, allow_1_msat_fee_overpay, .. } = args; let mut expected_total_fee_msat = 0; + assert_eq!(expected_paths.len(), per_path_claim_amt_msats.len()); + assert_eq!(expected_paths.len(), per_path_msgs.len()); for (i, ((expected_route, path_claim_amt_msat), (path_msgs, next_hop))) in expected_paths🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 4168 - 4173, The loop zips expected_paths, per_path_claim_amt_msats and per_path_msgs which can silently truncate if their lengths differ; before the nested zip/iterate (around the loop using expected_paths, per_path_claim_amt_msats and per_path_msgs in lightning/src/ln/functional_test_utils.rs) add explicit length checks comparing expected_paths.len(), per_path_claim_amt_msats.len(), and per_path_msgs.len() and fail fast (panic or return a Result::Err) with a clear error mentioning the mismatched collection names (expected_paths, per_path_claim_amt_msats, per_path_msgs) so any length drift is caught rather than silently truncating during the zip/enumerate loop.lightning/src/ln/blinded_payment_tests.rs (1)
346-351: Add an explicit malformed-update length assertion before indexing.This makes failures clearer than an index panic if message expectations regress.
Suggested tweak
let mut updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()); +assert_eq!(updates.update_fail_malformed_htlcs.len(), 1); // Blinded receive failures are surfaced to the sender as malformed onion blinding errors. nodes[0].node.handle_update_fail_malformed_htlc( nodes[1].node.get_our_node_id(), &updates.update_fail_malformed_htlcs[0], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 346 - 351, Before indexing updates.update_fail_malformed_htlcs, add an explicit length assertion to make test failures clearer: after calling get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()) assert that updates.update_fail_malformed_htlcs.len() > 0 (or == 1 if the test expects exactly one) so the subsequent call to nodes[0].node.handle_update_fail_malformed_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_malformed_htlcs[0]) is safe and yields a clear assertion error instead of an index panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 3709-3714: The current single-part check uses
receiving_channel_ids.len() == 1 which can be true for multipart claims that all
arrived via the same channel; replace this with a real single-part check by
asserting the number of incoming parts is 1 (e.g., check the length of the
collection that tracks individual incoming part amounts/HTLCs rather than
channel IDs). Specifically, in the block using receiving_channel_ids, change the
condition to verify the actual parts count (look for a variable like
receiving_part_amounts, incoming_htlcs, parts, or similar in the surrounding
code) and only run the dummy_hops_total_fee_msat(...) ==
*dummy_hops_skimmed_fee_msat assertion when that parts-count equals 1, keeping
the same use of recv_value and dummy_tlvs/dummy_hops_skimmed_fee_msat.
---
Nitpick comments:
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 346-351: Before indexing updates.update_fail_malformed_htlcs, add
an explicit length assertion to make test failures clearer: after calling
get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()) assert that
updates.update_fail_malformed_htlcs.len() > 0 (or == 1 if the test expects
exactly one) so the subsequent call to
nodes[0].node.handle_update_fail_malformed_htlc(nodes[1].node.get_our_node_id(),
&updates.update_fail_malformed_htlcs[0]) is safe and yields a clear assertion
error instead of an index panic.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 4168-4173: The loop zips expected_paths, per_path_claim_amt_msats
and per_path_msgs which can silently truncate if their lengths differ; before
the nested zip/iterate (around the loop using expected_paths,
per_path_claim_amt_msats and per_path_msgs in
lightning/src/ln/functional_test_utils.rs) add explicit length checks comparing
expected_paths.len(), per_path_claim_amt_msats.len(), and per_path_msgs.len()
and fail fast (panic or return a Result::Err) with a clear error mentioning the
mismatched collection names (expected_paths, per_path_claim_amt_msats,
per_path_msgs) so any length drift is caught rather than silently truncating
during the zip/enumerate loop.
In `@lightning/src/ln/msgs.rs`:
- Around line 6157-6165: Add a positive round-trip test that exercises the
Some(_) branch for the TLV field dummy_hops_skimmed_fee_msat: construct an
UpdateAddHTLC instance (the same one used in the existing test) but set
dummy_hops_skimmed_fee_msat = Some(<u64 value>), then call
update_add_htlc.encode() and either assert the encoded bytes equal the expected
hex for that TLV or decode the bytes back and assert the decoded UpdateAddHTLC
matches the original; reference the UpdateAddHTLC type and the
update_add_htlc.encode()/decode path used in the existing test to locate where
to add this new assertion.
In `@lightning/src/ln/onion_utils.rs`:
- Around line 2577-2579: Add a defensive debug assertion before computing
this_dummy_fee to ensure upstream validation holds: assert that msg.amount_msat
>= outgoing_amt_msat (this mirrors the contract enforced by
check_blinded_forward()). Then keep the existing saturating_sub/saturating_add
logic that computes this_dummy_fee and updates dummy_hops_skimmed_fee_msat;
reference the variables msg.amount_msat, outgoing_amt_msat, this_dummy_fee, and
dummy_hops_skimmed_fee_msat so you place the debug_assert immediately above the
current calculation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9915b22c-1346-4079-8351-6dac3dc31402
📒 Files selected for processing (15)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
from
44f98c6 to
b68b4ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lightning/src/ln/async_payments_tests.rs (1)
987-989: Deduplicate default dummy TLV setup used across many tests.Line 987 introduces a pattern repeated throughout this file. A small helper will reduce boilerplate and future edit drift.
♻️ Suggested refactor
+fn default_dummy_tlvs() -> [DummyTlvs; DEFAULT_PAYMENT_DUMMY_HOPS] { + [DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS] +} + // ... -let dummy_tlvs = [DummyTlvs::default(); DEFAULT_PAYMENT_DUMMY_HOPS]; +let dummy_tlvs = default_dummy_tlvs();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 987 - 989, Multiple tests repeat creating a default DummyTlvs array and calling with_dummy_tlvs; extract this boilerplate into a small helper function (e.g., fn default_dummy_tlvs() -> [DummyTlvs; DEFAULT_PAYMENT_DUMMY_HOPS]) and use it where currently created, updating calls like PassAlongPathArgs::new(...).with_dummy_tlvs(&dummy_tlvs) to use .with_dummy_tlvs(&default_dummy_tlvs()) (or pass the helper result into a local let) to remove duplication while keeping existing types and names (referencing DummyTlvs, DEFAULT_PAYMENT_DUMMY_HOPS, PassAlongPathArgs::new, and with_dummy_tlvs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 4006-4017: The helper dummy_hops_total_fee_msat currently iterates
dummy_tlvs forward, but fee accumulation must proceed from the recipient-facing
dummy hop backward; change the loop to iterate dummy_tlvs in reverse (e.g.,
iterate over dummy_tlvs.iter().rev() or index from len-1 down to 0) so you
compute each hop's fee based on the amount destined for the next hop, keeping
the existing base_fee_msat/proportional fee calculation and the amount_msat +=
fee_msat update logic intact.
---
Nitpick comments:
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 987-989: Multiple tests repeat creating a default DummyTlvs array
and calling with_dummy_tlvs; extract this boilerplate into a small helper
function (e.g., fn default_dummy_tlvs() -> [DummyTlvs;
DEFAULT_PAYMENT_DUMMY_HOPS]) and use it where currently created, updating calls
like PassAlongPathArgs::new(...).with_dummy_tlvs(&dummy_tlvs) to use
.with_dummy_tlvs(&default_dummy_tlvs()) (or pass the helper result into a local
let) to remove duplication while keeping existing types and names (referencing
DummyTlvs, DEFAULT_PAYMENT_DUMMY_HOPS, PassAlongPathArgs::new, and
with_dummy_tlvs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9e706d1-afeb-4ed4-8e0a-fb72b7b7eda8
📒 Files selected for processing (5)
lightning/src/blinded_path/payment.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/functional_test_utils.rslightning/src/routing/router.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- lightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
2 times, most recently
from
673a49f to
cdb8c22
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lightning/src/ln/channelmanager.rs (1)
551-553:⚠️ Potential issue | 🟡 MinorTighten the remaining dummy-fee docs.
Line 551 still has the
preceedingtypo, and the sentence on Lines 552-553 reads awkwardly. Mirroring the cleaned-up wording used above on the publicdummy_hops_skimmed_fee_msatfield would keep the two comment blocks consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 551 - 553, Fix the typo and tighten the doc comment for the private dummy-fee field: change "preceeding" to "preceding" and reword the two-sentence block so it mirrors the cleaner wording used for the public dummy_hops_skimmed_fee_msat field—state that this amount (in msats) is the extra fee skimmed by the dummy hops preceding the HTLC and is earned by the final receiver in addition to Self::value, and note it is set in Event::PaymentClaimable.
🧹 Nitpick comments (5)
lightning/src/ln/msgs.rs (1)
6157-6157: Add one positive round-trip test fordummy_hops_skimmed_fee_msat = Some(_).These updates keep existing encodings stable, but we still need explicit coverage for non-
Noneencoding/decoding of the new TLV.✅ Suggested test addition
#[test] fn test_update_add_htlc_accountable_encoding() { @@ } +#[test] +fn encoding_update_add_htlc_with_dummy_hops_skimmed_fee() { + let mut msg = test_update_add_htlc(); + msg.dummy_hops_skimmed_fee_msat = Some(42); + + let encoded = msg.encode(); + let decoded: msgs::UpdateAddHTLC = + LengthReadable::read_from_fixed_length_buffer(&mut &encoded[..]).unwrap(); + + assert_eq!(decoded.dummy_hops_skimmed_fee_msat, Some(42)); + assert_eq!(decoded.skimmed_fee_msat, None); +} + fn do_test_htlc_accountable_from_u8(accountable_override: Option<u8>, expected: Option<bool>) {Also applies to: 7057-7057
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` at line 6157, Add a positive round-trip unit test that constructs the same message type that contains the dummy_hops_skimmed_fee_msat field (set dummy_hops_skimmed_fee_msat = Some(a_nonzero_msat)), serializes it using the crate's encode/Writeable logic and then deserializes it back, asserting the deserialized struct equals the original (including the Some(...) value). Locate the message/struct that defines dummy_hops_skimmed_fee_msat in lightning/src/ln/msgs.rs, create a test analogous to existing TLV round-trip tests, pick a representative value (e.g. 12345), and add the same positive test at the other spot noted in the review so both encodings/decodings for non-None are covered.lightning/src/ln/async_payments_tests.rs (1)
987-994: Add one non-default dummy-hop case.These updates wire the new helper plumbing through the async-flow tests, but in this file every new call site still uses
DummyTlvs::default(). That leaves the new skimmed-fee path and non-default dummy-hop CLTV deltas unexercised here; one end-to-end case with a non-zero skimmed fee would make this much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 987 - 994, Add a second end-to-end case that uses a non-default DummyTlvs array so the skimmed-fee path and non-default dummy-hop CLTV deltas are exercised: construct a dummy_tlvs_nondefault (same length as DEFAULT_PAYMENT_DUMMY_HOPS) with a non-zero skimmed fee and altered CLTV delta values, then call PassAlongPathArgs::with_dummy_tlvs and ClaimAlongRouteArgs::with_dummy_tlvs using dummy_tlvs_nondefault (mirroring the existing flow that calls do_pass_along_path, extract_payment_preimage, and claim_payment_along_route) so one test run uses DummyTlvs::default() and another uses the non-default array.lightning/src/ln/channelmanager.rs (1)
1380-1382: Consider centralizing the dummy-fee aggregation.The same
iter().map(...unwrap_or(0)).sum()now exists in three paths. A small helper would make it harder forClaimingPayment,PaymentClaimable, andPaymentClaimedto drift on future changes.Also applies to: 8349-8350, 20211-20215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 1380 - 1382, Several places (constructors/serializers for ClaimingPayment, PaymentClaimable, and PaymentClaimed) duplicate payment.htlcs.iter().map(|s| s.dummy_hops_skimmed_fee_msat.unwrap_or(0)).sum(); extract this into a single helper like fn aggregate_dummy_hops_fee_msat(payment: &Payment) -> u64 (or an impl method on Payment) and replace the three duplicated calls with that helper; update the three sites that set dummy_hops_skimmed_fee_msat to call aggregate_dummy_hops_fee_msat(payment) so future changes affect all paths consistently.lightning/src/ln/blinded_payment_tests.rs (1)
472-479: Assert the sender-side failure here too.This only proves that node 1 queued some failure. It doesn't exercise the upstream
update_fail_*mapping that the dummy-hop plumbing also changed. Please deliver the failure to node 0 and assert the resultingPaymentFailedConditions, like the underpaid test just above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 472 - 479, The test currently only asserts node 1 queued a failure (expect_htlc_handling_failed_destinations! + checking updates from get_htlc_update_msgs) but never delivers that failure to the sender; update the test to forward the collected updates from get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()) to nodes[0] (simulate delivering the update_fail/update_fail_malformed to the sender) and then assert the sender-side result by checking the PaymentFailedConditions on nodes[0] (same pattern as the underpaid test above), verifying the expected failure reason and counts; use the existing helpers (get_htlc_update_msgs, check_added_monitors, and the PaymentFailedConditions assertion used in the underpaid test) to perform the delivery and assertion.lightning/src/ln/channel.rs (1)
12387-12390: Bundle the two fee options into a single parameter object.These call paths now take two adjacent
Option<u64>fee arguments with the same type, so a swapped call site still compiles and silently changes accounting. A small metadata struct would make the API self-describing; until then, it’s worth double-checking every caller keeps the two values in the intended order.♻️ Possible shape
+struct HtlcFeeMetadata { + dummy_hops_skimmed_fee_msat: Option<u64>, + skimmed_fee_msat: Option<u64>, +} + pub fn queue_add_htlc<F: FeeEstimator, L: Logger>( &mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, - source: HTLCSource, onion_routing_packet: msgs::OnionPacket, - dummy_hops_skimmed_fee_msat: Option<u64>, skimmed_fee_msat: Option<u64>, + source: HTLCSource, onion_routing_packet: msgs::OnionPacket, + fee_metadata: HtlcFeeMetadata, blinding_point: Option<PublicKey>, accountable: bool,Also applies to: 12435-12437, 12785-12787
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channel.rs` around lines 12387 - 12390, The method signature exposes two adjacent Option<u64> fee parameters (dummy_hops_skimmed_fee_msat and skimmed_fee_msat) which can be accidentally swapped; define a small metadata struct (e.g., FeeAmounts or SkimFees) with named fields for the two Option<u64> values, replace the two Option<u64> parameters with a single parameter of that struct in the method signature that contains &mut self, amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, blinding_point, accountable, and update all call sites accordingly; apply the same change for the other two affected signatures/locations noted (the other occurrences around the indicated ranges) so callers pass a named struct instance instead of two positional Option<u64>s.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/blinded_path/payment.rs`:
- Around line 415-419: The constant DEFAULT_DUMMY_HOP_MAX_CLTV_EXPIRY currently
sets an absolute HTLC expiry ceiling (used by DummyTlvs::default()) to 3000
which conflicts with PaymentConstraints::max_cltv_expiry semantics; change the
default to be effectively unbounded (e.g., use u32::MAX or None-equivalent) or
remove the hard cap here and instead compute a finite ceiling at
path-construction time based on the current chain height inside the blinded-path
builder so DummyTlvs::default() does not produce expiries that are already
expired on real networks.
In `@lightning/src/events/mod.rs`:
- Around line 905-907: Update the doc comment for the new field
dummy_hops_skimmed_fee_msat to explicitly state the legacy/compatibility
behavior: when the TLV is absent (older LDKs), it defaults to 0 and therefore
callers cannot distinguish “not present” from “serialized as zero”; document
that consumers should treat 0 as “no skimmed fee information present” for
compatibility. Apply the same documentation note to the other newly added
TLV-backed field(s) nearby (the additional fields added around lines 971–973) so
all new fields clearly describe the legacy default-of-0 behavior.
In `@lightning/src/ln/functional_test_utils.rs`:
- Line 3913: ClaimAlongRouteArgs currently has a single shared field dummy_tlvs:
Vec<DummyTlvs> but PassAlongPathArgs is path-scoped, so change
ClaimAlongRouteArgs::dummy_tlvs to be per-path (e.g. Vec<Vec<DummyTlvs>> or
Vec<Option<Vec<DummyTlvs>>> aligned with expected_paths) and update all code
that constructs or consumes ClaimAlongRouteArgs (including places referring to
dummy_tlvs at the spots you marked around 3960 and 3994–3997) to zip/index
dummy_tlvs with expected_paths when building claim-side checks; ensure helper
code that calls/pass PassAlongPathArgs still produces per-path DummyTlvs and
that claim-time fee check logic iterates the per-path dummy_tlvs rather than
reusing a single shared vector.
- Around line 1323-1328: The current removal uses
remaining_htlcs.iter().position(|htlc| claimed_htlc_matches_path(origin_node,
path, htlc)) which only finds the first match and can silently pick arbitrarily
if multiple HTLCs match; change this to assert uniqueness by
collecting/filtering matches first (e.g., let matches: Vec<_> =
remaining_htlcs.iter().enumerate().filter(|(_,h)|
claimed_htlc_matches_path(origin_node, path, h)).collect();
assert_eq!(matches.len(), 1, "expected exactly one matching ClaimedHTLC for
path"); then remove the matched entry using the recorded index and return its
value_msat) — reference: remaining_htlcs, claimed_htlc_matches_path,
origin_node, path.
---
Duplicate comments:
In `@lightning/src/ln/channelmanager.rs`:
- Around line 551-553: Fix the typo and tighten the doc comment for the private
dummy-fee field: change "preceeding" to "preceding" and reword the two-sentence
block so it mirrors the cleaner wording used for the public
dummy_hops_skimmed_fee_msat field—state that this amount (in msats) is the extra
fee skimmed by the dummy hops preceding the HTLC and is earned by the final
receiver in addition to Self::value, and note it is set in
Event::PaymentClaimable.
---
Nitpick comments:
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 987-994: Add a second end-to-end case that uses a non-default
DummyTlvs array so the skimmed-fee path and non-default dummy-hop CLTV deltas
are exercised: construct a dummy_tlvs_nondefault (same length as
DEFAULT_PAYMENT_DUMMY_HOPS) with a non-zero skimmed fee and altered CLTV delta
values, then call PassAlongPathArgs::with_dummy_tlvs and
ClaimAlongRouteArgs::with_dummy_tlvs using dummy_tlvs_nondefault (mirroring the
existing flow that calls do_pass_along_path, extract_payment_preimage, and
claim_payment_along_route) so one test run uses DummyTlvs::default() and another
uses the non-default array.
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 472-479: The test currently only asserts node 1 queued a failure
(expect_htlc_handling_failed_destinations! + checking updates from
get_htlc_update_msgs) but never delivers that failure to the sender; update the
test to forward the collected updates from get_htlc_update_msgs(&nodes[1],
&nodes[0].node.get_our_node_id()) to nodes[0] (simulate delivering the
update_fail/update_fail_malformed to the sender) and then assert the sender-side
result by checking the PaymentFailedConditions on nodes[0] (same pattern as the
underpaid test above), verifying the expected failure reason and counts; use the
existing helpers (get_htlc_update_msgs, check_added_monitors, and the
PaymentFailedConditions assertion used in the underpaid test) to perform the
delivery and assertion.
In `@lightning/src/ln/channel.rs`:
- Around line 12387-12390: The method signature exposes two adjacent Option<u64>
fee parameters (dummy_hops_skimmed_fee_msat and skimmed_fee_msat) which can be
accidentally swapped; define a small metadata struct (e.g., FeeAmounts or
SkimFees) with named fields for the two Option<u64> values, replace the two
Option<u64> parameters with a single parameter of that struct in the method
signature that contains &mut self, amount_msat, payment_hash, cltv_expiry,
source, onion_routing_packet, blinding_point, accountable, and update all call
sites accordingly; apply the same change for the other two affected
signatures/locations noted (the other occurrences around the indicated ranges)
so callers pass a named struct instance instead of two positional Option<u64>s.
In `@lightning/src/ln/channelmanager.rs`:
- Around line 1380-1382: Several places (constructors/serializers for
ClaimingPayment, PaymentClaimable, and PaymentClaimed) duplicate
payment.htlcs.iter().map(|s| s.dummy_hops_skimmed_fee_msat.unwrap_or(0)).sum();
extract this into a single helper like fn aggregate_dummy_hops_fee_msat(payment:
&Payment) -> u64 (or an impl method on Payment) and replace the three duplicated
calls with that helper; update the three sites that set
dummy_hops_skimmed_fee_msat to call aggregate_dummy_hops_fee_msat(payment) so
future changes affect all paths consistently.
In `@lightning/src/ln/msgs.rs`:
- Line 6157: Add a positive round-trip unit test that constructs the same
message type that contains the dummy_hops_skimmed_fee_msat field (set
dummy_hops_skimmed_fee_msat = Some(a_nonzero_msat)), serializes it using the
crate's encode/Writeable logic and then deserializes it back, asserting the
deserialized struct equals the original (including the Some(...) value). Locate
the message/struct that defines dummy_hops_skimmed_fee_msat in
lightning/src/ln/msgs.rs, create a test analogous to existing TLV round-trip
tests, pick a representative value (e.g. 12345), and add the same positive test
at the other spot noted in the review so both encodings/decodings for non-None
are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 978a0a67-0298-4365-a556-2dd74f393e37
📒 Files selected for processing (15)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
from
cdb8c22 to
0badc51
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lightning/src/ln/channelmanager.rs (1)
551-554:⚠️ Potential issue | 🟡 MinorTypo: "preceeding" → "preceding"
Line 551 still contains the misspelling "preceeding" which should be "preceding".
📝 Suggested fix
- /// The amount (in msats) skimmed off by the dummy hops preceeding the HTLC. + /// The amount (in msats) skimmed off by the dummy hops preceding the HTLC.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 551 - 554, Fix the typo in the doc comment for the field dummy_hops_skimmed_fee_msat: change "preceeding" to "preceding" in the comment describing the dummy hops before the HTLC so the documentation reads correctly for the struct/channelmanager.rs field dummy_hops_skimmed_fee_msat.lightning/src/ln/functional_test_utils.rs (1)
1323-1328:⚠️ Potential issue | 🟠 MajorEnforce an actually unique
ClaimedHTLCmatch here.
position(...)still accepts the first remaining match, so duplicate matches stay order-dependent. That can mis-assign per-path claim amounts and downstream dummy-hop fee expectations when more than one HTLC satisfiesclaimed_htlc_matches_path(...).💡 Proposed fix
.map(|path| { - let idx = remaining_htlcs - .iter() - .position(|htlc| claimed_htlc_matches_path(origin_node, path, htlc)) - .expect("each path must have a unique matching claimed HTLC"); + let mut matching_indices = remaining_htlcs + .iter() + .enumerate() + .filter_map(|(idx, htlc)| { + claimed_htlc_matches_path(origin_node, path, htlc).then_some(idx) + }); + let idx = matching_indices + .next() + .expect("each path must have a matching claimed HTLC"); + assert!( + matching_indices.next().is_none(), + "each path must have a unique matching claimed HTLC" + ); remaining_htlcs.remove(idx).value_msat })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/functional_test_utils.rs` around lines 1323 - 1328, The code currently uses remaining_htlcs.iter().position(|htlc| claimed_htlc_matches_path(origin_node, path, htlc)) which only grabs the first match and allows duplicates to create order-dependent behavior; change this to explicitly find all matching indices (e.g., iterate with enumerate and collect indices where claimed_htlc_matches_path(origin_node, path, htlc) is true), assert that exactly one match exists, then remove the HTLC at that single index and return its value_msat; reference remaining_htlcs, claimed_htlc_matches_path, origin_node, path to locate and update this logic.
🧹 Nitpick comments (6)
lightning/src/ln/msgs.rs (1)
3638-3638: Add a round-trip test for TLV type65539withSomevalue.Line 3638 wiring looks correct, but current fixtures only exercise the
Nonepath. A targeted encode/decode assertion forSome(...)would guard against regressions.Proposed test addition
+ #[test] + fn encoding_update_add_htlc_with_dummy_hops_skimmed_fee_msat() { + let mut msg = test_update_add_htlc(); + msg.dummy_hops_skimmed_fee_msat = Some(42); + + let encoded = msg.encode(); + let decoded: msgs::UpdateAddHTLC = + LengthReadable::read_from_fixed_length_buffer(&mut &encoded[..]).unwrap(); + + assert_eq!(decoded.dummy_hops_skimmed_fee_msat, Some(42)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` at line 3638, Add a unit test that verifies a round-trip encode/decode for TLV type 65539 when the value is Some(...). Construct a TLV entry using the existing symbol dummy_hops_skimmed_fee_msat with a non-zero Some(u64) payload, call the same TLV serialization and deserialization helpers used by other tests (encode -> decode), and assert the decoded map contains the 65539 entry with the exact Some value you encoded; place the test near the existing TLV fixtures so it exercises the previously-uncovered Some(...) path.lightning/src/ln/async_payments_tests.rs (1)
3353-3356: Make the per-path dummy-hop vectors differ.Both legs passed to
with_per_path_dummy_tlvsare clones of the same value, so a bug that reuses the first path's dummy-hop data for every leg would still pass here. Giving the two legs slightly different base-fee or CLTV settings would turn this into a real check of the new per-path plumbing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/async_payments_tests.rs` around lines 3353 - 3356, The test currently passes identical dummy-tlv vectors for every path, hiding bugs that reuse the first path's data; update the call to ClaimAlongRouteArgs::with_per_path_dummy_tlvs (used in claim_payment_along_route) to supply distinct per-path vectors instead of vec![dummy_tlvs.to_vec(); expected_route.len()]. Construct a Vec of length expected_route.len() where at least one path has a slightly different dummy TLV (e.g., alter base_fee or CLTV in one dummy_tlvs clone) so the two legs differ and the test will fail if the implementation reuses the first path's dummy-hop data.lightning/src/blinded_path/payment.rs (2)
130-138: Call out constraint-side effects in the public docs.This docstring currently reads like dummy hops only tweak relay fees/CLTV, but
DummyTlvsalso changespayment_constraints. Sincenew_with_dummy_hopsis now public, it's worth stating thathtlc_minimum_msat/max_cltv_expirycan make an otherwise plausible path reject payments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/blinded_path/payment.rs` around lines 130 - 138, The public doc for BlindedPaymentPath::new_with_dummy_hops should explicitly call out that providing DummyTlvs not only affects relay fee/CLTV values but also mutates payment_constraints (via the DummyTlvs handling), which can raise htlc_minimum_msat or lower max_cltv_expiry and therefore cause otherwise-valid paths to be rejected; update the new_with_dummy_hops docstring to mention these constraint-side effects and name the specific fields (htlc_minimum_msat and max_cltv_expiry) so callers know to validate constraints when using DummyTlvs.
391-442: Add a direct unit test for the new default dummy-hop contract.These defaults now drive route budgeting and are exercised indirectly by the async-payment suite, but this module still has no local assertion over
DummyTlvs::default()or dummy-hopcompute_payinfoaggregation. A small unit test here would catch accidental default changes much earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/blinded_path/payment.rs` around lines 391 - 442, Add a small unit test that directly asserts the documented defaults and their effect: call DummyTlvs::default() and verify its fields equal DEFAULT_DUMMY_HOP_CLTV_EXPIRY_DELTA, DEFAULT_DUMMY_HOP_FEE_PROPORTIONAL_MILLIONTHS, DEFAULT_DUMMY_HOP_FEE_BASE_MSAT, DEFAULT_DUMMY_HOP_MAX_CLTV_EXPIRY, and DEFAULT_DUMMY_HOP_HTLC_MINIMUM_MSAT, and additionally test payinfo aggregation by using the module’s compute_payinfo behavior (e.g., apply the DummyTlvs payment_relay folding/compute_payinfo across N dummy hops) to assert fees and CLTV deltas aggregate linearly/predictably; place this test in the same file under a #[cfg(test)] mod with clear assertions so accidental changes to DummyTlvs::default() or aggregation logic are caught.lightning/src/ln/blinded_payment_tests.rs (1)
283-296: Consider deriving CLTV delta fromdummy_tlvsfor maintainability.The hardcoded
(21 + 33)duplicates values from thedummy_tlvsconfiguration above. If the test configuration changes, this line could easily be missed.♻️ Suggested improvement
let payment_event = SendEvent::from_event(ev.clone()); -let expected_claimable_cltv = payment_event.msgs[0].cltv_expiry - (21 + 33) as u32; +let dummy_cltv_delta: u32 = dummy_tlvs.iter() + .map(|tlv| tlv.payment_relay.cltv_expiry_delta as u32) + .sum(); +let expected_claimable_cltv = payment_event.msgs[0].cltv_expiry - dummy_cltv_delta;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 283 - 296, The test currently hardcodes the CLTV delta as (21 + 33) when computing expected_claimable_cltv; instead derive that delta from the dummy_tlvs used earlier so changes to the test config aren’t missed. Locate where SendEvent::from_event(ev.clone()) is used and PassAlongPathArgs::new(...).with_dummy_tlvs(&dummy_tlvs).with_payment_claimable_cltv(...), compute the claimable_cltv by summing the appropriate CLTV delta values from the dummy_tlvs structure (the two TLV hop deltas referenced by the path) and use that computed value when calling with_payment_claimable_cltv to replace the hardcoded (21 + 33).lightning/src/ln/channel.rs (1)
12388-12390: Consider a typed wrapper for the skimmed-fee inputs.Two adjacent
Option<u64>parameters are easy to reverse at call sites, and a swap would compile cleanly. A small metadata struct would make these helpers much harder to misuse.♻️ API sketch
+pub struct HtlcFeeMetadata { + dummy_hops_skimmed_fee_msat: Option<u64>, + skimmed_fee_msat: Option<u64>, +} + pub fn queue_add_htlc<F: FeeEstimator, L: Logger>( &mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, - dummy_hops_skimmed_fee_msat: Option<u64>, skimmed_fee_msat: Option<u64>, + fee_metadata: HtlcFeeMetadata, blinding_point: Option<PublicKey>, accountable: bool,Also applies to: 12436-12438, 12785-12787
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channel.rs` around lines 12388 - 12390, The two adjacent Option<u64> parameters dummy_hops_skimmed_fee_msat and skimmed_fee_msat are error-prone; define a small typed wrapper (e.g. SkimmedFees { dummy_hops_msat: Option<u64>, skimmed_msat: Option<u64> } or two newtypes like DummyHopsSkimmedFeeMsat(Option<u64>) and SkimmedFeeMsat(Option<u64>)), update the function signatures that currently accept those parameters (the function in channel.rs with parameters including HTLCSource, onion_routing_packet, dummy_hops_skimmed_fee_msat, skimmed_fee_msat, blinding_point, accountable) and the other affected call sites (also the similar signatures at the other ranges you noted) to take the wrapper instead, and propagate the change through callers, deriving/implementing Copy/Clone/Debug and convenient From/Into helpers to minimize call-site churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 4076-4082: The current summation zips htlcs with args.dummy_tlvs,
making the expected_dummy_hops_skimmed_fee_msat order-dependent; instead map
each PaymentClaimed.htlcs entry to its corresponding path dummy TLVs via
expected_paths (or the same mapping used earlier) and then call
dummy_hops_total_fee_msat(htlc.value_msat, path_dummy_tlvs) for each mapped pair
before summing; update the same logic in the other Event::PaymentClaimed arm so
both branches construct expected_dummy_hops_skimmed_fee_msat by first resolving
htlc→path_dummy_tlvs mapping (using expected_paths/args) and then summing with
dummy_hops_total_fee_msat rather than zipping htlcs directly with
args.dummy_tlvs.
In `@lightning/src/routing/router.rs`:
- Around line 180-185: The current htlc_minimum_msat uses
cmp::max(inbound_htlc_minimum_msat,
dummy_tlvs.payment_constraints.htlc_minimum_msat) which diverges from the
per-dummy-hop folding used in compute_payinfo/amt_to_forward_msat; instead,
replicate the fold used in lightning/src/blinded_path/payment.rs (the loop that
calls amt_to_forward_msat for each dummy hop) to propagate the dummy hops' fees
into an effective required msat before taking the max with
details.inbound_htlc_minimum_msat; update the calculation that constructs
htlc_minimum_msat (near payment_relay/dummy_tlvs) to fold
dummy_tlvs.payment_relay/price/fee fields through amt_to_forward_msat (or the
same helper used by compute_payinfo) and then use cmp::max with
details.inbound_htlc_minimum_msat so the gating matches compute_payinfo.
---
Duplicate comments:
In `@lightning/src/ln/channelmanager.rs`:
- Around line 551-554: Fix the typo in the doc comment for the field
dummy_hops_skimmed_fee_msat: change "preceeding" to "preceding" in the comment
describing the dummy hops before the HTLC so the documentation reads correctly
for the struct/channelmanager.rs field dummy_hops_skimmed_fee_msat.
In `@lightning/src/ln/functional_test_utils.rs`:
- Around line 1323-1328: The code currently uses
remaining_htlcs.iter().position(|htlc| claimed_htlc_matches_path(origin_node,
path, htlc)) which only grabs the first match and allows duplicates to create
order-dependent behavior; change this to explicitly find all matching indices
(e.g., iterate with enumerate and collect indices where
claimed_htlc_matches_path(origin_node, path, htlc) is true), assert that exactly
one match exists, then remove the HTLC at that single index and return its
value_msat; reference remaining_htlcs, claimed_htlc_matches_path, origin_node,
path to locate and update this logic.
---
Nitpick comments:
In `@lightning/src/blinded_path/payment.rs`:
- Around line 130-138: The public doc for
BlindedPaymentPath::new_with_dummy_hops should explicitly call out that
providing DummyTlvs not only affects relay fee/CLTV values but also mutates
payment_constraints (via the DummyTlvs handling), which can raise
htlc_minimum_msat or lower max_cltv_expiry and therefore cause otherwise-valid
paths to be rejected; update the new_with_dummy_hops docstring to mention these
constraint-side effects and name the specific fields (htlc_minimum_msat and
max_cltv_expiry) so callers know to validate constraints when using DummyTlvs.
- Around line 391-442: Add a small unit test that directly asserts the
documented defaults and their effect: call DummyTlvs::default() and verify its
fields equal DEFAULT_DUMMY_HOP_CLTV_EXPIRY_DELTA,
DEFAULT_DUMMY_HOP_FEE_PROPORTIONAL_MILLIONTHS, DEFAULT_DUMMY_HOP_FEE_BASE_MSAT,
DEFAULT_DUMMY_HOP_MAX_CLTV_EXPIRY, and DEFAULT_DUMMY_HOP_HTLC_MINIMUM_MSAT, and
additionally test payinfo aggregation by using the module’s compute_payinfo
behavior (e.g., apply the DummyTlvs payment_relay folding/compute_payinfo across
N dummy hops) to assert fees and CLTV deltas aggregate linearly/predictably;
place this test in the same file under a #[cfg(test)] mod with clear assertions
so accidental changes to DummyTlvs::default() or aggregation logic are caught.
In `@lightning/src/ln/async_payments_tests.rs`:
- Around line 3353-3356: The test currently passes identical dummy-tlv vectors
for every path, hiding bugs that reuse the first path's data; update the call to
ClaimAlongRouteArgs::with_per_path_dummy_tlvs (used in
claim_payment_along_route) to supply distinct per-path vectors instead of
vec![dummy_tlvs.to_vec(); expected_route.len()]. Construct a Vec of length
expected_route.len() where at least one path has a slightly different dummy TLV
(e.g., alter base_fee or CLTV in one dummy_tlvs clone) so the two legs differ
and the test will fail if the implementation reuses the first path's dummy-hop
data.
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 283-296: The test currently hardcodes the CLTV delta as (21 + 33)
when computing expected_claimable_cltv; instead derive that delta from the
dummy_tlvs used earlier so changes to the test config aren’t missed. Locate
where SendEvent::from_event(ev.clone()) is used and
PassAlongPathArgs::new(...).with_dummy_tlvs(&dummy_tlvs).with_payment_claimable_cltv(...),
compute the claimable_cltv by summing the appropriate CLTV delta values from the
dummy_tlvs structure (the two TLV hop deltas referenced by the path) and use
that computed value when calling with_payment_claimable_cltv to replace the
hardcoded (21 + 33).
In `@lightning/src/ln/channel.rs`:
- Around line 12388-12390: The two adjacent Option<u64> parameters
dummy_hops_skimmed_fee_msat and skimmed_fee_msat are error-prone; define a small
typed wrapper (e.g. SkimmedFees { dummy_hops_msat: Option<u64>, skimmed_msat:
Option<u64> } or two newtypes like DummyHopsSkimmedFeeMsat(Option<u64>) and
SkimmedFeeMsat(Option<u64>)), update the function signatures that currently
accept those parameters (the function in channel.rs with parameters including
HTLCSource, onion_routing_packet, dummy_hops_skimmed_fee_msat, skimmed_fee_msat,
blinding_point, accountable) and the other affected call sites (also the similar
signatures at the other ranges you noted) to take the wrapper instead, and
propagate the change through callers, deriving/implementing Copy/Clone/Debug and
convenient From/Into helpers to minimize call-site churn.
In `@lightning/src/ln/msgs.rs`:
- Line 3638: Add a unit test that verifies a round-trip encode/decode for TLV
type 65539 when the value is Some(...). Construct a TLV entry using the existing
symbol dummy_hops_skimmed_fee_msat with a non-zero Some(u64) payload, call the
same TLV serialization and deserialization helpers used by other tests (encode
-> decode), and assert the decoded map contains the 65539 entry with the exact
Some value you encoded; place the test near the existing TLV fixtures so it
exercises the previously-uncovered Some(...) path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7363ffa9-ff5f-49f7-b6be-aaf6d8bfc0bc
📒 Files selected for processing (15)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/functional_tests.rslightning/src/ln/htlc_reserve_unit_tests.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/ln/payment_tests.rslightning/src/routing/router.rs
shaavan
force-pushed
the
dummy-follow-review
branch
from
0badc51 to
8d58839
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
shaavan
force-pushed
the
dummy-follow-review
branch
from
8d58839 to
8a4c7d8
Compare
shaavan
force-pushed
the
dummy-follow-review
branch
from
8a4c7d8 to
f04ce79
Compare
Propagate dummy-hop skimmed fees through HTLC peeling, receive-side persistence, and claim events while keeping the accumulated amount as trusted local state instead of serializing it into update_add_htlc. Dummy-hop fees are derived by the recipient while peeling locally handled padding hops. Persisting them alongside receive tracking and surfacing them on PaymentClaimable and PaymentClaimed lets receivers account for that revenue without changing the delivered amount_msat, while keeping the value off the wire avoids treating peer-provided data as trusted accounting input. Manually decode the legacy update_add_htlc TLV so older peers and serialized state remain readable, and document the local accumulator in the receive path to make the trust boundary explicit. Co-Authored-By: OpenAI Codex <codex@openai.com>
Update the payment-claim test helpers to compute expected forwarded amounts and dummy-hop skimmed fees from path-ordered HTLC data. This covers both single-path and multi-path blinded payments with dummy hops. Once dummy-hop fees are exposed separately, the helpers can no longer treat the payees amount_msat or the events HTLC ordering as a proxy for the amounts forwarded along each route. PaymentClaimed.htlcs are event-ordered, while dummy TLVs are tracked per expected path. Recover the claimed amounts in expected-path order before recomputing per-path dummy-hop fees, and assert the claimable and claimed fee fields against that reconstructed view. Co-Authored-By: OpenAI Codex <codex@openai.com>
Replace the zeroed DummyTlvs defaults with stable relay-like fee, CLTV, and HTLC minimum values, and reuse the same minimum aggregation logic in both blinded payinfo construction and router-created blinded path admission. Dummy hops only provide cover if they resemble plausible forwarding hops. Once those defaults become non-trivial, the router also has to reject candidate blinded paths whose requested amount falls below the minimum implied by the hidden tail; otherwise path admission and advertised payinfo can diverge. Document the default constants in code, share the helper that folds downstream minimums backward through relay fees, and add router coverage for the under-minimum admission case. Co-Authored-By: OpenAI Codex <codex@openai.com>
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`.
shaavan
force-pushed
the
dummy-follow-review
branch
from
f04ce79 to
942e455
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lightning/src/ln/channelmanager.rs (1)
551-554:⚠️ Potential issue | 🟡 MinorTypo in documentation: "preceeding" should be "preceding".
Line 551 contains a misspelling that will appear in generated documentation.
📝 Proposed fix
- /// The amount (in msats) skimmed off by the dummy hops preceeding the HTLC. + /// The amount (in msats) skimmed off by the dummy hops preceding the HTLC.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channelmanager.rs` around lines 551 - 554, Update the doc comment for the struct field dummy_hops_skimmed_fee_msat: fix the misspelling "preceeding" to "preceding" so the generated documentation reads correctly; locate the comment above the dummy_hops_skimmed_fee_msat field in channelmanager.rs and replace the typo while keeping the surrounding text intact.
🧹 Nitpick comments (4)
lightning/src/ln/msgs.rs (1)
7155-7167: Cover a mixed-TLV legacy payload too.This fixture only proves the empty-TLV-stream case. Setting
hold_htlcoraccountablebefore appending65539would better lock down the manual decoder against mixed-stream ordering regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/msgs.rs` around lines 7155 - 7167, The test update_add_htlc_ignores_legacy_dummy_hops_skimmed_fee_tlv currently only verifies an empty TLV stream; modify the fixture so the encoded buffer contains a mixed-TLV legacy payload by setting a TLV-bearing flag on base_msg (e.g., set hold_htlc or accountable on the UpdateAddHTLC message) before appending the legacy TLV bytes (65539) to encoded, then decode and assert equality as before to ensure the manual decoder handles mixed-stream ordering correctly; update references to base_msg, encoded, hold_htlc/accountable, and the legacy TLV id (65539) when making the change.lightning/src/ln/blinded_payment_tests.rs (2)
476-478: Assert the exact failure update path here.
update_fail_htlcs.len() + update_fail_malformed_htlcs.len() == 1is too loose for a regression test: it still passes if this starts surfacing through the wrong wire error. Please pin whichever queue this case is supposed to use and the corresponding failure code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 476 - 478, The test currently asserts only that one failure exists by summing updates.update_fail_htlcs and updates.update_fail_malformed_htlcs which is too permissive; change this to assert the exact queue and failure code expected — e.g., assert updates.update_fail_htlcs.len() == 1 (or updates.update_fail_malformed_htlcs.len() == 1 if the malformed path is expected) and then inspect the single entry returned from get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()) to assert the specific failure code/payload matches the expected wire error (compare the failure_code or failure_packet field on the update entry). Keep the check_added_monitors(&nodes[1], 1) as-is.
320-337: Make this regression independent ofDummyTlvs::default().This test is trying to pin a specific hidden-fee underpayment case, but
DummyTlvs::default()now follows configurable defaults. An unrelated default change would alter the scenario under test without touching this file. Prefer explicit relay/constraint values here, like the preceding success case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/blinded_payment_tests.rs` around lines 320 - 337, The test currently relies on DummyTlvs::default() which makes the scenario brittle; replace the DummyTlvs::default() entries passed into BlindedPaymentPath::new_with_dummy_hops with explicit DummyTlvs instances that set the relay constraint fields (e.g., fee_base_msat/fee_proportional_millionths, cltv_expiry_delta or any relay requirement fields present on DummyTlvs) to the exact values you need for the hidden-fee underpayment case, and use those two explicit instances in the dummy_tlvs array so blinded_path.payinfo.fee_base_msat and .cltv_expiry_delta are deterministically produced by BlindedPaymentPath::new_with_dummy_hops. Ensure the field names you initialize match the DummyTlvs struct in tests so the scenario no longer depends on defaults.lightning/src/ln/channel.rs (1)
12385-12390: Consider a small metadata struct for these outbound fee fields.
dummy_hops_skimmed_fee_msatandskimmed_fee_msatare now adjacentOption<u64>parameters on three call-heavy methods. Grouping them withblinding_pointwould make this path much less brittle to silent argument swaps in future changes.Also applies to: 12432-12437, 12782-12786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightning/src/ln/channel.rs` around lines 12385 - 12390, Introduce a small metadata struct (e.g., OutboundFeeMeta) to hold the adjacent outbound fee fields and related optional blinding: move dummy_hops_skimmed_fee_msat: Option<u64>, skimmed_fee_msat: Option<u64>, and blinding_point: Option<PublicKey> into that struct, then update the signatures and call sites for queue_add_htlc (and the other two methods referenced at the other locations) to accept a single OutboundFeeMeta parameter instead of three Option<u64>/Option<PublicKey> arguments; update all call sites to construct and pass OutboundFeeMeta to avoid brittle positional swaps and keep accountable (bool) as a separate argument only if it must remain distinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 469-471: The while loop that repeatedly calls
needs_pending_htlc_processing() and process_pending_htlc_forwards() can hang if
needs_pending_htlc_processing() never becomes false; replace it with a bounded
loop: iterate a fixed max number of times (e.g., MAX_ITER = 100) calling
nodes[1].node.process_pending_htlc_forwards() while
nodes[1].node.needs_pending_htlc_processing() and break early if it clears, then
add a final assertion that nodes[1].node.needs_pending_htlc_processing() is
false so the test fails fast and clearly if the condition remains true; update
the test around the existing calls to needs_pending_htlc_processing() and
process_pending_htlc_forwards() accordingly.
In `@lightning/src/routing/router.rs`:
- Around line 198-200: When constructing the announced-recipient fallback path
(the branch that returns new_with_dummy_hops(&[]) / a direct dummy-hop path),
apply the same effective minimum HTLC check you use for forwarded candidates:
compute the effective htlc_minimum_msat (as compared against
amount_msats.unwrap_or(u64::MAX)) and if amount is below that minimum, return
None instead of unconditionally returning new_with_dummy_hops(&[]); update the
code around the call to new_with_dummy_hops to reuse the existing amount_msats
and htlc_minimum_msat comparison so fixed-amount blinded paths won’t be created
when amount < effective minimum.
---
Duplicate comments:
In `@lightning/src/ln/channelmanager.rs`:
- Around line 551-554: Update the doc comment for the struct field
dummy_hops_skimmed_fee_msat: fix the misspelling "preceeding" to "preceding" so
the generated documentation reads correctly; locate the comment above the
dummy_hops_skimmed_fee_msat field in channelmanager.rs and replace the typo
while keeping the surrounding text intact.
---
Nitpick comments:
In `@lightning/src/ln/blinded_payment_tests.rs`:
- Around line 476-478: The test currently asserts only that one failure exists
by summing updates.update_fail_htlcs and updates.update_fail_malformed_htlcs
which is too permissive; change this to assert the exact queue and failure code
expected — e.g., assert updates.update_fail_htlcs.len() == 1 (or
updates.update_fail_malformed_htlcs.len() == 1 if the malformed path is
expected) and then inspect the single entry returned from
get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id()) to assert the
specific failure code/payload matches the expected wire error (compare the
failure_code or failure_packet field on the update entry). Keep the
check_added_monitors(&nodes[1], 1) as-is.
- Around line 320-337: The test currently relies on DummyTlvs::default() which
makes the scenario brittle; replace the DummyTlvs::default() entries passed into
BlindedPaymentPath::new_with_dummy_hops with explicit DummyTlvs instances that
set the relay constraint fields (e.g.,
fee_base_msat/fee_proportional_millionths, cltv_expiry_delta or any relay
requirement fields present on DummyTlvs) to the exact values you need for the
hidden-fee underpayment case, and use those two explicit instances in the
dummy_tlvs array so blinded_path.payinfo.fee_base_msat and .cltv_expiry_delta
are deterministically produced by BlindedPaymentPath::new_with_dummy_hops.
Ensure the field names you initialize match the DummyTlvs struct in tests so the
scenario no longer depends on defaults.
In `@lightning/src/ln/channel.rs`:
- Around line 12385-12390: Introduce a small metadata struct (e.g.,
OutboundFeeMeta) to hold the adjacent outbound fee fields and related optional
blinding: move dummy_hops_skimmed_fee_msat: Option<u64>, skimmed_fee_msat:
Option<u64>, and blinding_point: Option<PublicKey> into that struct, then update
the signatures and call sites for queue_add_htlc (and the other two methods
referenced at the other locations) to accept a single OutboundFeeMeta parameter
instead of three Option<u64>/Option<PublicKey> arguments; update all call sites
to construct and pass OutboundFeeMeta to avoid brittle positional swaps and keep
accountable (bool) as a separate argument only if it must remain distinct.
In `@lightning/src/ln/msgs.rs`:
- Around line 7155-7167: The test
update_add_htlc_ignores_legacy_dummy_hops_skimmed_fee_tlv currently only
verifies an empty TLV stream; modify the fixture so the encoded buffer contains
a mixed-TLV legacy payload by setting a TLV-bearing flag on base_msg (e.g., set
hold_htlc or accountable on the UpdateAddHTLC message) before appending the
legacy TLV bytes (65539) to encoded, then decode and assert equality as before
to ensure the manual decoder handles mixed-stream ordering correctly; update
references to base_msg, encoded, hold_htlc/accountable, and the legacy TLV id
(65539) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b732bb5-c4ac-4a58-b09a-e3586a1028fd
📒 Files selected for processing (12)
lightning/src/blinded_path/payment.rslightning/src/events/mod.rslightning/src/ln/async_payments_tests.rslightning/src/ln/blinded_payment_tests.rslightning/src/ln/channel.rslightning/src/ln/channelmanager.rslightning/src/ln/functional_test_utils.rslightning/src/ln/msgs.rslightning/src/ln/offers_tests.rslightning/src/ln/onion_payment.rslightning/src/ln/onion_utils.rslightning/src/routing/router.rs
Comment on lines
+469
to
+471
| while nodes[1].node.needs_pending_htlc_processing() { | ||
| nodes[1].node.process_pending_htlc_forwards(); | ||
| } |
There was a problem hiding this comment.
Bound the pending-HTLC processing loop.
If a regression leaves needs_pending_htlc_processing() stuck high, this test hangs instead of failing fast. A small bounded loop plus a final assertion gives a much clearer failure mode.
Suggested hardening
-while nodes[1].node.needs_pending_htlc_processing() {
- nodes[1].node.process_pending_htlc_forwards();
-}
+for _ in 0..4 {
+ if !nodes[1].node.needs_pending_htlc_processing() {
+ break;
+ }
+ nodes[1].node.process_pending_htlc_forwards();
+}
+assert!(!nodes[1].node.needs_pending_htlc_processing());📝 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.
Suggested change
| while nodes[1].node.needs_pending_htlc_processing() { | |
| nodes[1].node.process_pending_htlc_forwards(); | |
| } | |
| for _ in 0..4 { | |
| if !nodes[1].node.needs_pending_htlc_processing() { | |
| break; | |
| } | |
| nodes[1].node.process_pending_htlc_forwards(); | |
| } | |
| assert!(!nodes[1].node.needs_pending_htlc_processing()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightning/src/ln/blinded_payment_tests.rs` around lines 469 - 471, The while
loop that repeatedly calls needs_pending_htlc_processing() and
process_pending_htlc_forwards() can hang if needs_pending_htlc_processing()
never becomes false; replace it with a bounded loop: iterate a fixed max number
of times (e.g., MAX_ITER = 100) calling
nodes[1].node.process_pending_htlc_forwards() while
nodes[1].node.needs_pending_htlc_processing() and break early if it clears, then
add a final assertion that nodes[1].node.needs_pending_htlc_processing() is
false so the test fails fast and clearly if the condition remains true; update
the test around the existing calls to needs_pending_htlc_processing() and
process_pending_htlc_forwards() accordingly.
Comment on lines
+198
to
+200
| if amount_msats.unwrap_or(u64::MAX) < htlc_minimum_msat { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Apply the fixed-amount minimum guard to the announced-recipient fallback too.
Line 198 only rejects under-minimum amounts while building forwarded candidates. If those candidates get filtered out and the recipient is announced, Lines 231-235 still return a direct dummy-hop path unconditionally. That can publish a fixed-amount blinded path whose effective htlc_minimum_msat is above amount_msats, leading to a later failure or forced overpayment. Reuse the same effective-minimum check before the &[] new_with_dummy_hops call.
Also applies to: 231-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightning/src/routing/router.rs` around lines 198 - 200, When constructing
the announced-recipient fallback path (the branch that returns
new_with_dummy_hops(&[]) / a direct dummy-hop path), apply the same effective
minimum HTLC check you use for forwarded candidates: compute the effective
htlc_minimum_msat (as compared against amount_msats.unwrap_or(u64::MAX)) and if
amount is below that minimum, return None instead of unconditionally returning
new_with_dummy_hops(&[]); update the code around the call to new_with_dummy_hops
to reuse the existing amount_msats and htlc_minimum_msat comparison so
fixed-amount blinded paths won’t be created when amount < effective minimum.
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
1 participant
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.
PR #4501
Summary by CodeRabbit
New Features
Bug Fixes