Add a payment_metadata map in blinded * path contexts

[TheBlueMatt]

Similar to how BOLT 11 payments can use a `payment_metadata` to
provide arbitrary bytes in the invoice to be communicated back to
them when receiving, its useful to be able to provide some bytes
which are communicated back upon receiving a payment.

Here we do so in the BOLT 12 blinded * contexts, offering a
`BTreeMap<u64, Vec<u8>>` instead to enable more easily including
multiple sets of data.

Also note that a `Router` building a blinded path is allowed to
modify the `payment_metadata` without breaking the payment.

Tests by claude

We do so both in the blinded message and payment paths, supporting async payments when data is injected in the blinded payment paths (eg via the Router). We don't expose building offers with metadata yet.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

After a thorough review of the entire PR diff — examining all changed files, serialization patterns, metadata propagation paths, TLV ordering, backward compatibility, and test coverage — I found no new issues beyond those already covered in prior review passes.

Verification summary:

• TLV type 1 (odd = backward-compatible optional) used consistently for all payment_metadata fields across OffersContext::InvoiceRequest, Bolt12OfferContext, AsyncBolt12OfferContext, and Bolt12RefundContext.
• TLV field ordering maintained correctly in all structs/enums (type 1 follows type 0 everywhere).
• BigSizeKeyedMap deserialization: bounded by LengthLimitedRead, rejects duplicate keys. The ? operator in the loop causes early return on read exhaustion, so a malicious len declaration cannot cause unbounded iteration.
• Metadata propagation verified through all three paths: (1) OffersContext::InvoiceRequest → channelmanager extraction → Bolt12OfferContext, (2) Router::create_blinded_payment_paths override via TestRouter, (3) AsyncBolt12OfferContext → Bolt12OfferContext conversion for keysend.
• Both DerivedKeys and ExplicitKeys invoice builder paths receive payment_metadata (lines 17129, 17154 of channelmanager.rs are in mutually exclusive match arms).
• AccountableBool<bool> → AccountableBool<&bool> Writeable change is correct for the (option, encoding) macro expansion pattern.
• WithoutLength<&&$features> addition is harmless/defensive (not strictly required by current code paths but follows the existing pattern of WithoutLength<&&String> in ser.rs).
• All pattern matches on OffersContext::InvoiceRequest and all construction sites correctly include payment_metadata.
• PaymentContext::payment_metadata() accessor covers all three variants exhaustively.
• Prior review comments are resolved: typos fixed, false positive doc reference acknowledged.

No issues found.

[codecov]

Codecov Report

❌ Patch coverage is 85.10638% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.43%. Comparing base (42e198c) to head (f2c4167).
⚠️ Report is 93 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/ser.rs	74.07%	1 Missing and 6 partials ⚠️
lightning/src/blinded_path/payment.rs	71.42%	6 Missing ⚠️
lightning/src/util/test_utils.rs	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4584      +/-   ##
==========================================
- Coverage   87.15%   86.43%   -0.73%     
==========================================
  Files         161      158       -3     
  Lines      109251   109371     +120     
  Branches   109251   109371     +120     
==========================================
- Hits        95215    94531     -684     
- Misses      11560    12286     +726     
- Partials     2476     2554      +78     

Flag	Coverage Δ
fuzzing-fake-hashes	5.07% <0.00%> (-26.08%)	⬇️
fuzzing-real-hashes	22.76% <1.26%> (-0.16%)	⬇️
tests	86.16% <85.10%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

Looks good, but it would be nice to provide a bit more ergonomic/self-descriptive types here. At the very least we should document what the semantics of the u64s are (or rather, that there are ~none and the user is free to set them).

Tagging @jkczyz as second reviewer on this.

[tnull]

See above: what are we expecting users to set the u64 to?

Shouldn't we rather use a dedicated PaymentMetadata type for this, or even an Option<Box<dyn Writeable>> or similar?

[TheBlueMatt]

Answered at #4584 (comment) but I do really want to force users into the K-V-map box here. Can do a type if we want but its a bit awkward that we have a separate BOLT 11 and BOLT 12 PaymentMetadata type...

[jkczyz]

The Box<dyn Writeable> would get a bit ugly at read time. You'd need to parameterize the onion message parsing with your types.

[jkczyz]

Why use a map? Wouldn't a user's wrapper struct be just as efficient encoding -- or could be even more efficient if each part is of a fixed length? Deserialization would be simpler, too, if they didn't need to iterate a map.

[TheBlueMatt]

