fix(bolt12): Make CurrencyCode a validated wrapper type #3814

erickcestari · 2025-05-30T16:36:52Z

After performing differential fuzzing using bitcoinfuzz with rust-lightning and Core Lightning (CLN), I noticed that rust-lightning does not verify whether the offer_currency field contains valid UTF-8.

This commit convert CurrencyCode from type alias to struct with validation, ensuring
ISO 4217 compliance (3 ASCII uppercase letters) at construction time.

ldk-reviews-bot · 2025-05-30T16:36:55Z

👋 Thanks for assigning @jkczyz 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.

codecov · 2025-05-30T16:46:19Z

Codecov Report

Attention: Patch coverage is 73.33333% with 20 lines in your changes missing coverage. Please review.

Project coverage is 89.87%. Comparing base (78fee88) to head (71562a9).
Report is 88 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/offers/offer.rs	68.25%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3814      +/-   ##
==========================================
+ Coverage   89.52%   89.87%   +0.34%     
==========================================
  Files         157      160       +3     
  Lines      125100   129205    +4105     
  Branches   125100   129205    +4105     
==========================================
+ Hits       112002   116123    +4121     
+ Misses      10408    10389      -19     
- Partials     2690     2693       +3

☔ 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.

erickcestari · 2025-05-30T16:52:26Z

I took a closer look, and it seems this test vector has an invalid currency length in UTF-8 encoding, rather than being an invalid UTF-8 string itself.

  {
    "description": "Malformed: invalid currency UTF-8",
    "valid": false,
    "bolt12": "lno1qcpgqsg2q4q5cj2rg5tzzqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqg"
  },

data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202

Looking at the data part, we observe 06 (currency type), 02 (length), and 8041 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

I'll open a PR on Bolts to fix it.

(edited)

TheBlueMatt

Aren't the currencies supposed to be the three-char currency specification standard? ie they should be ASCII only (and probably all-caps letter only, never numbers)?

jkczyz

Thanks for jumping on this!

lightning/src/offers/offer.rs

jkczyz · 2025-05-30T20:30:35Z

I took a closer look, and it seems this test vector has an invalid currency length in UTF-8 encoding, rather than being an invalid UTF-8 string itself.
  {
    "description": "Malformed: invalid currency UTF-8",
    "valid": false,
    "bolt12": "lno1qcpgqsg2q4q5cj2rg5tzzqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqg"
  },
data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202

Looking at the data part, we observe 06 (currency type), 02 (length), and 2804 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

I'll open a PR on Bolts to fix it.

Ah, yeah either the test vector or the comment is wrong lol

erickcestari · 2025-05-30T22:17:29Z

Aren't the currencies supposed to be the three-char currency specification standard? ie they should be ASCII only (and probably all-caps letter only, never numbers)?

Initially, I considered just aligning it with the UTF-8 type for consistency. But yes, it makes more sense to enforce stricter validation to ensure it's valid ASCII and adheres to the expected format.

ldk-reviews-bot · 2025-06-02T00:00:14Z

🔔 1st Reminder

Hey @TheBlueMatt @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.

tnull · 2025-06-02T09:12:02Z

Ah, funny, seems this fixes #3813 we just opened. Will link the issue.

thomash-acinq · 2025-06-02T12:12:59Z

data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202

Looking at the data part, we observe 06 (currency type), 02 (length), and 2804 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

No, it's 06 (currency type), 02 (length), and 8041 (value). It seems that you've copied the '2' to read '2804'.

erickcestari · 2025-06-02T12:47:35Z

data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202
Looking at the data part, we observe 06 (currency type), 02 (length), and 2804 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

No, it's 06 (currency type), 02 (length), and 8041 (value). It seems that you've copied the '2' to read '2804'.

Yes, My mistake on the transcription.

It's invalid UTF-8, but the decoder fails due to the length mismatch (expecting 3 bytes for currency) rather than the UTF-8 validation itself.

TheBlueMatt

Rather than checking this when deserializing an offer, can we change CurrencyCode to be a real wrapper so that we can enforce it on creation/deserialization of that struct? Currently I believe we will let users build offers that we will refuse to parse.

ldk-reviews-bot · 2025-06-02T15:07:18Z

🔔 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.

jkczyz · 2025-06-02T16:25:28Z

