Skip to content

Conversation

@jgmcalpine
Copy link

@jgmcalpine jgmcalpine commented Jan 30, 2026

Closes #4292

Note: This is a breaking change for InvoiceBuilder. Downstream consumers (like ldk-node) will need to update their builder calls to pass PaymentHash directly.

Refactors RawBolt11Invoice, RawDataPart, and TaggedField to use
lightning_types::payment::PaymentHash instead of bitcoin::hashes::sha256::Hash.
This improves type safety and aligns the raw invoice types with the high-level
Bolt11Invoice.

This is a follow-up to #4293 to complete the migration of invoice types to PaymentHash.

Specific changes:

  • TaggedField::PaymentHash now holds a PaymentHash.
  • InvoiceBuilder::payment_hash now accepts a PaymentHash directly.
  • Implemented Base32 traits for PaymentHash in lightning-invoice to support
    serialization.
  • Updated tests to resolve name collisions between the PaymentHash type and
    the TaggedField variant.
  • Updated downstream usage in the lightning crate (channelmanager and
    invoice_utils) to remove redundant hash conversions now that the builder
    accepts the strong type.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 30, 2026

👋 I see @valentinewallace was un-assigned.
If you'd like another reviewer assignment, please click here.

@jgmcalpine jgmcalpine force-pushed the invoice-raw-use-payment-hash branch 5 times, most recently from da905d6 to f967d8a Compare January 30, 2026 18:56
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
let payment_hash = invoice.payment_hash();
let payment_hash =
PaymentHash(Sha256::from_byte_array(invoice.payment_hash().0).to_byte_array());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? Same below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch. That was a redundant round-trip left over from when I was refactoring the types. Simplified it to just use invoice.payment_hash() directly

@TheBlueMatt TheBlueMatt removed the request for review from valentinewallace January 30, 2026 20:42
@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

❌ Patch coverage is 99.37107% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.09%. Comparing base (f9ad345) to head (59b1623).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
lightning-invoice/src/de.rs 98.63% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4363   +/-   ##
=======================================
  Coverage   86.09%   86.09%           
=======================================
  Files         156      156           
  Lines      102804   102853   +49     
  Branches   102804   102853   +49     
=======================================
+ Hits        88508    88553   +45     
+ Misses      11788    11784    -4     
- Partials     2508     2516    +8     
Flag Coverage Δ
tests 86.09% <99.37%> (+<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.

@jgmcalpine jgmcalpine force-pushed the invoice-raw-use-payment-hash branch from f967d8a to 6b9f7ec Compare January 30, 2026 20:57
@jgmcalpine jgmcalpine force-pushed the invoice-raw-use-payment-hash branch from 6b9f7ec to 59b1623 Compare January 30, 2026 21:01
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.

Bolt11Invoice::payment_hash should return a PaymentHash

3 participants