The intention is both to force users to think about forward compatibility from day one (in the sense that its already a K-V) but also because we want to reserve some keys in LDK (eg in #4463).

[jkczyz]

The intention is both to force users to think about forward compatibility from day one (in the sense that its already a K-V)

Isn't that what using our serialization macros provide? Blinded paths are also not expected to be useable indefinitely, so the compatibility argument isn't as strong, IMO.

FWIW, the rationale given in the commit message only mentions composability.

but also because we want to reserve some keys in LDK (eg in #4463).

Wouldn't it be simpler to add a dedicated field for anything LDK-specific? Then we wouldn't need custom parsing / serialization logic (for each variant of PaymentContext) or need to worry about checking if users took a reserved type. This would need to be added somewhere in the code vs a simple declaration in the impl_writeable_tlv_based uses.

[TheBlueMatt]

Isn't that what using our serialization macros provide? Blinded paths are also not expected to be useable indefinitely, so the compatibility argument isn't as strong, IMO.

Right, for cases where there's a single top-level struct which the downstream code wants to use.

Wouldn't it be simpler to add a dedicated field for anything LDK-specific?

Yea, this is probably true for #4463, but I'm not sure how this would work, eg, if LDK Node had something it wanted to add? Would we add a field here just for LDK Node's usage? Or would LDK Node define a struct which then had a similar user-defined sub-struct?

I guess its not clear to me how exactly a struct would work, anyway, would we parameterize ~everything by the custom context struct?

[jkczyz]

Right, for cases where there's a single top-level struct which the downstream code wants to use.

We could advise to include a top-level struct with a field holding a sub-struct with their data. Then they would simply add a new struct / field for another use case. Alternatively, their serialization could have a version number if they ever want to swap-out a top-level struct.

Yea, this is probably true for #4463, but I'm not sure how this would work, eg, if LDK Node had something it wanted to add? Would we add a field here just for LDK Node's usage? Or would LDK Node define a struct which then had a similar user-defined sub-struct?

Right, I guess we'd need to know how LDK Node would communicate to LDK to set the field and how it can be communicated back to LDK Node when handling a message sent over the blinded path. For payment paths, we'd expose it through PaymentClaimable. But how will it be set?

For the LSP case, I thought the idea would be to communicate data through the blinded message path. And then LDK would have logic for setting data in the blinded payment path when handing an invoice request containing that data.

For other use cases, we'd probably force them to use OffersMessageFlow (or a different system / handler if this is not offers-related blinded path usage). We could make LDK Node use OffersMessageFlow, too. Or just provide the tighter integration with LDK as described above.

I guess its not clear to me how exactly a struct would work, anyway, would we parameterize ~everything by the custom context struct?

I was saying to just have a Vec<u8> and allow consumers to parse as needed. They could use a custom struct and parameterize everything with it. But for LDK-specific data, we'd have our own dedicated struct / field.

[TheBlueMatt]

I was saying to just have a Vec and allow consumers to parse as needed. They could use a custom struct and parameterize everything with it. But for LDK-specific data, we'd have our own dedicated struct / field.

Ah, yea, I really don't love that. At the cost of two extra bytes forcing them to have a specified key effectively forces them to have forward/backwards compat (which, indeed, doesn't matter as much in blinded payment paths but we're also using the same field in offers blinded paths, which are long-lived) and allows user data to live parallel with LDK Node (or other LDK non-lightning-crate) data. It seems very much worth it to have that and I don't really think the API complexity cost is all that high here.

[TheBlueMatt]

Where did we land on this after last thursday? I don't recall any alternatives that were actually better, did we decide on some kind of metadata wrapper struct or to leave this as a btreemap?

[tnull]

Still not particularly a fan of the BTreeMap<u64, Vec<u8>> approach, but fine if we really think this is the way to go. FWIW, LDK Node will only a single entry to serialize a TLV-based payment metadata object (holding also extensible fields), so from that point of view it could also just be a simple Vec<u8> in parallel to what BOLT11 invoices hold.

However, if we really go this way, we should at least clarify what ranges of the u64 are reserved for LDK. Telling users that we might do something that might conflict, maybe, with any number they pick is not an API we should provide, IMO.

Apart from that the changes look good, as mentioned above. rustfmt is unhappy though.

[TheBlueMatt]

Fixed rustfmt and addressed doc nits.

[tnull]

Fine by me, though I'm really not sure if this API choice really improves anything over a simple Vec<u8>. In fact it might rather complicate things, but don't feel to strongly about it. Feel free to squash from my side.

[TheBlueMatt]

Yea, I do feel strongly that a K-V map/TLV set is a way better API than a flat set of bytes that might get used by lightning-liquidity and leave some downstream code (like LDK Node!) annoyed they can't use it :)

[tnull]

Yea, I do feel strongly that a K-V map/TLV set is a way better API than a flat set of bytes that might get used by lightning-liquidity and leave some downstream code (like LDK Node!) annoyed they can't use it :)

I don't think lightning-liquidity would use this directly, no? We'd still use the same object as for BOLT11 metadata in LDK Node, no? Anyways, fine by me to land once Jeff's also reviewed.

[TheBlueMatt]

I imagine in #4463 the router will be using this directly, no?

[tnull]

I imagine in #4463 the router will be using this directly, no?

Well, no, as mentioned elsewhere and above, so far we intend to reuse the same TLV-based PaymentMetadata/PaymentMetadatTlvs structs introduced in lightningdevkit/ldk-node#899. But I'll see how it exactly works out when I come to rebase #4463.

[TheBlueMatt]

I don't see why it wouldn't just store that data in a specific field in the BTreeMap? That is what I'd understood you intended to do (and I'm not sure how it could be implemented any other way, really?)

[tnull]

I don't see why it wouldn't just store that data in a specific field in the BTreeMap? That is what I'd understood you intended to do (and I'm not sure how it could be implemented any other way, really?)

Because we already have (to have) that identical data structure in LDK Node. Given we now won't LSP params in general it also doesn't make sense to split the held data into two different versions of LSP parameters (one living in LDK, one in LDK Node), which will also have overlap.

[TheBlueMatt]

Right I'm not saying that we'd have two totally separate structs but rather in BOLT 11 LDK Node would have to deal with putting that struct in the invoice whereas for BOLT 12, the router in lightning-liquidity would do it. It's not ideal to have the asymmetry but ultimately BOLT 11 and 12 are totally different animals in terms of when an invoice is created so...

[ldk-reviews-bot]

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.