Rather than checking this when deserializing an offer, can we change CurrencyCode to be a real wrapper so that we can enforce it on creation/deserialization of that struct? Currently I believe we will let users build offers that we will refuse to parse.

They would be helpful as upcoming OfferMessageFlow changes to support currencies could benefit from a dedicated type rather than a variant in an enum (cc: @shaavan).

jkczyz

Will defer to @TheBlueMatt on some of these comments,

jkczyz · 2025-06-03T14:57:39Z

lightning/src/offers/offer.rs

+	/// Creates a new CurrencyCode from a 3-byte array.
+	///
+	/// Returns an error if the bytes are not valid UTF-8 or not all ASCII uppercase.
+	pub fn new(code: [u8; 3]) -> Result<Self, Bolt12SemanticError> {


Should we use the unit type () for the error given and map at the call site given it can't be any other variant than InvalidCurrencyCode?

jkczyz · 2025-06-03T14:58:26Z

lightning/src/offers/offer.rs

+
+	/// Returns the currency code as a string slice.
+	pub fn as_str(&self) -> &str {
+		unsafe { core::str::from_utf8_unchecked(&self.0) }


Not sure if we should just use from_utf8().expect().

lightning/src/offers/offer.rs

jkczyz · 2025-06-03T15:01:44Z

lightning/src/offers/offer.rs

+pub struct CurrencyCode([u8; 3]);
+
+impl CurrencyCode {
+	/// Creates a new CurrencyCode from a 3-byte array.


s/CurrencyCode/`CurrencyCode`

Likewise in other docs.

Convert CurrencyCode from type alias to struct with validation, ensuring ISO 4217 compliance (3 ASCII uppercase letters) at construction time.

ldk-reviews-bot requested a review from TheBlueMatt May 30, 2025 16:47

erickcestari force-pushed the validate-currency-code branch from 7232027 to 748747b Compare May 30, 2025 16:48

TheBlueMatt reviewed May 30, 2025

View reviewed changes

jkczyz reviewed May 30, 2025

View reviewed changes

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

erickcestari force-pushed the validate-currency-code branch from 748747b to 91e633a Compare May 31, 2025 15:05

erickcestari changed the title ~~fix(bolt12): Add UTF-8 validation for offer currency field~~ fix(bolt12): Add ASCII validation for offer currency field May 31, 2025

erickcestari requested review from TheBlueMatt and jkczyz May 31, 2025 15:06

tnull mentioned this pull request Jun 2, 2025

Validate ISO 4217 compliance in BOLT12 CurrencyCode #3813

Open

tnull linked an issue Jun 2, 2025 that may be closed by this pull request

Validate ISO 4217 compliance in BOLT12 CurrencyCode #3813

Open

TheBlueMatt reviewed Jun 2, 2025

View reviewed changes

erickcestari force-pushed the validate-currency-code branch 2 times, most recently from d9b2684 to 1ebbcd0 Compare June 3, 2025 12:38

erickcestari changed the title ~~fix(bolt12): Add ASCII validation for offer currency field~~ fix(bolt12): Make CurrencyCode a validated wrapper type Jun 3, 2025

jkczyz reviewed Jun 3, 2025

View reviewed changes

fix(bolt12): Make CurrencyCode a validated wrapper type

71562a9

Convert CurrencyCode from type alias to struct with validation, ensuring ISO 4217 compliance (3 ASCII uppercase letters) at construction time.

erickcestari force-pushed the validate-currency-code branch from 1ebbcd0 to 71562a9 Compare June 3, 2025 15:30

fix(bolt12): Make CurrencyCode a validated wrapper type #3814

Are you sure you want to change the base?

fix(bolt12): Make CurrencyCode a validated wrapper type #3814

Conversation

erickcestari commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erickcestari commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkczyz commented May 30, 2025

Uh oh!

erickcestari commented May 30, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

tnull commented Jun 2, 2025

Uh oh!

thomash-acinq commented Jun 2, 2025

Uh oh!

erickcestari commented Jun 2, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

jkczyz commented Jun 2, 2025

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erickcestari commented May 30, 2025 •

edited

Loading

ldk-reviews-bot commented May 30, 2025 •

edited

Loading

codecov bot commented May 30, 2025 •

edited

Loading

erickcestari commented May 30, 2025 •

edited

Loading