Skip to content

Check metadata length for bolt11 invoices#4740

Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom
elnosh:check-metadata-length
Jun 24, 2026
Merged

Check metadata length for bolt11 invoices#4740
valentinewallace merged 1 commit into
lightningdevkit:mainfrom
elnosh:check-metadata-length

Conversation

@elnosh

@elnosh elnosh commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

I mentioned this here

Similar to the description check, I think this length should be checked that is less than the limit for tagged fields in bolt11 invoices. Otherwise it can hit this assert during serialization.

Check that the metadata when creating a bolt11 invoice is below
the bolt11 limit of 639 bytes for tagged fields.
@ldk-reviews-bot

ldk-reviews-bot commented Jun 23, 2026

Copy link
Copy Markdown

I've assigned @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

Copy link
Copy Markdown
Collaborator

No issues found.

The change correctly checks payment metadata length against MAX_TAGGED_FIELD_DATA_BYTES (639). I verified the threshold is right: a 640-byte metadata yields ceil(640*8/5) = 1024 base32 groups, which would trip the len < 1024 assert at ser.rs:427, while 639 bytes yields exactly 1023 groups. The error-recording pattern (self.error = Some(...) checked later in build_raw at lib.rs:734) is consistent with how description() handles DescriptionTooLong, and the test covers the 640-byte rejection case.

Minor (non-blocking) observations, not posted inline as they are style preferences:

  • The new CreationError::PaymentMetadataTooLong doc comment hardcodes "639 bytes" while Description::new's doc was updated to reference the constant — slightly inconsistent.
  • No test asserts the boundary (639 bytes accepted), only the over-limit (640) rejection.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.89%. Comparing base (1c1a4ad) to head (87ea15c).

Files with missing lines Patch % Lines
lightning-invoice/src/lib.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4740      +/-   ##
==========================================
- Coverage   86.90%   86.89%   -0.01%     
==========================================
  Files         161      161              
  Lines      111680   111688       +8     
  Branches   111680   111688       +8     
==========================================
+ Hits        97052    97054       +2     
- Misses      12115    12126      +11     
+ Partials     2513     2508       -5     
Flag Coverage Δ
fuzzing-fake-hashes 7.77% <0.00%> (-0.01%) ⬇️
fuzzing-real-hashes 29.82% <0.00%> (-0.01%) ⬇️
tests 86.24% <90.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 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

Copy link
Copy Markdown

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@valentinewallace valentinewallace merged commit 1a80ef5 into lightningdevkit:main Jun 24, 2026
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants