Introduce recurrence TLV support for BOLT12 messages#4614
Conversation
Add draft BOLT 12 recurrence fields to `Offer`, including the public recurrence types and the TLV mapping for optional and compulsory recurrence. Update offer serialization and parsing to preserve `recurrence`, `recurrence_base`, `recurrence_paywindow`, and `recurrence_limit`. Reject recurrence TLVs on refund-like message types that do not support them, and update offer-related test fixtures for the expanded TLV stream. This introduces the offer-side recurrence data model and wire handling without yet implementing recurrence-aware invoice-request validation or payment flow behavior. Spec reference: BOLT 12 offers recurrence TLV fields draft.
Add targeted unit tests for the new offer recurrence TLV behavior. The tests cover compulsory and optional recurrence parsing, invalid recurrence field combinations on offers, direct `start_time` coverage for second/day/month schedules, and rejection of recurrence TLVs on refunds. This locks in the current wire-level recurrence behavior and the calendar-based `start_time` helper semantics, while guarding the message-type boundary where refunds must reject recurrence fields. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add parsing and serialization for recurring invoice request fields, including the recurrence counter, basetime start offset, cancellation flag, and the provisional request-side recurrence token. Validate that invoice requests use the recurrence form required by the offer and thread the new fields through the shared invoice-request and refund TLV plumbing plus the payment context copies used when handling offer payments. Spec reference: https://github.com/rustyrussell/bolts/blob/guilt/offers-recurrence/12-offer-encoding.md#tlv-fields-for-invoice_request
Add focused invoice request tests for the new recurrence TLVs. The new coverage exercises both supported recurrence encodings and the invalid combinations that should be rejected during parsing. Also extend refund parsing coverage to assert that invoice-request recurrence TLVs remain rejected on refund messages, which protects the shared TLV plumbing added by the recurrence feature commit. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add invoice-side recurrence fields so recurring offers can carry a resolved basetime and opaque token forward to the next invoice request. This wires recurrence state through invoice creation, serialization, and static-invoice validation. The change is needed to let invoice handling participate in the recurrence flow introduced on offers and invoice requests, while keeping refunds and static invoices strict about rejecting unexpected recurrence data. Document the new public recurrence API surfaced by this work so the commit builds cleanly under deny(missing_docs).
Add targeted coverage for the invoice recurrence fields introduced by the preceding feature commit. The new tests prove that recurring invoice TLVs round-trip on offer invoices, that incomplete recurrence TLVs are rejected, and that refund/static invoice parsers still reject unexpected recurrence data. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
I've assigned @joostjager as a reviewer! |
| if recurrence_token.is_some() { | ||
| // TODO: "Token verification and basetime resolve logic to be implemented in follow-up commits." | ||
| Err(()) |
There was a problem hiding this comment.
Bug: verify_recurrence_token unconditionally returns Err(()) for any token-bearing request, which means verify_using_metadata / verify_using_recipient_data will fail entirely for any recurring invoice request that echoes a token (i.e., every follow-up recurring payment once the payee starts issuing tokens).
This makes the verified path — the only path that populates resolved_basetime — completely broken for follow-up recurring payments with tokens. Since the respond_using_derived_keys* methods on VerifiedInvoiceRequest are the primary production API for creating invoices, this silently prevents the recurring flow from working end-to-end.
If this is intentionally deferred, consider returning Ok(None) with a debug_assert! or a log warning instead of hard-failing. Hard-failing Err(()) means any payer sending a valid recurring request with a token gets silently dropped with no indication of why.
| /// Verifies that the request was for an offer created using the given key by checking the | ||
| /// metadata from the offer. | ||
| /// metadata from the offer. |
There was a problem hiding this comment.
Bug (formatting): The doc-comment continuation lines for verify_using_metadata have been corrupted by the insertion of verify_recurrence_token above. The first line of the doc comment has no leading tab, but the continuation line has triple-tab indentation (\t\t\t///). This will produce garbled rustdoc output. Same issue affects verify_using_recipient_data below at line ~1087.
Should be:
| /// Verifies that the request was for an offer created using the given key by checking the | |
| /// metadata from the offer. | |
| /// metadata from the offer. | |
| /// Verifies that the request was for an offer created using the given key by checking the | |
| /// metadata from the offer. |
| /// Verifies that the request was for an offer created using the given key by checking a nonce | ||
| /// included with the [`BlindedMessagePath`] for which the request was sent through. | ||
| /// included with the [`BlindedMessagePath`] for which the request was sent through. |
There was a problem hiding this comment.
Same doc-comment indentation corruption as above:
| /// Verifies that the request was for an offer created using the given key by checking a nonce | |
| /// included with the [`BlindedMessagePath`] for which the request was sent through. | |
| /// included with the [`BlindedMessagePath`] for which the request was sent through. | |
| /// Verifies that the request was for an offer created using the given key by checking a nonce | |
| /// included with the [`BlindedMessagePath`] for which the request was sent through. |
| .map(Some) | ||
| .ok_or(Bolt12SemanticError::InvalidMetadata) | ||
| }, | ||
| (None, None) => Ok(Some(created_at.as_secs())), |
There was a problem hiding this comment.
Bug (latent): When the offer has Recurrence::Optional and the invoice request has no recurrence fields (a valid non-recurring payment), this code path reaches (None, None) and returns Ok(Some(created_at.as_secs())). This is incorrect — a non-recurring payment shouldn't produce a basetime.
Currently harmless because recurrence_fields() always returns Ok(None), but when that TODO is completed, this will cause the invoice builder to receive a spurious basetime for non-recurring payments to optional-recurrence offers.
Consider adding a guard that returns Ok(None) when the invoice request has no recurrence fields:
(None, None) => {
// Non-recurring payment to optional-recurrence offer
if $contents.invoice_request_recurrence().is_none() {
Ok(None)
} else {
Ok(Some(created_at.as_secs()))
}
}| pub struct WithOfferBasetimeRecurrence { | ||
| counter: u32, | ||
| start: u32, | ||
| token: Option<Vec<u8>>, | ||
| cancel: Option<()>, | ||
| } | ||
|
|
||
| /// Recurrence fields for invoice requests whose offer recurrence is anchored to the first | ||
| /// successful payment instead of an explicit basetime. | ||
| /// | ||
| /// `counter` identifies which recurring request this is in the payer's sequence. `token`, when | ||
| /// present, is the opaque recurrence token echoed from the previous invoice. `cancel`, when | ||
| /// present, marks the request as a recurrence cancellation. | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct WithoutOfferBasetimeRecurrence { | ||
| counter: u32, | ||
| token: Option<Vec<u8>>, | ||
| cancel: Option<()>, | ||
| } |
There was a problem hiding this comment.
API issue: WithOfferBasetimeRecurrence and WithoutOfferBasetimeRecurrence are pub structs with private fields, and InvoiceRequestRecurrence::new() is pub(crate). This means external crate users can call the public set_invoice_request_recurrence() builder method but cannot construct the InvoiceRequestRecurrence value it requires. Either:
- Add public constructor methods / builder pattern for the recurrence types, or
- Make the struct fields
pub, or - Add a public
InvoiceRequestRecurrence::new(...)constructor.
| pub struct InvoiceRecurrence { | ||
| recurrence_basetime: u64, | ||
| recurrence_token: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
API issue: InvoiceRecurrence is pub but both fields are private and there are no accessor methods. Consumers of Bolt12Invoice have no way to read the recurrence basetime or token from a received invoice. At minimum, add public getters:
impl InvoiceRecurrence {
pub fn recurrence_basetime(&self) -> u64 { self.recurrence_basetime }
pub fn recurrence_token(&self) -> &[u8] { &self.recurrence_token }
}| (6, recurrence_counter.map(HighZeroBytesDroppedBigSize), option), | ||
| (8, recurrence_start.map(HighZeroBytesDroppedBigSize), option), | ||
| (10, recurrence_cancel, option), | ||
| // Speculative request-side recurrence token, distinct from the future invoice-side token. | ||
| (12, recurrence_token.map(|token| WithoutLength(token)), option), |
There was a problem hiding this comment.
Backward compatibility concern: All new TLV types (6, 8, 10, 12) are even. Per the LDK TLV framework, unknown even types cause DecodeError::UnknownRequiredFeature. This means older LDK versions will fail to deserialize any InvoiceRequestFields containing recurrence data.
Since InvoiceRequestFields is embedded in Bolt12OfferContext (stored in blinded payment paths), this will cause payment context decoding failures on nodes that haven't upgraded, potentially breaking payment processing during rolling upgrades.
Consider using odd TLV types (e.g., 7, 9, 11, 13) so older nodes can gracefully ignore the recurrence fields they don't understand.
Review SummaryIssues found in PR #4614: Inline comments posted:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4614 +/- ##
==========================================
- Coverage 87.08% 86.43% -0.66%
==========================================
Files 161 158 -3
Lines 109255 110131 +876
Branches 109255 110131 +876
==========================================
+ Hits 95147 95188 +41
- Misses 11627 12366 +739
- Partials 2481 2577 +96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| pub(crate) fn recurrence_fields( | ||
| _invoice_request: &InvoiceRequest, _recurrence_basetime: Option<u64>, | ||
| ) -> Result<Option<InvoiceRecurrence>, Bolt12SemanticError> { | ||
| //TODO: Future commits will introduce the recurrence token creation logic |
There was a problem hiding this comment.
Should we just do this now? This PR isn't, really, all that big.
|
|
||
| /// Sets all recurrence-related fields for this invoice request in one call. | ||
| /// | ||
| /// `invoice_request_recurrence` must match the offer's recurrence configuration. Use |
There was a problem hiding this comment.
This is a weird API - instead of having the caller tell us exactly what they want in the invoice, the caller should tell us the information they know (the current time, maybe when they first paid the offer, etc) and we should calculate what should go in the invreq.
| /// Explicit base time for period 0 when the offer defines one. | ||
| base: Option<RecurrenceBase>, | ||
| /// Fields shared by both recurrence encodings. | ||
| fields: RecurrenceFields, |
There was a problem hiding this comment.
Should we have Recurrance be a struct where the fields are inlined and the compulsory/not is a bool or a separate enum? That would simplify accessing the fields.
| /// - the entire previous period, PLUS | ||
| /// - the entire current period being paid for. | ||
| // | ||
| // Spec Commentary: |
There was a problem hiding this comment.
I assume the commentary comments are intended to be removed pre-merge?
4 participants
This PR introduces the initial recurrence support needed for recurring BOLT12 flows in LDK and adds recurrence TLV support across BOLT12 messages.
Notes
This PR primarily wires recurrence serialization, parsing, validation, and message propagation through the BOLT12 flow.
The opaque recurrence token is now supported throughout the wire format and message handling pipeline.