Commit to payment_metadata in inbound payment HMAC

[TheBlueMatt]

When payment_metadata is set in a BOLT 11 invoice, users expect to
receive it back as-is in the payment onion. In order to ensure it
isn't tampered with, they presumably will add an HMAC, or worse, not
add one and forget that it can be tampered with.

Instead, here we include it in the HMAC computation for the payment
secret. This ensures that the sender must relay the correct
metadata for the payment to be accepted by the receiver, binding
the metadata to the payment cryptographically.

The metadata is only included in the HMAC when present, so existing
payments without metadata continue to verify correctly. However,
this does break receiving payments with metadata today. On an
upgrade this seems acceptable to me given we have seen almost no
use of payment metadata in practice.

[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]

I've completed a thorough review of the entire PR diff. Let me verify my prior inline comment corrections.

My prior review left the following comments:

1. pending_changelog/matt-commit-to-metadata.txt:2 — typo "comitted". Now resolved — the current text shows "committed".
2. lightning/src/ln/inbound_payment.rs:182 — claimed None and Some(&[]) are indistinguishable. This was incorrect — the length prefix ((metadata.len() as u64).to_le_bytes()) adds 8 zero bytes for Some(&[]) but nothing for None, so they ARE properly distinguished.
3. lightning/src/ln/channelmanager.rs:15078 — missing docs for get_payment_preimage's payment_metadata param. Still applies.
4. lightning/src/ln/channelmanager.rs:14994 — missing docs for create_inbound_payment's payment_metadata param. Still applies.

No new issues found beyond what was already flagged. Here is my summary:

Review Summary

No new issues found. All prior comments have been reviewed for current applicability.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved. The typo "comitted" has been corrected to "committed".
• lightning/src/ln/inbound_payment.rs:182 — Correction: My prior comment about None vs Some(&[]) being cryptographically indistinguishable was incorrect. The length prefix ((metadata.len() as u64).to_le_bytes()) is always included when metadata is Some, even for empty metadata, making them properly distinguishable. The implementation is correct.
• lightning/src/ln/channelmanager.rs:14994 — Still applicable. create_inbound_payment docs don't mention payment_metadata.
• lightning/src/ln/channelmanager.rs:15078 — Still applicable. get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed here.

Areas verified as correct:

• HMAC computation is consistent across all four sites: create, create_from_hash, verify/derive_ldk_payment_preimage, and get_payment_preimage.
• Length-prefixed metadata in HMAC properly distinguishes None from Some(&[]).
• SpontaneousPayment intentionally does not commit to metadata (no invoice metadata for keysend).
• MPP parts enforce metadata consistency via check_merge (outbound_payment.rs:909).
• InvoiceBuilder::payment_metadata() correctly defaults to required; optional_payment_metadata() available for legacy compat.
• create_bolt11_invoice properly passes metadata to both the HMAC creation and the invoice builder.
• All callers of modified public APIs updated with the new parameter.
• BOLT 12 and phantom invoice paths correctly pass None for metadata.

Cross-cutting observations:

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata and later calls get_payment_preimage(..., None), it will silently fail with APIMisuseError. A test here would catch regressions and serve as documentation.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @valentinewallace! 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.

[valentinewallace]

LGTM aside from CI and one or two of Claude's doc nits

[valentinewallace]

nit: this could've been a separate commit

[elnosh]

check length of payment_metadata and return error if greater than max allowed length of field in invoice?

[TheBlueMatt]

Hmm, lightning-invoice isn't aware of a limit - if there is one we should enforce it everywhere which seems like an orthogonal PR.

[elnosh]

shouldn't it be aware of the protocol limit here? https://github.com/TheBlueMatt/rust-lightning/blob/4bf195c5edae74699e9d7a9f598fa99a04679c29/lightning-invoice/src/ser.rs#L427

so passing metadata in the Bolt11InvoiceParameters above this would cause ldk to panic.

	#[test]
	fn test_create_invoice_payment_metadata_too_long() {
		let chanmon_cfgs = create_chanmon_cfgs(2);
		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
		let description = Bolt11InvoiceDescription::Direct(
			Description::new("Some description".to_string()).unwrap(),
		);
		let invoice_params = Bolt11InvoiceParameters {
			amount_msats: Some(10_000),
			description,
			payment_metadata: Some(vec![0; 640]),
			..Default::default()
		};
		let _ = nodes[1].node.create_bolt11_invoice(invoice_params);
	}

[tnull]

@TheBlueMatt any chance to get this into 0.3 still? We'd need it to make lightningdevkit/ldk-node#899 safe, which we want to do given we're now doing #4584 ^^

And, given this PR breaks backwards compat. for payment metadata users, we'll probably want to have the breakage happen before we start using payment metadata in LDK Node (i.e. lightningdevkit/ldk-node#899).

Feel free to object, but for that reason I'm adding this to the 0.3 milestone.

[TheBlueMatt]

Rebased and ~addressed feedback.

[codecov]

Codecov Report

❌ Patch coverage is 94.26230% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.13%. Comparing base (5455058) to head (4bf195c).
⚠️ Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/inbound_payment.rs	95.23%	4 Missing ⚠️
lightning/src/ln/channelmanager.rs	87.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4528      +/-   ##
==========================================
+ Coverage   86.11%   86.13%   +0.01%     
==========================================
  Files         157      157              
  Lines      108772   108823      +51     
  Branches   108772   108823      +51     
==========================================
+ Hits        93668    93730      +62     
+ Misses      12487    12478       -9     
+ Partials     2617     2615       -2     

Flag	Coverage Δ
tests	86.13% <94.26%> (+0.01%)	⬆️

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.