Review: Introduce Custom Tlvs for Bolt11 and Bolt12 Payers #9
Conversation
Custom TLVs allow the payer, to add additional custom data to the onion packet, for the payee, which can be used from anywhere between description to custom authentication. While the user had ability to set custom_tlvs using send_payment API, it was not possible from the simpler pay_for_bolt11_invoice API. This commit fixes that by introducing custom_tlvs in it. The next commit will do the same for Bol12 payment flow too.
|
Warning Rate limit exceeded@shaavan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughAdded a new Changes
Sequence DiagramsequenceDiagram
participant Caller as Application / Test
participant CM as ChannelManager
participant Store as PendingOutboundPaymentStore
participant RON as RecipientOnionFields
participant Sender as Payment Sender
Caller->>CM: pay_for_bolt11_invoice(..., custom_tlvs)
CM->>Store: add_new_awaiting_invoice(..., custom_tlvs)
Store->>Store: persist AwaitingInvoice{..., custom_tlvs}
Note right of Store: Invoice arrives
Caller->>CM: mark_invoice_received_and_get_details(...)
CM->>Store: transition AwaitingInvoice -> InvoiceReceived (carry custom_tlvs)
Store->>RON: RecipientOnionFields::with_custom_tlvs(custom_tlvs)
alt invalid TLVs
RON-->>Store: Err(InvalidCustomTlvs)
Store-->>CM: SendingFailed(InvalidCustomTlvs)
CM-->>Caller: notify failure
else valid TLVs
RON->>Sender: include TLVs in onion packet
Sender->>Sender: send payment
Sender-->>Caller: payment success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 full review |
✅ Actions performedFull review triggered. |
![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
♻️ Duplicate comments (3)
lightning/src/ln/channelmanager.rs (3)
2378-2380: Consistent TLV example across documentation.The same TLV number concern applies here as noted in the previous comment on lines 2275-2277.
Also applies to: 2438-2440
12881-12900: Breaking API change is consistent.The addition of
custom_tlvsparameter maintains consistency with other payment API changes. The same TLV validation and call site concerns apply as noted in the previous comment on lines 5552-5566.
12923-12944: Consistent API extension.This router-based variant maintains consistency with
create_refund_builder. The same validation concerns apply.
🧹 Nitpick comments (3)
lightning/src/ln/outbound_payment.rs (1)
746-787: Custom TLV validation logic is solid; consider tightening docs/tests
RecipientOnionFields::with_custom_tlvscorrectly enforces:
- TLV types ≥ 2^16 (custom range),
- exclusion of standardized experimental types (keysend 5482373484 and 77_777),
- strict increasing order with
sort_unstable_by_keyplus aprev_typecheck, and- uniqueness (duplicates rejected via
prev >= *typ).The new unit test exercises invalid low-range types and keysend, and a valid increasing set, which covers the main paths.
Two minor polish suggestions:
- Update the doc comment to also mention the 77_777 reservation, since it’s actively rejected.
- Optionally add a test that verifies 77_777 is rejected, and one that confirms duplicates or unsorted input are rejected, to lock in the invariants.
Also applies to: 2858-2878
lightning/src/ln/async_payments_tests.rs (1)
550-553: LGTM! API signature updates are correct.All test call sites have been properly updated to accommodate the new
custom_tlvsparameter inpay_for_offer. The empty vectors passed here maintain existing test behavior while allowing the API to accept custom TLVs when needed.As a future enhancement, consider adding dedicated test cases that exercise the custom TLVs functionality by passing non-empty vectors to verify end-to-end propagation of custom TLV data through the payment flow.
Also applies to: 711-714, 786-789, 918-921, 1019-1022, 1128-1131, 1191-1194, 1277-1280, 1374-1377, 1475-1478, 1721-1724, 1818-1821, 1936-1936, 2380-2383, 2715-2718, 2780-2783, 2884-2887
lightning/src/ln/bolt11_payment_tests.rs (1)
58-58: LGTM! API signature updates are correct.All test call sites have been properly updated to accommodate the new
custom_tlvsparameter inpay_for_bolt11_invoice. The empty vectors maintain existing test behavior.Consider adding test coverage for custom TLVs in BOLT11 payments to complement the existing payment metadata tests, ensuring the feature works end-to-end for both invoice types.
Also applies to: 72-72, 128-128, 142-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lightning-liquidity/tests/lsps2_integration_tests.rs(4 hunks)lightning/src/ln/async_payments_tests.rs(17 hunks)lightning/src/ln/bolt11_payment_tests.rs(4 hunks)lightning/src/ln/channelmanager.rs(12 hunks)lightning/src/ln/invoice_utils.rs(1 hunks)lightning/src/ln/max_payment_path_len_tests.rs(1 hunks)lightning/src/ln/offers_tests.rs(38 hunks)lightning/src/ln/outbound_payment.rs(21 hunks)lightning/src/ln/payment_tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T12:47:06.857Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/invoice_utils.rs
📚 Learning: 2025-11-28T12:48:22.008Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/invoice_utils.rslightning/src/ln/outbound_payment.rs
🧬 Code graph analysis (3)
lightning/src/ln/channelmanager.rs (1)
lightning/src/ln/outbound_payment.rs (2)
custom_tlvs(784-786)custom_tlvs(799-801)
lightning/src/ln/async_payments_tests.rs (1)
lightning/src/offers/signer.rs (1)
default(197-199)
lightning/src/ln/offers_tests.rs (1)
lightning/src/ln/channelmanager.rs (3)
default(704-713)default(12771-12779)pay_for_offer(13056-13082)
🪛 GitHub Actions: SemVer checks
lightning/src/ln/outbound_payment.rs
[error] 2059-2059: E0433: failed to resolve: use of unresolved module or unlinked crate std while compiling outbound_payment.rs. The code uses std::mem::take (or std::mem), but std is not resolved in this context.
🔇 Additional comments (16)
lightning-liquidity/tests/lsps2_integration_tests.rs (1)
1218-1227: Bolt11 payer callsites correctly updated for custom TLVs parameterThe added
vec![]argument matches the newpay_for_bolt11_invoicesignature and keeps these LSPS2 integration tests behaviorally identical while wiring through the new custom-TLVs parameter. No issues seen.Also applies to: 1692-1701, 1884-1893, 2222-2231
lightning/src/ln/invoice_utils.rs (1)
710-721: Test call updated correctly for new pay_for_bolt11_invoice arityThe added
vec![]argument aligns this test with the newpay_for_bolt11_invoicesignature without changing its behavior. Looks good.lightning/src/ln/outbound_payment.rs (7)
81-99: Custom TLVs added to payment lifecycle and TLV schema look consistentAdding
custom_tlvs: Vec<(u64, Vec<u8>)>toPendingOutboundPayment::AwaitingInvoiceand::InvoiceReceivedand wiring them into the TLV schema ((9, custom_tlvs, optional_vec)/(7, custom_tlvs, optional_vec)) matches the existing encoding style and keeps upgrade semantics intact. This lets HRN/BOLT12 flows preserve user-specified TLVs across state transitions and persistence. No functional issues spotted here.Also applies to: 2768-2786
520-550: New RetryableSendFailure::InvalidCustomTlvs variant is well-scopedIntroducing
RetryableSendFailure::InvalidCustomTlvsto specifically represent invalidRecipientOnionFields::custom_tlvsis a clean separation fromOnionPacketSizeExceeded. It correctly documents that the error is about invalid ranges/values (as opposed to size) and is only used on early validation paths, so it remains cheap to handle and reason about.
923-969: Bolt11 payer plumbing for custom TLVs is correctly integratedThe updated
pay_for_bolt11_invoice:
- Extends the API with
custom_tlvs: Vec<(u64, Vec<u8>)>at a sensible position.- Builds
RecipientOnionFields::secret_only(*invoice.payment_secret())and applies.with_custom_tlvs(custom_tlvs), enforcing type range and ordering before any routing work.- Maps validation failure to
Bolt11PaymentError::SendingFailed(RetryableSendFailure::InvalidCustomTlvs).- Continues to populate
payment_metadatafrom the invoice and reuses the existing routing and retry logic unchanged.This is a clean, early-fail integration point for user-specified TLVs and keeps retry semantics intact.
1067-1075: Mapping InvalidCustomTlvs to PaymentFailureReason::UnexpectedError is acceptableIn the BOLT12 path, adding
RetryableSendFailure::InvalidCustomTlvsto the error-to-PaymentFailureReasonmapping (asUnexpectedError) keeps the match exhaustive. Given that BOLT12 currently constructsRecipientOnionFieldswith an emptycustom_tlvsset in this module, this case shouldn’t be hit here, but having a conservative fallback reason is fine.If other call sites start feeding custom TLVs into BOLT12 routes, double-check that this mapping (UnexpectedError vs. a more specific reason) matches your desired UX.
1984-2033: Custom TLVs correctly propagated into AwaitingInvoice from offersThe changes to:
received_offer(&self, payment_id, custom_tlvs, retryable_invoice_request)andadd_new_awaiting_invoice(&self, payment_id, custom_tlvs, ...)ensure that user-specified
custom_tlvsare stored inPendingOutboundPayment::AwaitingInvoiceat both the initial creation and the AwaitingOffer→AwaitingInvoice transition. This keeps state consistent for later invoice handling and matches the new enum fields and TLV schema.
2858-2878: RecipientOnionFields custom TLV test covers main invalid/valid paths
test_recipient_onion_fields_with_custom_tlvs:
- Confirms types below 2^16 are rejected.
- Confirms the reserved keysend type is rejected.
- Confirms a couple of valid, in-range, strictly increasing types are accepted.
This is a useful sanity check for
with_custom_tlvsand should prevent regressions in the validation logic.
3055-3160: add_new_awaiting_invoice callsites updated correctlyAll test helpers that create
AwaitingInvoicepayments now pass an explicitvec![]forcustom_tlvs, which matches the newadd_new_awaiting_invoicesignature. Behavior remains unchanged for these tests, since they don’t exercise custom TLVs.lightning/src/ln/payment_tests.rs (1)
5403-5406: Custom TLVs parameter correctly threaded with empty vectorAdapting
pay_for_bolt11_invoiceto the newcustom_tlvsparameter viavec![]preserves the existing test semantics (no custom TLVs) and matches the expected argument order and types.lightning/src/ln/max_payment_path_len_tests.rs (1)
522-528: pay_for_offer updated to new custom_tlvs API with no behavior changeThe extra
vec![]argument aligns this call with the updatedpay_for_offersignature, explicitly specifying “no custom TLVs” while leaving the test’s behavior unchanged.lightning/src/ln/offers_tests.rs (1)
309-309: LGTM! API updates correctly propagated through tests.All test calls to
create_refund_builder,create_refund_builder_using_router, andpay_for_offerhave been consistently updated with the newcustom_tlvsparameter. Passing empty vectors (vec![]) is appropriate for tests that don't specifically exercise custom TLV functionality.Also applies to: 476-476, 498-498, 574-574, 605-605, 665-665, 761-761, 830-830, 889-889, 951-951, 998-998, 1078-1078, 1152-1152, 1211-1211, 1283-1283, 1380-1380, 1438-1438, 1521-1521, 1546-1546, 1622-1622, 1638-1638, 1717-1717, 1751-1751, 1822-1822, 1833-1833, 1874-1874, 1885-1885, 1920-1920, 1942-1942, 1977-1977, 2010-2010, 2013-2013, 2034-2034, 2040-2040, 2092-2092, 2159-2159, 2208-2208, 2300-2300, 2384-2384, 2469-2469
lightning/src/ln/channelmanager.rs (4)
13056-13141: Consistent API extensions across offer payment functions.All three offer payment functions (
pay_for_offer,pay_for_offer_from_hrn,pay_for_offer_with_quantity) have been updated consistently with the newcustom_tlvsparameter. The implementation correctly passes the parameter through the closure toadd_new_awaiting_invoice.However, the same TLV validation and call site verification concerns noted in earlier comments apply to these functions as well.
15789-15794: Appropriate use of empty custom TLVs for internal flow.The empty
vec![]passed toreceived_offeris correct for this internal automated payment flow where no custom TLVs are needed.
5552-5566: All TLV validation concerns have been addressed.The
custom_tlvsparameter is properly validated viaRecipientOnionFields::with_custom_tlvs(), which enforces: TLV type range (>= 65536), rejection of reserved TLVs, sorting by type number, and uniqueness. Size limits are enforced by the 1300-byte onion packet constraint with comprehensive test coverage inmax_payment_path_len_tests.rs. All existing call sites across the codebase have been updated to provide the parameter.
2275-2277: The documentation example uses an invalid TLV type number that will fail validation.The example uses
343493u64for the TLV type, but the validation inRecipientOnionFields::with_custom_tlvs()requires all custom TLV type numbers to be >= 65536 (enforced by the checkif *typ < 1 << 16). Since 343493 < 65536, this will cause validation to fail when invoked.Replace the TLV type with a valid number >= 65536, such as
65537u64:/// # let custom_tlvs = vec![ /// # (65537u64, b"hello".to_vec()), /// # ];⛔ Skipped due to learnings
Learnt from: shaavan Repo: shaavan/rust-lightning PR: 8 File: lightning/src/ln/channelmanager.rs:15488-15508 Timestamp: 2025-11-28T12:48:22.008Z Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.Learnt from: shaavan Repo: shaavan/rust-lightning PR: 8 File: lightning/src/ln/channelmanager.rs:15468-15486 Timestamp: 2025-11-28T12:47:06.857Z Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
shaavan
force-pushed
the
payment-custom-review
branch
from
ad73e34 to
bc0fd53
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
lightning/src/ln/outbound_payment.rs (4)
520-550:RetryableSendFailure::InvalidCustomTlvssemantics are reasonable but docs could be clearerThe new variant cleanly separates “invalid TLV values/ranges” from “too large onion” (
OnionPacketSizeExceeded), which is useful. The doc string “are of invalid range” is a bit vague; consider clarifying that it covers TLV type-number validity and reserved-type usage (e.g., keysend, async-invoice types), not size.
1150-1212: Static-invoice path uses emptyRecipientOnionFields; no custom TLVs yetFor static-invoice async payments,
static_invoice_receivedstill callsonion_utils::set_max_path_lengthwithRecipientOnionFields::spontaneous_empty()and latersend_payment_for_static_invoicealso usesRecipientOnionFields::spontaneous_empty().If the intent of this PR is “custom TLVs for BOLT11 and BOLT12 payers” in general, you might eventually want to thread
custom_tlvsinto theStaticInvoiceReceivedflow as well (including its state variant and serialization), so async/static BOLT12 payments can carry the same TLVs.If static invoices are intentionally excluded from custom TLVs for now, this is fine as-is, but it may be worth documenting that distinction.
2052-2085:mark_invoice_received_and_get_detailsis correct but does an extra cloneGood points:
- It moves
custom_tlvsout ofAwaitingInvoicewithcore::mem::take(custom_tlvs)(no-std friendly).- It constructs an
InvoiceReceivedstate with the samecustom_tlvs.- It returns
(payment_hash, retry, config, custom_tlvs_vec, is_newly_marked)for callers that need to build onion fields.Minor nit: you currently
takeintocustom, thencustom.clone()into theInvoiceReceivedvariant and returncustom—this doubles the allocation. Given TLV lists are usually tiny, this is low impact, but you could:
- Move
custominto the new state, and- Clone only for the returned value (or vice versa),
to avoid one copy.
Not urgent, just a potential micro-optimization.
2861-2882: Custom TLV unit test covers key validation cases
test_recipient_onion_fields_with_custom_tlvs:
- Verifies types < 2^16 are rejected.
- Verifies keysend’s experimental type and your reserved async-invoice type are rejected.
- Verifies a minimal valid set (≥2^16, strictly increasing) succeeds.
That gives good coverage of the most important invariants. If you want, you could add a small case for duplicate or unsorted types being rejected, but not strictly necessary.
lightning/src/ln/offers_tests.rs (1)
173-183: Offer/refund path tests still only exercise emptycustom_tlvs; consider adding a non-empty caseAll updated tests call the high-level APIs (
pay_for_offer,create_refund_builder{_using_router}) withvec![]ascustom_tlvs. That’s fine for existing behavior, but it means we don’t yet have an end-to-end test that:
- Calls
pay_for_offeror the refund builders with non-empty custom TLVs, and- Asserts that those TLVs make it into the onion / recipient context as expected.
Given this PR’s goal, you may eventually want a dedicated Offers test that round-trips a small custom TLV set through the full BOLT12 offer/refund flow.
lightning/src/ln/channelmanager.rs (1)
2275-2277: Good example, but consider documenting TLV type selection guidelines.The documentation example correctly uses an odd TLV type (343493), which allows nodes to safely ignore it if not understood. However, consider adding a comment or documentation explaining:
- Use odd TLV types (e.g., 343493) for custom/experimental TLVs that can be ignored
- Avoid even TLV types unless all nodes must understand them
- Check BOLT specifications for reserved TLV ranges
Example improvement:
# let custom_tlvs = vec![ -# (343493u64, b"hello".to_vec()), +# (343493u64, b"hello".to_vec()), // Use odd TLV types for optional custom data # ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lightning/src/ln/async_payments_tests.rs(17 hunks)lightning/src/ln/channelmanager.rs(12 hunks)lightning/src/ln/max_payment_path_len_tests.rs(1 hunks)lightning/src/ln/offers_tests.rs(38 hunks)lightning/src/ln/outbound_payment.rs(26 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/async_payments_tests.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T12:47:06.857Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/outbound_payment.rs
📚 Learning: 2025-11-28T12:48:22.008Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/outbound_payment.rs
🧬 Code graph analysis (1)
lightning/src/ln/offers_tests.rs (1)
lightning/src/ln/channelmanager.rs (1)
pay_for_offer(13056-13082)
⏰ 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, stable)
- GitHub Check: linting
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: check_release
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
- GitHub Check: check_release
- GitHub Check: build-tx-sync (macos-latest, stable)
- GitHub Check: linting
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
- GitHub Check: semver-checks
🔇 Additional comments (31)
lightning/src/ln/outbound_payment.rs (13)
80-98: Threadingcustom_tlvsthrough AwaitingInvoice/InvoiceReceived looks consistentAdding
custom_tlvs: Vec<(u64, Vec<u8>)>toPendingOutboundPayment::AwaitingInvoiceand::InvoiceReceivedplus wiring it intocreate_pending_paymentand retry logic correctly preserves payer-provided TLVs across the BOLT12 async invoice lifecycle. I don’t see missing state transitions or obvious ownership bugs here.
685-801:RecipientOnionFields::with_custom_tlvsvalidation and invariants look solidThe function:
- Enforces custom TLV type numbers ≥ 2^16 and rejects reserved types (keysend and async-invoice request).
- Sorts by type and then enforces strict monotonic increase, preventing duplicates or unsorted inputs.
- Ensures
custom_tlvsstored inRecipientOnionFieldsalways satisfy the “strictly increasing types” invariant expected by comments andcheck_merge.This matches the documented behavior and should integrate cleanly with onion serialization and MPP merging.
811-826: MPP merge behavior for custom TLVs is consistent with spec expectations
check_mergeenforces equality ofpayment_secretandpayment_metadataand then:
- Requires all even-typed custom TLVs to be identical across parts.
- Intersects the full custom TLV sets, so odd-typed TLVs present in only some parts are dropped.
Given
with_custom_tlvsguarantees sorted, unique TLVs, this logic is deterministic and spec-compliant. No changes needed here.
923-969: BOLT11pay_for_bolt11_invoicecustom TLVs path is correct and fail-fastThe new
custom_tlvs: Vec<(u64, Vec<u8>)>parameter is:
- Passed into
RecipientOnionFields::secret_only(*invoice.payment_secret()).- Validated via
.with_custom_tlvs, mapping any error toRetryableSendFailure::InvalidCustomTlvswrapped inBolt11PaymentError::SendingFailed.Because this occurs before adding a pending outbound payment, invalid TLVs never enter state, and callers can fix parameters and retry. This is a good separation between parameter validation and routing.
1023-1076: HandlingInvalidCustomTlvsin BOLT12 internal route-finding is future-proofThe added arm:
RetryableSendFailure::InvalidCustomTlvs => PaymentFailureReason::UnexpectedError,ensures any future
InvalidCustomTlvserrors bubbling up from route construction will translate into a concretePaymentFailureReasonand causeabandon_payment. Todayfind_initial_routedoesn’t emit this variant, but having the match arm is harmless and keeps the code exhaustive.
1890-1950: Propagating custom TLVs into Retryable state is correctly wired
add_new_pending_paymentandcreate_pending_paymentnow:
- Take
recipient_onion: RecipientOnionFieldsby value.- Copy
recipient_onion.custom_tlvsinto thePendingOutboundPayment::Retryable { custom_tlvs, .. }field.- Use this stored
custom_tlvslater when reconstructingRecipientOnionFieldsfor retries.This ensures retries and startup recovery preserve the original custom TLVs, which is exactly what we want.
1952-2004: AwaitingOffer/Received-offer path now threadscustom_tlvsinto AwaitingInvoiceThe
received_offermethod’s newcustom_tlvsargument is inserted into theAwaitingInvoicestate, andadd_new_awaiting_invoicelikewise now storescustom_tlvs. This is consistent with howpay_for_offerinchannelmanager.rspasses TLVs intoadd_new_awaiting_invoice, and it makes the offer → invoice flow carry TLVs end-to-end.
2013-2035:add_new_awaiting_invoicesignature change is consistent across call sitesThe new parameter order:
add_new_awaiting_invoice( payment_id, custom_tlvs, expiration, retry_strategy, route_params_config, retryable_invoice_request, )matches how
pay_for_offerconstructs it (payment_id, custom_tlvs, expiry, retry, config, invreq). The internal state setup plusawaiting_invoiceflag logic are unchanged except for storingcustom_tlvs, which is correct.
2040-2049:mark_invoice_receivedwrapper preserves DuplicateInvoice semanticsRefactoring
mark_invoice_receivedto callmark_invoice_received_and_get_detailsand interpret the finalboolas “was this the first time we marked it?” keeps previous behavior:
- First invoice → Ok(()).
- Duplicate invoice for same payment →
Bolt12PaymentError::DuplicateInvoice.This also ensures the new custom-TLV plumbing doesn’t leak into the public helper.
1593-1637: Retry path reconstructs onion fields with cloned custom TLVs as intendedIn
find_route_and_send_payment, forPendingOutboundPayment::Retryable { .. }you:
- Rebuild
RecipientOnionFieldsfrom storedpayment_secret,payment_metadata, andcustom_tlvs.clone().- Generate new per-path session privs and insert them into the payment.
This ensures retries use exactly the same per-recipient onion payload (including custom TLVs) while only changing the routing. Looks correct and consistent with the initial send.
2708-2790: Serialization changes for AwaitingInvoice/InvoiceReceived are compatibleAdding:
(9, custom_tlvs, optional_vec)to theAwaitingInvoiceTLV layout, and(7, custom_tlvs, optional_vec)to theInvoiceReceivedlayout,as
optional_vecfields means:
- Older serialized data (without those TLVs) will deserialize with an empty vec.
- Newer nodes write the field without breaking older ones that ignore unknown TLVs within these variants.
Together with the existing
Retryable.custom_tlvsTLV, this gives a coherent on-disk representation of custom TLVs throughout the payment lifecycle.
3067-3163: Test updates foradd_new_awaiting_invoice’s new parameter are correctThe various tests (
removes_stale_awaiting_invoice_*,removes_abandoned_awaiting_invoice, BOLT12 invoice expiry/route failure tests) now call:add_new_awaiting_invoice( payment_id, vec![], expiration, Retry::Attempts(0), RouteParametersConfig::default(), None, )This keeps behavior identical while satisfying the new signature. No issues here.
991-1019: Potentially stuck BOLT12 payment when custom TLVs are invalidIn
send_payment_for_bolt12_invoice:
- You transition the payment from
AwaitingInvoicetoInvoiceReceivedviamark_invoice_received_and_get_details.- Then you build a
RecipientOnionFields::spontaneous_empty().with_custom_tlvs(custom_tlvs).- If
with_custom_tlvsfails, you returnBolt12PaymentError::SendingFailed(RetryableSendFailure::InvalidCustomTlvs)without callingabandon_payment.Given:
InvoiceReceivedis treated as "pre-HTLC-lock-in" (is_pre_htlc_lock_inreturns true),remove_stale_paymentsnever prunesInvoiceReceived,- and there is no immediate
PaymentFailedevent here,you can end up with a permanent
InvoiceReceivedentry forpayment_idif the user supplies invalid custom TLVs. Future attempts with the samePaymentIdmay behave oddly or be blocked.Consider treating this like other immediate-send failures (e.g., expired or route-not-found) by:
- Calling
self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events)before returning, and- Ensuring a corresponding
Event::PaymentFailedis emitted.This keeps behavior consistent with other
SendingFailedcases and prevents lingering state.lightning/src/ln/max_payment_path_len_tests.rs (3)
193-327: Custom-TLV size tests for one-hop blinded path are well constructedThe
one_hop_blinded_path_with_custom_tlvtest:
- Computes
max_custom_tlv_lenby subtracting serialized payload length (without custom TLV), TLV type/length overhead, payload varint overhead, and HMAC length from 1300 bytes.- Builds
RecipientOnionFields::spontaneous_empty().with_custom_tlvs(...)with that length and verifies:
- Payment from the introduction node succeeds.
- Adding one byte triggers
OnionPacketSizeExceededboth at onion construction and viasend_payment.- Shrinking the TLV allows a two-hop route.
This is a strong integration test for how custom TLVs interact with blinded paths and the onion size limit.
329-481: Multi-hop blinded path test correctly validates custom TLV impact on max path lengthThe
blinded_path_with_custom_tlvtest similarly:
- Uses
onion_utils::build_onion_payloadsto get the total reserved bytes without custom TLVs.- Derives
max_custom_tlv_lenfor a chosenCUSTOM_TLV_TYPE.- Asserts:
- Max-sized TLV works from the last unblinded hop.
- +1 byte causes
OnionPacketSizeExceededand direct onion construction failure.- Reducing TLV length by
INTERMED_PAYLOAD_LEN_ESTIMATEenables an extra unblinded hop.This nicely exercises both the sending API and the low-level onion builder for blinded routes with custom TLVs.
518-529: Updatedpay_for_offercall matches new signatureThe
bolt12_invoice_too_large_blinded_pathstest now calls:bob.node.pay_for_offer( &offer, Some(5000), payment_id, vec![], OptionalOfferPaymentParams::default(), )which aligns with the new
pay_for_offer(&Offer, Option<u64>, PaymentId, Vec<(u64, Vec<u8>)>, OptionalOfferPaymentParams)signature. Semantics of the test are unchanged (no custom TLVs in this scenario).lightning/src/ln/offers_tests.rs (12)
304-316:create_refund_with_no_blinded_pathupdated builder call is consistentThe test now calls:
create_refund_builder_using_router( &router, 10_000_000, absolute_expiry, payment_id, vec![], Retry::Attempts(0), RouteParametersConfig::default(), )which matches the new
(amount, expiry, payment_id, custom_tlvs, retry, config)ordering and keeps behavior identical (empty TLVs).
571-576: Short-lived refund builder now passes emptycustom_tlvsexplicitly
create_refund_builder(…, payment_id, vec![], Retry::Attempts(0), RouteParametersConfig::default())is consistent with the updated signature and preserves the original test intent.
603-607: Router-based refund builder update is correctly parameterizedThe long-lived refund test’s call to
create_refund_builder_using_routernow suppliesvec![]for custom TLVs, beforeRetryandRouteParametersConfig, which matches the new function signature.
760-772: Two-hop refund builder in blinded-path test aligns with new APIThe two-hop blinded refund test updates
create_refund_builderwithvec![]forcustom_tlvs. This is mechanically correct and doesn’t alter the functional assertions in the test.
888-900: One-hop refund builder test correctly insertscustom_tlvsparameterSame pattern here:
create_refund_builder(..., payment_id, vec![], Retry::Attempts(0), RouteParametersConfig::default())matches the new signature and preserves semantics.
995-1003: Refund without blinded paths: builder call updated correctlyThe “pays_for_refund_without_blinded_paths” test’s
create_refund_builderinvocation now passes avec![]custom_tlvsargument, in the expected position. The rest of the test logic remains unchanged.
1150-1155: Refund builder in multi-intro-node test matches new signatureThe refund in
send_invoice_for_refund_with_distinct_reply_pathis also updated to addvec![]forcustom_tlvs. This keeps the duplicate-introduction-node behavior test intact.
1716-1720: Refund builder in “fails_authentication_when_handling_invoice_for_refund” is consistentAgain, the extra
vec![]correctly occupies thecustom_tlvsslot; no change to the authentication failure semantics.
1940-1945: Unsupported-chain refund test correctly updated for new builder signatureThe refund created with
.chain(Network::Signet)now originates from a builder call that includesvec![]for custom TLVs; the UnsupportedChain assertion remains valid.
1873-1887: Refund builder in “fails_creating_refund_or_sending_invoice_without_connected_peers” is updated consistentlyBoth the failing
create_refund_builderand the succeeding one (after reconnection) includevec![]forcustom_tlvs, in the correct argument order. Behavior is unchanged.
2032-2041: Duplicate-payment-id refund test wired to new builder signatureThe duplicate-ID tests now call
create_refund_builder(..., payment_id, vec![], Retry::Attempts(0), RouteParametersConfig::default())in both the initial success and the expected-failure case. Signature usage is correct.
475-499:pay_for_offertest calls correctly adapted to newcustom_tlvsparameterAll occurrences like:
bob.node.pay_for_offer(&offer, None, payment_id, vec![], Default::default()) david.node.pay_for_offer(&offer, None, payment_id, vec![], Default::default()) nodes[0].node.pay_for_offer(&offer, Some(amt_msat), payment_id, vec![], Default::default())match the new API in
channelmanager.rs:pay_for_offer(&Offer, Option<u64>, PaymentId, Vec<(u64, Vec<u8>)>, OptionalOfferPaymentParams)and maintain the tests’ focus on path and authentication behavior rather than TLVs.
lightning/src/ln/channelmanager.rs (3)
13055-13067: LGTM - Consistent API extension across offer payment methods.The custom TLVs parameter is consistently integrated across all three offer payment methods:
pay_for_offerpay_for_offer_from_hrnpay_for_offer_with_quantityAll methods follow the same pattern: accept
custom_tlvsparameter and pass it through toadd_new_awaiting_invoice(). This consistency is good for API usability.Also applies to: 13085-13097, 13128-13141
15789-15794: Good backward compatibility handling.Passing an empty
vec![]forcustom_tlvsin internal code paths that don't yet utilize custom TLVs is appropriate. This maintains functionality while allowing the API to be extended consistently.
5552-5566: TLV validation is properly implemented in the payment flow.The
custom_tlvsparameter is validated inRecipientOnionFields::with_custom_tlvs()before being used inpay_for_bolt11_invoice(). The validation includes:
- Type range enforcement: rejects types < 2^16 to prevent reserved BOLT ranges
- Duplicate prevention: enforces strictly increasing type numbers
- Keysend exclusion: explicitly rejects the standardized keysend type (5482373484)
- Even TLV consistency: enforced at the HTLC consistency layer for multi-part payments
The implementation returns
InvalidCustomTlvserror on validation failure, and overall onion packet size is limited to 1300 bytes. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lightning/src/ln/max_payment_path_len_tests.rs (1)
520-528: Custom-TLV parameter wiring intopay_for_offerlooks correctPassing
vec![]as the newcustom_tlvsargument matches the updated API and keeps this test focused purely on oversized blinded paths, without altering behavior. The change is safe and semantically appropriate here.If you find yourself writing
vec![]at many call sites, consider a small helper (e.g.,NoCustomTlvs) or an overload that defaults this parameter to improve readability.lightning/src/ln/payment_tests.rs (1)
5403-5406: Adaptation topay_for_bolt11_invoice’s custom-TLV parameter is correctThe added
vec![]cleanly satisfies the newcustom_tlvsparameter without changing the semantics ofmax_out_mpp_path, which continues to test MPP path utilization rather than TLV handling.As with other call sites, you might later introduce a small helper or defaulting API to avoid repeating
vec![]everywhere when no custom TLVs are needed.lightning/src/ln/offers_tests.rs (1)
309-309: LGTM! Mechanical updates to test call sites.All test call sites correctly updated to include the new
custom_tlvsparameter (passed asvec![]). The empty vectors preserve existing test behavior while maintaining API compatibility with the new signature.Consider adding dedicated tests to verify custom TLV propagation through the Bolt12 payment flow—e.g., passing non-empty TLV data and asserting it appears in the expected invoice/payment structures.
Also applies to: 476-476, 498-498, 574-574, 605-605, 665-665, 761-761, 830-830, 889-889, 951-951, 998-998, 1078-1078, 1152-1152, 1211-1211, 1283-1283, 1380-1380, 1438-1438, 1521-1521, 1546-1546, 1622-1622, 1638-1638, 1717-1717, 1751-1751, 1822-1822, 1833-1833, 1874-1874, 1885-1885, 1920-1920, 1942-1942, 1977-1977, 2010-2010, 2013-2013, 2034-2034, 2040-2040, 2092-2092, 2159-2159, 2208-2208, 2300-2300, 2384-2384, 2469-2469
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lightning-liquidity/tests/lsps2_integration_tests.rs(4 hunks)lightning/src/ln/async_payments_tests.rs(17 hunks)lightning/src/ln/bolt11_payment_tests.rs(4 hunks)lightning/src/ln/channelmanager.rs(12 hunks)lightning/src/ln/invoice_utils.rs(1 hunks)lightning/src/ln/max_payment_path_len_tests.rs(1 hunks)lightning/src/ln/offers_tests.rs(38 hunks)lightning/src/ln/outbound_payment.rs(26 hunks)lightning/src/ln/payment_tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T12:47:06.857Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/payment_tests.rslightning/src/ln/invoice_utils.rslightning/src/ln/outbound_payment.rs
📚 Learning: 2025-11-28T12:48:22.008Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/invoice_utils.rslightning/src/ln/outbound_payment.rs
🧬 Code graph analysis (3)
lightning/src/ln/channelmanager.rs (1)
lightning/src/ln/outbound_payment.rs (2)
custom_tlvs(784-786)custom_tlvs(799-801)
lightning/src/ln/invoice_utils.rs (2)
lightning/src/ln/channel.rs (2)
None(11586-11586)None(14731-14731)lightning/src/ln/channelmanager.rs (2)
default(704-713)default(12771-12779)
lightning/src/ln/offers_tests.rs (1)
lightning/src/ln/channelmanager.rs (1)
pay_for_offer(13056-13082)
⏰ 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). (11)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
🔇 Additional comments (24)
lightning/src/ln/async_payments_tests.rs (1)
550-553: Updated pay_for_offer call sites correctly pass empty custom TLVsAll updated
pay_for_offerinvocations now supplyvec![]in the newcustom_tlvsparameter position while leaving payment IDs, amounts, and params/retry handling unchanged. This keeps existing test behavior (no custom TLVs) and matches the expanded API surface.Also applies to: 711-714, 786-789, 918-921, 1019-1022, 1128-1131, 1191-1194, 1277-1280, 1374-1377, 1475-1478, 1721-1724, 1818-1821, 1936-1936, 2380-2383, 2715-2718, 2884-2887
lightning-liquidity/tests/lsps2_integration_tests.rs (1)
1220-1227: pay_for_bolt11_invoice test calls aligned with new custom_tlvs parameterEach
pay_for_bolt11_invoiceuse now includes an explicitvec![]for the newcustom_tlvsargument, preserving prior behavior and keeping the rest of the parameters (IDs, options, retry) intact.Also applies to: 1694-1701, 1886-1893, 2224-2231
lightning/src/ln/invoice_utils.rs (1)
713-720: create_and_pay_for_bolt11_invoice test updated for custom_tlvs without behavior changeThe test’s
pay_for_bolt11_invoicecall now passesvec![]into the newcustom_tlvsslot while retaining the existing retry and default params, so it continues to verify the same flow with no custom TLVs.lightning/src/ln/bolt11_payment_tests.rs (1)
58-58: LGTM! Bolt11 test call sites correctly updated.All
pay_for_bolt11_invoicecalls properly updated with the newcustom_tlvsparameter (passed asvec![]). The changes maintain existing test coverage for payment metadata propagation.Also applies to: 72-72, 128-128, 142-142
lightning/src/ln/channelmanager.rs (10)
2275-2279: LGTM!The documentation example is clear and uses a valid odd TLV type number (343493), which correctly follows the BOLT specification for custom/optional TLVs.
2377-2382: LGTM!Documentation example is consistent with the
pay_for_bolt11_invoiceexample pattern.
2437-2443: LGTM!Documentation example is consistent with other API examples.
5551-5566: LGTM!The
custom_tlvsparameter is properly added to the function signature and propagated to the underlyingpending_outbound_payments.pay_for_bolt11_invoicecall.
12880-12900: LGTM!The
create_refund_builderfunction properly accepts and propagates thecustom_tlvsparameter toadd_new_awaiting_invoice.
12922-12944: LGTM!The
create_refund_builder_using_routerfunction mirrors thecreate_refund_builderimplementation forcustom_tlvshandling.
13055-13067: LGTM!The
custom_tlvsparameter is properly captured by the closure and moved intoadd_new_awaiting_invoice. The ownership semantics are correct.
13085-13097: LGTM!Consistent implementation with
pay_for_offer.
13128-13141: LGTM!Consistent implementation with other
pay_for_offervariants.
15789-15794: No custom_tlvs propagation needed for HRN-resolved offer payments.The hardcoded
vec![]is correct and intentional. HRN-based offer payments created through DNS resolution don't support custom TLVs at initiation time—they only store amount and payer note. Custom TLVs are only supported for direct offer payments viapay_for_offer,pay_for_offer_from_hrn, andpay_for_offer_with_quantity, which have them as explicit parameters separate fromOptionalOfferPaymentParams.lightning/src/ln/outbound_payment.rs (10)
80-98: LGTM! Custom TLVs field additions are well-integrated.The
custom_tlvsfield has been correctly added to bothAwaitingInvoiceandInvoiceReceivedvariants, allowing custom TLVs to be tracked through the payment state machine from invoice request to invoice receipt and payment initiation.
548-549: LGTM! New error variant appropriately handles custom TLV validation failures.The
InvalidCustomTlvsvariant correctly extendsRetryableSendFailureto surface validation errors fromRecipientOnionFields::with_custom_tlvs().
923-969: LGTM! Bolt11 payment correctly integrates custom TLVs with proper validation.The
pay_for_bolt11_invoicefunction now acceptscustom_tlvsand correctly:
- Validates them via
RecipientOnionFields::with_custom_tlvs()- Maps validation failures to
Bolt11PaymentError::SendingFailed(RetryableSendFailure::InvalidCustomTlvs)- Incorporates them into the payment flow
972-1020: LGTM! Bolt12 invoice payment correctly propagates custom TLVs from state.The function correctly:
- Retrieves
custom_tlvsfrommark_invoice_received_and_get_details- Validates and builds
RecipientOnionFieldswith error mapping- Passes the
recipient_onionwith custom TLVs to the internal payment function
1067-1076: LGTM! Error mapping for custom TLV validation is consistent.The
InvalidCustomTlvserror is correctly mapped toPaymentFailureReason::UnexpectedError, consistent with other validation failures likeOnionPacketSizeExceeded.
1236-1319: Static invoices intentionally omit custom TLV support.The
send_payment_for_static_invoicefunction usesRecipientOnionFields::spontaneous_empty()without custom TLVs. This appears intentional as theStaticInvoiceReceivedvariant (lines 103-115) does not have acustom_tlvsfield, unlikeAwaitingInvoiceandInvoiceReceivedwhich do support them.
1988-2037: LGTM! State transition functions correctly handle custom TLVs.Both
received_offerandadd_new_awaiting_invoiceproperly accept and storecustom_tlvsin theAwaitingInvoicestate, enabling custom TLVs to be tracked from the initial payment request through invoice receipt.
2051-2086: LGTM! State transition correctly usescore::mem::takeand handles custom TLVs.The implementation correctly:
- Uses
core::mem::take(custom_tlvs)to move TLVs fromAwaitingInvoice(line 2063)- Clones them for both storage in
InvoiceReceivedand the return value (lines 2068, 2071)- Returns custom TLVs from both newly-marked and already-marked invoice states
Note: The previous review issue regarding
std::mem::takevscore::mem::takehas been resolved correctly.
2720-2824: LGTM! TLV serialization correctly handles custom_tlvs with backward compatibility.The
custom_tlvsfield is serialized usingoptional_vecin both:
AwaitingInvoice(TLV type 9, line 2772)InvoiceReceived(TLV type 7, line 2789)Using
optional_vecensures backward compatibility with versions that don't have this field, and forward compatibility when reading older serialized data.
3058-3387: LGTM! Test updates appropriately pass empty custom TLVs vectors.All test calls to
add_new_awaiting_invoicecorrectly passvec![]forcustom_tlvs, which is appropriate for tests not specifically focused on custom TLV functionality. This maintains test focus while adapting to the new signature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lightning/src/ln/offers_tests.rs (2)
185-203: Consider refactoring to reduce code duplication.This helper is nearly identical to
route_bolt12_payment(lines 166-183), differing only in the custom TLVs parameter. Consider refactoring by:
- Adding an optional
custom_tlvsparameter to the existingroute_bolt12_paymenthelper, or- Creating a shared internal implementation that both functions call
This would reduce maintenance burden and prevent the two implementations from diverging.
</invoke_comment_end>
1371-1494: Consider verifying custom TLVs are actually received by the recipient.While this test validates that custom TLVs don't break the payment flow, it doesn't verify that the recipient actually receives the custom TLVs in the payment. Consider adding an assertion that checks the custom TLVs are present in the
PaymentClaimableevent orRecipientOnionFields.Additionally, this test duplicates significant code from
pays_bolt12_invoice_asynchronously(lines 1279-1369). Consider extracting a shared test helper that accepts an optionalcustom_tlvsparameter to reduce duplication.</invoke_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lightning/src/ln/offers_tests.rs(40 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lightning/src/ln/offers_tests.rs (4)
lightning/src/ln/outbound_payment.rs (4)
custom_tlvs(784-786)custom_tlvs(799-801)new(473-479)new(858-876)lightning/src/offers/invoice_request.rs (2)
amount_msats(1145-1156)amount_msats(1199-1201)lightning/src/offers/flow.rs (1)
new(116-147)lightning/src/offers/offer.rs (12)
offer(1892-1892)offer(1903-1903)offer(1982-1982)offer(1991-1991)offer(2033-2033)offer(2049-2049)offer(2058-2058)offer(2073-2073)offer(2079-2079)offer(2087-2087)offer(2095-2095)offer(2103-2103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
- GitHub Check: benchmark
- GitHub Check: linting
- GitHub Check: build-tx-sync (macos-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: check_release
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: semver-checks
- GitHub Check: semver-checks
🔇 Additional comments (1)
lightning/src/ln/offers_tests.rs (1)
329-329: LGTM: Test updates correctly migrate to new API signature.All test call sites have been properly updated to pass
vec![]for the newcustom_tlvsparameter, maintaining existing behavior. The migration is consistent and correct across all affected functions (pay_for_offer,create_refund_builder,create_refund_builder_using_router).</invoke_comment_end>
Also applies to: 496-496, 518-518, 594-594, 625-625, 685-685, 781-781, 850-850, 909-909, 971-971, 1018-1018, 1098-1098, 1172-1172, 1231-1231, 1303-1303, 1525-1525, 1583-1583, 1666-1666, 1691-1691, 1767-1767, 1783-1783, 1862-1862, 1896-1896, 1967-1967, 1978-1978, 2019-2019, 2030-2030, 2065-2065, 2087-2087, 2122-2122, 2155-2155, 2158-2158, 2179-2179, 2185-2185, 2237-2237, 2304-2304, 2353-2353, 2445-2445, 2529-2529, 2614-2614
Bolt11 payments let the payer attach arbitrary custom TLVs in the onion packet, allowing metadata ranging from descriptions to custom authentication. Bolt12 currently lacks this capability. This commit introduces a `custom_tlvs` field in the Bolt12-related PendingOutboundPayment variants, enabling the same extensibility with minimal changes. A follow-up commit will add the user-facing API to set these TLVs.
shaavan
force-pushed
the
payment-custom-review
branch
from
b0143d7 to
a527304
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/channelmanager.rs (1)
12880-12945: Update offer builder functions to match refund builder API.The
create_offer_builderandcreate_offer_builder_using_routerfunctions (lines 12806 and 12825) lack thecustom_tlvs,retry_strategy, androute_params_configparameters present in the corresponding refund builders. Additionally, the offer builders do not calladd_new_awaiting_invoiceto track the payment. These functions should be updated consistently with the refund builder changes to support custom TLVs and proper payment parameter configuration.
🧹 Nitpick comments (5)
lightning/src/ln/outbound_payment.rs (3)
520-550: NewRetryableSendFailure::InvalidCustomTlvsis well-scoped but error semantics are genericIntroducing
InvalidCustomTlvsas its ownRetryableSendFailurevariant is a good separation from routing/expiry issues, and its documentation clearly ties it toRecipientOnionFields::custom_tlvs. However, downstream mapping currently treats it asPaymentFailureReason::UnexpectedError, which may be a bit opaque from an API consumer standpoint; consider a more specificPaymentFailureReason(or expanding docs) in a follow-up to surface “caller constructed invalid custom TLVs” more directly.
1288-1305: Static-invoice send currently omits custom TLVs by design
send_payment_for_static_invoicecontinues to useRecipientOnionFields::spontaneous_empty()and passes that intosend_payment_for_bolt12_invoice_internalwithout anycustom_tlvs. This is consistent with the async static-invoice flow, which does not expose a public surface for custom TLVs yet. If you later extend static invoices to support caller-specified custom TLVs, this is the place to thread them through.
2861-2882: Newtest_recipient_onion_fields_with_custom_tlvsprovides good boundary coverageThe test exercises:
- TLVs below the custom range (0, 1) → error.
- Forbidden experimental types (keysend and 77_777) → error.
- Valid sorted high-range types → success.
One optional improvement is to add a case with duplicate type numbers to confirm the uniqueness check via
sort_unstable_by_key+prev >= *typbehaves as intended, but the current coverage already hits the most important boundaries.lightning/src/ln/offers_tests.rs (2)
185-203: Helper correctly threads custom TLVs through PassAlongPathArgsThis mirrors
route_bolt12_paymentand just adds.with_custom_tlvs(custom_tlvs), so behavior stays consistent while exercising the new TLV plumbing. If you ever want to avoid the extraclone()s at call sites, you could take animpl Into<Vec<(u64, Vec<u8>)>>or a slice and clone internally, but that’s purely a test‑only micro-optimization.
1371-1494: New async Bolt12 custom‑TLV test is sound; consider asserting TLV propagation explicitlyThe flow here closely tracks
pays_bolt12_invoice_asynchronously, withcustom_tlvsthreaded through:
pay_for_offerwhen initiating the offer payment,route_bolt12_payment_with_custom_tlvsfor the simulated HTLC forward,ClaimAlongRouteArgs::with_custom_tlvs(custom_tlvs)on fulfillment.The
InvoiceReceivedidempotency andsend_payment_for_bolt12_invoiceerror transitions (DuplicateInvoice → UnexpectedInvoice afterIDEMPOTENCY_TIMEOUT_TICKS) are exercised correctly, and thePaymentContextextracted fromPaymentClaimableis checked against the expectedBolt12Offercontext.Two possible tightenings for this test:
- If
Event::PaymentClaimableand/or outbound payment state exposes the received custom TLVs, adding anassert_eq!against the originalcustom_tlvswould directly validate end‑to‑end TLV propagation, not just that the payment still succeeds.- Given this scenario is specifically an offer payment, the
PaymentPurpose::Bolt12RefundPaymentmatch arm is effectively dead code; you could simplify the match to only acceptBolt12OfferPaymentand treat other variants as an error for stricter expectations.Functionally this looks correct; these suggestions would just make the test’s intent and coverage sharper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lightning/src/ln/async_payments_tests.rs(17 hunks)lightning/src/ln/channelmanager.rs(12 hunks)lightning/src/ln/invoice_utils.rs(4 hunks)lightning/src/ln/max_payment_path_len_tests.rs(1 hunks)lightning/src/ln/offers_tests.rs(40 hunks)lightning/src/ln/outbound_payment.rs(26 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/invoice_utils.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T12:47:06.857Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.857Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are handled during the parsing phase, so they don't need to be performed again in the recurrence logic handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/outbound_payment.rs
📚 Learning: 2025-11-28T12:48:22.008Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:22.008Z
Learning: In `lightning/src/ln/channelmanager.rs`, limit checks for invoice request recurrence fields are already handled during the parsing phase, so they don't need to be re-validated in the recurrence handling code.
Applied to files:
lightning/src/ln/channelmanager.rslightning/src/ln/outbound_payment.rs
🧬 Code graph analysis (2)
lightning/src/ln/channelmanager.rs (1)
lightning/src/ln/outbound_payment.rs (2)
custom_tlvs(784-786)custom_tlvs(799-801)
lightning/src/ln/offers_tests.rs (3)
lightning/src/ln/outbound_payment.rs (2)
custom_tlvs(784-786)custom_tlvs(799-801)lightning/src/offers/invoice_request.rs (3)
amount_msats(1145-1156)amount_msats(1199-1201)as_ref(572-574)lightning/src/offers/offer.rs (15)
absolute_expiry(907-909)offer(1892-1892)offer(1903-1903)offer(1982-1982)offer(1991-1991)offer(2033-2033)offer(2049-2049)offer(2058-2058)offer(2073-2073)offer(2079-2079)offer(2087-2087)offer(2095-2095)offer(2103-2103)as_ref(859-861)as_ref(1172-1174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: linting
- GitHub Check: benchmark
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
- GitHub Check: semver-checks
- GitHub Check: semver-checks
- GitHub Check: benchmark
- GitHub Check: linting
- GitHub Check: check_release
- 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, beta)
- GitHub Check: build-tx-sync (ubuntu-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, 1.75.0)
🔇 Additional comments (19)
lightning/src/ln/async_payments_tests.rs (1)
550-553: LGTM: Consistent test updates for new API parameter.All
pay_for_offercall sites have been correctly updated to include the newcustom_tlvsparameter (passed asvec![]). The changes are mechanical, consistent, and appropriate for tests that don't exercise custom TLV functionality.Also applies to: 711-714, 786-789, 918-921, 1019-1022, 1128-1131, 1191-1194, 1277-1280, 1374-1377, 1475-1478, 1721-1724, 1818-1821, 1936-1936, 2380-2383, 2715-2718, 2780-2783, 2884-2887
lightning/src/ln/outbound_payment.rs (11)
80-99: Threadingcustom_tlvsthrough AwaitingInvoice → InvoiceReceived is consistent and state-safeAdding
custom_tlvs: Vec<(u64, Vec<u8>)>to bothPendingOutboundPayment::AwaitingInvoiceand::InvoiceReceivedcleanly carries user-provided TLVs through the pre-HTLC lifecycle. Combined with the latercore::mem::takeinmark_invoice_received_and_get_details, this avoids duplication and ensures a single authoritative copy when transitioning toInvoiceReceived. The fields are only present on pre-lock-in states, so they won’t leak into retry bookkeeping or fulfilled/abandoned variants unintentionally.
756-771:RecipientOnionFields::with_custom_tlvsvalidation logic looks correct and robustThe builder method correctly enforces:
- TLV types in the custom range (
typ >= 1 << 16),- exclusion of reserved experimental types (keysend
5482373484and77_777),- strict ordering and uniqueness via
sort_unstable_by_keyplus theprev >= *typcheck.The sort + uniqueness check ensures the invariant documented in
custom_tlvs()and keeps serialization stable. The method also avoids partial state by only assigningself.custom_tlvsafter all validation passes. Overall this is a solid API surface for callers constructing custom TLVs.
811-824:check_mergecorrectly enforces even-TLV equality and intersects full TLV setsGiven the guarantees from
with_custom_tlvs(sorted, unique TLVs), the merge logic:
- Enforces that all even-numbered TLVs (critical types) match exactly between HTLC parts.
- Reduces both
tlvsandfurther_tlvsto their intersection, ensuring only HTLC-order-independent data survives.This matches the spec intent for multi-part payments and preserves invariants without introducing ordering bugs. Complexity is quadratic in TLV count but acceptable given expected small vectors.
923-969: BOLT11 path: validation and error propagation forcustom_tlvsare wired correctlyExtending
pay_for_bolt11_invoiceto takecustom_tlvs: Vec<(u64, Vec<u8>)>and immediately run them throughRecipientOnionFields::secret_only(...).with_custom_tlvs(custom_tlvs)ensures:
- Invalid TLVs are caught before any routing or state changes.
- Callers receive a precise
Bolt11PaymentError::SendingFailed(RetryableSendFailure::InvalidCustomTlvs).Payment metadata is still taken purely from the invoice, so there’s no ambiguity between invoice-provided payload and user-provided TLVs. The rest of the flow is unchanged, so behavior is preserved for callers passing
vec![].
991-1019: BOLT12 path:custom_tlvspropagation from invoice-awaiting state through send is coherent
mark_invoice_received_and_get_detailsnow returnscustom_tlvsalongside hash/retry/config, andsend_payment_for_bolt12_invoiceuses them to build:let recipient_onion = RecipientOnionFields::spontaneous_empty() .with_custom_tlvs(custom_tlvs) .map_err(|_| Bolt12PaymentError::SendingFailed(RetryableSendFailure::InvalidCustomTlvs))?;This mirrors the BOLT11 behavior: invalid TLVs are rejected before route lookup, and state transitions only carry raw TLVs; their validation happens right before onion construction. The additional
InvalidCustomTlvscase in theRetryableSendFailurematch is also correctly mapped to an abandon withPaymentFailureReason::UnexpectedError.
1026-1076: Internal BOLT12 send: newrecipient_onionparameter and error mapping are consistent
send_payment_for_bolt12_invoice_internalnow takes an explicitrecipient_onion: RecipientOnionFields, which matches how non-BOLT12 payments are handled and keeps onion-related data out of the mutableRouteParameters. The addedRetryableSendFailure::InvalidCustomTlvsarm in the route-construction error mapping properly abandons the payment and emits aBolt12PaymentError::SendingFailed(e), so callers observe the same error variant they triggered.
1989-2005:add_new_pending_paymentextension withbolt12_invoiceis consistent with retry semanticsAllowing
add_new_pending_paymentto take an optionalPaidBolt12Invoiceand storing it inRetryableviacreate_pending_paymentmakes BOLT12 retries able to emitPaymentSentwith proof-of-payment information. The change is orthogonal tocustom_tlvsbut integrates cleanly with the extended outbound state.
1988-2011:received_offerandadd_new_awaiting_invoicenow correctly capture usercustom_tlvsFor BOLT12 offers resolved via DNS:
received_offeracceptscustom_tlvsand populates them intoPendingOutboundPayment::AwaitingInvoice.add_new_awaiting_invoiceaccepts and storescustom_tlvsdirectly when starting from an awaiting-invoice state.These flows ensure that any app-level TLVs associated with an offer-based payment are persisted prior to invoice arrival and later consumed when building the actual
RecipientOnionFields. No extra copies or lifetimes issues appear here.
2040-2085: Invoice-received transition handlescustom_tlvsand no_std correctly
mark_invoice_receivedandmark_invoice_received_and_get_detailsnow:
- Move
custom_tlvsfromAwaitingInvoicetoInvoiceReceivedusingcore::mem::take(custom_tlvs)(no_std-friendly).- Return
customby value so the send path can constructRecipientOnionFieldswithout re-locking the map.- Handle repeated calls on
InvoiceReceivedby cloning existingcustom_tlvsand returningis_newly_marked = false, preserving idempotency.This avoids double consumption of the TLV vec and matches the existing comment about idempotent event generation.
2720-2790: TLV upgrade mapping forAwaitingInvoice/InvoiceReceivedincludescustom_tlvscleanlyAdding:
(9, custom_tlvs, optional_vec)to theAwaitingInvoiceschema, and(7, custom_tlvs, optional_vec)to theInvoiceReceivedschemaensures
custom_tlvsare persisted across restarts and safely ignored by older versions (as an optional vec). The legacy_max_total_routing_fee_msathandling is unchanged, and there are no ordering conflicts with existing TLV type numbers. This is aligned with other optional fields in the enum and maintains backward-compatibility semantics for downgrade.
3059-3239: Updated tests passingvec![]intoadd_new_awaiting_invoicekeep semantics intactThe various tests for staleness, abandonment, and BOLT12 flows now call:
outbound_payments.add_new_awaiting_invoice( payment_id, vec![], expiration, Retry::Attempts(0), RouteParametersConfig::default(), None, )or similar, which simply stores an empty
custom_tlvsvec in the new field. Because empty TLVs are the pre-existing behavior, this preserves original semantics while satisfying the new API. The assertions about pending payments and events remain logically unchanged.lightning/src/ln/max_payment_path_len_tests.rs (1)
518-528:pay_for_offercall updated correctly with emptycustom_tlvsThe new
vec![]argument matches the expandedpay_for_offersignature wherecustom_tlvs: Vec<(u64, Vec<u8>)>sits betweenpayment_idandOptionalOfferPaymentParams. Supplying an empty vec preserves prior behavior while keeping this test focused on “too-large blinded paths” rather than TLV content.lightning/src/ln/channelmanager.rs (4)
12880-12901: Consistent implementation across refund builder variants.Both
create_refund_builderandcreate_refund_builder_using_routercorrectly threadcustom_tlvsthrough toadd_new_awaiting_invoice. The implementations are consistent with each other.Also applies to: 12922-12945
13055-13067: Consistent custom TLV propagation across all offer payment variants.All three offer payment functions (
pay_for_offer,pay_for_offer_from_hrn,pay_for_offer_with_quantity) consistently capture and passcustom_tlvstoadd_new_awaiting_invoicewithin their respective closures. The implementation is correct and uniform.Also applies to: 13085-13097, 13128-13141
2272-2282: Custom TLV examples are well-documented with comprehensive validation in place.The documentation examples correctly demonstrate custom TLV usage. The validation logic in
RecipientOnionFields::with_custom_tlvs()enforces:
- TLV type range: must be ≥ 2^16 (65536) to use custom range, preventing protocol violations
- Reserved types rejected: keysend (5482373484) and async payments (77_777)
- Uniqueness and strict ordering enforced
The example TLV type 343493 is valid (within custom range) and properly follows BOLT conventions as an optional type. Tests confirm validation catches invalid types and edge cases like oversized payloads trigger
OnionPacketSizeExceedederror appropriately.
15789-15795: Remove this comment — async payments correctly handle custom TLVs.The
received_offerfunction properly accepts and stores thecustom_tlvsparameter (line 1989 in outbound_payment.rs, persisted at line 2002). The emptyvec![]passed at line 15792 is intentional design, not a limitation: the DNSSEC async payment flow doesn't require custom TLVs, and they are appropriately left empty. Custom TLVs are a first-class field in the payment state and will be used when the invoice is received. This is correct behavior.Likely an incorrect or invalid review comment.
lightning/src/ln/offers_tests.rs (2)
329-329: Refund builder callsites correctly updated for new custom_tlvs parameterAll
create_refund_builder/create_refund_builder_using_routerinvocations now passvec![]immediately before theRetryargument, matching the likely new signature(amount, expiry, payment_id, custom_tlvs, retry, route_params_cfg). This keeps existing tests behaviorally identical (no custom TLVs) while exercising the new API shape; the usage looks consistent across all sites.Also applies to: 595-595, 625-625, 781-781, 909-909, 1018-1018, 1172-1172, 1583-1583, 1862-1862, 1896-1896, 2019-2019, 2030-2030, 2087-2087, 2179-2179, 2185-2185, 2304-2304, 2353-2353
496-496: pay_for_offer callsites now pass explicit empty custom TLVsEach
pay_for_offercall has been updated to pass an explicitvec in the new parameter slot before the retry/config argument, preserving prior semantics (no custom TLVs by default) while conforming to the updated API. The amount / payment_id / retry arguments remain unchanged, and the pattern is consistent across all tests, including failure-path scenarios.Also applies to: 518-518, 685-685, 850-850, 971-971, 1098-1098, 1231-1231, 1304-1304, 1397-1397, 1667-1667, 1692-1692, 1767-1767, 1783-1783, 1967-1967, 1978-1978, 2065-2065, 2122-2122, 2155-2155, 2158-2158, 2237-2237, 2445-2445, 2529-2529, 2615-2615
| /// To use default settings, call the function with [`RouteParametersConfig::default`]. | ||
| pub fn pay_for_bolt11_invoice( | ||
| &self, invoice: &Bolt11Invoice, payment_id: PaymentId, amount_msats: Option<u64>, | ||
| route_params_config: RouteParametersConfig, retry_strategy: Retry, | ||
| custom_tlvs: Vec<(u64, Vec<u8>)>, route_params_config: RouteParametersConfig, | ||
| retry_strategy: Retry, | ||
| ) -> Result<(), Bolt11PaymentError> { | ||
| let best_block_height = self.best_block.read().unwrap().height; | ||
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | ||
| self.pending_outbound_payments.pay_for_bolt11_invoice( | ||
| invoice, | ||
| payment_id, | ||
| amount_msats, | ||
| custom_tlvs, | ||
| route_params_config, | ||
| retry_strategy, | ||
| &self.router, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Extract pay_for_bolt11_invoice function signature
rg -n -A10 'pub fn pay_for_bolt11_invoice' lightning/src/ln/channelmanager.rs | head -20Repository: shaavan/rust-lightning
Length of output: 622
🏁 Script executed:
#!/bin/bash
# Extract create_refund_builder function signature and context around lines 12883-12884
sed -n '12875,12895p' lightning/src/ln/channelmanager.rsRepository: shaavan/rust-lightning
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Search for all payment-related functions with custom_tlvs parameter
rg -n 'pub fn.*custom_tlvs' lightning/src/ln/channelmanager.rsRepository: shaavan/rust-lightning
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Search for create_offer_builder and other payment builders with custom_tlvs
rg -n -A6 'pub fn create_offer_builder' lightning/src/ln/channelmanager.rsRepository: shaavan/rust-lightning
Length of output: 535
🏁 Script executed:
#!/bin/bash
# Search for all pay_ functions to understand the full API surface
rg -n 'pub fn pay_' lightning/src/ln/channelmanager.rs | head -20Repository: shaavan/rust-lightning
Length of output: 266
🏁 Script executed:
#!/bin/bash
# Get the full signature of create_offer_builder with custom_tlvs pattern
sed -n '12400,12550p' lightning/src/ln/channelmanager.rs | rg -n -B5 -A10 'pub fn create_offer_builder'Repository: shaavan/rust-lightning
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Extract pay_for_offer function signature
sed -n '13056,13075p' lightning/src/ln/channelmanager.rsRepository: shaavan/rust-lightning
Length of output: 793
🏁 Script executed:
#!/bin/bash
# Extract pay_for_offer_with_quantity function signature
sed -n '13129,13150p' lightning/src/ln/channelmanager.rsRepository: shaavan/rust-lightning
Length of output: 802
Parameter ordering inconsistency in custom_tlvs APIs.
The addition of custom_tlvs to pay_for_bolt11_invoice uses the parameter order custom_tlvs, route_params_config, retry_strategy, which differs from create_refund_builder where it's custom_tlvs, retry_strategy, route_params_config (lines 12882-12884). This inconsistency across payment APIs could confuse users. Consider standardizing the parameter order for all payment functions that accept these parameters.
🤖 Prompt for AI Agents
In lightning/src/ln/channelmanager.rs around lines 5551-5566, the
pay_for_bolt11_invoice signature orders parameters as custom_tlvs,
route_params_config, retry_strategy which is inconsistent with other APIs (e.g.
create_refund_builder uses custom_tlvs, retry_strategy, route_params_config);
change the pay_for_bolt11_invoice parameter order to match the established
convention (custom_tlvs, retry_strategy, route_params_config), update the
function signature, all call sites, any doc comments and tests to the new
ordering, and run the build/tests to ensure no breakage.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.