Review Branch: ReceiveAuthKey verification for Blinded Payment Path #5
Conversation
WalkthroughThreads a ReceiveAuthKey through blinded-payment path construction and decryption, replaces UnauthenticatedReceiveTlvs with ReceiveTlvs, adds payment_tlvs_authenticated to inbound payloads, enforces TLV authentication in HTLC processing, removes nonce/HMAC helpers, updates Router/offers APIs, tests, fuzzers, and ChannelManager deserialization compatibility. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Router
participant BPP as BlindedPaymentPath
participant Hops as blinded_hops
Caller->>Router: create_blinded_payment_paths(recipient, receive_auth_key, first_hops, tlvs, amount, secp)
Router->>BPP: BlindedPaymentPath::new(intermediate_nodes, payee_id, receive_auth_key, payee_tlvs, limits, cltv, secp)
BPP->>Hops: blinded_hops(..., payee_tlvs, session_priv, receive_auth_key)
Hops-->>BPP: hops
BPP-->>Router: Vec<BlindedPaymentPath>
Router-->>Caller: Result<Vec<...>, ()>
note right of Router: ReceiveAuthKey is threaded into construction and decryption
sequenceDiagram
autonumber
actor Peer
participant Node
participant Decoder as OnionDecoder
participant Proc as create_recv_pending_htlc_info
Peer->>Node: update_add_htlc(onion)
Node->>Decoder: decode_incoming_update_add_htlc_onion(..., receive_auth_key)
Decoder-->>Node: hop_data { payment_tlvs_authenticated }
Node->>Proc: create_recv_pending_htlc_info(..., hop_data, logger)
alt payment_tlvs_authenticated == true
Proc-->>Node: PendingHTLCInfo (accept)
else
Proc-->>Node: Err(UnauthenticatedPayload) (reject)
end
note right of Proc: Authentication enforced before further validation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightning/src/offers/flow.rs (1)
812-867: Update remaining call sites to include the router argument
• lightning/src/ln/async_payments_tests.rs:331, 698, 1678 – pass&router(e.g.,&recipient.routeror appropriate&R)
• lightning/src/ln/channelmanager.rs:5432 – updatetest_check_refresh_async_receive_offer_cache(false)to include peers, channels, router
• lightning/src/onion_message/messenger.rs:2261 and lightning/src/ln/channelmanager.rs:14928 – pass router intohandle_offer_paths
• lightning/src/offers/flow.rs:1410 and 1575 – include router increate_static_invoice_for_servercalls
🧹 Nitpick comments (8)
lightning/src/ln/onion_utils.rs (2)
1683-1685: New variant name/doc nit and wire-mapping follow-up
- Wording: “Received payload with unauthenticated payload.” → “Received unauthenticated payload.”
- If you want remote failure codes (BADONION|PERM|25) to map back to this variant, also include it in impl_from_u16_for_htlc_reason; otherwise code 25 will become UnknownFailureCode.
Suggested doc fix:
- /// Received payload with unauthenticated payload. + /// Received unauthenticated payload. UnauthenticatedPayload,If desired, add it to the From mapping (see separate comment with diff).
1789-1814: Map code 25 to UnauthenticatedPayload in FromCurrently, From won’t return UnauthenticatedPayload; remote BADONION|PERM|25 will be UnknownFailureCode. If that’s unintended, add the variant here.
impl_from_u16_for_htlc_reason!( LocalHTLCFailureReason, [ TemporaryNodeFailure, PermanentNodeFailure, RequiredNodeFeature, InvalidOnionVersion, InvalidOnionHMAC, InvalidOnionKey, TemporaryChannelFailure, PermanentChannelFailure, RequiredChannelFeature, UnknownNextPeer, AmountBelowMinimum, FeeInsufficient, IncorrectCLTVExpiry, CLTVExpiryTooSoon, IncorrectPaymentDetails, FinalIncorrectCLTVExpiry, FinalIncorrectHTLCAmount, ChannelDisabled, CLTVExpiryTooFar, InvalidOnionPayload, MPPTimeout, - InvalidOnionBlinding, + InvalidOnionBlinding, + UnauthenticatedPayload, ] );lightning/src/ln/msgs.rs (1)
3594-3594: Fetch receive_auth_key lazily (only for blinded payloads)Micro-optimization: call get_receive_auth_key() only when decrypting encrypted TLVs.
Apply this refactor in both readers:
- let receive_auth_key = node_signer.get_receive_auth_key(); @@ - if let Some(blinding_point) = intro_node_blinding_point.or(update_add_blinding_point) { + if let Some(blinding_point) = intro_node_blinding_point.or(update_add_blinding_point) { + let receive_auth_key = node_signer.get_receive_auth_key(); ... - match ChaChaDualPolyReadAdapter::read(&mut reader, (rho, receive_auth_key.0))? { + match ChaChaDualPolyReadAdapter::read(&mut reader, (rho, receive_auth_key.0))? {Repeat the same pattern in InboundTrampolinePayload::read.
Also applies to: 3759-3759
lightning/src/ln/blinded_payment_tests.rs (2)
2014-2027: Correct use of ReceiveAuthKey in blinded_hops constructionUsing Some(receive_auth_key) for the receive hop is appropriate; the failure branch uses None to simulate auth failure. Consider adding a dedicated negative test case explicitly using a mismatched ReceiveAuthKey to assert failure deterministically.
1369-1374: Rewrite or remove the ignored authentication testThe HMAC/Nonce-era test is now stale. Either delete it or refactor to verify ReceiveAuthKey-based auth failure by constructing the second path with a mismatched key.
Suggested minimal refactor:
@@ -#[ignore] fn fails_receive_tlvs_authentication() { @@ - let receive_auth_key = chanmon_cfgs[1].keys_manager.get_receive_auth_key(); + let receive_auth_key = chanmon_cfgs[1].keys_manager.get_receive_auth_key(); @@ // Test authentication works normally. @@ // Swap in a different nonce to force authentication to fail. let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None); - let payee_tlvs = ReceiveTlvs { ... }; - // payee_tlvs.authentication.1 = Nonce([0u8; 16]); + let payee_tlvs = ReceiveTlvs { + payment_secret, + payment_constraints: PaymentConstraints { + max_cltv_expiry: u32::max_value(), + htlc_minimum_msat: chan_upd.htlc_minimum_msat, + }, + payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}), + }; + // Use a mismatched ReceiveAuthKey to force auth failure: + let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); @@ - let blinded_path = BlindedPaymentPath::new( - &[], nodes[1].node.get_our_node_id(), receive_auth_key, + let blinded_path = BlindedPaymentPath::new( + &[], nodes[1].node.get_our_node_id(), mismatched_receive_auth_key, payee_tlvs, u64::MAX, TEST_FINAL_CLTV as u16, &chanmon_cfgs[1].keys_manager, &secp_ctx ).unwrap();This preserves the test intent under the new auth mechanism.
Also applies to: 1384-1392, 1395-1398, 1411-1428
lightning/src/offers/flow.rs (1)
312-317: Do not cache receive_auth_key in OffersMessageFlowOffersMessageFlow stores the receive_auth_key passed at construction, so any rotations on the underlying NodeSigner won’t be picked up. Consider:
- invoking node_signer.get_receive_auth_key() on each call to create_blinded_paths instead of using the cached self.receive_auth_key, or
- adding a setter on OffersMessageFlow to refresh the cached key after rotation.
lightning/src/ln/onion_payment.rs (1)
248-266: Auth gate + logger threading added to create_recv_pending_htlc_infoGood addition of a generic logger and centralized check_authentication closure. Consider raising the log level from trace to info to surface dropped unauthenticated HTLCs.
- log_trace!(logger, "Received an unauthenticated receive payment message"); + log_info!(logger, "Received an unauthenticated receive payment message");lightning/src/routing/router.rs (1)
283-291: Update Router trait docs to mention the new receive_auth_key requirementDocs still reference only tlvs; add a brief note about receive_auth_key being required for authenticated blinded paths.
Apply this doc tweak:
- /// Creates [`BlindedPaymentPath`]s for payment to the `recipient` node. The channels in `first_hops` - /// are assumed to be with the `recipient`'s peers. The payment secret and any constraints are - /// given in `tlvs`. + /// Creates [`BlindedPaymentPath`]s for payment to the `recipient` node. The channels in `first_hops` + /// are assumed to be with the `recipient`'s peers. The payment secret and any constraints are + /// given in `tlvs`. The `receive_auth_key` is required to authenticate the blinded payment paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(18 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(13 hunks)lightning/src/ln/channelmanager.rs(9 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(13 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(4 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
lightning/src/util/test_utils.rs (1)
lightning/src/routing/router.rs (14)
first_hops(156-205)first_hops(7884-7884)first_hops(7908-7908)first_hops(8311-8311)first_hops(8321-8321)first_hops(8380-8380)first_hops(8540-8540)first_hops(8620-8620)first_hops(8685-8685)first_hops(8739-8739)first_hops(8859-8859)first_hops(8875-8875)first_hops(8887-8887)first_hops(9031-9031)
lightning/src/offers/flow.rs (4)
lightning/src/routing/router.rs (4)
create_blinded_payment_paths(129-220)create_blinded_payment_paths(245-253)create_blinded_payment_paths(286-290)a(3617-3617)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)lightning/src/ln/channelmanager.rs (4)
usable_channels(5743-5743)test_create_blinded_payment_paths(12930-12942)check_refresh_async_receive_offer_cache(5409-5428)timer_tick_occurred(7808-8056)lightning/src/ln/async_payments_tests.rs (1)
create_static_invoice_builder(272-306)
lightning/src/ln/max_payment_path_len_tests.rs (2)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/ln/onion_payment.rs (2)
lightning/src/ln/onion_utils.rs (7)
shared_secret(2279-2290)new(783-783)new(2164-2164)new(2168-2170)new(2174-2176)new(2731-2733)new(3949-3951)lightning/src/ln/channelmanager.rs (2)
new(3847-3925)new(16114-16135)
lightning/src/ln/blinded_payment_tests.rs (4)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/offers/flow.rs (1)
get_receive_auth_key(192-194)lightning/src/util/test_utils.rs (2)
get_receive_auth_key(1730-1732)get_receive_auth_key(1822-1824)lightning/src/sign/mod.rs (3)
get_receive_auth_key(880-880)get_receive_auth_key(2220-2222)get_receive_auth_key(2393-2395)
lightning/src/ln/channelmanager.rs (1)
lightning/src/offers/flow.rs (1)
new(118-149)
fuzz/src/invoice_request_deser.rs (1)
lightning/src/offers/static_invoice.rs (3)
invoice(801-827)sign(340-353)sign(385-387)
lightning/src/blinded_path/payment.rs (3)
lightning/src/ln/channelmanager.rs (13)
new(3847-3925)new(16114-16135)read(612-615)read(662-665)read(1250-1264)read(1497-1499)read(15355-15396)read(15447-15492)read(15497-15544)read(15626-15659)read(15949-15965)read(16164-16170)node(19056-19056)lightning/src/ln/msgs.rs (8)
new(2509-2511)new(2544-2546)read(1147-1171)read(1175-1181)read(3344-3355)read(3404-3408)read(3591-3749)read(3756-3898)lightning/src/blinded_path/message.rs (1)
new(68-85)
lightning/src/ln/msgs.rs (1)
lightning/src/routing/router.rs (1)
blinded_path(1761-1768)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: build (self-hosted, stable)
🔇 Additional comments (55)
lightning/src/offers/signer.rs (1)
43-51: Reserved HMAC inputs note looks goodClear reservation of [5], [7], [8] avoids collisions after ReceiveAuthKey adoption. No code action required.
lightning/src/ln/onion_utils.rs (3)
2049-2050: Debug assertion for UnauthenticatedPayloadAsserting empty data for this reason is correct and consistent with other BADONION cases.
1886-1888: Confirm no collisions for new serialization discriminants — verify that 42 and 43 aren’t already used in theser_failure_reasons!macro.
1726-1727: Confirm no collision with BOLT #4 codes
BADONION | PERM | 25 is not defined in the BOLT #4 failure_code table (code 25 is unassigned/reserved), so using it for UnauthenticatedPayload does not conflict with any existing codes.lightning/src/ln/msgs.rs (4)
35-35: ChaChaDualPolyReadAdapter + ReceiveTlvs import: LGTMAdapter swap and imports align with the new authenticated TLV flow.
Also applies to: 59-59
3664-3716: Forward-hop AAD gating and Receive authentication plumbed: LGTM
- Fails if used_aad on forward hops (correct).
- Surfaces used_aad on receive as payment_tlvs_authenticated (good).
3812-3863: Trampoline blinded path: AAD gating + flag propagation: LGTMMirrors the payment path behavior; looks correct.
2342-2343: Add documentation for payment_tlvs_authenticated
Add a brief doc clarifying that this flag indicates authenticated receive TLVs (via ReceiveAuthKey AAD) and is internal-only:pub struct InboundOnionBlindedReceivePayload { @@ - pub payment_tlvs_authenticated: bool, + /// True if the decrypted receive TLVs were authenticated using our ReceiveAuthKey (AAD). + /// Internal-only; not a wire-serialized field. + pub payment_tlvs_authenticated: bool,fuzz/src/chanmon_consistency.rs (1)
130-137: LGTM! Signature updated to match Router trait changes.The
create_blinded_payment_pathsmethod signature correctly adds the_receive_auth_key: ReceiveAuthKeyparameter in the expected position, aligning with the broader authentication refactor across the codebase.lightning/src/ln/async_payments_tests.rs (3)
272-306: LGTM! Simplified authentication flow.The removal of the
entropyparameter and its derivation fromcreate_static_invoice_builderis consistent with the broader refactor where authentication (viaReceiveAuthKey) is now managed internally by the node's keys manager rather than being explicitly threaded through the API.
1861-1861: LGTM! Simplified TLV access pattern.The change from
tlvs.tlvs.payment_constraints.max_cltv_expirytotlvs.payment_constraints.max_cltv_expirycorrectly reflects the structural change fromUnauthenticatedReceiveTlvstoReceiveTlvs, removing unnecessary nesting.
1570-1585: LGTM! Test API simplified.The removal of the
keys_managerparameter fromtest_create_blinded_payment_pathscalls is consistent with the refactor whereReceiveAuthKeyis now obtained internally rather than being explicitly passed. This simplifies the test helper API.lightning/src/ln/max_payment_path_len_tests.rs (2)
13-16: LGTM! Import updated to use new type.The import change from
UnauthenticatedReceiveTlvstoReceiveTlvscorrectly reflects the authentication refactor across the codebase.
215-234: LGTM! Blinded path construction updated correctly.The changes properly integrate the new authentication flow:
payee_tlvsnow usesReceiveTlvsdirectly (line 215)receive_auth_keyis obtained from the keys manager (line 223)BlindedPaymentPath::newreceives thereceive_auth_keyparameter (line 228)This aligns with the broader authentication refactor replacing nonce-based authentication with
ReceiveAuthKey.fuzz/src/refund_deser.rs (2)
13-21: LGTM! Imports updated for authentication refactor.The changes correctly update imports to:
- Replace
UnauthenticatedReceiveTlvswithReceiveTlvs(line 15)- Add
ReceiveAuthKeyimport (line 21)These align with the broader authentication refactor across the codebase.
67-109: LGTM! Fuzzing test updated for new authentication flow.The changes properly integrate the
ReceiveAuthKeyauthentication:
- Hardcoded
receive_auth_keyis appropriate for fuzzing (line 71)payee_tlvsusesReceiveTlvsdirectly (line 73)- Simplified access to
payment_constraints(line 90)BlindedPaymentPath::newcorrectly receivesreceive_auth_keyparameter (line 102)The hardcoded key value
[41; 32]is acceptable for fuzzing purposes as it provides deterministic behavior for test reproducibility.lightning/src/ln/blinded_payment_tests.rs (4)
78-96: Good migration to ReceiveTlvs + ReceiveAuthKey in path builderConstructing ReceiveTlvs and passing receive_auth_key into BlindedPaymentPath::new is correct and matches the new API. No issues.
163-178: Consistent ReceiveAuthKey threading across testsAll call sites build ReceiveTlvs and pass receive_auth_key to BlindedPaymentPath::new. This keeps tests aligned with the new verification. Looks good.
Also applies to: 216-229, 1325-1340, 1394-1398, 1425-1428
1629-1630: Local TestEcdhSigner::get_receive_auth_key implementations are fineReturning a fixed ReceiveAuthKey for tests/vectors is acceptable. No action needed.
Also applies to: 1942-1942
1-2490: No leftover UnauthenticatedReceiveTlvs references found
Ripgrep search returned zero matches—no action needed.fuzz/src/invoice_request_deser.rs (1)
86-116: Fuzzer updated correctly for new auth and TLVs
- Deterministic ReceiveAuthKey is fine for fuzzing.
- Using ReceiveTlvs and passing receive_auth_key to BlindedPaymentPath::new is correct.
Also applies to: 134-144
lightning/src/util/test_utils.rs (1)
290-307: Router test shim updated to new signatureAdding receive_auth_key and forwarding it maintains parity with DefaultRouter. Looks good.
Also applies to: 297-304
lightning/src/offers/flow.rs (2)
91-96: ReceiveAuthKey ownership in OffersMessageFlowStoring receive_auth_key and exposing a getter is fine. Construction and use look consistent.
Also applies to: 112-149, 192-195
315-317: Blinded path creation now uses ReceiveAuthKey and ReceiveTlvscreate_blinded_paths delegates receive_key correctly; create_blinded_payment_paths builds ReceiveTlvs and passes receive_auth_key into Router::create_blinded_payment_paths. CLTV math uses saturating_add; looks safe.
Also applies to: 320-356
fuzz/src/full_stack.rs (1)
159-162: Router signature update looks correctAdded ReceiveAuthKey parameter and kept body unreachable! as before. Unused args are properly prefixed with underscores.
lightning/src/ln/onion_payment.rs (4)
283-301: Enforce authentication for BlindedReceiveEarly reject on unauthenticated payload before constraint checks is correct and minimizes work/leakage.
316-332: Enforce authentication for TrampolineBlindedReceiveSame proper early reject as above; matches intent for blinded paths.
460-461: Pass logger by reference to decoderSwitch to logger.deref() aligns with generic L: Deref usage; no behavior change.
502-504: Propagate logger into receive-path constructionUpdated call and parameter order look correct; passes logger.deref() per new signature.
lightning/src/routing/router.rs (3)
211-214: Consistent fallback call: same note on ReceiveAuthKey ownershipThe same ownership consideration for receive_auth_key applies here when constructing the single-hop fallback path.
See the earlier comment for verification steps.
199-205: No change needed: ReceiveAuthKey is Copy
ReceiveAuthKey derivesCloneandCopy, so passing it by value across multiple iterations is safe and intended.
129-135: Signature change applied to allcreate_blinded_payment_pathsusages
AllRouterimplementations (including FixedRouter, DefaultRouter, and fuzz routers), test helpers intest_utils.rs, and call sites inoffers/flow.rshave been updated to includeReceiveAuthKeyand the reordered parameters; no outdated usages remain.lightning/src/ln/channelmanager.rs (9)
38-38: LGTM: Import consolidation.The consolidation of
blinded_path::paymentimports removes duplication and improves code organization.
4884-4884: Logger parameter threaded for enhanced receive path.The addition of the logger parameter to
create_recv_pending_htlc_infoenables logging during HTLC receive processing, which is necessary for the ReceiveAuthKey verification flow introduced in this PR.
6995-6995: Consistent logger threading for phantom receives.The logger parameter addition maintains consistency with the receive path changes, ensuring phantom payment processing also benefits from the enhanced logging and authentication verification.
14815-14815: Node signer parameter enables authentication signing.The addition of
&self.node_signerparameter threads through signing capability needed for ReceiveAuthKey-based authentication in the invoice response flow.
17879-17879: Test imports consolidated.The import consolidation maintains the necessary test utilities while simplifying the import statement.
18704-18704: Explicit test logger instantiation.The explicit
TestLogger::new()instantiation improves test code clarity and aligns with the consolidated imports.
18726-18726: Test updated for new logger parameter.The test correctly threads the logger parameter to match the updated function signature, maintaining test coverage of error handling.
18749-18749: Test success case updated consistently.The success path test correctly includes the logger parameter, ensuring comprehensive test coverage of the updated function signature.
18774-18774: CLTV test updated with inline logger.The inline
TestLogger::new()usage is appropriate for this focused test case and maintains consistency with the updated function signature.lightning/src/blinded_path/payment.rs (14)
17-17: LGTM! Necessary imports for authentication.The imports of
ChaChaDualPolyReadAdapterandReceiveAuthKeyare required for the new authentication mechanism being introduced.Also applies to: 27-27
92-110: LGTM! Signature updated to include authentication key.The
receive_auth_keyparameter is correctly added and passed through toSelf::new.
119-155: LGTM! Method signature and TLV access updated.The
receive_auth_keyparameter is correctly added and threaded through toblinded_hops. The direct access topayee_tlvs(line 136) aligns with the simplifiedReceiveTlvsstructure.
328-335: LGTM! SimplifiedReceiveTlvsstructure.The flattened structure removes the nested authentication wrapper, which is now handled at the encryption/decryption level via
ReceiveAuthKey. This simplification improves code clarity.
535-573: LGTM! Deserialization correctly enforces required fields.The
Readableimplementation forBlindedPaymentTlvscorrectly requires all three fields ofReceiveTlvsto be present (lines 567-569).
575-609: LGTM! Consistent deserialization for Trampoline TLVs.The
Readableimplementation forBlindedTrampolineTlvsis consistent withBlindedPaymentTlvsand correctly enforces required fields.
616-634: LGTM! Authentication key correctly associated with payee.The
blinded_hopsfunction now associates theReceiveAuthKeywith the payee node (line 623), while intermediate nodes receiveNone. This correctly implements the authentication model where only the recipient verifies the blinded path.
691-743: LGTM! Updated to use flattenedReceiveTlvsstructure.The
compute_payinfofunction signature and field access (line 729) are updated to work with the flattenedReceiveTlvsstructure.
815-815: LGTM! Test imports updated.The test imports correctly reflect the type changes.
865-869: LGTM! Test updated for new structure.The test correctly constructs
ReceiveTlvswith all required fields.
883-887: LGTM! Test updated consistently.The test update is consistent with the new
ReceiveTlvsstructure.
940-944: LGTM! All test updates consistent.All remaining tests are updated consistently to use the flattened
ReceiveTlvsstructure with all required fields.Also applies to: 999-1003, 1068-1072
509-523: Ensure backward compatibility forReceiveTlvsTLV changes.No version‐guarding or migration logic was found around
ReceiveTlvs/ReceiveAuthKey. Please manually verify that:
- Pre-v0.2 LDK nodes gracefully reject or ignore the new TLV fields (12, 65536, 65537) without breaking.
- Authentication checks still prevent bypassing on older versions.
218-236: Ignoreused_aadintentionally:ChaChaDualPolyReadAdapter::readalready enforces authenticity via the Poly1305 tag check; theused_aadflag merely indicates which MAC variant matched and can safely be ignored here.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
shaavan
force-pushed
the
aad-payment-review
branch
from
13c4041 to
694476f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lightning/src/ln/onion_payment.rs (2)
286-296: Blinded receive constraints compare against upstream CLTV; should use onion’s final cltv (cltv_expiry_height).For final-hop blinded receipts,
PaymentConstraints.max_cltv_expiryshould be validated against the onion’s declared final CLTV (cltv_expiry_height), not the HTLC’smsg.cltv_expiry. Using the upstream value can spuriously fail valid payments or accept invalid ones when they differ.Apply this fix:
- check_blinded_payment_constraints( - sender_intended_htlc_amt_msat, cltv_expiry, &payment_constraints - ) + check_blinded_payment_constraints( + sender_intended_htlc_amt_msat, cltv_expiry_height, &payment_constraints + )
318-327: Same issue in trampoline-blinded receive: compare constraints to cltv_expiry_height.Validate
PaymentConstraintsagainst the onion’s final CLTV height, notmsg.cltv_expiry.- check_blinded_payment_constraints( - sender_intended_htlc_amt_msat, cltv_expiry, &payment_constraints, - ) + check_blinded_payment_constraints( + sender_intended_htlc_amt_msat, cltv_expiry_height, &payment_constraints, + )lightning/src/ln/blinded_payment_tests.rs (1)
1455-1458: Align expected failure with UnauthenticatedPayload.Runtime now returns
LocalHTLCFailureReason::UnauthenticatedPayloadfor auth failures. Update the assertion:- PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[]), + PaymentFailedConditions::new().expected_htlc_error_data(LocalHTLCFailureReason::UnauthenticatedPayload, &[]),lightning/src/offers/flow.rs (1)
91-94: Avoid stale ReceiveAuthKey: handle key rotationOffersMessageFlow stores ReceiveAuthKey by value. If the NodeSigner rotates this key, newly decoded inbound onions will use the fresh key, while paths built here will still embed/expect the old key, breaking authentication.
Mitigations:
- Fetch the key at use-sites instead of storing (plumb NodeSigner or a getter into the flow), or
- Store receive_auth_key behind a Mutex/RwLock and expose a setter to update on rotation, and ensure all path-creation calls read the latest value.
At minimum, document that the key must be stable for the lifetime of generated paths.
♻️ Duplicate comments (1)
lightning/src/routing/router.rs (1)
183-185: Still need saturating_add for max_cltv_expiry.Switching to
checked_addjust drops the hop when the addition overflows, which is the same overflow hazard previously flagged. Per the earlier review you should callsaturating_add(after casting the delta if needed) so the route stays valid without wrapping.
🧹 Nitpick comments (5)
lightning/src/ln/channelmanager.rs (1)
18774-18774: Consider using a named logger variable for consistency.While the inline
&TestLogger::new()works correctly, other tests in this file create a namedloggervariable (see lines 18704, 18726, 18749). For consistency and slightly better readability, consider following the same pattern.Apply this diff to align with the pattern used in other tests:
+ let logger = TestLogger::new(); }, [0; 32], PaymentHash([0; 32]), 100, TEST_FINAL_CLTV + 1, None, true, None, current_height, &TestLogger::new()); + }, [0; 32], PaymentHash([0; 32]), 100, TEST_FINAL_CLTV + 1, None, true, None, current_height, &logger);lightning/src/ln/onion_payment.rs (1)
256-266: Log level for unauthenticated payloads.Consider
warninstead ofinfofor unauthenticated receive payloads to aid ops triage.- log_info!(logger, "Received an unauthenticated receive payment message"); + log_warn!(logger, "Received an unauthenticated receive payment message");lightning/src/ln/blinded_payment_tests.rs (1)
1369-1372: Remove stale comment; the test now targets ReceiveAuthKey-based auth.These lines suggest redundancy post-(HMAC, Nonce) removal. The test has been adapted to ReceiveAuthKey. Drop the comment to avoid confusion.
lightning/src/blinded_path/payment.rs (2)
612-614: Nit: fix comment typo“payment bilnded path's” → “payment blinded path’s”.
Apply:
-/// Represents the padding round off size (in bytes) that -/// is used to pad payment bilnded path's [`BlindedHop`] +/// Represents the padding round-off size (in bytes) used to pad payment blinded path's [`BlindedHop`].
101-101: Style: prefer u64::MAX over u64::max_value()Idiomatic Rust uses u64::MAX. Optional consistency cleanup.
- let htlc_maximum_msat = u64::max_value(); + let htlc_maximum_msat = u64::MAX;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(18 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(13 hunks)lightning/src/ln/channelmanager.rs(9 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(12 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(5 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lightning/src/offers/signer.rs
- lightning/src/ln/msgs.rs
- lightning/src/ln/async_payments_tests.rs
- lightning/src/ln/onion_utils.rs
- fuzz/src/invoice_request_deser.rs
🧰 Additional context used
🧬 Code graph analysis (8)
lightning/src/ln/channelmanager.rs (1)
lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/util/test_utils.rs (1)
lightning/src/routing/router.rs (14)
first_hops(156-207)first_hops(7886-7886)first_hops(7910-7910)first_hops(8313-8313)first_hops(8323-8323)first_hops(8382-8382)first_hops(8542-8542)first_hops(8622-8622)first_hops(8687-8687)first_hops(8741-8741)first_hops(8861-8861)first_hops(8877-8877)first_hops(8889-8889)first_hops(9033-9033)
lightning/src/ln/onion_payment.rs (1)
lightning/src/ln/channel.rs (6)
new(1091-1093)new(1205-1214)new(12825-12876)new(13190-13241)None(11074-11074)None(14118-14118)
lightning/src/routing/router.rs (4)
lightning/src/offers/flow.rs (1)
create_blinded_payment_paths(322-356)fuzz/src/chanmon_consistency.rs (1)
create_blinded_payment_paths(130-136)fuzz/src/full_stack.rs (1)
create_blinded_payment_paths(158-164)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)
lightning/src/ln/blinded_payment_tests.rs (4)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/offers/flow.rs (1)
get_receive_auth_key(192-194)lightning/src/util/test_utils.rs (2)
get_receive_auth_key(1730-1732)get_receive_auth_key(1822-1824)lightning/src/sign/mod.rs (3)
get_receive_auth_key(880-880)get_receive_auth_key(2220-2222)get_receive_auth_key(2393-2395)
lightning/src/blinded_path/payment.rs (3)
lightning/src/ln/channelmanager.rs (13)
new(3847-3925)new(16114-16135)read(612-615)read(662-665)read(1250-1264)read(1497-1499)read(15355-15396)read(15447-15492)read(15497-15544)read(15626-15659)read(15949-15965)read(16164-16170)node(19056-19056)lightning/src/ln/msgs.rs (8)
new(2509-2511)new(2544-2546)read(1147-1171)read(1175-1181)read(3344-3355)read(3404-3408)read(3591-3749)read(3756-3898)lightning/src/blinded_path/message.rs (1)
new(68-85)
lightning/src/ln/max_payment_path_len_tests.rs (2)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/offers/flow.rs (4)
lightning/src/routing/router.rs (4)
create_blinded_payment_paths(129-222)create_blinded_payment_paths(247-255)create_blinded_payment_paths(288-292)a(3619-3619)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)lightning/src/ln/channelmanager.rs (4)
usable_channels(5743-5743)test_create_blinded_payment_paths(12930-12942)check_refresh_async_receive_offer_cache(5409-5428)timer_tick_occurred(7808-8056)lightning/src/ln/async_payments_tests.rs (1)
create_static_invoice_builder(272-306)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
- GitHub Check: linting
- GitHub Check: benchmark
- GitHub Check: check_release
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
🔇 Additional comments (13)
lightning/src/ln/channelmanager.rs (2)
4881-4884: LGTM!The logger parameter addition follows standard Rust patterns for passing logger references.
14814-14816: Approve router parameter addition
TheRoutertrait already provides the necessary methods (including blinded‐path auth viacreate_blinded_payment_paths) andfind_routefor invoice responses, so passing&self.routerhere is correct.lightning/src/ln/onion_payment.rs (1)
460-504: Logger deref plumbed correctly.Passing
logger.deref()intodecode_incoming_update_add_htlc_onionandcreate_recv_pending_htlc_infoaligns with the new generic constraints.lightning/src/ln/blinded_payment_tests.rs (1)
78-96: ReceiveTlvs and ReceiveAuthKey adoption looks correct.Construction of
ReceiveTlvs, retrieval ofreceive_auth_key, and passing it intoBlindedPaymentPath::neware consistent with the new API.Also applies to: 161-178, 216-230, 333-341
fuzz/src/chanmon_consistency.rs (1)
130-134: Router API updated coherently.The additional
ReceiveAuthKeyandReceiveTlvsincreate_blinded_payment_pathsmatch the new flow. No issues.lightning/src/ln/max_payment_path_len_tests.rs (1)
213-234: Blinded path setup updated correctly.Using
ReceiveTlvsand injectingreceive_auth_keyintoBlindedPaymentPath::newis correct.fuzz/src/refund_deser.rs (1)
71-81: LGTM on ReceiveAuthKey/ReceiveTlvs integration.Fuzz harness creates
ReceiveTlvsand threadsreceive_auth_keyintoBlindedPaymentPath::newcorrectly.Also applies to: 100-109
lightning/src/util/test_utils.rs (1)
290-309: LGTM! Signature update aligns with Router trait changes.The
create_blinded_payment_pathsmethod correctly addsreceive_auth_keyas the second parameter and properly forwards it to the underlying router in both execution paths (with and without expected paths). The parameter ordering matches the pattern established in the codebase.fuzz/src/full_stack.rs (1)
158-164: LGTM! Fuzz router stub correctly updated.The signature change adds
_receive_auth_keyas the second parameter to match the Router trait's updated interface. The_prefix correctly indicates the parameter is unused in this stub implementation.lightning/src/blinded_path/payment.rs (2)
618-624: Good: ReceiveAuthKey applied only to payee hopPassing Some(local_node_receive_key) only for the final (receive) hop aligns with authenticating the receiver context without burdening forward hops.
227-234: Enforced AAD for payee hop decryption Verifiedconstruct_blinded_hopspasses the payee’sReceiveAuthKeytoencrypt_payloadas AAD andChaChaDualPolyReadAdapter::readuses it on decryption, ensuring authentication.lightning/src/offers/flow.rs (2)
320-356: Router plumbing for ReceiveAuthKey looks correctPassing receive_auth_key through to Router::create_blinded_payment_paths and building ReceiveTlvs in-flow is coherent with the new auth model.
Please re-run impacted tests/fuzzers touching blinded payment path creation to ensure no regressions in path count/eligibility and inbound auth.
1399-1400: Signatures updated to thread Router where neededcheck_refresh_static_invoices and create_static_invoice_for_server correctly accept a router and reuse it downstream. No issues found.
Also applies to: 1615-1617
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lightning/src/offers/flow.rs (3)
812-866: Breaking API change: Additional parameters required.The addition of
router: &Randusable_channels: Vec<ChannelDetails>parameters is a breaking change. Callers will need to update their invocations to provide routing information and channel details, which are necessary for creating authenticated blinded payment paths.This change aligns with the PR's authentication objectives and is correctly implemented.
941-1017: Breaking API change: Additional parameters required.Similar to
create_static_invoice_builder, this method now requiresrouter: &Randusable_channels: Vec<ChannelDetails>parameters. Callers must be updated accordingly.The implementation correctly uses these parameters to create authenticated blinded payment paths.
1323-1345: Inconsistent router parameter passing pattern.This method takes
router: R(by value), while other public methods likecreate_static_invoice_builder(line 812) andcreate_response_for_invoice_request(line 941) takerouter: &R(by reference). Taking the router by value prevents callers from reusing it, which could be problematic.Consider changing the signature to accept
router: &Rfor consistency:pub fn check_refresh_async_receive_offer_cache<R: Deref>( - &self, peers: Vec<MessageForwardNode>, usable_channels: Vec<ChannelDetails>, router: R, + &self, peers: Vec<MessageForwardNode>, usable_channels: Vec<ChannelDetails>, router: &R, timer_tick_occurred: bool, ) -> Result<(), ()>Then update line 1341 to pass
&routerinstead ofrouter, and adjust the private methodcheck_refresh_static_invoicessignature similarly.
🧹 Nitpick comments (5)
lightning/src/ln/channelmanager.rs (1)
18774-18774: Consider extracting TestLogger for consistency.The inline
TestLogger::new()creation is less readable than the variable approach used at line 18704. For consistency and readability, consider extracting it to a variable.Apply this diff:
+ let logger = TestLogger::new(); }, [0; 32], PaymentHash([0; 32]), 100, TEST_FINAL_CLTV + 1, None, true, None, current_height, - &TestLogger::new()); + &logger);lightning/src/blinded_path/payment.rs (1)
509-513: Clarify the comment about TLV field reuse.The comment states that authentication TLV fields were "removed in LDK v0.2" and can now be reused. However, it's unclear:
- What specific TLV field numbers are being reused
- Whether there are any backward compatibility concerns
- Whether old nodes might still send the old authentication TLVs
Consider expanding the comment to specify the exact TLV field numbers and any compatibility considerations.
lightning/src/ln/blinded_payment_tests.rs (1)
1369-1372: Remove or reword the redundant TODO-style comment.The block suggests the test is redundant, but the test below actively validates ReceiveAuthKey-based auth failure. Drop the comment to avoid confusion.
Apply this diff to remove the comment:
-/// This test became redundant after removing (HMAC, Nonce)-based verification. -/// After review, we can either remove it entirely or update it to work with -/// ReceiveAuthKey-based verification.lightning/src/ln/msgs.rs (2)
3651-3716: Blinded payment decode: AAD gating and auth propagation.
- Using ChaChaDualPolyReadAdapter with (rho, receive_auth_key) is right.
- Rejecting Forward when
used_aadis true is correct.- Propagating
used_aadintopayment_tlvs_authenticatedis good.Optional: consider a more specific DecodeError (or log) when AAD/auth fails to aid debugging;
InvalidValueis generic.Ensure send-side uses AAD only for Receive TLVs to preserve interop.
3758-3759: Deferget_receive_auth_key()in trampoline path to avoid unnecessary work.Only needed for blinded branches. Move the call inside the
if let Some(blinding_point)block (after computingrho), mirroring the non-trampoline path.Apply this diff near the start of InboundTrampolinePayload::read:
- let receive_auth_key = node_signer.get_receive_auth_key(); @@ - let mut s = Cursor::new(&enc_tlvs); + let receive_auth_key = node_signer.get_receive_auth_key(); + let mut s = Cursor::new(&enc_tlvs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(18 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(13 hunks)lightning/src/ln/channelmanager.rs(9 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(12 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(5 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
🔇 Additional comments (50)
lightning/src/offers/signer.rs (1)
39-50: Approve changesNo active references remain to the deprecated functions or types; only the commented-out constant persists.
lightning/src/ln/async_payments_tests.rs (2)
1861-1861: LGTM! Simplified access pattern aligns with ReceiveTlvs refactoring.The change from the nested structure (
tlvs.tlvs.payment_constraints.max_cltv_expiry) to the flattened structure (tlvs.payment_constraints.max_cltv_expiry) is correct and consistent with the broader refactor introducingReceiveTlvsin place ofUnauthenticatedReceiveTlvs.
1861-1861: AllBlindedPaymentTlvs::Receiveusages updated; no.tlvs.tlvsremainslightning/src/offers/flow.rs (7)
26-26: LGTM! Import change aligns with authentication enforcement.The change to import
ReceiveTlvs(instead ofUnauthenticatedReceiveTlvs) correctly supports the PR's goal of enforcing authentication for blinded payment paths.
91-91: LGTM! Field addition is correct.The
receive_auth_keyfield is properly stored and used throughout the implementation for authenticating blinded payment paths.
192-194: LGTM! Getter method is correctly implemented.The
get_receive_auth_keymethod provides clean access to the authentication key.
322-356: Authentication enforcement implemented correctly.The changes correctly:
- Retrieve
receive_auth_keyfrom the struct (line 331)- Use
ReceiveTlvsinstead ofUnauthenticatedReceiveTlvs(line 342), enforcing authentication- Pass the authentication key to the router (line 350)
This properly implements the PR's objective of adding authentication to blinded payment paths.
361-377: LGTM! Test method correctly updated.The test wrapper method signature correctly matches the updated private method.
1398-1456: LGTM! Private method correctly updated.The addition of
usable_channelsandrouterparameters enables the method to create authenticated blinded payment paths. The implementation correctly passes these parameters tocreate_static_invoice_for_server.
1614-1661: LGTM! Private method correctly updated.The addition of the
usable_channelsparameter and the use ofreceive_auth_keythroughout the method correctly enable creation of authenticated static invoices with proper blinded payment paths.lightning/src/ln/channelmanager.rs (3)
38-38: LGTM!The import of payment context types aligns with the PR's authentication enhancements for blinded payment paths.
14815-14816: Signature alignment confirmed: The arguments (&self.node_signer, &self.router, invoice_request, amount_msats, payment_hash, payment_secret, self.list_usable_channels()) match thecreate_response_for_invoice_requestdefinition inflow.rs.
4884-4884: All create_recv_pending_htlc_info invocations include the logger argument.lightning/src/ln/onion_payment.rs (6)
248-255: Function signature changes look correct.The addition of the logger parameter with appropriate bounds enables logging within the authentication checks, which improves observability for security-sensitive operations.
282-284: Authentication check properly enforced for blinded receive payloads.The authentication check is correctly invoked for
InboundOnionBlindedReceivePayload, ensuring that unauthenticated receive payments are rejected.
313-316: Authentication check properly enforced for trampoline blinded receive payloads.The authentication check is correctly invoked for
InboundOnionBlindedReceivePayloadwithin trampoline receive, ensuring consistent security across both standard and trampoline paths.
460-460: Logger correctly threaded to onion decoding.The logger is now passed to
decode_incoming_update_add_htlc_onion, enabling logging within the decoding process.
502-503: Parameters correctly aligned in create_recv_pending_htlc_info callThe last five arguments (None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height, logger.deref()) map precisely to
phantom_shared_secret,allow_underpay,counterparty_skimmed_fee_msat,current_height, andloggerin the signature.
256-266: Verify failure_code mapping for UnauthenticatedPayload
It’s currently set to BADONION | PERM | 25—confirm this aligns with the BOLT 4 failure code for an unauthenticated onion payload.lightning/src/blinded_path/payment.rs (10)
17-17: Import addition is used correctly.The
ChaChaDualPolyReadAdapterimport is utilized in line 232 for authenticated deserialization.
27-27: Import addition is used correctly.The
ReceiveAuthKeyimport is threaded through multiple functions and used for authentication in the deserialization process.
92-110: ReceiveAuthKey correctly threaded through one-hop path creation.The
receive_auth_keyparameter is properly passed through to thenewfunction, ensuring authentication is enforced for one-hop blinded payment paths.
119-155: ReceiveAuthKey correctly integrated into multi-hop path creation.The
receive_auth_keyis properly passed to bothcompute_payinfo(line 136, as a reference topayee_tlvswhich contains it implicitly) andblinded_hops(line 150), ensuring authentication is enforced throughout the path construction.
227-235: Authentication key correctly retrieved and used in deserialization.The
receive_auth_keyis obtained from thenode_signerand passed toChaChaDualPolyReadAdapter::readfor authenticated decryption. This ensures that only the intended recipient with the correct authentication key can decrypt the payload.
567-570: Deserialization correctly enforces required fields.The
BlindedPaymentTlvs::Receivevariant now properly requirespayment_secret,payment_constraints, andpayment_context, returningDecodeError::InvalidValueif any are missing. This is consistent with the new authentication model where these fields must be present and authenticated.
603-606: Deserialization correctly enforces required fields for trampoline paths.Similar to the standard receive path, the
BlindedTrampolineTlvs::Receivevariant now properly requires all three fields, ensuring consistency across both path types.
618-623: ReceiveAuthKey correctly integrated into blinded hops construction.The
local_node_receive_keyparameter is properly paired with thepayee_node_idin line 623, ensuring that the final hop in the blinded path can authenticate the payload.
692-692: Function signature change maintains consistency.The
compute_payinfofunction now takespayee_tlvs: &ReceiveTlvsas a reference, which is consistent with the new type system whereReceiveTlvsreplaces the previous unauthenticated variant.
815-815: Test code properly updated to use ReceiveTlvs.All test cases have been updated to construct
ReceiveTlvswith the required fields (payment_secret,payment_constraints, andpayment_context), ensuring tests reflect the new authentication model.Also applies to: 865-869, 883-887, 940-944, 999-1003, 1068-1072
lightning/src/ln/blinded_payment_tests.rs (7)
20-20: Imports updated for ReceiveTlvs/ReceiveAuthKey look correct.No issues spotted.
163-179: One‑hop path test updated properly.New ReceiveTlvs + receive_auth_key usage is consistent.
216-229: MPP one‑hop path update looks good.Correct parameter order and key retrieval.
1325-1340: Custom TLVs test aligned with new API.Correctly uses ReceiveTlvs and passes receive_auth_key.
2014-2027: Trampoline single‑hop receive: auth key threaded correctly.Passing Some(receive_auth_key) into construct_blinded_hops is consistent with the new flow.
1629-1630: Test node signer updated to return ReceiveAuthKey.Matches new trait requirements.
Also applies to: 1942-1943
78-96: All BlindedPaymentPath::new call sites updated with receive_auth_key
No deprecatedUnauthenticatedReceiveTlvsfound; everyBlindedPaymentPath::new(…)invocation now includes the newreceive_auth_keyparameter.lightning/src/ln/max_payment_path_len_tests.rs (2)
15-16: Import switch to ReceiveTlvs is correct.No other changes needed here.
215-234: Blinded path setup updated correctly.
- ReceiveTlvs built with expected fields.
- receive_auth_key fetched and passed in the new position to BlindedPaymentPath::new.
fuzz/src/chanmon_consistency.rs (2)
131-134: Router::create_blinded_payment_paths signature updated.ReceiveAuthKey parameter added; body remains unreachable as before. OK.
352-354: NodeSigner now implements get_receive_auth_key.Conforms with updated trait.
fuzz/src/invoice_request_deser.rs (2)
15-16: Imports updated for ReceiveTlvs/ReceiveAuthKey.Consistent with new APIs.
Also applies to: 23-23
86-87: Invoice response builder updated correctly for authenticated receive.
- Adds a deterministic ReceiveAuthKey for fuzzing.
- Uses ReceiveTlvs and passes receive_auth_key to BlindedPaymentPath::new.
- Adjusted field access on payment_constraints is correct.
Also applies to: 108-116, 125-125, 135-144
fuzz/src/full_stack.rs (2)
159-162: Router::create_blinded_payment_paths signature updated.ReceiveAuthKey parameter added; consistent with broader changes.
442-444: KeyProvider implements get_receive_auth_key.Aligns with trait changes.
lightning/src/util/test_utils.rs (1)
290-309: LGTM! Signature updated to include ReceiveAuthKey.The
create_blinded_payment_pathsmethod signature correctly addsreceive_auth_key: ReceiveAuthKeyas the second parameter and threads it through to the underlying router call, aligning with the broader refactor to authenticate blinded payment paths.fuzz/src/refund_deser.rs (2)
13-22: LGTM! Imports updated for ReceiveTlvs and ReceiveAuthKey.The import changes correctly reflect the shift from
UnauthenticatedReceiveTlvstoReceiveTlvsand add the necessaryReceiveAuthKeyimport for the new authentication flow.
67-109: LGTM! Fuzz test updated to use ReceiveTlvs and ReceiveAuthKey.The changes correctly:
- Initialize
receive_auth_keywith test data ([41; 32])- Construct
payee_tlvsasReceiveTlvsinstead ofUnauthenticatedReceiveTlvs- Update field access from
payee_tlvs.tlvs().payment_constraintstopayee_tlvs.payment_constraints, reflecting the new direct field structure- Thread
receive_auth_keythrough toBlindedPaymentPath::newThe hardcoded test values are appropriate for fuzzing.
lightning/src/ln/msgs.rs (3)
35-35: Import of ReceiveTlvs is correct for new Receive adapter path.No issues.
59-59: Switch to ChaChaDualPolyReadAdapter looks good.Fits the new auth/AAD flow.
3812-3863: Trampoline blinded decode mirrors payment path; AAD check and propagation LGTM.Symmetric handling of
used_aadandpayment_tlvs_authenticatedis correct.
- Verify NodeSigner provides stable/rotated ReceiveAuthKey semantics as intended.
- Confirm no legacy senders set AAD on forward trampoline TLVs; otherwise this becomes a breaking change.
| let payee_tlvs = ReceiveTlvs { | ||
| payment_secret, | ||
| payment_constraints: PaymentConstraints { | ||
| max_cltv_expiry: u32::max_value(), | ||
| htlc_minimum_msat: chan_upd.htlc_minimum_msat, | ||
| }, | ||
| payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}), | ||
| }; | ||
| let nonce = Nonce([43u8; 16]); | ||
| let mut payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key); | ||
| payee_tlvs.authentication.1 = Nonce([0u8; 16]); | ||
| // Use a mismatched ReceiveAuthKey to force auth failure: | ||
| let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); | ||
|
|
There was a problem hiding this comment.
Make mismatched key derivation robust.
Using ReceiveAuthKey([0;32]) could theoretically match the node’s key if it ever returns zeros. Safer: flip a bit of the real key to guarantee mismatch.
-// Use a mismatched ReceiveAuthKey to force auth failure:
-let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]);
+// Use a guaranteed-mismatched ReceiveAuthKey to force auth failure:
+let mut real = chanmon_cfgs[1].keys_manager.get_receive_auth_key().0;
+real[0] ^= 0x01;
+let mismatched_receive_auth_key = ReceiveAuthKey(real);📝 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.
| let payee_tlvs = ReceiveTlvs { | |
| payment_secret, | |
| payment_constraints: PaymentConstraints { | |
| max_cltv_expiry: u32::max_value(), | |
| htlc_minimum_msat: chan_upd.htlc_minimum_msat, | |
| }, | |
| payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}), | |
| }; | |
| let nonce = Nonce([43u8; 16]); | |
| let mut payee_tlvs = payee_tlvs.authenticate(nonce, &expanded_key); | |
| payee_tlvs.authentication.1 = Nonce([0u8; 16]); | |
| // Use a mismatched ReceiveAuthKey to force auth failure: | |
| let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); | |
| let payee_tlvs = ReceiveTlvs { | |
| payment_secret, | |
| payment_constraints: PaymentConstraints { | |
| max_cltv_expiry: u32::max_value(), | |
| htlc_minimum_msat: chan_upd.htlc_minimum_msat, | |
| }, | |
| payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}), | |
| }; | |
| // Use a guaranteed-mismatched ReceiveAuthKey to force auth failure: | |
| let mut real = chanmon_cfgs[1].keys_manager.get_receive_auth_key().0; | |
| real[0] ^= 0x01; | |
| let mismatched_receive_auth_key = ReceiveAuthKey(real); |
🤖 Prompt for AI Agents
In lightning/src/ln/blinded_payment_tests.rs around lines 1412 to 1422, creating
the mismatched ReceiveAuthKey with ReceiveAuthKey([0u8; 32]) is unsafe because
an all-zero key could theoretically match; instead construct the mismatched key
by cloning the actual receive_auth_key bytes and flipping a single bit (e.g.,
XOR one byte with 0x01) to guarantee a different value; replace the zeroed array
with a derived variant from the real key so the mismatch is deterministic and
provably not equal.
| pub payment_tlvs_authenticated: bool, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
New field payment_tlvs_authenticated added to blinded receive payload.
Good addition. Please add a brief doc comment clarifying semantics (true when payment TLVs were authenticated using ReceiveAuthKey). Also verify no external code destructures this struct without .. as this is a source-compatible change only if not destructured exhaustively.
Run to verify usages:
🏁 Script executed:
#!/bin/bash
# Confirm NodeSigner has the new API and check destructuring of InboundOnionBlindedReceivePayload
rg -n -C2 'trait\s+NodeSigner\b|impl\s+NodeSigner\b|get_receive_auth_key' --type=rust
rg -n -C2 'InboundOnionBlindedReceivePayload\s*\{' --type=rustLength of output: 28194
Add doc comment for payment_tlvs_authenticated
Document that this flag is true when payment TLVs were authenticated using ReceiveAuthKey. No external code exhaustively destructures InboundOnionBlindedReceivePayload, so this addition remains source-compatible.
🤖 Prompt for AI Agents
In lightning/src/ln/msgs.rs around lines 2342 to 2343, add a Rust doc comment
for the struct field `payment_tlvs_authenticated` explaining that the flag is
true when payment TLVs were authenticated using `ReceiveAuthKey`; insert a ///
doc line immediately above the field with that text (or similar concise
wording), keeping the comment-only change to remain source-compatible.
694476f to
1c3d125
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lightning/src/ln/blinded_payment_tests.rs (1)
1417-1418: This issue was already flagged in a previous review.Using
ReceiveAuthKey([0u8; 32])is unsafe as it could theoretically match the node's actual key if it ever returns zeros. The previous review comment suggests deriving the mismatched key from the real key by flipping a bit to guarantee mismatch.lightning/src/ln/msgs.rs (1)
2342-2343: Doc comment forpayment_tlvs_authenticatedis still missing.The earlier review asked for a brief doc explaining when this flag is set. Please add the comment so downstream users know it’s true only when the TLVs were authenticated with
ReceiveAuthKey.pub custom_tlvs: Vec<(u64, Vec<u8>)>, - pub payment_tlvs_authenticated: bool, + /// True if the decrypted payment TLVs were authenticated with `ReceiveAuthKey`. + pub payment_tlvs_authenticated: bool,lightning/src/routing/router.rs (1)
183-185: Overflow-safe CLTV aggregation: resolved with saturating_addUsing saturating_add prevents u32 overflow and avoids silently dropping candidates. Good fix.
🧹 Nitpick comments (4)
lightning/src/ln/max_payment_path_len_tests.rs (1)
15-16: Migration to ReceiveTlvs + ReceiveAuthKey — LGTMImports, TLVs construction, and BlindedPaymentPath::new call updated correctly. Minor nit:
secp_ctxneedn’t bemut.Apply:
- let mut secp_ctx = Secp256k1::new(); + let secp_ctx = Secp256k1::new();Also applies to: 216-235
fuzz/src/invoice_request_deser.rs (1)
24-24: ReceiveAuthKey threading + ReceiveTlvs migration — LGTMFuzzer compiles against new API and uses ReceiveAuthKey consistently. Optional: derive the key via a NodeSigner in future to mirror production paths; not required for fuzzing.
Also applies to: 86-87, 108-116, 125-126, 137-143
fuzz/src/refund_deser.rs (1)
21-22: ReceiveTlvs + ReceiveAuthKey adoption — LGTMConsistent with crate-wide API changes. Optional: consider deriving ReceiveAuthKey from a signer for higher fidelity; fine as-is for fuzzing.
Also applies to: 71-72, 73-80, 90-91, 102-103
lightning/src/routing/router.rs (1)
287-293: Trait docs/signature reflect authentication requirementDocs and Router trait updated coherently. Consider noting this as a breaking change in release notes/changelog.
Confirm downstream crates are updated to the new trait signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(17 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(13 hunks)lightning/src/ln/channelmanager.rs(10 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(12 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(6 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/async_payments_tests.rs
- lightning/src/util/test_utils.rs
- fuzz/src/chanmon_consistency.rs
🧰 Additional context used
🧬 Code graph analysis (9)
lightning/src/ln/blinded_payment_tests.rs (4)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/offers/flow.rs (1)
get_receive_auth_key(192-194)lightning/src/util/test_utils.rs (2)
get_receive_auth_key(1730-1732)get_receive_auth_key(1822-1824)lightning/src/sign/mod.rs (3)
get_receive_auth_key(880-880)get_receive_auth_key(2220-2222)get_receive_auth_key(2393-2395)
lightning/src/ln/max_payment_path_len_tests.rs (3)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/ln/channelmanager.rs (2)
new(3847-3925)new(16114-16135)lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/ln/onion_payment.rs (1)
lightning/src/ln/onion_utils.rs (1)
shared_secret(2281-2292)
lightning/src/ln/msgs.rs (1)
lightning/src/ln/channelmanager.rs (13)
new(3847-3925)new(16114-16135)read(612-615)read(662-665)read(1250-1264)read(1497-1499)read(15355-15396)read(15447-15492)read(15497-15544)read(15626-15659)read(15949-15965)read(16164-16170)read(16197-17856)
lightning/src/ln/channelmanager.rs (1)
lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/blinded_path/payment.rs (3)
lightning/src/ln/channelmanager.rs (13)
new(3847-3925)new(16114-16135)read(612-615)read(662-665)read(1250-1264)read(1497-1499)read(15355-15396)read(15447-15492)read(15497-15544)read(15626-15659)read(15949-15965)read(16164-16170)node(19057-19057)lightning/src/ln/msgs.rs (8)
new(2509-2511)new(2544-2546)read(1147-1171)read(1175-1181)read(3344-3355)read(3404-3408)read(3591-3749)read(3756-3898)lightning/src/blinded_path/message.rs (3)
new(68-85)blinded_hops(171-173)blinded_hops(693-733)
lightning/src/routing/router.rs (4)
lightning/src/offers/flow.rs (1)
create_blinded_payment_paths(322-356)fuzz/src/chanmon_consistency.rs (1)
create_blinded_payment_paths(130-136)fuzz/src/full_stack.rs (1)
create_blinded_payment_paths(158-164)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)
fuzz/src/invoice_request_deser.rs (1)
lightning/src/offers/static_invoice.rs (3)
invoice(801-827)sign(340-353)sign(385-387)
lightning/src/offers/flow.rs (3)
lightning/src/routing/router.rs (4)
create_blinded_payment_paths(129-222)create_blinded_payment_paths(247-255)create_blinded_payment_paths(288-292)a(3619-3619)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)lightning/src/ln/channelmanager.rs (4)
usable_channels(5743-5743)test_create_blinded_payment_paths(12930-12942)check_refresh_async_receive_offer_cache(5409-5428)timer_tick_occurred(7808-8056)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: linting
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: linting
- GitHub Check: check_release
- GitHub Check: semver-checks
🔇 Additional comments (39)
lightning/src/offers/signer.rs (1)
43-50: Legacy HMAC references fully removed; confirm version mention.All removed HMAC functions and constants no longer appear in code, and tests use
ReceiveAuthKey/ReceiveTlvsas expected. Verify that “LDK v0.2” is correctly referenced in changelogs or docs:#!/bin/bash echo "=== Searching for 'LDK v0.2' across repository ===" rg -n 'LDK v0.2'lightning/src/ln/channelmanager.rs (6)
38-38: LGTM!Import addition for payment context types aligns with the PR's ReceiveAuthKey integration.
6995-6995: Logger parameter added for phantom receive processing.Consistent with the previous change, adding
&*self.loggerenables logging during phantom receive creation.
17879-17879: LGTM!Test utility import updated to support the test modifications below.
18704-18704: Test updates align with function signature changes.The tests correctly create
TestLoggerinstances and pass references to match the updated function signatures. The pattern of creating owned loggers in tests (let logger = TestLogger::new()) and passing direct references (&logger) is appropriate and differs correctly from production code's&*self.loggerpattern.Also applies to: 18726-18726, 18749-18749, 18759-18759, 18775-18775
14815-14815: Confirm correct signature for create_response_for_invoice_request
Verified in lightning/src/offers/flow.rs:941–944 that the function expects&self, signer, router, invoice_request, amount_msats, …and the call passes&self.node_signer, &self.routerin the proper order.
4884-4884: Logger parameter added for observability.The addition of
&*self.loggertocreate_recv_pending_htlc_infoimproves HTLC receive logging. Verify that the argument order matches the updated function signature.lightning/src/ln/onion_payment.rs (2)
282-285: Auth enforced for blinded receive pathsAuthentication is checked for both BlindedReceive and TrampolineBlindedReceive. This is the right place in the flow.
Also applies to: 316-317
248-267: UnauthenticatedPayload mapping and serialization are correct
LocalHTLCFailureReason::UnauthenticatedPayload is defined, failure_code() returns BADONION | PERM | 25, and its serialization is covered by existing tests.fuzz/src/full_stack.rs (1)
159-162: Router signature update to accept ReceiveAuthKey — LGTMInterface alignment only; no behavior change.
lightning/src/ln/blinded_payment_tests.rs (7)
20-20: LGTM! Import update aligns with authentication refactor.The change from
UnauthenticatedReceiveTlvstoReceiveTlvsis consistent with the broader refactor to enforce authentication in blinded payment paths.
78-96: LGTM! Helper function properly updated for authentication.The function correctly:
- Constructs
ReceiveTlvswith payment constraints- Obtains
receive_auth_keyfrom the keys manager- Passes the authentication key to
BlindedPaymentPath::new()
163-178: LGTM! Test correctly implements authenticated blinded path.The test properly obtains the
receive_auth_keyand passes it toBlindedPaymentPath::new(), consistent with the new authentication mechanism.
1454-1454: LGTM! Correct error type for authentication failure.Changing the expected error from
InvalidOnionBlindingtoUnauthenticatedPayloadis appropriate, as this test specifically verifies that authentication failures are properly detected and reported with the correct error type.
1626-1626: LGTM! Test implementations use appropriate fixed keys.The
TestEcdhSignerimplementations return a fixed test keyReceiveAuthKey([41; 32]), which is appropriate for test mocks and provides deterministic behavior.Also applies to: 1939-1939
1325-1340: LGTM! Test correctly updated for authenticated blinded paths.The test follows the proper pattern of obtaining
receive_auth_keyfrom the keys manager and passing it toBlindedPaymentPath::new().
2011-2023: LGTM! Trampoline test properly integrates authentication.The test correctly:
- Constructs
ReceiveTlvswith payment constraints- Obtains the
receive_auth_keyfrom the keys manager- Passes it with
Some(receive_auth_key)toconstruct_blinded_hops, indicating the authentication key is optional for intermediate hops but required for the final recipientlightning/src/routing/router.rs (5)
31-31: Import of ReceiveAuthKey looks correctMatches the new API flow; no issues.
202-205: Plumbing ReceiveAuthKey into BlindedPaymentPath::newArgument order and usage look consistent with the new constructor. LGTM.
214-216: Fallback path includes ReceiveAuthKeyOne-hop/fallback case updated consistently. Looks good.
248-251: FixedRouter signature updatedUnderscored params correctly avoid unused warnings. No concerns.
132-134: API change applied: signature updated consistently AllRouter::create_blinded_payment_pathsimplementations and invocations now includesecp_ctxin the correct position.lightning/src/blinded_path/payment.rs (9)
232-233: LGTM!The integration of
ChaChaDualPolyReadAdapterwith the receive authentication key is correct and consistent with the pattern used inlightning/src/ln/msgs.rs.
509-513: Clarify backward compatibility implications.The comment indicates that blinded payment paths created with LDK versions prior to v0.2 will fail authentication checks and cannot be used. This is a breaking change in terms of protocol compatibility.
Please confirm:
- This breaking change is intentional and acceptable
- It will be documented in release notes or migration guide
- There's no need to maintain backward compatibility with pre-v0.2 paths
567-570: LGTM!The
ReceiveTlvsconstruction correctly handles all required fields with proper error handling for missing values.
603-606: LGTM!Consistent with
BlindedPaymentTlvs::Receiveconstruction pattern.
615-633: LGTM!Correctly threads
ReceiveAuthKeyto only the payee node (line 622) while intermediate nodes receiveNone, which is appropriate since only the final recipient needs to authenticate and decrypt the receive TLVs.
690-742: LGTM!The parameter type change from
UnauthenticatedReceiveTlvstoReceiveTlvsis consistent with the authentication model refactoring, and all field accesses are correct.
814-814: LGTM!All test cases have been properly updated to use
ReceiveTlvsdirectly with all required fields. The test coverage appears comprehensive.Also applies to: 864-864, 882-882, 939-939, 998-998, 1067-1067
92-111: Verify breaking API change
Automated searches returned no invocations ofBlindedPaymentPath::one_hop. As this is a public-API signature change requiring a newreceive_auth_keyargument, manually confirm that all call sites—including examples, tests, fuzz targets, and any external integrations—have been updated accordingly.
119-155: AllBlindedPaymentPath::newcallers updated forreceive_auth_key. Router, unit tests (including max_payment_path_len_tests.rs and blinded_payment_tests.rs) and fuzz harnesses now include the new parameter.lightning/src/offers/flow.rs (8)
26-26: LGTM!Import change is consistent with the removal of
UnauthenticatedReceiveTlvstype.
361-377: LGTM!Test helper correctly updated to match the internal method signature.
812-867: LGTM!The removal of
entropy_sourceparameter is consistent with the changes increate_blinded_payment_paths.
941-1017: LGTM!Signature changes are consistent with the broader refactoring to remove
entropy_sourcedependencies.
1323-1345: LGTM!Signature refactoring improves readability without changing functionality.
1398-1456: LGTM!Signature simplification is consistent with
check_refresh_async_receive_offer_cache.
1614-1661: LGTM!Method signature updates are consistent with the broader refactoring. The logic flow correctly passes parameters through the call chain.
322-356: Confirm persistentreceive_auth_keydesign and document rotation
- The
entropy_sourceparameter was removed and all callers were updated to use the persistentFlow::receive_auth_key(no remaining uses ofentropy_source).- Please confirm this shift to a long-lived auth key is intentional, does not weaken privacy/security, and that any key-rotation strategy is documented.
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 (1)
lightning/src/offers/flow.rs (1)
812-867: Breaking API change: update all call sites
create_static_invoice_buildernow requiresusable_channelsandpeers. Remaining 4-argument invocations must be updated, for example in:
- lightning/src/ln/async_payments_tests.rs:331
- lightning/src/ln/async_payments_tests.rs:698
- lightning/src/ln/async_payments_tests.rs:1678
♻️ Duplicate comments (4)
lightning/src/ln/blinded_payment_tests.rs (1)
1410-1416: Make mismatched ReceiveAuthKey deterministic (avoid all‑zero key)Use a derived key with a flipped bit to guarantee mismatch instead of [0u8; 32].
Apply this diff:
- // Use a mismatched ReceiveAuthKey to force auth failure: - let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); + // Use a guaranteed-mismatched ReceiveAuthKey to force auth failure: + let mut real = chanmon_cfgs[1].keys_manager.get_receive_auth_key().0; + real[0] ^= 0x01; + let mismatched_receive_auth_key = ReceiveAuthKey(real);Also applies to: 1417-1419
lightning/src/ln/onion_payment.rs (2)
459-461: Call updated to pass logger — resolved.decode_incoming_update_add_htlc_onion now receives logger.deref(); matches the revised signature.
500-504: create_recv_pending_htlc_info call updated with new args — resolved.current_height and logger.deref() are passed; argument order matches the function definition.
To ensure no stale callsites remain, run:
#!/bin/bash rg -nP 'create_recv_pending_htlc_info\s*\(' -C2lightning/src/ln/msgs.rs (1)
2342-2342: Documentpayment_tlvs_authenticatedsemanticsAdd a brief doc line clarifying it is true when payment TLVs were authenticated using ReceiveAuthKey.
pub struct InboundOnionBlindedReceivePayload { pub sender_intended_htlc_amt_msat: u64, pub total_msat: u64, pub cltv_expiry_height: u32, pub payment_secret: PaymentSecret, pub payment_constraints: PaymentConstraints, pub payment_context: PaymentContext, pub intro_node_blinding_point: Option<PublicKey>, pub keysend_preimage: Option<PaymentPreimage>, pub invoice_request: Option<InvoiceRequest>, pub custom_tlvs: Vec<(u64, Vec<u8>)>, + /// True iff the payment TLVs were authenticated (AAD) using `ReceiveAuthKey`. pub payment_tlvs_authenticated: bool, }
🧹 Nitpick comments (6)
lightning/src/routing/router.rs (1)
132-134: API change: create_blinded_payment_paths now requires ReceiveAuthKeyThis is a breaking change for Router consumers. Please:
- Document migration steps (CHANGELOG/RELEASE NOTES).
- Consider adding a convenience helper on DefaultRouter to derive a ReceiveAuthKey (from EntropySource) to ease callers.
lightning/src/ln/blinded_payment_tests.rs (2)
81-81: Optional: prefer u32::MAX over u32::max_value()Minor modernization/readability tweak; no behavior change.
Also applies to: 166-166, 219-219
90-91: Optional: drop unnecessarymuton Secp256k1 context
Secp256k1::new()returns an immutable context;mutis not required.Also applies to: 173-174, 201-202, 1390-1391, 1525-1526
lightning/src/ln/onion_payment.rs (1)
248-267: Auth gate and logger threading added — sound.The new logger param and check_authentication closure are correct; returning UnauthenticatedPayload with warn log is appropriate.
Consider renaming allow_underpay to allow_skimmed_fees (or similar) for clarity.
Confirm we only require authentication for blinded receives (not for unblinded Receive/TrampolineReceive) per the intended protocol.
lightning/src/ln/msgs.rs (2)
3660-3716: ReceiveAuthKey plumbing and auth flag enforcement are correct
- BlindedReceive now carries
payment_tlvs_authenticatedfromused_aad, andonion_payment.rsrejects unauthenticated payloads viacheck_authentication.NodeSigner::get_receive_auth_keyis exposed in trait and impls; consider adding aReceiveAuthKeymethod to replace.0tuple access.- Add unit tests for
InboundOnionBlindedReceivePayloaddecoding to assertpayment_tlvs_authenticatedis set/unset based on AAD usage.
3758-3864: Add tests for trampoline receive AAD handling
Verified thatpayment_tlvs_authenticatedis correctly propagated andcheck_authenticationis invoked for trampoline receive paths; add tests covering both authenticated and unauthenticated flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(17 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(13 hunks)lightning/src/ln/channelmanager.rs(10 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(12 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(6 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
🔇 Additional comments (38)
lightning/src/ln/async_payments_tests.rs (1)
1861-1861: LGTM! Clean structural refactoring.The field access path has been correctly updated to match the flattened TLV structure, removing one level of indirection. This aligns with the PR's refactoring of the authentication model for blinded payment paths.
lightning/src/ln/channelmanager.rs (5)
38-38: LGTM!The import additions for payment context types are straightforward and align with the broader authentication key flow changes mentioned in the PR summary.
17879-17879: LGTM!The addition of
TestLoggerimports and instantiation in test code is standard practice for supporting the new logger parameters in function calls.Also applies to: 18704-18704, 18759-18759
18726-18726: LGTM!The test code correctly passes the
&loggerparameter tocreate_recv_pending_htlc_info, consistent with the production code changes.Also applies to: 18749-18749, 18775-18775
14815-14815: No change required for router parametercreate_response_for_invoice_request expects a router implementing Deref, so passing &self.router matches its signature.
4884-4884: Allcreate_recv_pending_htlc_infocall sites include the logger parameter
Verified every invocation now ends with the logger argument.lightning/src/blinded_path/payment.rs (3)
218-236: LGTM! Authentication implementation looks correct.The integration of
ReceiveAuthKeyfor authenticating encrypted payloads follows the expected pattern. The use ofChaChaDualPolyReadAdapterwith the authentication key provides cryptographic verification of the payload.
119-155: Verified: allBlindedPaymentPath::newcallers now include the newreceive_auth_keyparameter.
92-111: Breaking API change: addedreceive_auth_keyparameter
Confirm all callers ofBlindedPaymentPath::one_hophave been updated; a search for usage (rg -nP --type=rust '\bone_hop\s*\(') returned no matches, so please manually verify any call sites.lightning/src/offers/flow.rs (5)
322-356: LGTM! Consistent parameter threading.The function correctly threads
receive_auth_keythrough to the router'screate_blinded_payment_pathsand properly constructsReceiveTlvswith payment constraints.
1398-1456: LGTM! Well-structured implementation.The function correctly handles invoice refresh by:
- Iterating over offers needing updates
- Continuing on errors rather than failing entirely
- Avoiding lock contention by enqueueing messages in a separate loop
1614-1661: LGTM! Proper parameter integration.The function correctly integrates the new
usable_channelsparameter for creating blinded payment paths in the static invoice.
941-1017: Internal call sites updated; no other usages found.
The only invocation inlightning/src/ln/channelmanager.rshas been adjusted for the newrouterandusable_channelsparameters.
1323-1345: No breaking external API. The added parameters toFlow::check_refresh_async_receive_offer_cacheare fully encapsulated by ChannelManager’s unchanged public method, so callers need no updates.Likely an incorrect or invalid review comment.
lightning/src/routing/router.rs (4)
31-31: LGTM: new import is usedReceiveAuthKey is correctly imported and used; no dead code.
183-185: Good fix: use saturating_add to avoid overflowThis restores safe behavior for max_cltv_expiry adjustment.
202-202: Confirm ReceiveAuthKey reuse semantics across pathsYou pass the same receive_auth_key into each BlindedPaymentPath::new. Confirm this key must be constant per invoice/recipient (not per path). If per‑path keys are desired, derive subkeys deterministically (e.g., HKDF with path index).
Also applies to: 214-214
248-251: LGTM: FixedRouter signature updatedParam is intentionally unused; consistent with this router’s purpose.
fuzz/src/chanmon_consistency.rs (1)
130-137: LGTM: Router trait signature updated correctly.The
create_blinded_payment_pathssignature now includes the_receive_auth_key: ReceiveAuthKeyparameter and usesReceiveTlvs, aligning with the new authenticated payment path construction flow.lightning/src/ln/max_payment_path_len_tests.rs (1)
215-235: LGTM: Test correctly migrated to new authentication model.The test now:
- Directly constructs
ReceiveTlvs(line 215-222)- Obtains
receive_auth_keyfrom the keys manager (line 223)- Passes
receive_auth_keytoBlindedPaymentPath::new(line 228)This eliminates the previous nonce-based HMAC authentication step, simplifying the flow.
fuzz/src/invoice_request_deser.rs (1)
86-143: LGTM: Fuzz test migrated to simplified authentication flow.Key changes:
- Hardcoded
ReceiveAuthKey([41; 32])is appropriate for fuzz testing (line 86)- Direct
ReceiveTlvsconstruction without intermediate authentication (lines 108-115)- Direct field access
payee_tlvs.payment_constraintsindicates flatter TLV structure (line 125)receive_auth_keythreaded through toBlindedPaymentPath::new(line 137)fuzz/src/full_stack.rs (1)
159-162: LGTM: Consistent Router trait signature update.The signature matches the changes in other fuzz files, properly adding
_receive_auth_key: ReceiveAuthKeyand updating_tlvstoReceiveTlvs.fuzz/src/refund_deser.rs (1)
71-108: LGTM: Consistent fuzz test migration.This follows the same pattern as
invoice_request_deser.rs:
- Hardcoded
ReceiveAuthKeyfor testing (line 71)- Direct
ReceiveTlvsconstruction (lines 73-80)- Direct field access
payee_tlvs.payment_constraints(line 90)receive_auth_keypassed to path construction (line 102)lightning/src/offers/signer.rs (1)
43-50: LGTM: HMAC constants properly reserved to prevent type confusion.The removal of
PAYMENT_TLVS_HMAC_INPUTand related functions aligns with the migration toReceiveAuthKey-based authentication. Reserving the values[5; 16],[7; 16], and[8; 16]is a good security practice to prevent type confusion attacks if these constants are inadvertently reused.lightning/src/ln/blinded_payment_tests.rs (10)
20-20: Import updates look correctSwitched to ReceiveTlvs and related symbols. LGTM.
78-86: Correctly thread ReceiveAuthKey and ReceiveTlvs into path constructionGood migration: build ReceiveTlvs, fetch receive_auth_key, and pass both to BlindedPaymentPath::new. LGTM.
Also applies to: 88-95
163-171: One-hop path: auth key and ReceiveTlvs usage is soundBlindedPaymentPath::new invocation matches new API. LGTM.
Also applies to: 175-177
216-224: MPP to one-hop: new authentication flow integratedReceiveTlvs + receive_auth_key passed correctly. LGTM.
Also applies to: 226-228
1325-1334: Custom TLVs flow: updated ReceiveTlvs/auth key wiringConstruction and usage align with new API. LGTM.
Also applies to: 1334-1334, 1337-1339
1379-1387: Auth success case: updated ReceiveTlvs/auth key wiringMatches expectations before forced failure. LGTM.
Also applies to: 1391-1394
1454-1455: Updated expected error aligns with new unauthenticated payload semanticsExpecting UnauthenticatedPayload with empty data is correct here. LGTM.
1627-1627: Test signer: get_receive_auth_key stub is fineStatic key for tests is acceptable. LGTM.
1939-1939: Inbound decoding test: get_receive_auth_key stub is fineConsistent with above. LGTM.
2011-2024: Trampoline single-hop receive: authenticated ReceiveTlvs pathEncoding ReceiveTlvs and passing receive_auth_key into blinded hops looks correct. LGTM.
lightning/src/util/test_utils.rs (1)
291-294: Router::create_blinded_payment_paths signature update handled correctlyNew receive_auth_key parameter added and forwarded to inner router; fallbacks preserved. LGTM.
Also applies to: 297-305
lightning/src/ln/onion_payment.rs (1)
283-285: Authentication enforced for blinded receive paths — LGTM.The early check prevents unauthenticated blinded payments from proceeding.
Also applies to: 316-317
lightning/src/ln/msgs.rs (2)
35-35: LGTM: import ReceiveTlvsRequired for the new decode path. No concerns.
59-59: LGTM: import ChaChaDualPolyReadAdapterMatches the new authenticated-TLV decode flow.
| // Note: Authentication TLV field was removed in LDK v0.2 following the | ||
| // introduction of `ReceiveAuthKey`-based authentication for inbound | ||
| // `BlindedPaymentPaths`s. Because we do not support receiving to those | ||
| // contexts anymore (they will fail the `ReceiveAuthKey`-based | ||
| // authentication checks), we can reuse those fields here. |
There was a problem hiding this comment.
🧩 Analysis chain
Verify backward compatibility implications.
The comment indicates that TLV fields previously used for authentication are being reused, and old contexts will fail the new ReceiveAuthKey-based authentication. This appears to be an intentional breaking change.
Please confirm:
- This breaking change is documented in migration/upgrade notes
- The version compatibility matrix is updated
- There are no supported upgrade paths from versions using the old authentication TLV fields
You may want to search documentation for mentions of this breaking change:
🏁 Script executed:
#!/bin/bash
# Description: Search for documentation of the authentication change
fd -e md -e txt | xargs rg -l "ReceiveAuthKey|authentication.*TLV|v0\.2" Length of output: 383
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' pending_changelog/3917-blinded-path-auth.txtLength of output: 103
🏁 Script executed:
#!/bin/bash
# Search for compatibility matrix in documentation
fd -e md -e txt | xargs rg -n 'compatibil'
# Search for migration or upgrade guides
fd -e md -e txt | xargs rg -n 'migration|upgrade'Length of output: 2270
Document blinded-path auth breaking change and upgrade guidance.
- Mirror “invalidates existing
Refunds containing blinded paths” under v0.2.0 in CHANGELOG.md. - Add an upgrade/migration section (e.g. docs/UPGRADE.md) clarifying removal of the TLV auth field and lack of rollback path.
- Update the compatibility matrix to flag this breaking change for v0.2.0.
🤖 Prompt for AI Agents
In lightning/src/blinded_path/payment.rs around lines 509 to 513, add
documentation and upgrade guidance for the LDK v0.2 breaking change: update
CHANGELOG.md to mirror the note that v0.2.0 invalidates existing Refunds
containing blinded paths, add a new section in docs/UPGRADE.md explaining
removal of the TLV authentication field, the ReceiveAuthKey-based authentication
change, and that there is no rollback path (include recommended migration steps
for operators), and update the compatibility matrix (e.g., docs/COMPATIBILITY.md
or README) to mark v0.2.0 as a breaking change for blinded-path auth; ensure the
messages are concise and reference this file/lines for context.
1c3d125 to
2980c17
Compare
Extends the work started in [PR#3917](lightningdevkit#3917) by adding ReceiveAuthKey-based verification for Blinded Payment Paths. This reduces space previously taken by individual ReceiveTlvs and aligns the verification logic with that used for Blinded Message Paths.
Now that we have introduced an alternate mechanism for authentication in the codebase, we can safely remove the now redundant (hmac, nonce) fields from the Payment ReceiveTlvs's while maintaining the security of the onion messages.
shaavan
force-pushed
the
aad-payment-review
branch
from
2980c17 to
cf551f6
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
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/msgs.rs (1)
3664-3683: Rejecting blinded forwards on authenticated TLVs breaks valid flows
ChaChaDualPolyReadAdapter::readsetsused_aadwhenever the sender authenticated a TLV with the receive auth key. For forwards that is legitimate: intermediate hops will never use the auth key, but the sender could still mark the TLVs as authenticated. The newif … || used_aad { return Err(DecodeError::InvalidValue) }rejects those packets outright, which means any authenticated blinded forward now fails to decode. The auth bit should only gate final-hop variants, notBlindedPaymentTlvs::Forward.Please drop the
|| used_aadguard (or rework it so forwards ignore the flag) to restore compatibility with valid authenticated forward payloads while keeping the receive case check.
♻️ Duplicate comments (2)
lightning/src/ln/onion_utils.rs (1)
1769-1773: Treat UnauthenticatedPayload as a recipient failure
UnauthenticatedPayloadbubbles all the way up as an intermediate-hop error becauseis_recipient_failure()still returnsfalse. That causes the router to penalize the previous public channel instead of marking the payment as a final-hop failure, undoing the intended scoring fix. Please include the new variant in the recipient check.self.failure_code() == LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code() || *self == LocalHTLCFailureReason::FinalIncorrectCLTVExpiry || *self == LocalHTLCFailureReason::FinalIncorrectHTLCAmount || *self == LocalHTLCFailureReason::MPPTimeout + || *self == LocalHTLCFailureReason::UnauthenticatedPayloadlightning/src/ln/blinded_payment_tests.rs (1)
1417-1418: Unsafe mismatched key construction—use bit-flip instead of zeros.Using
ReceiveAuthKey([0u8; 32])as the mismatched key is unsafe because if the real key ever returns zeros, the test would incorrectly pass (the keys would match). To guarantee a mismatch, flip a bit of the real key instead.Apply this diff to ensure a guaranteed mismatch:
-// Use a mismatched ReceiveAuthKey to force auth failure: -let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); +// Use a guaranteed-mismatched ReceiveAuthKey to force auth failure: +let mut real = chanmon_cfgs[1].keys_manager.get_receive_auth_key().0; +real[0] ^= 0x01; +let mismatched_receive_auth_key = ReceiveAuthKey(real);Based on past review comments.
🧹 Nitpick comments (3)
lightning/src/ln/max_payment_path_len_tests.rs (1)
224-234: Minor nit: unnecessary mut on secp_ctxsecp_ctx isn’t mutated. You can drop mut for clarity.
- let mut secp_ctx = Secp256k1::new(); + let secp_ctx = Secp256k1::new();fuzz/src/invoice_request_deser.rs (1)
125-126: Use saturating_add to avoid overflow under fuzzingPayee constraint math can overflow u32 in edge cases. Prefer saturating_add.
- max_cltv_expiry: payee_tlvs.payment_constraints.max_cltv_expiry + 40, + max_cltv_expiry: payee_tlvs.payment_constraints + .max_cltv_expiry + .saturating_add(40),lightning/src/routing/router.rs (1)
287-292: Public trait break acknowledged; docs updatedDoc comment clarifies
receive_auth_keyrequirement. Consider also:
- Add CHANGELOG entry and brief migration guide note.
- Optional: accept
&ReceiveAuthKeyto avoid copying sensitive material (if the type is non-trivial).Please confirm a CHANGELOG/migration note exists for the added parameter and param reordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(17 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(12 hunks)lightning/src/ln/channelmanager.rs(10 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(12 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(4 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fuzz/src/chanmon_consistency.rs
- lightning/src/offers/flow.rs
🧰 Additional context used
🧬 Code graph analysis (7)
lightning/src/ln/channelmanager.rs (1)
lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/ln/blinded_payment_tests.rs (4)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/offers/flow.rs (1)
get_receive_auth_key(192-194)lightning/src/util/test_utils.rs (2)
get_receive_auth_key(1730-1732)get_receive_auth_key(1822-1824)lightning/src/sign/mod.rs (3)
get_receive_auth_key(880-880)get_receive_auth_key(2220-2222)get_receive_auth_key(2393-2395)
lightning/src/routing/router.rs (4)
lightning/src/offers/flow.rs (1)
create_blinded_payment_paths(322-356)fuzz/src/chanmon_consistency.rs (1)
create_blinded_payment_paths(130-136)fuzz/src/full_stack.rs (1)
create_blinded_payment_paths(158-164)lightning/src/util/test_utils.rs (1)
create_blinded_payment_paths(290-308)
lightning/src/ln/msgs.rs (1)
lightning/src/ln/channelmanager.rs (2)
new(3847-3925)new(16114-16135)
lightning/src/ln/max_payment_path_len_tests.rs (3)
lightning/src/blinded_path/payment.rs (1)
new(119-155)lightning/src/ln/channelmanager.rs (2)
new(3847-3925)new(16114-16135)lightning/src/offers/flow.rs (1)
new(118-149)
lightning/src/util/test_utils.rs (1)
lightning/src/routing/router.rs (14)
first_hops(156-207)first_hops(7886-7886)first_hops(7910-7910)first_hops(8313-8313)first_hops(8323-8323)first_hops(8382-8382)first_hops(8542-8542)first_hops(8622-8622)first_hops(8687-8687)first_hops(8741-8741)first_hops(8861-8861)first_hops(8877-8877)first_hops(8889-8889)first_hops(9033-9033)
lightning/src/blinded_path/payment.rs (3)
lightning/src/ln/channelmanager.rs (13)
new(3847-3925)new(16114-16135)read(612-615)read(662-665)read(1250-1264)read(1497-1499)read(15355-15396)read(15447-15492)read(15497-15544)read(15626-15659)read(15949-15965)read(16164-16170)node(19057-19057)lightning/src/ln/msgs.rs (8)
new(2509-2511)new(2544-2546)read(1147-1171)read(1175-1181)read(3344-3355)read(3404-3408)read(3591-3749)read(3756-3898)lightning/src/blinded_path/message.rs (3)
new(68-85)blinded_hops(171-173)blinded_hops(693-733)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: linting
- GitHub Check: check_release
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: linting
- GitHub Check: check_release
- GitHub Check: semver-checks
- GitHub Check: semver-checks
🔇 Additional comments (24)
lightning/src/ln/channelmanager.rs (4)
38-38: LGTM: Import addition aligns with PR objectives.The addition of payment context imports (
AsyncBolt12OfferContext,Bolt12OfferContext,PaymentContext) is consistent with the PR's goal of adding ReceiveAuthKey verification for blinded payment paths. While the visible hunks don't show the usage of these types, this is expected as we're only seeing fragments of the changes.
4884-4884: LGTM: Logger parameter additions are consistent.The addition of
&*self.loggeras the final parameter tocreate_recv_pending_htlc_infois applied consistently across call sites. This pattern correctly dereferences and borrows the logger, which is appropriate for Rust trait object parameters.Also applies to: 6995-6995
14815-14815: LGTM: Parameter update aligns with authentication flow changes.The addition of
&self.node_signerand&self.routerparameters tocreate_response_for_invoice_requestis consistent with the PR's objective to thread ReceiveAuthKey through the payment flow. The parameters are correctly borrowed.
17879-17879: LGTM: Test code updates are consistent and correct.The test code has been updated systematically to:
- Import
TestLoggerfrom test utilities- Instantiate
TestLogger::new()in test functions- Pass
&loggertocreate_recv_pending_htlc_infocallsAll changes follow the same pattern and correctly match the updated function signatures in the production code.
Also applies to: 18704-18704, 18726-18726, 18749-18749, 18759-18759, 18775-18775
lightning/src/ln/async_payments_tests.rs (1)
1861-1863: TLV access update looks correctSwitching to tlvs.payment_constraints.max_cltv_expiry matches the new ReceiveTlvs layout. LGTM.
lightning/src/util/test_utils.rs (1)
291-305: Router API update threaded cleanlyAdded receive_auth_key param and forwarded to inner router correctly. No issues spotted.
lightning/src/offers/signer.rs (1)
43-51: Reserved HMAC inputs comment syncReservation notes (including [8;16]) are clear and prevent reuse. LGTM.
lightning/src/ln/max_payment_path_len_tests.rs (2)
15-16: Import migration to ReceiveTlvsUpdated import aligns with new API. OK.
215-223: ReceiveTlvs construction matches new typeFields populated correctly, consistent with router usage. OK.
fuzz/src/invoice_request_deser.rs (1)
135-143: BlindedPaymentPath::new updated usage looks goodPassing receive_auth_key and ReceiveTlvs matches the new constructor. OK.
fuzz/src/full_stack.rs (1)
158-164: LGTM! Fuzz stub updated correctly.The
create_blinded_payment_pathssignature has been updated to include the new_receive_auth_key: ReceiveAuthKeyparameter. The method correctly returnsunreachable!()as expected for a fuzz test stub.fuzz/src/refund_deser.rs (1)
71-109: LGTM! Clean migration to ReceiveTlvs and ReceiveAuthKey.The code has been cleanly migrated from the old authentication flow:
ReceiveTlvsis now constructed directly instead ofUnauthenticatedReceiveTlvsfollowed by authentication- A
ReceiveAuthKey([41; 32])is consistently used (matching test utilities elsewhere)- The
receive_auth_keyis properly threaded through toBlindedPaymentPath::newThe simplification from
payee_tlvs.tlvs().payment_constraintstopayee_tlvs.payment_constraintscorrectly reflects the updated type structure.lightning/src/ln/blinded_payment_tests.rs (6)
47-96: LGTM! Helper function updated correctly.The
blinded_payment_pathhelper has been properly updated to:
- Use
ReceiveTlvsinstead of the old unauthenticated variant- Retrieve
receive_auth_keyfrom the keys manager- Thread the key through to
BlindedPaymentPath::new
154-193: LGTM! Test updated correctly.The
one_hop_blinded_pathtest has been cleanly updated to useReceiveTlvsand retrieve thereceive_auth_keyfrom the keys manager before passing it to path construction.
196-272: LGTM! MPP test updated correctly.The multi-path payment test has been properly updated to use the new authentication key flow.
1315-1367: LGTM! Custom TLVs test updated correctly.The test correctly constructs
ReceiveTlvs, retrieves the authentication key, and threads it through path construction.
1626-1626: LGTM! Test signer implementations updated correctly.Both
TestEcdhSignerimplementations correctly provideget_receive_auth_key()returning a consistent test keyReceiveAuthKey([41; 32]), which aligns with test utilities elsewhere in the codebase.Also applies to: 1939-1939
2011-2026: LGTM! Trampoline test updated correctly.The test properly constructs
ReceiveTlvsand threads thereceive_auth_keythrough the blinded hop construction. The pattern of passingSome(receive_auth_key)in the tuple is consistent with the new API for authenticated receive hops.lightning/src/routing/router.rs (6)
31-31: Import of ReceiveAuthKey looks correctNew dependency is cleanly introduced; no issues.
183-185: Good: avoid overflow withsaturating_addPrevents CLTV overflow while preserving candidate paths.
202-204: Pass-through ofreceive_auth_keyto BlindedPaymentPath::new is correctOrdering matches the updated constructor; no issues spotted.
214-216: Fallback path:receive_auth_keyplumbed correctlyConsistent with the multi-hop path case.
248-251: FixedRouter signature updated appropriatelyUnderscored args avoid warnings; behavior unchanged.
132-134: Approve: signature change applied – callers updated
Add a migration note documenting theReceiveAuthKeyparameter addition and reordering.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
lightning/src/ln/onion_utils.rs (2)
1768-1773: UnauthenticatedPayload must be classified as recipient failure.
UnauthenticatedPayloadis a final-hop error indicating the recipient cannot authenticate the payment TLVs, yet it is omitted fromis_recipient_failure(). This causes the error to be misclassified as an intermediate-hop failure, leading to incorrect pathfinding penalties against the last public hop instead of the actual recipient.Apply this diff to fix the classification:
fn is_recipient_failure(&self) -> bool { self.failure_code() == LocalHTLCFailureReason::IncorrectPaymentDetails.failure_code() || *self == LocalHTLCFailureReason::FinalIncorrectCLTVExpiry || *self == LocalHTLCFailureReason::FinalIncorrectHTLCAmount || *self == LocalHTLCFailureReason::MPPTimeout + || *self == LocalHTLCFailureReason::UnauthenticatedPayload }Note: This issue was previously flagged in an earlier review but the fix was not applied.
1730-1730: Verify interoperability of custom failure code.
UnauthenticatedPayloadusesBADONION | PERM | 24, which is not defined in BOLT #4. According to previous review research, using ad-hoc failure codes breaks interoperability with other Lightning implementations. Consider either:
- Mapping to an existing BOLT #4 code that approximates this error condition
- Submitting a BOLT proposal to add a standardized code for unauthenticated payloads
- Documenting the interoperability implications if proceeding with the custom code
What is the current status of BOLT proposals for adding an "unauthenticated payload" failure code to BOLT #4?lightning/src/ln/msgs.rs (1)
2342-2343: Documentpayment_tlvs_authenticated.Please add the brief
///doc string clarifying that this flag is set when the blinded payment TLVs were authenticated with the ReceiveAuthKey. That context is still missing here as previously requested.lightning/src/blinded_path/payment.rs (2)
232-235: DUPLICATE: Missingused_aadvalidation allows unauthenticated payloads.This critical issue was already flagged in a previous review. The
used_aadflag fromChaChaDualPolyReadAdapteris not checked, so legacy payloads without the Poly1305 authentication tag will still decrypt successfully, bypassing the newReceiveAuthKey-based authentication mechanism.
509-513: DUPLICATE: Document the breaking change and provide upgrade guidance.This issue was already flagged in a previous review. The comment indicates that TLV fields are being reused and old contexts will fail authentication, but this breaking change needs to be properly documented in CHANGELOG.md, migration guides, and the compatibility matrix.
lightning/src/ln/blinded_payment_tests.rs (1)
1417-1418: Make mismatched key derivation robust.As flagged in a previous review, using
ReceiveAuthKey([0; 32])is fragile. Derive the mismatched key by flipping a bit of the real key to guarantee inequality and make the test self-documenting.-// Use a mismatched ReceiveAuthKey to force auth failure: -let mismatched_receive_auth_key = ReceiveAuthKey([0u8; 32]); +// Use a guaranteed-mismatched ReceiveAuthKey to force auth failure: +let mut real = chanmon_cfgs[1].keys_manager.get_receive_auth_key().0; +real[0] ^= 0x01; +let mismatched_receive_auth_key = ReceiveAuthKey(real);lightning/src/routing/router.rs (3)
132-134: DefaultRouter::create_blinded_payment_paths signature updated — OKParam order matches trait; type bounds intact.
183-185: Correctly avoid CLTV overflow with saturating_addPrevents u32 wrap/drop; resolves prior risk.
287-293: Router trait: added receive_auth_key and updated docs — OKSignature and doc align; breaking change acknowledged elsewhere.
🧹 Nitpick comments (3)
lightning/src/ln/max_payment_path_len_tests.rs (1)
215-234: Good migration to ReceiveTlvs + ReceiveAuthKey; tiny cleanup
- Constructing ReceiveTlvs and passing receive_auth_key into BlindedPaymentPath::new aligns with the new API. LGTM.
- Minor: secp_ctx doesn’t need to be mutable.
Apply this cleanup:
- let mut secp_ctx = Secp256k1::new(); + let secp_ctx = Secp256k1::new();fuzz/src/invoice_request_deser.rs (1)
86-116: Fuzz updated to ReceiveTlvs + ReceiveAuthKey — OK; optional realism
- Swapping to ReceiveTlvs and threading receive_auth_key into BlindedPaymentPath::new is correct. LGTM.
- Optional: Instead of a hard-coded ReceiveAuthKey([41; 32]), consider obtaining it via a NodeSigner impl to mirror prod flow (if convenient in fuzz).
Also applies to: 126-145
fuzz/src/refund_deser.rs (1)
71-81: Correct ReceiveAuthKey threading in refund fuzzSwitch to ReceiveTlvs and passing receive_auth_key into BlindedPaymentPath::new looks good. Optionally derive the key via NodeSigner for parity with production code if desired.
Also applies to: 90-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
fuzz/src/chanmon_consistency.rs(1 hunks)fuzz/src/full_stack.rs(1 hunks)fuzz/src/invoice_request_deser.rs(5 hunks)fuzz/src/refund_deser.rs(4 hunks)lightning/src/blinded_path/payment.rs(17 hunks)lightning/src/ln/async_payments_tests.rs(1 hunks)lightning/src/ln/blinded_payment_tests.rs(12 hunks)lightning/src/ln/channelmanager.rs(10 hunks)lightning/src/ln/max_payment_path_len_tests.rs(2 hunks)lightning/src/ln/msgs.rs(12 hunks)lightning/src/ln/onion_payment.rs(5 hunks)lightning/src/ln/onion_utils.rs(4 hunks)lightning/src/offers/flow.rs(10 hunks)lightning/src/offers/signer.rs(1 hunks)lightning/src/routing/router.rs(7 hunks)lightning/src/util/test_utils.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, 1.63.0)
- GitHub Check: benchmark
🔇 Additional comments (21)
lightning/src/ln/onion_utils.rs (1)
1683-1689: Enum variant and serialization implemented correctly.The
UnauthenticatedPayloadvariant is properly integrated into the enum definition, serialization mappings, and debug assertions. The 32-byte error data requirement (matchingInvalidOnionBlinding) is appropriate for this BADONION-flagged error.Also applies to: 1890-1891, 2052-2052
lightning/src/ln/onion_payment.rs (3)
248-255: Generic logger parameter enables flexible logging context.The refactor to make
create_recv_pending_htlc_infogeneric overL: DerefwhereL::Target: Loggerallows this function to accept various logger types while maintaining compile-time dispatch. This is a clean approach for threading logging context through the payment processing pipeline.
256-266: Authentication guard correctly fails fast.The
check_authenticationclosure properly validatespayment_tlvs_authenticatedand returnsUnauthenticatedPayloaderror before expensive constraint validation. The guard is correctly applied to bothBlindedReceiveandTrampolineBlindedReceivebranches, ensuring consistent authentication enforcement across all blinded payment paths.The early-exit pattern here prevents wasted processing on unauthenticated payloads and provides clear error semantics to the caller.
Also applies to: 282-284, 313-316
460-461: Logger dereferencing applied correctly at callsites.Both
decode_incoming_update_add_htlc_onion(line 460) andcreate_recv_pending_htlc_info(line 503) correctly passlogger.deref()to satisfy the generic logger parameter constraint. The Deref trait is properly utilized to convert fromL: DereftoL::Target: Logger.Also applies to: 503-504
lightning/src/ln/channelmanager.rs (2)
38-38: LGTM! Import changes support parameter threading.The added imports for
PaymentContexttypes and explicitTestLoggerimport align with the function signature changes throughout the file.Also applies to: 17879-17879
18704-18704: LGTM! Test code simplified with explicit import.The usage of
TestLogger::new()instead oftest_utils::TestLogger::new()is cleaner and follows from the explicit import in line 17879.Also applies to: 18759-18759
lightning/src/blinded_path/payment.rs (2)
92-110: LGTM: ReceiveAuthKey threaded through path construction.The addition of the
receive_auth_keyparameter toone_hopandnewmethods correctly threads the authentication key through the blinded payment path construction. The parameter is properly passed tocompute_payinfoandblinded_hops.Also applies to: 119-155
617-633: LGTM: Receive auth key correctly attached to final hop.The
blinded_hopsfunction correctly chains thelocal_node_receive_keyonly with the payee node (line 622), ensuring the authentication key is only applied to the final receiving hop as intended.lightning/src/offers/flow.rs (2)
322-356: LGTM: Blinded payment path creation updated correctly.The
create_blinded_payment_pathsmethod (and its test variant) correctly:
- Retrieves
receive_auth_keyfromself.receive_auth_key(line 331)- Constructs
ReceiveTlvswith all required fields (lines 342-346)- Passes the auth key to the router's
create_blinded_payment_pathsmethod (line 350)Also applies to: 361-377
812-867: LGTM: Consistent use of ReceiveAuthKey across offer and invoice methods.All methods that create blinded payment paths (
create_static_invoice_builder,create_response_for_invoice_request, async payment methods) consistently:
- Use
ReceiveTlvsinstead of the deprecatedUnauthenticatedReceiveTlvs- Thread
receive_auth_keythrough to the router- Construct payment contexts correctly
Also applies to: 941-1017, 1323-1345, 1398-1456, 1614-1661
fuzz/src/chanmon_consistency.rs (1)
130-136: LGTM: FuzzRouter signature updated to match Router trait.The
create_blinded_payment_pathssignature correctly adds the_receive_auth_key: ReceiveAuthKeyparameter and updates_tlvsto useReceiveTlvs, matching the updated Router trait definition. Since this is a fuzzing stub that callsunreachable!(), no implementation changes are needed.lightning/src/ln/async_payments_tests.rs (1)
1861-1863: Updated TLV access path looks correctUsing tlvs.payment_constraints.max_cltv_expiry matches the new ReceiveTlvs shape. LGTM.
lightning/src/util/test_utils.rs (1)
290-304: Router shim correctly forwards new receive_auth_key parameterSignature and forwarding to self.router.create_blinded_payment_paths(...) match the updated trait/API. LGTM.
fuzz/src/full_stack.rs (1)
158-165: LGTM!The updated signature correctly threads
ReceiveAuthKeythrough the Router trait for fuzzing. The stub appropriately callsunreachable!()as this code path should not execute in fuzzing scenarios.lightning/src/ln/blinded_payment_tests.rs (3)
78-96: LGTM!The migration from
UnauthenticatedReceiveTlvstoReceiveTlvsand integration ofReceiveAuthKeyis implemented correctly. The authentication key is properly retrieved from the keys manager and threaded through the path construction.
163-178: LGTM!Test correctly updated to use the new authentication mechanism with
ReceiveTlvsandReceiveAuthKey.
1325-1340: LGTM!Test updates consistently apply the new authentication mechanism across multiple test cases. The migration from
UnauthenticatedReceiveTlvstoReceiveTlvswith properReceiveAuthKeyhandling is correctly implemented.Also applies to: 1379-1394, 2011-2026
lightning/src/routing/router.rs (4)
31-31: Import of ReceiveAuthKey looks correctUsed in new signatures; no issues.
202-205: Propagating receive_auth_key to BlindedPaymentPath::newThreading the key through path construction looks correct.
214-216: Fallback one-hop path uses receive_auth_key — OKConsistent with above call.
248-251: FixedRouter signature updated — OKUnused params properly underscored; debug_assert + Err keeps release safe.
| create_recv_pending_htlc_info(decoded_hop, shared_secret, msg.payment_hash, | ||
| msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat, | ||
| current_height) | ||
| current_height, &*self.logger) |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Parameter threading is consistent.
The addition of logger parameters to create_recv_pending_htlc_info and signer/router parameters to create_response_for_invoice_request is applied consistently across all visible call sites.
To ensure all call sites have been updated, run the following verification script:
Also applies to: 6995-6995, 14815-14815, 18726-18726, 18749-18749, 18775-18775
🏁 Script executed:
#!/bin/bash
# Description: Verify all call sites of modified functions have been updated with new parameters.
echo "=== Checking create_recv_pending_htlc_info call sites ==="
# Search for calls without the logger parameter (old signature)
# Expected: no matches with old signature pattern
rg -n "create_recv_pending_htlc_info\(" --type rust -A 2 | rg -v "&.*logger"
echo ""
echo "=== Checking create_response_for_invoice_request call sites ==="
# Search for calls to verify all include node_signer and router
rg -n "create_response_for_invoice_request\(" --type rust -C 2Length of output: 2079
Add missing logger argument to create_recv_pending_htlc_info calls
Several call sites still use the old signature; update them to include the new logger parameter:
- lightning/src/ln/onion_payment.rs:500–502
- lightning/src/ln/channelmanager.rs:4882–4884
- lightning/src/ln/channelmanager.rs:6985–6987
- lightning/src/ln/channelmanager.rs:18724–18726
- lightning/src/ln/channelmanager.rs:18747–18748
- lightning/src/ln/channelmanager.rs:18762–18764
Ensure production code passes &*self.logger and tests pass &logger.
🤖 Prompt for AI Agents
In lightning/src/ln/channelmanager.rs around line 4884 (and also update the
other listed call sites: lightning/src/ln/onion_payment.rs:500–502,
lightning/src/ln/channelmanager.rs:4882–4884, 6985–6987, 18724–18726,
18747–18748, 18762–18764), the calls to create_recv_pending_htlc_info are
missing the new logger parameter; fix each call by appending the logger argument
— in production code pass &*self.logger and in unit/test code pass &logger — so
every create_recv_pending_htlc_info invocation has the additional logger
parameter and compiles/tests pass.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests