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 @valentinewallace 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]

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK

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

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK

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

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK

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

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK

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

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK

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

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK

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

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

• pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
• lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
• lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
• lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

• No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

[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);
}

[elnosh]

somewhat late but shouldn't we check this? metadata passed should be small but it could still panic if the payment_metadata passed is above the limit - we also check it in the description

rust-lightning/lightning-invoice/src/lib.rs

Lines 1679 to 1686 in e01ecce

	pubfnnew(description:String) -> Result<Description,CreationError>{
	if description.len() > 639{
	Err(CreationError::DescriptionTooLong)
	}else{
	Ok(Description(UntrustedString(description)))
	}
	}

[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.12%. Comparing base (946ee09) to head (44828f7).
⚠️ Report is 7 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.12% +0.01% 
==========================================
Files 157 157 Lines 108841 108922 +81 Branches 108841 108922 +81 ==========================================
+ Hits 93725 93812 +87 + Misses 12497 12495 -2 + Partials 2619 2615 -4 

Flag	Coverage Δ
tests	86.12% <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.

[tnull]

Changes look good, feel free to squash.

[tnull]

It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

[TheBlueMatt]

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

[tnull]

Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

[TheBlueMatt]

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

[TheBlueMatt]

Rebased and squashed.

[TheBlueMatt]

Gonna land this, @tnull confirmed offline its at least good enough even if we have followups later.

[tnull]

Post-merge ACK