Skip to content

Fix Borrow/Hash inconsistency on Payment* types#4692

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
TheBlueMatt:2026-06-borrow-hash-inconsistency
Jun 15, 2026
Merged

Fix Borrow/Hash inconsistency on Payment* types#4692
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
TheBlueMatt:2026-06-borrow-hash-inconsistency

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Borrow'd values are required to Hash identically to the original object so that a Borrowed key can be used in place of an owned one in a HashMap lookup.

We'd violated this on our Payment* types, which we fix here. Note that changing the Hash implementation is generally not considered an API-breaking change and this seems like a useful fix.

Reported by Project Loupe.

@ldk-reviews-bot

ldk-reviews-bot commented Jun 15, 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

ldk-claude-review-bot commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

No issues found.

All manual Hash impls correctly hash via self.borrow() (the &[u8] slice path), guaranteeing consistency with the Borrow<[u8]> contract; Eq is unchanged and remains consistent; and the CryptoHash rename correctly disambiguates bitcoin::hashes::Hash from core::hash::Hash.

This version of the PR resolves my prior cross-cutting concern: PaymentId (channelmanager.rs:649), InterceptId (channelmanager.rs:699), and ChannelId (types.rs:122) now all receive manual Hash impls too. OfferId (offers/offer.rs:122) has Borrow<[u8]> but does not derive Hash, so it is correctly unaffected.

The claim_payment_internal change to payment_preimage.into() is behaviorally equivalent to the previous explicit SHA256 hashing (via the From<PaymentPreimage> for PaymentHash impl) and is a reasonable de-duplication.

No inline comments posted — the changed lines are correct and the previously-flagged sibling types are now fixed.

`Borrow`'d values are required to `Hash` identically to the
original object so that a `Borrow`ed key can be used in place of an
owned one in a `HashMap` lookup.

We'd violated this on our `Payment*` types, which we fix here. Note
that changing the `Hash` implementation is generally not considered
an API-breaking change and this seems like a useful fix.

Reported by Project Loupe.
@TheBlueMatt
TheBlueMatt force-pushed the 2026-06-borrow-hash-inconsistency branch from 419c8ed to a1ad1a3 Compare June 15, 2026 15:15
@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.

@TheBlueMatt
TheBlueMatt merged commit facd140 into lightningdevkit:main Jun 15, 2026
1 check passed
@TheBlueMatt

Copy link
Copy Markdown
Collaborator Author

Skipping backport because this isn't actually a bug, even if the code is super sketch - rust-bitcoin/rust-bitcoin#6365

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