Fix `Borrow`/`Hash` inconsistency on `Payment*` types
What the developer wrote
Authored by Matt Corallo
Fix `Borrow`/`Hash` inconsistency on `Payment*` types
`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.
What changed, and why it matters
This commit fixes a Rust programming contract violation in several Lightning payment identifier types. In Rust, if a type can be 'borrowed' as a byte slice and used as a HashMap key, the borrowed form and the owned form must produce the same hash value. The old code used the type's automatic derived hash (which hashed the whole struct, including wrapper metadata) while borrowing only the inner byte array, so a borrowed key and an owned key could hash differently. That breaks HashMap lookups: you might store a value under one key and then be unable to find it with a borrowed version of the same key. The patch replaces the derived Hash implementation with one that hashes only the borrowed byte slice, restoring consistency. The commit message notes the issue was reported by Project Loupe.
Review all HashMap/HashSet uses keyed by PaymentHash, PaymentPreimage, PaymentSecret, PaymentId, InterceptId, or ChannelId to confirm the fix resolves observed lookup issues. No immediate exploit mitigation is required, but verify that no persisted hash-based state (e.g., serialized maps) relied on the old hash values, since hashes now change. Consider adding regression tests that insert with an owned key and look up with a borrowed key.
Security signals we found
Borrow/Hash contract violation in Rust standard collection key types
Potential HashMap lookup failure for payment/channel identifiers
Manual Hash implementation now delegates to borrowed byte slice
Reported by external party (Project Loupe)
Evidence from the diff
The patch removes #[derive(Hash)] from PaymentHash, PaymentPreimage, PaymentSecret, PaymentId, InterceptId, and ChannelId, and instead implements core::hash::Hash manually by borrowing &[u8] and hashing that slice. This satisfies Rust’s Borrow trait contract: for any type T implementing Borrow, hash(t) must equal hash(q). Previously, the derived Hash hashed the tuple struct layout, while Borrow returned only the inner [u8; 32], so HashMap/HashSet lookups using borrowed keys could fail to match owned keys. The change is purely a correctness fix for collections keyed by these types; it also disambiguates the imported Hash trait from bitcoin::hashes::Hash by renaming the latter to CryptoHash in touched files.
Changed components
lightning-types/src/payment.rs (PaymentHash, PaymentPreimage, PaymentSecret)lightning/src/ln/channelmanager.rs (PaymentId, InterceptId, HTLCSource hash import cleanup)lightning/src/ln/types.rs (ChannelId)Inspect captured patch +63 / −15
diff --git a/lightning-types/src/payment.rs b/lightning-types/src/payment.rs
index 0f0fcf7..efdab8b 100644
--- a/lightning-types/src/payment.rs
+++ b/lightning-types/src/payment.rs
@@ -10,15 +10,16 @@
//! Types which describe payments in lightning.
use core::borrow::Borrow;
+use core::hash::{Hash, Hasher};
-use bitcoin::hashes::{sha256::Hash as Sha256, Hash as _};
+use bitcoin::hashes::{sha256::Hash as Sha256, Hash as CryptoHash};
use bitcoin::hex::display::impl_fmt_traits;
/// The payment hash is the hash of the [`PaymentPreimage`] which is the value used to lock funds
/// in HTLCs while they transit the lightning network.
///
/// This is not exported to bindings users as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
+#[derive(Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
pub struct PaymentHash(pub [u8; 32]);
impl Borrow<[u8]> for PaymentHash {
@@ -27,6 +28,13 @@ impl Borrow<[u8]> for PaymentHash {
}
}
+impl Hash for PaymentHash {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let slice: &[u8] = self.borrow();
+ Hash::hash(slice, state);
+ }
+}
+
impl_fmt_traits! {
impl fmt_traits for PaymentHash {
const LENGTH: usize = 32;
@@ -37,7 +45,7 @@ impl_fmt_traits! {
/// or in a lightning channel.
///
/// This is not exported to bindings users as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
+#[derive(Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
pub struct PaymentPreimage(pub [u8; 32]);
impl Borrow<[u8]> for PaymentPreimage {
@@ -46,6 +54,13 @@ impl Borrow<[u8]> for PaymentPreimage {
}
}
+impl Hash for PaymentPreimage {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let slice: &[u8] = self.borrow();
+ Hash::hash(slice, state);
+ }
+}
+
impl_fmt_traits! {
impl fmt_traits for PaymentPreimage {
const LENGTH: usize = 32;
@@ -55,7 +70,7 @@ impl_fmt_traits! {
/// Converts a `PaymentPreimage` into a `PaymentHash` by hashing the preimage with SHA256.
impl From<PaymentPreimage> for PaymentHash {
fn from(value: PaymentPreimage) -> Self {
- PaymentHash(Sha256::hash(&value.0).to_byte_array())
+ PaymentHash(<Sha256 as CryptoHash>::hash(&value.0).to_byte_array())
}
}
@@ -63,7 +78,7 @@ impl From<PaymentPreimage> for PaymentHash {
/// multi-part HTLCs together into a single payment.
///
/// This is not exported to bindings users as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
+#[derive(Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
pub struct PaymentSecret(pub [u8; 32]);
impl Borrow<[u8]> for PaymentSecret {
@@ -72,6 +87,13 @@ impl Borrow<[u8]> for PaymentSecret {
}
}
+impl Hash for PaymentSecret {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let slice: &[u8] = self.borrow();
+ Hash::hash(slice, state);
+ }
+}
+
impl_fmt_traits! {
impl fmt_traits for PaymentSecret {
const LENGTH: usize = 32;
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 6398613..eac3aef 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -26,7 +26,7 @@ use bitcoin::transaction::Transaction;
use bitcoin::hash_types::{BlockHash, Txid};
use bitcoin::hashes::hmac::Hmac;
use bitcoin::hashes::sha256::Hash as Sha256;
-use bitcoin::hashes::{Hash, HashEngine, HmacEngine};
+use bitcoin::hashes::{Hash as CryptoHash, HashEngine, HmacEngine};
use bitcoin::secp256k1::Secp256k1;
use bitcoin::secp256k1::{PublicKey, SecretKey};
@@ -174,6 +174,7 @@ use crate::ln::script::ShutdownScript;
use core::borrow::Borrow;
use core::cell::RefCell;
use core::convert::Infallible;
+use core::hash::{Hash, Hasher};
use core::ops::Deref;
use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
use core::time::Duration;
@@ -619,7 +620,7 @@ impl Ord for ClaimableHTLC {
/// a payment and ensure idempotency in LDK.
///
/// This is not exported to bindings users as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq)]
+#[derive(Copy, Clone, PartialEq, Eq)]
pub struct PaymentId(pub [u8; Self::LENGTH]);
impl PaymentId {
@@ -651,6 +652,13 @@ impl Borrow<[u8]> for PaymentId {
}
}
+impl Hash for PaymentId {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let slice: &[u8] = self.borrow();
+ Hash::hash(slice, state);
+ }
+}
+
impl_fmt_traits! {
impl fmt_traits for PaymentId {
const LENGTH: usize = 32;
@@ -673,7 +681,7 @@ impl Readable for PaymentId {
/// An identifier used to uniquely identify an intercepted HTLC to LDK.
///
/// This is not exported to bindings users as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq)]
+#[derive(Copy, Clone, PartialEq, Eq)]
pub struct InterceptId(pub [u8; 32]);
impl InterceptId {
@@ -693,6 +701,14 @@ impl Borrow<[u8]> for InterceptId {
&self.0[..]
}
}
+
+impl Hash for InterceptId {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let slice: &[u8] = self.borrow();
+ Hash::hash(slice, state);
+ }
+}
+
impl_fmt_traits! {
impl fmt_traits for InterceptId {
const LENGTH: usize = 32;
@@ -941,7 +957,7 @@ pub use self::fuzzy_channelmanager::*;
pub(crate) use self::fuzzy_channelmanager::*;
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
-impl core::hash::Hash for HTLCSource {
+impl Hash for HTLCSource {
fn hash<H: core::hash::Hasher>(&self, hasher: &mut H) {
match self {
HTLCSource::PreviousHopData(prev_hop_data) => {
@@ -7915,7 +7931,8 @@ impl<
Ok(res) => res,
Err(onion_utils::OnionDecodeErr::Malformed { err_msg, reason }) => {
let sha256_of_onion =
- Sha256::hash(&onion_packet.hop_data).to_byte_array();
+ <Sha256 as CryptoHash>::hash(&onion_packet.hop_data)
+ .to_byte_array();
// In this scenario, the phantom would have sent us an
// `update_fail_malformed_htlc`, meaning here we encrypt the error as
// if it came from us (the second-to-last hop) but contains the sha256
@@ -9485,7 +9502,7 @@ impl<
}
fn claim_payment_internal(&self, payment_preimage: PaymentPreimage, custom_tlvs_known: bool) {
- let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).to_byte_array());
+ let payment_hash: PaymentHash = payment_preimage.into();
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
@@ -10098,7 +10115,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
let derived_key;
let session_priv = if path.has_trampoline_hops() {
let session_priv_hash =
- Sha256::hash(&session_priv.secret_bytes()).to_byte_array();
+ <Sha256 as CryptoHash>::hash(&session_priv.secret_bytes()).to_byte_array();
derived_key = SecretKey::from_slice(&session_priv_hash[..]).unwrap();
&derived_key
} else {
diff --git a/lightning/src/ln/types.rs b/lightning/src/ln/types.rs
index fd8ccba..62ce89b 100644
--- a/lightning/src/ln/types.rs
+++ b/lightning/src/ln/types.rs
@@ -20,10 +20,11 @@ use crate::util::ser::{Readable, Writeable, Writer};
#[allow(unused_imports)]
use crate::prelude::*;
-use bitcoin::hashes::{sha256::Hash as Sha256, Hash as _, HashEngine as _};
+use bitcoin::hashes::{sha256::Hash as Sha256, Hash as CryptoHash, HashEngine as _};
use bitcoin::hex::display::impl_fmt_traits;
use core::borrow::Borrow;
+use core::hash::{Hash, Hasher};
/// A unique 32-byte identifier for a channel.
/// Depending on how the ID is generated, several varieties are distinguished
@@ -33,7 +34,7 @@ use core::borrow::Borrow;
/// A _temporary_ ID is generated randomly.
/// (Later revocation-point-based _v2_ is a possibility.)
/// The variety (context) is not stored, it is relevant only at creation.
-#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)]
+#[derive(Clone, Copy, Eq, Ord, PartialEq, PartialOrd)]
pub struct ChannelId(pub [u8; 32]);
impl ChannelId {
@@ -93,7 +94,8 @@ impl ChannelId {
our_revocation_basepoint: &RevocationBasepoint,
) -> Self {
let our_revocation_point_bytes = our_revocation_basepoint.0.serialize();
- Self(Sha256::hash(&[[0u8; 33], our_revocation_point_bytes].concat()).to_byte_array())
+ let hash_input = &[[0u8; 33], our_revocation_point_bytes].concat();
+ Self(<Sha256 as CryptoHash>::hash(hash_input).to_byte_array())
}
/// Indicates whether this is a V2 channel ID for the given local and remote revocation basepoints.
@@ -123,6 +125,13 @@ impl Borrow<[u8]> for ChannelId {
}
}
+impl Hash for ChannelId {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ let slice: &[u8] = self.borrow();
+ Hash::hash(slice, state);
+ }
+}
+
impl_fmt_traits! {
impl fmt_traits for ChannelId {
const LENGTH: usize = 32;
Why this scored 63/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.