Review: BOLT12 Recurrence: Proof-of-Concept Implementation (Core TLVs + Payee Flow) #8
Conversation
This commit begins the introduction of BOLT12 recurrence support in LDK. It adds the core recurrence-related fields to `Offer`, enabling subscription-style and periodic payments as described in the draft spec. Since this is a PoC, the focus is on establishing the data model and documenting the intended semantics. Where the spec is ambiguous or redundant, accompanying comments note possible simplifications or improvements. This lays the foundation for the following commits, which will implement invoice-request parsing, payee-side validation, and period/paywindow handling. Spec reference: https://github.com/rustyrussell/bolts/blob/guilt/offers-recurrence/12-offer-encoding.md#tlv-fields-for-offers
WalkthroughAdds BOLT12 recurrence support and time-aware APIs across offers, invoices, invoice requests, refunds, ChannelManager recurrence session tracking, TLV wiring and validation, and test/fuzz updates to pass explicit timestamps (created_at/now). Changes
Sequence DiagramsequenceDiagram
participant Client
participant CM as ChannelManager
participant OB as OfferBuilder
participant IB as InvoiceBuilder
participant Store as RecurrenceStore
Client->>CM: create_offer_builder_with_recurrence(recurrence_fields)
CM->>OB: init OfferBuilder (embed recurrence_fields)
OB->>OB: write recurrence TLVs
OB->>Client: build() → Offer
Client->>CM: submit InvoiceRequest (includes recurrence_counter?)
CM->>Store: lookup active_recurrence_sessions(payer_signing_pubkey)
Store-->>CM: RecurrenceData or None
CM->>IB: create_invoice_builder(..., created_at)
IB->>IB: set invoice_recurrence_basetime (from TLV or created_at)
IB->>Client: build() → Invoice
Client->>CM: claim payment for invoice
CM->>Store: update next_payable_counter (on successful claim)
Store-->>CM: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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/invoice.rs (1)
1746-1757:invoice_recurrence_basetimeis never serialized, so the builder setter has no effectRight now:
InvoiceContents::ForOfferstoresinvoice_recurrence_basetime: Option<u64>.- Builders initialise it and
set_invoice_recurrence_basetimemutates it.- However,
InvoiceFields::as_tlv_streamalways emitsinvoice_recurrence_basetime: Noneand has no access to the value inInvoiceContents::ForOffer.Because
UnsignedBolt12Invoice::newandInvoiceContents::as_tlv_streamrely solely onInvoiceFields::as_tlv_streamwhen constructing theInvoiceTlvStream, any locally built invoice will omit TLV 177 even ifinvoice_recurrence_basetimewas set. This contrasts with your parsing-side “MUST have basetime whenever offer has recurrence” rule and makes the setter effectively a no-op.You probably want to plumb the basetime value into the TLV stream, e.g.:
- Store
invoice_recurrence_basetimeinInvoiceFields, or- Extend
InvoiceFields::as_tlv_streamto accept the basetime fromInvoiceContentsand setinvoice_recurrence_basetime: self.invoice_recurrence_basetime(or equivalent) instead ofNone.Until this is wired through, your node cannot emit spec-compliant recurring invoices even though it enforces basetime on parse.
Also applies to: 1417-1443, 1513-1524
fuzz/src/invoice_request_deser.rs (1)
90-105: Fuzz harness won’t compile: undefinednetworkand outdatedrespond_withcallTwo issues in
build_response:
- You compute:
let genesis_block = genesis_block(network); let now = Duration::from_secs(genesis_block.header.time as u64);but
networkis not defined anywhere, so this doesn’t compile.nowis also unused.
- The call
invoice_request.respond_with(vec![payment_path], payment_hash)?.build()has not been updated to the new
respond_withAPI that expects an explicitcreated_at: Durationparameter (as in the examples ininvoice.rs).For fuzzing you don’t need realistic timestamps; a fixed value is fine. A minimal fix is:
@@ fn build_response<T: secp256k1::Signing + secp256k1::Verification>( - let payment_hash = PaymentHash([42; 32]); - let genesis_block = genesis_block(network); - let now = Duration::from_secs(genesis_block.header.time as u64); - invoice_request.respond_with(vec![payment_path], payment_hash)?.build() + let payment_hash = PaymentHash([42; 32]); + let created_at = Duration::from_secs(0); + invoice_request + .respond_with(vec![payment_path], payment_hash, created_at)? + .build()This keeps the harness simple and in sync with the updated API.
Also applies to: 149-153
lightning/src/offers/invoice_request.rs (1)
840-854: Verification found a critical issue: Not all call sites have been updated.The fuzz target at
fuzz/src/invoice_request_deser.rs:152is callingrespond_withwith only 2 parameters (payment_path and payment_hash), but the method signature requires 3 parameters includingcreated_at. The variablenowis defined on line 149 but not passed to the method call.Current code:
let now = Duration::from_secs(genesis_block.header.time as u64); invoice_request.respond_with(vec![payment_path], payment_hash)?.build()Should pass
nowas the third parameter to match the method signature that accepts(payment_paths, payment_hash, created_at).
🧹 Nitpick comments (8)
lightning/src/offers/invoice.rs (1)
1762-1765: Use dedicated recurrence error variants instead of overloadingInvalidAmount/InvalidMetadataTwo places currently overload generic errors:
- Refund branch: presence of
invoice_recurrence_basetimeon what’s identified as aRefundreturnsBolt12SemanticError::InvalidAmount.- Offer branch: missing or inconsistent basetime is reported as
InvalidMetadata.Given you’ve introduced
UnexpectedRecurrenceandInvalidRecurrenceonBolt12SemanticError, this is a good opportunity to tighten semantics:
- Return
UnexpectedRecurrencewhen recurrence TLVs appear on refunds or other contexts that must not have recurrence.- Use
InvalidRecurrencefor basetime-missing / basetime-mismatch cases instead ofInvalidMetadata.This makes downstream error handling and fuzzing much clearer and keeps
InvalidAmount/InvalidMetadatafree for their original concerns.Also applies to: 227-231
lightning/src/offers/parse.rs (1)
227-231: New recurrence error variants look good but are not yet usedAdding
UnexpectedRecurrenceandInvalidRecurrencetoBolt12SemanticErroris a sensible way to distinguish recurrence issues from generic amount/metadata problems. As of this file’s changes they’re only defined, not produced.Once the recurrence validation paths are finalized (e.g., in
InvoiceContents::try_fromand any offer/invoice-request parsing), it would be worth returning these specific variants instead ofInvalidAmount/InvalidMetadataso callers can reliably detect recurrence-related failures.lightning/src/ln/outbound_payment.rs (1)
1169-1179: Minor: consider a clearer outward error thanUnknownRequiredFeaturesfor amount derivation failureIn
static_invoice_received, ifInvoiceBuilder::<DerivedSigningPubkey>::amount_msats(invreq)returnsErr(_), you:
debug_assert!(false, "LDK requires an msat amount..."),- call
abandon_with_entry!(entry, PaymentFailureReason::UnexpectedError);,- but then return
Err(Bolt12PaymentError::UnknownRequiredFeatures).Given this is an internal invariant violation around amount calculation, a more accurate outward error would be something like
Bolt12PaymentError::SendingFailed(RetryableSendFailure::RouteNotFound)or a dedicated “UnexpectedError”-style variant, rather than “UnknownRequiredFeatures”.Not critical (the path should be unreachable), but aligning the external error with the
PaymentFailureReason::UnexpectedErroryou emit would make debugging less confusing.lightning/src/offers/refund.rs (2)
584-593: Explicitcreated_atinRefund::respond_withis a clean API tighteningThreading
created_at: Durationthrough to<$builder>::for_refundlooks correct and aligns this path with other time-aware builders. The unknown-features guard remains in place and unchanged, which is good. One minor polish: the doc comment still frames this as mainly a non‑stdhelper, but it’s now the canonical path for bothstdandno_stdbuilds; consider clarifying that phrasing when you next touch the docs.
868-903: Rejecting recurrence TLVs when parsing refunds enforces intended semanticsExtending the
OfferTlvStreamandInvoiceRequestTlvStreamdestructuring to include the new recurrence fields, and then returningBolt12SemanticError::UnexpectedRecurrencewhenever any of them are present, is a good defensive choice: refunds remain strictly non‑recurring, and malformed or misrouted recurrence messages won’t silently parse as refunds. It may be worth adding a small negative test in this module that injects a non‑Nonerecurrence field into a refund TLV stream and assertsUnexpectedRecurrence, to pin this behavior against regressions.Also applies to: 952-959, 966-969
lightning/src/ln/channelmanager.rs (1)
15447-15452: Refactor: Extract timestamp computation into a helper method.The logic for computing
created_atis duplicated in multiple places (lines 13267-13272, 15447-15452). Extract this into a helper method to reduce duplication and improve maintainability.Consider adding a helper method like:
fn current_timestamp(&self) -> Duration { #[cfg(not(feature = "std"))] let timestamp = Duration::from_secs(self.highest_seen_timestamp.load(Ordering::Acquire) as u64); #[cfg(feature = "std")] let timestamp = std::time::SystemTime::now() .duration_since(std::time::SystemTime::UNIX_EPOCH) .expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH"); timestamp }lightning/src/offers/invoice_request.rs (2)
1602-1608: Incomplete paywindow validation.The TODO indicates paywindow enforcement is deferred. Ensure this is tracked for completion before the feature is considered production-ready.
Would you like me to open an issue to track the paywindow validation implementation?
1515-1587: Complex match statement could benefit from refactoring.The validation logic is functionally correct but the nested match with many arms is hard to follow. Consider extracting helper functions or using early returns to reduce nesting.
Additionally,
Bolt12SemanticError::InvalidMetadatais used for all recurrence validation failures. A more specific error variant (e.g.,InvalidRecurrenceFields) would improve error diagnostics for callers.Consider introducing a dedicated error variant for recurrence validation:
// In Bolt12SemanticError enum: InvalidRecurrenceFields,This would allow callers to distinguish recurrence-specific validation failures from other metadata issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
fuzz/src/invoice_request_deser.rs(3 hunks)fuzz/src/refund_deser.rs(2 hunks)lightning/src/ln/channelmanager.rs(15 hunks)lightning/src/ln/offers_tests.rs(7 hunks)lightning/src/ln/outbound_payment.rs(3 hunks)lightning/src/offers/flow.rs(11 hunks)lightning/src/offers/invoice.rs(61 hunks)lightning/src/offers/invoice_request.rs(21 hunks)lightning/src/offers/offer.rs(14 hunks)lightning/src/offers/parse.rs(1 hunks)lightning/src/offers/refund.rs(15 hunks)lightning/src/offers/static_invoice.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lightning/src/ln/channelmanager.rs (4)
lightning/src/offers/flow.rs (1)
create_offer_builder_with_recurrence(631-648)lightning/src/offers/invoice.rs (2)
payer_signing_pubkey(1250-1257)created_at(1270-1272)lightning/src/offers/invoice_request.rs (3)
payer_signing_pubkey(1203-1205)recurrence_counter(1191-1193)recurrence_cancel(1199-1201)lightning/src/offers/refund.rs (2)
payer_signing_pubkey(551-553)payer_signing_pubkey(722-724)
lightning/src/offers/invoice_request.rs (1)
lightning/src/offers/invoice.rs (5)
fields(1311-1316)payer_signing_pubkey(1250-1257)quantity(1243-1248)payment_paths(1266-1268)payment_hash(1287-1289)
lightning/src/offers/refund.rs (1)
lightning/src/offers/invoice_request.rs (3)
recurrence_counter(1191-1193)recurrence_start(1195-1197)recurrence_cancel(1199-1201)
⏰ 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). (15)
- 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: build-tx-sync (ubuntu-latest, 1.75.0)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: build-tx-sync (ubuntu-latest, 1.75.0)
- 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: benchmark
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: semver-checks
🔇 Additional comments (31)
lightning/src/ln/outbound_payment.rs (2)
1118-1219: Static-invoice handling and async payment timeout logic look consistentThe
static_invoice_receivedflow:
- Validates offer matching, unknown features, and invoice expiry against
duration_since_epoch.- Uses
InvoiceBuilder::<DerivedSigningPubkey>::amount_msats(invreq)for amount derivation, then drivesPaymentParameters::from_static_invoiceandRouteParameters::from_payment_params_and_value.- Enforces onion size via
set_max_path_lengthand derives an async timeout viaduration_since_epoch + ASYNC_PAYMENT_TIMEOUT_RELATIVE_EXPIRY.- Transitions correctly into
PendingOutboundPayment::StaticInvoiceReceived { .. }with all required fields set.This matches the broader async-payment design and maintains clear separation between AwaitingInvoice and StaticInvoiceReceived states.
3182-3221: BOLT12 expiry test correctly exercises created_at/relative_expiry integration
fails_sending_payment_for_expired_bolt12_invoice:
- Builds an offer and invoice request, then responds with an invoice using
created_at = now() - DEFAULT_RELATIVE_EXPIRY.- Relies on
DEFAULT_RELATIVE_EXPIRYfor the invoice’s relative expiry, so the absolute expiry is approximately “now” when constructed.- Uses
send_payment_for_bolt12_invoice, which feeds route params throughhas_expired, leading toRetryableSendFailure::PaymentExpiredand aPaymentFailedevent withPaymentFailureReason::PaymentExpired.This is a good end-to-end test of the new explicit-created_at flow and the interaction between invoice timestamps and outbound expiry checks.
lightning/src/ln/offers_tests.rs (2)
678-688: Wiringrecurrence_counterthrough testInvoiceRequestFieldslooks correctAll the
InvoiceRequestFieldsconstructions now explicitly setrecurrence_counter: None, which keeps the tests aligned with the expanded struct without changing behavior for these non‑recurring scenarios. Nothing further needed here.Also applies to: 838-847, 960-968, 1228-1237, 1294-1302, 1392-1401
2334-2343: Usingcreated_atwithrespond_using_derived_keysis consistent with the new APIPassing
created_at = alice.node.duration_since_epoch()intorespond_using_derived_keys(payment_paths, payment_hash, created_at)matches the updated signature and keeps the test focused on unknown‑required‑features handling. Just ensure other call‑sites use a similarly consistent notion ofcreated_at(e.g., node clock vs. external timestamp) so recurrence timing semantics remain coherent across the codebase.lightning/src/offers/static_invoice.rs (1)
466-478: Static invoices correctly reject recurrence TLVs
InvoiceContents::as_tlv_streamnow always emitsinvoice_recurrence_basetime: None, ensuring we never encode recurrence data for static invoices.- In
InvoiceContents::try_from(PartialInvoiceTlvStream), destructuringinvoice_recurrence_basetimeand mapping anySome(_)toBolt12SemanticError::UnexpectedRecurrenceenforces the “no recurrence on static invoices” invariant at parse time.- The updated test expectations for
invoice_recurrence_basetime: Nonekeep round‑trip checks honest.This is a tight, symmetric guard; looks good.
Also applies to: 660-719, 908-943
lightning/src/offers/offer.rs (4)
652-741: Core recurrence types and (de)serialization look sound
TimeUnit+Recurrenceare simple and well‑scoped;period_length_secsuses safe arithmetic withchecked_muland reasonable approximations for days/months, appropriate for PoC‑level timing logic.Recurrence,RecurrenceBase,RecurrencePaywindow, andRecurrenceLimithave clearWriteable/Readableimpls with sensible validation (e.g., rejecting zero periods and zero limits, bounded enum tags, etc.).RecurrenceData’s TLV layout is compact and explicit, with required vs optional fields clearly encoded.RecurrenceFieldsgroups the recurrence period, optional basetime, paywindow, and limit into a single, copyable struct that’s easy to plumb through the API.No correctness issues spotted here; the design is consistent and easy to reason about.
Also applies to: 768-861, 867-907, 943-955
223-257: Recurrence wiring intoOffer(builder, accessors, TLVs) is coherent
- Builders (
new,deriving_signing_pubkey) correctly initializeOfferContents.recurrence_fieldstoNone, keeping non‑recurring offers unchanged.- The new
recurrence(...)builder method simply overwritesrecurrence_fieldswith the suppliedRecurrenceFields, which is the expected “last call wins” behavior.Offer::recurrence_fields()andOfferContents::recurrence_fields()expose recurrence metadata cleanly by value; given the small struct size andCopy, this is fine.OfferContents::as_tlv_stream’s normalization:
- Emits
recurrence_compulsorywhen arecurrence_baseis present, otherwiserecurrence_optional.- Threads through
recurrence_paywindowandrecurrence_limitas‑is.- The TLV schema adds the recurrence types (24, 25, 26, 27, 29) in a forward‑compatible way that matches how you serialize
Recurrence*types.This all hangs together well and keeps the wire format and builder API in sync.
Also applies to: 271-313, 394-402, 647-648, 1029-1032, 1318-1320, 1389-1423, 1577-1609
1664-1688: Normalization of recurrence TLVs on parse is mostly good but could be stricterThe
OfferContents::try_fromrecurrence logic:
- Accepts only three shapes:
- No recurrence TLVs ⇒
recurrence_fields: Nonerecurrence_optionalwith norecurrence_baserecurrence_compulsorywith arecurrence_base- Treats all other combinations as
InvalidMetadata, which is a reasonable normalization strategy.One edge case worth tightening:
- Today,
recurrence_paywindowand/orrecurrence_limitcan appear without anyrecurrence_*period (bothrecurrence_compulsoryandrecurrence_optionalareNone). In that case, the(None, None, None)arm is taken and those TLVs are effectively ignored (recurrence_fields: None), even though their presence is semantically meaningless without a period.Consider instead:
- Rejecting any offer that has
recurrence_paywindoworrecurrence_limitbut no recurrence period (optional or compulsory) withInvalidMetadata. That would prevent partially‑specified recurrence offers from being silently treated as non‑recurring.You can implement this by checking
recurrence_paywindow/recurrence_limitbefore thematch, or by including them in the match arms and adding an explicit “no‑recurrence, no‑extras” case.[uggest_recommended_refactor]
Also applies to: 1729-1776
1853-1875: Default recurrence behavior in tests matches the new modelThe
builds_offer_with_defaultstest now asserts:
offer.recurrence_fields() == None- All recurrence TLVs (
recurrence_compulsory,recurrence_optional,recurrence_base,recurrence_paywindow,recurrence_limit) areNonein the serialized TLV stream.This is the right baseline invariant for non‑recurring offers and confirms the migration is backwards‑compatible for existing users.
lightning/src/offers/refund.rs (3)
731-787: Setting recurrence TLVs toNoneinRefundContents::as_tlv_streamis consistentThe explicit
recurrence_*andrecurrence_*-counter fields being set toNonein bothOfferTlvStreamRefandInvoiceRequestTlvStreamRefpreserves existing refund behavior while keeping the TLV layouts in sync with the extended offer/invoice types. This keeps refunds non‑recurring at the encoding level and avoids accidental emission of recurrence data.
1080-1117: Defaults test correctly asserts recurrence fields remain absent for refundsThe
builds_refund_with_defaultsexpectations updated to includerecurrence_*andrecurrence_counter/start/cancelasNonein theOfferTlvStreamRefandInvoiceRequestTlvStreamReftuples accurately reflect the new TLV structs and confirm that default refunds still carry no recurrence information.
1158-1200: Test call sites updated to passcreated_atintorespond_withconsistentlyAll
Refund::respond_withusages in the tests now supply an explicitcreated_at(now()), matching the new signature and keeping coverage for both explicit‑key and derived‑key flows, as well as the unknown‑required‑features error path. The updates look coherent and don’t alter the underlying assertions about metadata and payer‑data verification.Also applies to: 1243-1249, 1263-1269, 1283-1289, 1469-1473
lightning/src/ln/channelmanager.rs (4)
12824-12848: LGTM: Well-documented public API.The new
create_offer_builder_with_recurrencemethod is properly documented with clear explanations of the recurrence parameters and references to the base method for additional context.
15549-15552: LGTM: Recurrence basetime correctly applied to invoice builders.The recurrence basetime is properly set on both derived-keys and explicit-keys invoice builders when applicable. The optional chaining approach is appropriate.
Also applies to: 15578-15581
17433-17433: LGTM: Serialization properly handles backward compatibility.The deserialization logic correctly handles backward compatibility by defaulting
active_recurrence_sessionsto an empty map when TLV field 23 is not present in older serialized data. Theunwrap()on line 18347 is safe because of the default value on line 17433.Also applies to: 17452-17452, 18347-18347
15497-15508: Based on my investigation of the codebase, I've gathered the following information:Pay Window Boundary Analysis:
Code Implementation (lines 15497-15508):
- Lower bound uses
<:created_at.as_secs() < period_start - window.seconds_before as u64- Upper bound uses
>=:created_at.as_secs() >= period_start + window.seconds_after as u64- This creates a half-open interval:
[period_start - seconds_before, period_start + seconds_after)RecurrencePaywindow Definition (offer.rs):
seconds_before: u32- "Seconds before the period starts in which a payment SHOULD be allowed."seconds_after: u32- "Seconds after the period starts in which a payment SHOULD be allowed."Verification Findings:
- No test cases found in
offers_tests.rsthat specifically test boundary conditions for pay window validation- No explicit boundary semantics documented in code comments indicating whether boundaries should be inclusive or exclusive
- The implementation appears intentional, but lacks verification against BOLT12 specification
- No other contextual clues about the intended boundary behavior in the codebase
Limitation: I was unable to access the external BOLT12 specification to verify if the half-open interval
[period_start - seconds_before, period_start + seconds_after)matches the specification's definition.Rewritten Review Comment:
Verify pay window boundary conditions against BOLT12 specification.
The pay window validation creates a half-open interval with
<for the lower bound and>=for the upper bound:if created_at.as_secs() < period_start - window.seconds_before as u64 || created_at.as_secs() >= period_start + window.seconds_after as u64This results in the window
[period_start - seconds_before, period_start + seconds_after)(inclusive lower bound, exclusive upper bound). Confirm this boundary semantics aligns with BOLT12 specification requirements. Additionally, consider adding test cases to document and verify the intended boundary behavior.lightning/src/offers/invoice_request.rs (7)
189-193: LGTM: Recurrence fields properly initialized.The new recurrence fields are correctly initialized to
Nonein the builder's default contents, consistent with other optional fields.
690-719: Well-documented recurrence field semantics.The documentation clearly explains the relationship between
recurrence_counter,recurrence_start, and the computedperiod_index. The constraints onrecurrence_cancel(not allowed on first request) are also clearly documented.
770-792: LGTM: Accessor methods follow established patterns.The new accessor methods for recurrence fields are consistent with existing accessors in the macro.
1321-1323: TLV field assignments look correct.The TLV types 92, 93, 94 are within the valid
INVOICE_REQUEST_TYPESrange (80..160) and are sequential as expected.
1649-1661: LGTM: InvoiceRequestFields extended with recurrence_counter.The field is properly documented and follows the existing pattern. Including only
recurrence_counter(notrecurrence_startorrecurrence_cancel) inInvoiceRequestFieldsmakes sense since those are request-time parameters rather than payment context data.
1679-1679: LGTM: Serialization/deserialization for recurrence_counter.The TLV type 6 is used consistently for
recurrence_counterin both write and read paths, with properHighZeroBytesDroppedBigSizeencoding.Also applies to: 1692-1692, 1700-1700
1896-1896: Test updated to use newrespond_withsignature.The test correctly passes
now()as thecreated_atparameter.lightning/src/offers/flow.rs (8)
46-46: LGTM: RecurrenceFields import added.The import is properly added to support the new recurrence functionality.
541-567: LGTM: Internal method updated to support optional recurrence.The
create_offer_builder_internnow acceptsrecurrence_fields: Option<RecurrenceFields>and applies it to the builder when present. This is a clean way to share logic between recurring and non-recurring offer creation.
608-614: LGTM: Existing method passesNonefor recurrence.The existing
create_offer_buildercorrectly passesNoneto maintain backward compatibility.
616-648: New public API for recurring offers is well-documented.The
create_offer_builder_with_recurrencemethod has clear documentation explaining its purpose, relationship tocreate_offer_builder, and whatRecurrenceFieldscontrols.
933-975: Explicitcreated_atparameter improves no_std compatibility.The change from conditional std/no_std time handling to an explicit
created_at: Durationparameter is a good design choice. It makes the API uniform and shifts the responsibility of obtaining the current time to the caller.
991-1029: LGTM: Invoice builder method with derived keys updated consistently.The
created_atparameter is properly threaded through torespond_using_derived_keys.
1045-1083: LGTM: Invoice builder method without keys updated consistently.The
created_atparameter is properly threaded through torespond_with.
667-667: LGTM: Other offer builder methods passNonefor recurrence.Both
create_offer_builder_using_routerandcreate_async_receive_offer_buildercorrectly passNoneforrecurrence_fields, maintaining their existing non-recurring behavior.Also applies to: 692-692
| /// Tracks all active recurrence sessions for this node. | ||
| /// | ||
| /// Each entry is keyed by the payer’s `payer_signing_pubkey` from the | ||
| /// initial `invoice_request`. The associated `RecurrenceData` stores | ||
| /// everything the payee needs to validate incoming `invoice_request`s | ||
| /// and generate invoices for the appropriate recurrence period. | ||
| /// | ||
| /// This is used by the payee to: | ||
| /// - verify the correctness of each incoming `invoice_request` | ||
| /// (period offset, counter, basetime, etc.) | ||
| /// - ensure continuity across periods | ||
| /// - maintain recurrence state until cancellation or completion. | ||
| active_recurrence_sessions: Mutex<HashMap<PublicKey, RecurrenceData>>, |
There was a problem hiding this comment.
Consider adding a cleanup mechanism for stale recurrence sessions.
The active_recurrence_sessions map will grow indefinitely as payers create recurrence sessions. There's no mechanism to remove completed, cancelled, or expired sessions. Over time, this could lead to memory bloat, especially for nodes serving many recurring offers.
Consider implementing cleanup logic that:
- Removes sessions when recurrence reaches its limit (if specified)
- Removes sessions after a timeout period of inactivity
- Provides an API for manual session cleanup
Would you like me to suggest an implementation for session cleanup logic?
🤖 Prompt for AI Agents
In lightning/src/ln/channelmanager.rs around lines 2676 to 2688,
active_recurrence_sessions currently has no cleanup and will grow unbounded; add
session expiration and explicit removal points: extend RecurrenceData with
last_activity_at and optional max_occurrences/expiry_timestamp, update code
paths to remove the entry when a recurrence completes or is cancelled, and spawn
a periodic background pruning task (using the existing runtime/tokio) that locks
the Mutex, iterates the HashMap, and removes entries that are expired or
inactive beyond a configured timeout; also expose a public API method to
manually remove a session by payer_public_key and ensure tests cover automatic
pruning, manual removal, and removal on completion.
| match &purpose { | ||
| PaymentPurpose::Bolt12OfferPayment { | ||
| payment_context: Bolt12OfferContext { | ||
| invoice_request: InvoiceRequestFields { | ||
| payer_signing_pubkey, | ||
| recurrence_counter: Some(paid_counter), | ||
| .. | ||
| }, | ||
| .. | ||
| }, | ||
| .. | ||
| } => { | ||
| let mut sessions = self.active_recurrence_sessions.lock().unwrap(); | ||
|
|
||
| if let Some(data) = sessions.get_mut(payer_signing_pubkey) { | ||
| if data.next_payable_counter == *paid_counter { | ||
| data.next_payable_counter += 1; | ||
| } | ||
| } | ||
| }, | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
Recurrence counter advancement logic is fragile and doesn't handle edge cases.
The current implementation has several issues:
-
Out-of-order payments: The condition
data.next_payable_counter == *paid_countermeans if payment for counter 1 is claimed before counter 0, the state won't advance correctly. This could happen due to payment timing or routing delays. -
No duplicate claim protection: If the same payment is claimed multiple times (e.g., due to retries or bugs), the counter could advance incorrectly.
-
Silent failures: When the equality check fails, no error is logged or raised, making debugging difficult.
Consider:
- Using a more robust state machine that tracks which counters have been paid (e.g., a set or bitmap)
- Adding validation that
paid_counteris within expected bounds - Logging when unexpected counter values are encountered
- Ensuring idempotency for payment claims
🤖 Prompt for AI Agents
In lightning/src/ln/channelmanager.rs around lines 9522-9543, the current
equality check for advancing next_payable_counter is fragile (fails on
out-of-order payments, duplicates, and silently ignores unexpected counters);
change the logic to validate paid_counter bounds, make claim processing
idempotent by storing paid counters (e.g., a HashSet or bitmap) in the session
data, on receiving a paid_counter: if out of bounds log and reject, if already
present log duplicate and return, otherwise insert it and then advance
next_payable_counter in a loop while the next_payable_counter exists in the
paid-set; add logging for unexpected values and ensure proper locking and early
returns so repeated claims don't double-advance the counter.
There was a problem hiding this comment.
@coderabbitai isn't it idempotent? We update payable_counter, only when paid counter matches. If someone makes a duplicate claim, payable_counter, and paid counter won't match, because paid counter's already updated, right?
There was a problem hiding this comment.
@shaavan, you're absolutely right! I'm replying to your latest comment without the full context of the earlier conversation, but your analysis of the idempotency is correct.
The code is indeed idempotent with respect to duplicate claims because of the guard condition data.next_payable_counter == *paid_counter. Here's why:
-
First successful claim: If
next_payable_counter = 5andpaid_counter = 5, the condition matches, andnext_payable_counteris incremented to 6. -
Duplicate claim: If the same payment is claimed again with
paid_counter = 5, the conditionnext_payable_counter == paid_counterwill fail (6 ≠ 5), so no update occurs.
This ensures that duplicate claims for the same counter value won't cause the state to advance incorrectly. The counter only advances once per unique recurrence payment, which is the desired behavior for idempotency.
This commit adds the recurrence-related TLVs to `InvoiceRequest`, allowing payers to specify the intended period index, an optional starting offset, and (when applicable) a recurrence cancellation signal. Spec reference: https://github.com/rustyrussell/bolts/blob/guilt/offers-recurrence/12-offer-encoding.md#tlv-fields-for-invoice_request
This commit adds the recurrence-related TLVs to the `Invoice` encoding, allowing the payee to include `invoice_recurrence_basetime`. This field anchors the start time (UNIX timestamp) of the recurrence schedule and is required for validating period boundaries across successive invoices. Additional initialization logic, validation notes, and design considerations are documented inline within the commit. Spec reference: https://github.com/rustyrussell/bolts/blob/guilt/offers-recurrence/12-offer-encoding.md#invoices
This begins the payee-side recurrence implementation by adding a dedicated builder API for constructing Offers that include recurrence fields. The new `create_offer_builder_with_recurrence` helper mirrors the existing offer builder but ensures that the recurrence TLVs are always included, making it easier for users to define subscription-style Offers.
This commit adds a minimal state tracker in `ChannelManager` for handling inbound recurring BOLT12 payments. Each entry records the payer’s recurrence progress (offset, next expected counter, and basetime), giving the payee enough information to validate successive `invoice_request`s and produce consistent invoices. LDK inbound payments have historically been fully stateless. Introducing a stateful mechanism here is a deliberate PoC choice to make recurrence behavior correct and testable end-to-end. For production, we may instead push this state to the user layer, or provide hooks so nodes can manage their own recurrence state externally. For now, this internal tracker gives us a clear foundation to build and evaluate the recurrence flow.
…` split) This refactor removes the separate `respond_with` / `respond_with_no_std` variants and replaces them with a single unified `respond_using_derived_keys(created_at)` API. Reasoning: - Upcoming recurrence logic requires setting `invoice_recurrence_basetime` based on the invoice’s `created_at` timestamp. - For consistency with Offer and Refund builders, we want a single method that accepts an explicit `created_at` value at the callsite. - The only real difference between the std/no_std response paths was how `created_at` was sourced; once it becomes a parameter, the split becomes unnecessary. This change consolidates the response flow, reduces API surface, and makes future recurrence-related changes simpler and more uniform across Offer, InvoiceRequest, and Refund builders.
This commit adds payee-side handling for recurrence-enabled `InvoiceRequest`s. The logic now: - Distinguishes between one-off requests, initial recurring requests, and successive recurring requests. - Initializes a new `RecurrenceData` session on the first recurring request (counter = 0). - Validates successive requests against stored session state (offset, expected counter, basetime). - Enforces paywindow timing when applicable. - Handles recurrence cancellation by removing the session and returning no invoice. This forms the core stateful logic required for a node to act as a BOLT12 recurrence payee. Payment-acceptance and state-update logic will follow in the next commit.
This commit adds the final piece of the payee-side recurrence flow: updating the internal `next_payable_counter` once a recurring payment has been successfully claimed. The update is performed immediately before emitting the `PaymentClaimed` event, ensuring the counter is advanced only after the payment is fully completed and acknowledged by the node. This provides a clear correctness boundary and avoids premature state transitions. The approach is intentionally conservative for this PoC. Future refinements may place the update earlier in the pipeline or integrate it more tightly with the payment-claim flow, but the current design offers simple and reliable semantics.
shaavan
force-pushed
the
recurrence-review
branch
from
a46e1e3 to
6233697
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
lightning/src/offers/invoice.rs (1)
769-808: End‑to‑end TLV wiring forinvoice_recurrence_basetimeis correct; consider error type for refundsThe new
invoice_recurrence_basetime: Option<u64>field is cleanly threaded throughInvoiceFields,InvoiceFields::as_tlv_stream, and theInvoiceTlvStreamTLV (type 177), and is round‑tripped inTryFrom<PartialInvoiceTlvStream>without panics or missing cases. The refund branch explicitly rejects invoices that carry a recurrence basetime, but usingBolt12SemanticError::InvalidAmountfor this feels a bit misleading relative to the other recurrence checks that useInvalidMetadata; if you have a suitable metadata‑style variant available, you may want to switch to that for clearer semantics.Also applies to: 1415-1443, 1509-1522, 1711-1753, 1759-1763
lightning/src/ln/channelmanager.rs (3)
13267-13272: Consider extracting timestamp helper to reduce duplication.This timestamp creation pattern is duplicated (here and lines 15447-15452). Consider extracting to a helper method. This is optional - the current code works correctly.
// Example helper method (could be added to ChannelManager impl): fn current_timestamp(&self) -> Duration { #[cfg(not(feature = "std"))] return Duration::from_secs(self.highest_seen_timestamp.load(Ordering::Acquire) as u64); #[cfg(feature = "std")] return std::time::SystemTime::now() .duration_since(std::time::SystemTime::UNIX_EPOCH) .expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH"); }
15549-15552: Useif letinstead of.map()for side effects.Using
.map()and discarding the result is not idiomatic when performing side effects.- recurrence_basetime.map(|basetime| - builder.set_invoice_recurrence_basetime(basetime) - ); + if let Some(basetime) = recurrence_basetime { + builder.set_invoice_recurrence_basetime(basetime); + }The same applies to lines 15579-15581.
15464-15467: Clarify comment for one-off payment case.The comment mentions "payer didn't support recurrence but we set recurrence optional" - consider clarifying that this allows graceful degradation to a non-recurring payment when the offer has optional recurrence.
- // This represents case where the payer, didn't support recurrence - // but we set recurrence optional so we allow payer to pay one-off + // Payer sent a non-recurrence request to a recurrence-optional offer. + // Allow as a one-off payment (no basetime needed). (None, None, None) => { None },lightning/src/offers/offer.rs (2)
697-709: Refine month approximation before production.The 30-day month approximation is noted as "proof-of-concept stage" simplification. For production recurrence billing:
- Real calendar months vary (28-31 days)
- Critical for accurate payment scheduling and paywindow validation
- Could cause off-by-days errors accumulating over time
Consider adding a TODO or tracking issue to implement proper calendar-aware month handling before this moves beyond POC.
1729-1762: Consider more specific error for invalid recurrence combinations.Line 1761 returns
Bolt12SemanticError::InvalidMetadatafor invalid recurrence TLV combinations (e.g., both optional and compulsory present, or compulsory without base).While this works,
InvalidMetadatais semantically about offer metadata fields (type 4), not recurrence configuration. A more descriptive error (likeInvalidRecurrenceor reusingUnexpectedRecurrencefrom refund validation) would make debugging easier.Not blocking since the error is caught and propagated correctly, but worth considering for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
fuzz/src/invoice_request_deser.rs(4 hunks)fuzz/src/refund_deser.rs(3 hunks)lightning/src/ln/channelmanager.rs(15 hunks)lightning/src/ln/offers_tests.rs(7 hunks)lightning/src/ln/outbound_payment.rs(3 hunks)lightning/src/offers/flow.rs(11 hunks)lightning/src/offers/invoice.rs(60 hunks)lightning/src/offers/invoice_request.rs(21 hunks)lightning/src/offers/offer.rs(14 hunks)lightning/src/offers/refund.rs(15 hunks)lightning/src/offers/static_invoice.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/outbound_payment.rs
- fuzz/src/refund_deser.rs
- fuzz/src/invoice_request_deser.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:21.999Z
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.828Z
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.
📚 Learning: 2025-11-28T12:47:06.828Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.828Z
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/offers_tests.rslightning/src/ln/channelmanager.rslightning/src/offers/static_invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/invoice.rslightning/src/offers/flow.rs
📚 Learning: 2025-11-28T12:48:21.999Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:21.999Z
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/offers_tests.rslightning/src/ln/channelmanager.rslightning/src/offers/static_invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/offer.rslightning/src/offers/refund.rslightning/src/offers/invoice.rslightning/src/offers/flow.rs
🔇 Additional comments (21)
lightning/src/offers/invoice.rs (3)
389-407: Builder‑level recurrence basetime wiring looks consistent
invoice_recurrence_basetimeis initialized toNoneinfieldsand only set viaset_invoice_recurrence_basetimeforInvoiceContents::ForOffer, with adebug_assertguarding misuse on refunds. This keeps builder state coherent and avoids runtime impact in release builds; no changes needed here.Also applies to: 421-442
1786-1839: Recurrence validation on invoices is now robust; verify alignment with BOLT12 semanticsThe recurrence block now handles
recurrence_counter == None/Some(0)/Some(>0)without any unwraps, enforces presence ofinvoice_recurrence_basetimewhenever the offer has recurrence, and checks basetime equality againstrecurrence_baseorcreated_atfor the first invoice, which addresses the prior panic risk and yields clearInvalidMetadatafailures on bad combinations. The logic also treats “optional recurrence without basetime” via the absence ofrecurrence_base; please double‑check that using presence ofrecurrence_baseas the effective discriminator between “optional” and “compulsory” offers matches your reading of the current BOLT12 draft, especially if the spec ever allows compulsory recurrence without an explicit base.
1956-2130: Tests correctly exercise the newrespond_with(..., created_at)and recurrence TLVAll the updated tests now pass an explicit
created_at(now()ornow - one_hour) intorespond_with/respond_using_derived_keys, and the TLV expectations includeinvoice_recurrence_basetime: Nonefor non‑recurring cases. This gives good coverage for the new API shape and confirms that the additional TLV is emitted only when set.Also applies to: 2182-2237, 2245-2270, 2288-2317, 2328-2376, 2390-2403, 2427-2438, 2453-2494, 2504-2569, 2584-2626, 2640-2658, 2668-2702, 2713-2780, 2790-2816, 2834-2856, 2867-2900, 2911-2943, 2953-2979, 2995-3047, 3058-3106, 3116-3201, 3206-3267, 3284-3294, 3304-3327, 3338-3367, 3371-3416, 3420-3455, 3459-3569, 3605-3637, 3647-3681, 3686-3711, 3718-3727, 3731-3760
lightning/src/offers/invoice_request.rs (5)
184-193: Recurrence fields onInvoiceRequestare introduced cleanly with sensible defaults
InvoiceRequestContentsWithoutPayerSigningPubkey::create_contentsnow initializesrecurrence_counter/start/canceltoNone, and the struct’s new fields are well‑documented with clear semantics (counter vs start offset vs cancel flag). This keeps existing flows unchanged while enabling recurrence without surprising defaults.Also applies to: 690-722
724-805: Accessors and TLV exposure for recurrence look consistent end‑to‑endThe new
recurrence_counter,recurrence_start, andrecurrence_cancelgetters are wired fromInvoiceRequestContentsdown toInvoiceRequestContentsWithoutPayerSigningPubkey::as_tlv_stream, and theInvoiceRequestTlvStreamnow carries TLVs 92–94 with the expected types. This ensures the recurrence state round‑trips between in‑memory structs and wire encoding without special‑casing.Also applies to: 1157-1213, 1236-1269, 1311-1324
1515-1610: Recurrence validation and limit enforcement are thorough; verify spec edge‑casesThe
TryFrom<PartialInvoiceRequestTlvStream>match over(offer_recurrence, offer_base, recurrence_counter, recurrence_start, recurrence_cancel)fully enumerates the allowed shapes (no‑recurrence, optional w/o base, compulsory with base), returningInvalidMetadatafor any illegal combination, and enforcesrecurrence_limitusing a saturatingperiod_index = counter.saturating_add(offset), which avoids the previous overflow risk. This is internally consistent and ensures all semantic errors surface at parse time. Please just double‑check that (a) treatingrecurrence_startas forbidden when there’s no basetime, and (b) usingoffer_base.is_some()as the discriminator for “compulsory” vs “optional” recurrence exactly matches the current BOLT12 recurrence rules you intend to implement.
1067-1087: Propagation ofrecurrence_counterintoInvoiceRequestFieldsand its TLV is correct
VerifiedInvoiceRequest::fieldsnow copiesrecurrence_counterintoInvoiceRequestFields, and the new TLV entry (type 6) plus correspondingReadable/Writeablehandling round‑trip the value asHighZeroBytesDroppedBigSize<u32>. This givesPaymentContext::Bolt12Offerthe recurrence index needed by higher layers without altering behavior for non‑recurring requests.Also applies to: 1635-1702
840-873:respond_with(..., created_at)adoption in invoice‑request flows and tests looks goodThe explicit
created_at: Durationparameter is now threaded throughrespond_with/respond_using_signing_pubkeyand the higher‑level helpers, and the tests have been updated to passnow()consistently. This aligns invoice creation time across std and no‑std builds and keeps the builder APIs coherent.Also applies to: 964-973, 991-996, 1813-1835, 3275-3282
lightning/src/offers/flow.rs (2)
46-47: Recurrence‑aware offer builder is well‑factored and preserves existing behaviorImporting
RecurrenceFieldsand extendingcreate_offer_builder_internwith anOption<RecurrenceFields>(plus the newcreate_offer_builder_with_recurrencewrapper) cleanly centralizes recurrence handling: non‑recurring offers continue to passNone, while recurring offers get.recurrence(...)applied before paths are attached. The reuse of the same blinded‑path logic keeps privacy properties identical between recurring and non‑recurring offers.Also applies to: 541-574, 603-614, 616-648, 660-674, 686-695
934-975:created_atplumbed into invoice/refund builders gives consistent time control
create_invoice_builder_from_refundand both invoice‑request helpers now accept an explicitcreated_at: Durationand pass it to the underlyingrespond_using_derived_keys/respond_withcalls, while still computing relative expiry fromDEFAULT_RELATIVE_EXPIRY. This makes the time anchor for invoices explicit and aligns with the new builder APIs; just ensure all callers are feeding a consistent epoch source (e.g.,OffersMessageFlow::duration_since_epoch) so expiry checks behave predictably across std and no‑std builds.Also applies to: 991-1029, 1045-1083
lightning/src/ln/offers_tests.rs (2)
686-686: LGTM: Test fixtures updated for recurrence support.The addition of
recurrence_counter: NonetoInvoiceRequestFieldsacross all test cases correctly reflects the new public API field. Initializing toNoneis appropriate for these non-recurrence test scenarios.Also applies to: 845-845, 967-967, 1235-1235, 1301-1301, 1399-1399
2340-2340: LGTM: API method name updated.The change from
respond_with_no_stdtorespond_using_derived_keysaligns with the API evolution described in the PR summary.lightning/src/offers/static_invoice.rs (3)
477-477: LGTM: Recurrence basetime correctly excluded from static invoices.Setting
invoice_recurrence_basetime: Nonein the TLV stream is correct for static invoices, which do not support recurrence per the BOLT12 specification.
715-718: LGTM: Validation correctly enforces static invoice constraint.The semantic check properly rejects static invoices with recurrence basetime, as required by the BOLT12 specification. The error type
UnexpectedRecurrenceis appropriate and the validation is positioned correctly in the parsing flow.
918-922: LGTM: Test assertions updated for new TLV structure.The test expectations correctly include the new recurrence-related TLV fields (all set to
None), ensuring that the TLV stream structure is validated properly for static invoices without recurrence support.Also applies to: 937-937
lightning/src/ln/channelmanager.rs (4)
9522-9543: LGTM - Idempotent counter advancement.The guard condition
data.next_payable_counter == *paid_countercorrectly ensures idempotent behavior: duplicate claims for the same counter won't advance the state since the counter would already have been incremented.
12824-12848: LGTM - Clean API for recurring offers.Good documentation and straightforward delegation to the underlying flow implementation.
17430-17453: LGTM - Serialization follows established patterns.The TLV field 23 for
active_recurrence_sessionsuses the standard optional pattern. The initialization withSome(new_hash_map())ensures the.unwrap()at line 18347 is safe.
15500-15501: Repository infrastructure timeout prevents verification ofperiod_length_secs()implementation and parsing guarantees.The sandbox environment is unable to access the repository due to persistent cloning timeouts. Without direct code inspection, I cannot verify:
- Whether
period_length_secs()returnsOption<u64>oru64- The parsing-phase validation logic for
recurrence_paywindow- The invariants that guarantee
Somewhen paywindow is setThe provided learnings indicate that recurrence field limit checks are handled during parsing, suggesting the
.unwrap()may be safe given those invariants. However, this requires manual verification against the actual implementation to confirm the.unwrap()won't panic.lightning/src/offers/refund.rs (1)
584-593: LGTM! Refund recurrence handling is correct.The changes properly handle recurrence in the refund flow:
- The updated
respond_withsignature requiring explicitcreated_ataligns with the time-aware builder pattern introduced across the PR- TLV stream construction correctly sets all recurrence fields to
Nonefor refunds- Validation appropriately rejects any recurrence fields in refund TLVs with
UnexpectedRecurrenceerrors (both offer-level and invoice-request-level fields)- Test updates consistently pass the required
created_atparameterThis correctly enforces that refunds do not support recurrence.
Also applies to: 746-772, 883-959, 966-969
lightning/src/offers/offer.rs (1)
394-402: Well-designed recurrence implementation.The recurrence support is thoughtfully implemented:
RecurrenceFieldselegantly unifies optional/compulsory variants based onrecurrence_basepresence, simplifying the API while maintaining spec compliance- Deserialization normalization (lines 1729-1762) correctly validates TLV combinations and maps to internal representation
- TLV stream construction (lines 1389-1404) properly selects compulsory vs optional encoding
RecurrenceDatasession tracking structure provides the necessary state for payee flows- Clear inline documentation explains design rationale
The type definitions with
Readable/Writeableimplementations are clean and follow LDK patterns.Also applies to: 652-955, 1318-1320, 1389-1404, 1577-1609, 1729-1762
| let period_start = data.recurrence_basetime | ||
| + period_index as u64 * recurrence_fields.recurrence.period_length_secs().unwrap(); | ||
|
|
||
| if created_at.as_secs() < period_start - window.seconds_before as u64 | ||
| || created_at.as_secs() >= period_start + window.seconds_after as u64 | ||
| { | ||
| return None | ||
| } |
There was a problem hiding this comment.
Potential arithmetic underflow in paywindow validation.
The expression period_start - window.seconds_before as u64 can underflow if seconds_before exceeds period_start. This could occur for early periods with large seconds_before values, causing the check to behave unexpectedly.
Consider using saturating subtraction:
- if created_at.as_secs() < period_start - window.seconds_before as u64
+ if created_at.as_secs() < period_start.saturating_sub(window.seconds_before as u64)
|| created_at.as_secs() >= period_start + window.seconds_after as u64Also verify that period_start + window.seconds_after cannot overflow (though this is less likely given practical values).
📝 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 period_start = data.recurrence_basetime | |
| + period_index as u64 * recurrence_fields.recurrence.period_length_secs().unwrap(); | |
| if created_at.as_secs() < period_start - window.seconds_before as u64 | |
| || created_at.as_secs() >= period_start + window.seconds_after as u64 | |
| { | |
| return None | |
| } | |
| let period_start = data.recurrence_basetime | |
| period_index as u64 * recurrence_fields.recurrence.period_length_secs().unwrap(); | |
| if created_at.as_secs() < period_start.saturating_sub(window.seconds_before as u64) | |
| || created_at.as_secs() >= period_start + window.seconds_after as u64 | |
| { | |
| return None | |
| } |
🤖 Prompt for AI Agents
In lightning/src/ln/channelmanager.rs around lines 15500 to 15507, the
subtraction period_start - window.seconds_before as u64 can underflow for early
periods and the addition period_start + window.seconds_after as u64 could
overflow; replace the subtraction with a saturating_sub (e.g.,
period_start.saturating_sub(window.seconds_before as u64)) and replace the
addition with either checked_add (and handle the None case conservatively) or
saturating_add to prevent overflow, ensuring all casts to u64 are correct and
handling the checked_add None by treating it as out-of-range (return None) or
using a sensible saturating bound.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lightning/src/ln/channelmanager.rs (1)
15503-15507: Potential arithmetic underflow in paywindow validation.The expression
period_start - window.seconds_before as u64can underflow ifseconds_beforeexceedsperiod_start, causing the check to behave unexpectedly (wrapping to a very large value).Apply this diff to use saturating subtraction:
- if created_at.as_secs() < period_start - window.seconds_before as u64 + if created_at.as_secs() < period_start.saturating_sub(window.seconds_before as u64) || created_at.as_secs() >= period_start + window.seconds_after as u64Also consider using
saturating_addfor the addition to be consistent, though overflow is less likely given practical values.
🧹 Nitpick comments (8)
lightning/src/ln/offers_tests.rs (1)
680-688: Recurrence field added toInvoiceRequestFieldstest expectationsSetting
recurrence_counter: Nonein theseInvoiceRequestFieldsconstructions keeps the existing non-recurring offer/refund tests consistent with the new public struct shape and with how the production code will populate these fields for one-shot offers.If you later introduce tests that exercise recurring offers through this path, it may be worth refactoring these expectations to derive fields from the actual
invoice_request(or a helper) instead of hard-coding all fields, so new recurrence-related fields don’t have to be manually mirrored in multiple places. For now, this is perfectly fine and non-blocking.Also applies to: 838-847, 960-969, 1228-1237, 1393-1401
lightning/src/offers/static_invoice.rs (1)
457-488: Recurrence TLVs are correctly disallowed for static invoicesThe plumbing here looks solid:
InvoiceContents::as_tlv_stream()now always emitsinvoice_recurrence_basetime: Nonefor static invoices.- The
TryFrom<PartialInvoiceTlvStream> for InvoiceContentspath destructuresinvoice_recurrence_basetimeand returnsBolt12SemanticError::UnexpectedRecurrencewhen it’sSome, after the usual semantic checks.This cleanly enforces the “static invoices MUST NOT set recurrence” rule while allowing the shared invoice TLV structs to grow recurrence support.
Optional follow-up: add a focused test that constructs a TLV stream with
invoice_recurrence_basetimeset (e.g., by mutatinginvoice.as_tlv_stream()in the tests, similar to other negative cases) and asserts that parsing fails withUnexpectedRecurrence, to lock in this semantic guard.Also applies to: 660-733
lightning/src/ln/channelmanager.rs (3)
13267-13272: Consider extracting timestamp retrieval to a helper method.This pattern for obtaining
created_atis duplicated at lines 15447-15452. For a PoC this is acceptable, but consider extracting to a private helper method likefn current_timestamp(&self) -> Durationfor maintainability.fn current_timestamp(&self) -> Duration { #[cfg(not(feature = "std"))] { Duration::from_secs(self.highest_seen_timestamp.load(Ordering::Acquire) as u64) } #[cfg(feature = "std")] { std::time::SystemTime::now() .duration_since(std::time::SystemTime::UNIX_EPOCH) .expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH") } }
15549-15552: Avoid usingOption::map()for side effects.Using
mappurely for side effects discards the return value and obscures intent. Useif letfor clarity.- recurrence_basetime.map(|basetime| - builder.set_invoice_recurrence_basetime(basetime) - ); + if let Some(basetime) = recurrence_basetime { + builder.set_invoice_recurrence_basetime(basetime); + }
15578-15581: Same issue: avoid usingOption::map()for side effects.Apply the same fix as suggested for the derived keys path above.
- Ok((mut builder, context)) => { - recurrence_basetime.map(|basetime| - builder.set_invoice_recurrence_basetime(basetime) - ); + Ok((mut builder, context)) => { + if let Some(basetime) = recurrence_basetime { + builder.set_invoice_recurrence_basetime(basetime); + }lightning/src/offers/invoice_request.rs (1)
1515-1610: Recurrence combination validation and limit enforcement are soundThe
matchover(offer_recurrence, offer_base, recurrence_counter, recurrence_start, recurrence_cancel)cleanly separates:
- Non‑recurring offers (no recurrence TLVs allowed).
recurrence_optional(no base): allowing either no recurrence fields,counteronly, orcounter>0pluscancel, and rejectingrecurrence_startentirely.recurrence_compulsory(with base): requiring bothcounterandstart, forbidding cancel on the first period, and rejecting all other mixes.The subsequent limit enforcement:
- Only runs when the offer defines a recurrence limit and the request actually sets a
recurrence_counter.- Uses
counter.saturating_add(offset)to deriveperiod_index, avoiding overflow while still clamping correctly.- Rejects requests whose derived
period_indexexceeds the offer’s limit.This gives a clear, fail‑closed validation surface without panics or unchecked unwraps and centralizes recurrence limit checks at parse time, which aligns with the intent to avoid re‑checking in the runtime recurrence logic. Based on learnings, …
Also applies to: 1468-1485
lightning/src/offers/invoice.rs (2)
393-407: Invoice recurrence basetime field and setter are wired correctly; minor ergonomics nit
InvoiceFieldsnow carriesinvoice_recurrence_basetime: Option<u64>, defaulted toNoneinfields(), and exposed in TLVs via the new id 177. The builder‑sideset_invoice_recurrence_basetime:
- Correctly sets the basetime only for
InvoiceContents::ForOffer.- Uses a
debug_assert!to guard against misuse onForRefund, which is acceptable given it’spub(crate).This is a clean way to anchor recurring schedules at the invoice level. If you expect this setter to be used from multiple call sites, you might eventually prefer returning a
Resulton misuse instead of a debug‑only assertion, but that’s an optional ergonomic improvement rather than a correctness issue.Also applies to: 421-442, 769-811
1711-1755: Recurrence basetime parsing and validation for invoices is robustIn
InvoiceContents::try_from(PartialInvoiceTlvStream):
- The new
invoice_recurrence_basetimeis read from the invoice TLVs and stored inInvoiceFields.- Refund invoices are explicitly required not to carry a basetime; such a message is rejected early.
- For invoices that correspond to an offer with recurrence:
- A basetime is mandatory; its absence yields
InvalidMetadata.- When
recurrence_counterisNone, the basetime is still required but no further recurrence checks are applied, treating the invoice as a non‑recurring payment from a recurring offer.- For
counter == 0, basetime must equal the offer’s explicitrecurrence_baseif present, or the invoice’screated_attimestamp if not.- For
counter > 0, additional “SHOULD” consistency checks with prior invoices are documented as a TODO and intentionally left to higher layers.This avoids any unwraps, enforces the key MUST‑level invariants at parse time, and aligns invoice semantics with the recurrence data carried by the originating offer and invoice request.
Also applies to: 1756-1777, 1786-1839
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
fuzz/src/invoice_request_deser.rs(4 hunks)fuzz/src/refund_deser.rs(3 hunks)lightning/src/ln/channelmanager.rs(15 hunks)lightning/src/ln/offers_tests.rs(7 hunks)lightning/src/ln/outbound_payment.rs(3 hunks)lightning/src/offers/flow.rs(11 hunks)lightning/src/offers/invoice.rs(60 hunks)lightning/src/offers/invoice_request.rs(21 hunks)lightning/src/offers/offer.rs(14 hunks)lightning/src/offers/parse.rs(1 hunks)lightning/src/offers/refund.rs(15 hunks)lightning/src/offers/static_invoice.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:21.999Z
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.828Z
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.
📚 Learning: 2025-11-28T12:47:06.828Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15468-15486
Timestamp: 2025-11-28T12:47:06.828Z
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/outbound_payment.rslightning/src/ln/channelmanager.rsfuzz/src/invoice_request_deser.rslightning/src/offers/parse.rslightning/src/offers/static_invoice.rslightning/src/ln/offers_tests.rslightning/src/offers/refund.rslightning/src/offers/flow.rslightning/src/offers/invoice_request.rslightning/src/offers/invoice.rslightning/src/offers/offer.rs
📚 Learning: 2025-11-28T12:48:21.999Z
Learnt from: shaavan
Repo: shaavan/rust-lightning PR: 8
File: lightning/src/ln/channelmanager.rs:15488-15508
Timestamp: 2025-11-28T12:48:21.999Z
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.rsfuzz/src/invoice_request_deser.rslightning/src/offers/parse.rslightning/src/offers/static_invoice.rslightning/src/ln/offers_tests.rslightning/src/offers/refund.rslightning/src/offers/flow.rslightning/src/offers/invoice_request.rslightning/src/offers/invoice.rslightning/src/offers/offer.rs
🧬 Code graph analysis (2)
lightning/src/ln/channelmanager.rs (2)
lightning/src/offers/flow.rs (1)
create_offer_builder_with_recurrence(631-648)lightning/src/offers/invoice_request.rs (3)
payer_signing_pubkey(1203-1205)recurrence_counter(1191-1193)recurrence_cancel(1199-1201)
lightning/src/offers/flow.rs (2)
lightning/src/offers/offer.rs (13)
offer(2319-2319)offer(2330-2330)offer(2409-2409)offer(2418-2418)offer(2460-2460)offer(2476-2476)offer(2485-2485)offer(2500-2500)offer(2506-2506)offer(2514-2514)offer(2522-2522)offer(2530-2530)recurrence_fields(1318-1320)lightning/src/offers/static_invoice.rs (1)
created_at(539-541)
⏰ 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). (14)
- GitHub Check: build-tx-sync (macos-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
- GitHub Check: semver-checks
- GitHub Check: benchmark
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (macos-latest, 1.75.0)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: benchmark
- 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 (59)
lightning/src/ln/outbound_payment.rs (3)
3209-3211: LGTM! Test correctly updated to use newrespond_withAPI.The test now passes
created_atvianow()to the updatedrespond_withmethod, aligning with the API change that requires an explicit timestamp.
3256-3258: Test correctly updated for new API signature.
3319-3321: Test correctly updated for new API signature.lightning/src/offers/refund.rs (7)
584-593: API signature change looks correct.The
respond_withmethod now takes an explicitcreated_at: Durationparameter instead of relying on internal time sources. This is a breaking API change but aligns with the broader effort to support explicit time handling for recurrence features.
605-619: Derived keys variant correctly updated withcreated_atparameter.
746-772: Recurrence TLV fields correctly initialized toNonefor refunds.Refunds do not support recurrence, so all recurrence-related fields in the TLV stream are appropriately set to
None.
883-900: TLV stream parsing correctly destructures new recurrence fields.
952-969: Validation correctly rejects recurrence fields in refunds.Both offer-level recurrence fields (
recurrence_compulsory,recurrence_optional, etc.) and invoice-request-level fields (recurrence_counter,recurrence_start,recurrence_cancel) are properly validated and rejected withUnexpectedRecurrenceerror. This is appropriate since refunds should not contain recurrence information.
1095-1116: Test TLV stream expectations correctly updated with recurrence fields.
1158-1164: Test correctly updated to use newrespond_withsignature.lightning/src/offers/parse.rs (1)
227-230: New error variants are well-defined and appropriately documented.
UnexpectedRecurrencehandles cases where recurrence fields appear in contexts that don't support them (e.g., refunds), whileInvalidRecurrencehandles malformed recurrence data. Both variants integrate well with the broader parsing framework.fuzz/src/refund_deser.rs (2)
73-74: Network context now properly defined.This addresses the previous review feedback where
networkwas undefined. The explicitNetwork::Bitcoinandgenesis_block(network)initialization provides a deterministic context for timestamp computation.
116-118: Timestamp computation and API call correctly updated.Using the genesis block header time provides a deterministic and reproducible timestamp for fuzz testing. The
respond_withcall now correctly passes the requirednowparameter.fuzz/src/invoice_request_deser.rs (3)
87-89: Network and genesis block initialization is consistent with refund_deser.rs.Good pattern consistency across fuzz tests.
100-108:InvoiceRequestFieldscorrectly includes newrecurrence_counterfield.The field is set to
Nonewhich is appropriate for fuzz testing where recurrence is not being exercised.
154-155: Timestamp computation and respond_with call correctly updated.Consistent with the pattern in refund_deser.rs.
lightning/src/ln/offers_tests.rs (1)
2334-2343: Updatedrespond_using_derived_keyscall looks correctSwitching to
request.respond_using_derived_keys(payment_paths, payment_hash, created_at)and feedingcreated_atfromalice.node.duration_since_epoch()is consistent with the new time-aware API and preserves the intent of this test (forcing an invoice with unknown required features). No issues here.lightning/src/offers/static_invoice.rs (1)
906-943: TLV expectations for new recurrence fields in tests are in syncThe
builds_invoice_for_offer_with_defaultstest’s expectedOfferTlvStreamRefandInvoiceTlvStreamRefnow include:
recurrence_compulsory,recurrence_optional,recurrence_base,recurrence_paywindow,recurrence_limitfor the offer side, andinvoice_recurrence_basetimefor the invoice side,all set to
None. This matches the non-recurring semantics of static invoices and ensures the test suite stays aligned with the expanded TLV structs.Nothing to change here.
lightning/src/ln/channelmanager.rs (10)
50-56: LGTM!Import additions are appropriate for the new recurrence functionality.
93-103: LGTM!Import additions correctly bring in the necessary types for recurrence handling.
2676-2688: Well-documented field addition.The documentation clearly explains the purpose and usage of
active_recurrence_sessions. Note that session cleanup mechanisms were discussed in a previous review and remain a consideration for production readiness.
3979-3980: LGTM!Correct initialization of the recurrence sessions map.
9519-9543: LGTM!The recurrence counter advancement logic correctly ensures idempotency—duplicate claims for the same counter value won't cause the state to advance, as the equality check will fail after the first successful advancement.
12824-12848: LGTM!The new API follows the established pattern of the existing
create_offer_buildermethod and provides clear documentation about its purpose for recurring offers.
15434-15435: LGTM!Good naming choice for
verified_invoice_requestto clearly indicate the verification state.
17430-17453: LGTM!The serialization setup correctly uses an optional TLV field (23) for backwards compatibility with nodes that don't have recurrence sessions.
18347-18348: LGTM!The
unwrap()is safe here sinceactive_recurrence_sessionsis initialized withSome(new_hash_map())at line 17433 before the TLV read.
15500-15501: I'll verify the review comment about the potential panic fromunwrap()onperiod_length_secs(). Let me start by exploring the code structure and method implementation.
<function_calls>
shell
#!/bin/bashFirst, find the period_length_secs method definition
rg -n 'fn period_length_secs' --type rust
</function_calls><function_calls>
shell
#!/bin/bashGet context around line 15500-15501 to understand the code
sed -n '15490,15510p' lightning/src/ln/channelmanager.rs
</function_calls><function_calls>
shell
#!/bin/bashSearch for period_length_secs more broadly to understand its return type
rg -B5 -A10 'period_length_secs' --type rust | head -100
</function_calls>lightning/src/offers/invoice_request.rs (4)
184-195: Recurrence fields and TLV wiring look consistent end‑to‑endThe new recurrence fields (
recurrence_counter,recurrence_start,recurrence_cancel) are correctly:
- Initialized in
create_contentstoNone.- Added to
InvoiceRequestContentsWithoutPayerSigningPubkeywith clear semantics docs.- Exposed via
InvoiceRequestContentsandinvoice_request_accessors!.- Wired into
InvoiceRequestTlvStreamRefand the TLV stream macro with types 92–94.Struct layout, accessors, and TLV encoding/decoding are internally consistent and match the intended semantics.
Also applies to: 681-722, 1191-1202, 1250-1261, 1312-1324
1070-1087: Propagation ofrecurrence_counterintoInvoiceRequestFieldsand tests is correct
fields_accessor!now threadsrecurrence_counterfromInvoiceRequestContentsWithoutPayerSigningPubkeyintoInvoiceRequestFields, and theWriteable/Readableimpls serialize it as TLV id 6 withHighZeroBytesDroppedBigSize<u32>. The tests incopies_verified_invoice_request_fieldsassert that:
- The new field is present and
Noneby default.- Round‑tripping via TLV preserves it.
This cleanly exposes recurrence context to
PaymentContext::Bolt12Offerwithout altering existing fields.Also applies to: 1636-1662, 1672-1701, 3272-3282
840-870:respond_withAPI updated to be time‑aware; docs and tests are alignedThe
respond_with/respond_with_using_signing_pubkeysignatures now take acreated_at: core::time::Duration, and:
- Documentation correctly states that default expiry is computed relative to this
created_at.- Call sites in tests have been updated to pass an explicit
now()duration, keeping behavior deterministic and consistent withinvoice.rs’s builder logic.This is a straightforward, well‑integrated API tightening with no visible regressions in the surrounding tests.
Also applies to: 1895-1903, 2472-2483
1792-1831: Default‑path tests correctly assert new recurrence TLV defaultsIn
builds_invoice_request_with_defaults, the updated expectations forInvoiceRequestTlvStreamRefnow include all recurrence TLVs and assert they areNone. This ensures:
- Adding the new fields doesn’t change the encoding of existing non‑recurring flows.
- Future regressions (e.g., accidentally populating defaults) will be caught.
Test coverage for the default case looks solid.
lightning/src/offers/invoice.rs (3)
1437-1438: TLV 177 forinvoice_recurrence_basetimeis consistent with the new fieldThe new
invoice_recurrence_basetimeis:
- Emitted from
InvoiceFields::as_tlv_streamintoInvoiceTlvStreamRef.- Declared in
InvoiceTlvStreamas type 177 with(u64, HighZeroBytesDroppedBigSize)encoding.This keeps TLV representation in sync with the struct and matches the existing encoding style of other scalar u64 fields.
Also applies to: 1519-1520
390-407: Time‑aware builders and example/docs updates are coherentAcross
invoice_explicit_signing_pubkey_builder_methods!andinvoice_derived_signing_pubkey_builder_methods!, the builder constructors now take an explicitcreated_at: Duration, and the doc examples at the top of the file have been updated to callrespond_with(..., Duration::from_secs(0))/respond_with(..., pubkey, Duration::from_secs(0))accordingly.The new
fieldshelper stores thiscreated_atinInvoiceFields, which:
- Drives
created_at()accessors and expiry checks.- Keeps non‑
stdexpiry logic usingis_offer_or_refund_expired_no_std(self.invoice.created_at())consistent.The changes are cohesive and keep the public usage examples in sync with the API.
Also applies to: 219-239, 292-313
1437-1438: Tests updated appropriately forcreated_atplumbing and recurrence defaultsThe invoice tests now:
- Pass an explicit
now()Durationinto allrespond_with/respond_using_derived_keyshelpers.- Assert that, for non‑recurring offers/refunds,
invoice_recurrence_basetimeinInvoiceTlvStreamRefisNone, matching the defaulting behavior.- Exercise the rest of the TLV surface (paths, created_at, relative_expiry, payment_hash, amount, node_id, etc.) as before.
This gives good coverage for the new time‑aware API and confirms that recurrence basetime does not leak into non‑recurring flows by accident.
Also applies to: 1957-2130, 2132-3760
lightning/src/offers/offer.rs (14)
250-250: LGTM: Recurrence field initialization in builders.Both
OfferBuilder::newandderiving_signing_pubkeycorrectly initializerecurrence_fields: Noneas the default, allowing recurrence to be optionally set via the builder pattern.Also applies to: 305-305
652-709: Well-documented recurrence types with clear spec commentary.The
TimeUnit,Recurrence, andperiod_length_secs()helper are well-designed. The approximations for days (86,400s) and months (30 days) are reasonable for a PoC and clearly documented.One minor observation: the
debug_assert!on overflow at line 705 will only trigger in debug builds. In release builds, the function silently returnsNone, which is the correct behavior for production.
711-741: Readable/Writeable implementations are correct.The serialization uses
HighZeroBytesDroppedBigSizeconsistently with BOLT12 conventions. The deserialization correctly validates:
time_unit_bytemust be 0, 1, or 2periodmust be non-zeroThis matches the expected wire format.
779-798: RecurrenceBase serialization handles proportional flag correctly.The boolean is serialized as a single byte (0 or 1), and deserialization correctly rejects any other value. The basetime uses
HighZeroBytesDroppedBigSizefor compact encoding.
821-834: RecurrencePaywindow serialization is straightforward and correct.Both
seconds_beforeandseconds_afterare serialized using the expected compact format.
846-860: RecurrenceLimit correctly validates non-zero values.The deserialization rejects
value.0 == 0as invalid, which aligns with the semantic requirement that a limit of zero periods would be meaningless.
862-906: RecurrenceData TLV structure is well-documented.The fields capture the necessary state for tracking recurrence sessions on the payee side. Using
impl_writeable_tlv_based!with appropriate field types ensures consistent serialization.
908-954: RecurrenceFields design rationale is clearly documented.The decision to collapse
recurrence_optionalandrecurrence_compulsoryinto a single struct based on presence ofrecurrence_baseis well-reasoned and maintains backward compatibility with non-recurrence-aware nodes.
394-402: Builder methodrecurrence()follows established patterns.The method signature and implementation are consistent with other builder methods in this file, using the macro-based approach.
1028-1033: Accessorrecurrence_fields()correctly exposed in offer_accessors macro.The accessor returns the optional
RecurrenceFieldsvalue, consistent with other optional offer fields.
1389-1423: TLV stream serialization logic correctly maps RecurrenceFields.The serialization logic at lines 1389-1404 correctly determines whether to emit
recurrence_compulsoryorrecurrence_optionalbased on presence ofrecurrence_base. This maintains the invariant established in the design rationale.
1729-1762: TLV deserialization normalization is correct and well-documented.The match statement correctly handles the three valid cases:
- No recurrence TLVs →
Nonerecurrence_optionalwithout basetime → optional recurrencerecurrence_compulsorywith basetime → compulsory recurrenceThe catch-all case correctly returns
InvalidMetadatafor malformed combinations (e.g., both compulsory and optional present, or basetime without period).
1577-1609: TLV stream definitions follow BOLT12 conventions.The TLV type numbers (24, 25, 26, 27, 29) are correctly placed in the offer range and the inline comments provide helpful context for implementation clarity.
1853-1875: Test correctly validates default recurrence_fields is None.The test
builds_offer_with_defaultsproperly asserts thatrecurrence_fields()returnsNoneby default and that all recurrence-related TLV fields areNonein the TLV stream.lightning/src/offers/flow.rs (10)
46-46: RecurrenceFields import added correctly.The import enables use of
RecurrenceFieldsin the newcreate_offer_builder_with_recurrencemethod.
541-573: Internal builder method correctly extended for recurrence support.The
create_offer_builder_internmethod now acceptsrecurrence_fields: Option<RecurrenceFields>and applies it to the builder when present (lines 565-567). This keeps the common logic in one place while allowing both recurrence and non-recurrence offer creation paths.
602-614: Existing create_offer_builder correctly passes None for recurrence.The existing public API maintains backward compatibility by passing
Nonefor recurrence fields.
616-648: New create_offer_builder_with_recurrence method is well-documented.The method provides a clean public API for creating recurring offers. The documentation correctly references the parent method and explains the purpose of
RecurrenceFields. The implementation delegates to the internal method withSome(recurrence_fields).
667-674: create_offer_builder_using_router updated consistently.Passes
Nonefor recurrence fields, maintaining consistency with the non-router variant.
692-695: create_async_receive_offer_builder updated consistently.Passes
Nonefor recurrence fields, which is appropriate since async receive offers have different semantics.
933-975: create_invoice_builder_from_refund now accepts explicit created_at.The signature change from implicit time derivation to explicit
created_at: Durationparameter enables consistent time handling across std and no-std builds. The parameter is passed through torespond_using_derived_keysat line 968.
991-1029: create_invoice_builder_from_invoice_request_with_keys uses explicit created_at.The method now accepts
created_at: Durationand passes it torespond_using_derived_keys(line 1023). This aligns with the unified time handling approach across invoice creation paths.
1045-1083: create_invoice_builder_from_invoice_request_without_keys uses explicit created_at.Consistent with the derived-keys variant, this method now accepts and propagates
created_attorespond_with(line 1077).
933-935: Perfect! I have now verified all call sites. Here are my findings:All three public methods are properly updated with
created_atparameters at their call sites:
Line 13274 (
create_invoice_builder_from_refund): Thecreated_atparameter is passed on line 13284 (the last parameter before the closing parenthesis)Line 15540 (
create_invoice_builder_from_invoice_request_with_keys): Thecreated_atparameter is passed on line 15545Line 15569 (
create_invoice_builder_from_invoice_request_without_keys): Thecreated_atparameter is passed on line 15574All call sites in
lightning/src/ln/channelmanager.rshave been properly updated to pass the newcreated_at: Durationparameter.
All callers have been updated to pass the
created_at: Durationparameter correctly. The three public methods are called fromlightning/src/ln/channelmanager.rsonly, and all three call sites properly provide the timestamp value.
| check_invoice_signing_pubkey(&fields.signing_pubkey, &offer_tlv_stream)?; | ||
|
|
||
| if offer_tlv_stream.issuer_id.is_none() && offer_tlv_stream.paths.is_none() { | ||
| // Recurrence should not be present in Refund. | ||
| if fields.invoice_recurrence_basetime.is_some() { | ||
| return Err(Bolt12SemanticError::InvalidAmount); | ||
| } | ||
|
|
There was a problem hiding this comment.
Error variant for basetime on refunds is slightly misleading
For the refund branch, any presence of invoice_recurrence_basetime results in Err(Bolt12SemanticError::InvalidAmount). Functionally this is fine (the invoice is rejected), but semantically the situation is closer to malformed metadata than an amount problem.
Consider switching this to InvalidMetadata (or a more specific variant if added later) to keep error categorization aligned with what actually went wrong:
- if fields.invoice_recurrence_basetime.is_some() {
- return Err(Bolt12SemanticError::InvalidAmount);
- }
+ if fields.invoice_recurrence_basetime.is_some() {
+ return Err(Bolt12SemanticError::InvalidMetadata);
+ }This would make debugging and error handling a bit clearer without changing behavior.
🤖 Prompt for AI Agents
In lightning/src/offers/invoice.rs around lines 1756 to 1763, the refund branch
returns Err(Bolt12SemanticError::InvalidAmount) when invoice_recurrence_basetime
is present; change this to return Err(Bolt12SemanticError::InvalidMetadata) (or
a more specific metadata-related variant if your error enum provides one) so the
error semantics match malformed metadata rather than an amount issue; ensure the
chosen enum variant exists and adjust imports or tests accordingly.
Summary by CodeRabbit
New Features
Improvements
API Changes
✏️ Tip: You can customize this high-level summary in your review settings.