Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a PaymentId for inbound payments #3303

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add a PaymentId for inbound payments #3303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65530ad
Do not check the ordering of HTLCs in `PaymentClaim[able,ed]`
TheBlueMatt Sep 8, 2024
7014391
Add an `inbound_payment_id_secret` to `ChannelManager`
TheBlueMatt Sep 8, 2024
1fcbb9f
Add a `PaymentId` for inbound payments
TheBlueMatt Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) {
// Adding new calls to `EntropySource::get_secure_random_bytes` during startup can change all the
// keys subsequently generated in this test. Rather than regenerating all the messages manually,
// it's easier to just increment the counter here so the keys don't change.
keys_manager.counter.fetch_sub(3, Ordering::AcqRel);
keys_manager.counter.fetch_sub(4, Ordering::AcqRel);
let network_graph = Arc::new(NetworkGraph::new(network, Arc::clone(&logger)));
let gossip_sync =
Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger)));
Expand Down
31 changes: 29 additions & 2 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,14 @@ pub enum Event {
///
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds
claim_deadline: Option<u32>,
/// A unique ID describing this payment (derived from the list of HTLCs in the payment).
///
/// Payers may pay for the same [`PaymentHash`] multiple times (though this is unsafe and
/// an intermediary node may steal the funds). Thus, in order to accurately track when
/// payments are received and claimed, you should use this identifier.
///
/// Only filled in for payments received on LDK versions 0.1 and higher.
payment_id: Option<PaymentId>,
},
/// Indicates a payment has been claimed and we've received money!
///
Expand Down Expand Up @@ -795,6 +803,14 @@ pub enum Event {
///
/// Payments received on LDK versions prior to 0.0.124 will have this field unset.
onion_fields: Option<RecipientOnionFields>,
/// A unique ID describing this payment (derived from the list of HTLCs in the payment).
///
/// Payers may pay for the same [`PaymentHash`] multiple times (though this is unsafe and
/// an intermediary node may steal the funds). Thus, in order to accurately track when
/// payments are received and claimed, you should use this identifier.
///
/// Only filled in for payments received on LDK versions 0.1 and higher.
payment_id: Option<PaymentId>,
},
/// Indicates that a peer connection with a node is needed in order to send an [`OnionMessage`].
///
Expand Down Expand Up @@ -1431,7 +1447,7 @@ impl Writeable for Event {
},
&Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat,
ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id,
ref claim_deadline, ref onion_fields
ref claim_deadline, ref onion_fields, ref payment_id,
} => {
1u8.write(writer)?;
let mut payment_secret = None;
Expand Down Expand Up @@ -1477,6 +1493,7 @@ impl Writeable for Event {
(9, onion_fields, option),
(10, skimmed_fee_opt, option),
(11, payment_context, option),
(13, payment_id, option),
});
},
&Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => {
Expand Down Expand Up @@ -1633,7 +1650,10 @@ impl Writeable for Event {
// We never write the OpenChannelRequest events as, upon disconnection, peers
// drop any channels which have not yet exchanged funding_signed.
},
&Event::PaymentClaimed { ref payment_hash, ref amount_msat, ref purpose, ref receiver_node_id, ref htlcs, ref sender_intended_total_msat, ref onion_fields } => {
&Event::PaymentClaimed { ref payment_hash, ref amount_msat, ref purpose,
ref receiver_node_id, ref htlcs, ref sender_intended_total_msat, ref onion_fields,
ref payment_id,
} => {
19u8.write(writer)?;
write_tlv_fields!(writer, {
(0, payment_hash, required),
Expand All @@ -1643,6 +1663,7 @@ impl Writeable for Event {
(5, *htlcs, optional_vec),
(7, sender_intended_total_msat, option),
(9, onion_fields, option),
(11, payment_id, option),
});
},
&Event::ProbeSuccessful { ref payment_id, ref payment_hash, ref path } => {
Expand Down Expand Up @@ -1766,6 +1787,7 @@ impl MaybeReadable for Event {
let mut via_user_channel_id = None;
let mut onion_fields = None;
let mut payment_context = None;
let mut payment_id = None;
read_tlv_fields!(reader, {
(0, payment_hash, required),
(1, receiver_node_id, option),
Expand All @@ -1779,6 +1801,7 @@ impl MaybeReadable for Event {
(9, onion_fields, option),
(10, counterparty_skimmed_fee_msat_opt, option),
(11, payment_context, option),
(13, payment_id, option),
});
let purpose = match payment_secret {
Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context),
Expand All @@ -1795,6 +1818,7 @@ impl MaybeReadable for Event {
via_user_channel_id,
claim_deadline,
onion_fields,
payment_id,
}))
};
f()
Expand Down Expand Up @@ -2036,6 +2060,7 @@ impl MaybeReadable for Event {
let mut htlcs: Option<Vec<ClaimedHTLC>> = Some(vec![]);
let mut sender_intended_total_msat: Option<u64> = None;
let mut onion_fields = None;
let mut payment_id = None;
read_tlv_fields!(reader, {
(0, payment_hash, required),
(1, receiver_node_id, option),
Expand All @@ -2044,6 +2069,7 @@ impl MaybeReadable for Event {
(5, htlcs, optional_vec),
(7, sender_intended_total_msat, option),
(9, onion_fields, option),
(11, payment_id, option),
});
Ok(Some(Event::PaymentClaimed {
receiver_node_id,
Expand All @@ -2053,6 +2079,7 @@ impl MaybeReadable for Event {
htlcs: htlcs.unwrap_or_default(),
sender_intended_total_msat,
onion_fields,
payment_id,
}))
};
f()
Expand Down
84 changes: 76 additions & 8 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use bitcoin::constants::ChainHash;
use bitcoin::key::constants::SECRET_KEY_SIZE;
use bitcoin::network::Network;

use bitcoin::hashes::Hash;
use bitcoin::hashes::{Hash, HashEngine, HmacEngine};
use bitcoin::hashes::hmac::Hmac;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hash_types::{BlockHash, Txid};
Expand Down Expand Up @@ -366,6 +366,7 @@ pub(crate) struct HTLCPreviousHopData {
counterparty_node_id: Option<PublicKey>,
}

#[derive(PartialEq, Eq)]
enum OnionPayload {
/// Indicates this incoming onion payload is for the purpose of paying an invoice.
Invoice {
Expand All @@ -378,6 +379,7 @@ enum OnionPayload {
}

/// HTLCs that are to us and can be failed/claimed by the user
#[derive(PartialEq, Eq)]
struct ClaimableHTLC {
prev_hop: HTLCPreviousHopData,
cltv_expiry: u32,
Expand Down Expand Up @@ -409,6 +411,23 @@ impl From<&ClaimableHTLC> for events::ClaimedHTLC {
}
}

impl PartialOrd for ClaimableHTLC {
fn partial_cmp(&self, other: &ClaimableHTLC) -> Option<cmp::Ordering> {
Some(self.cmp(other))
}
}
impl Ord for ClaimableHTLC {
fn cmp(&self, other: &ClaimableHTLC) -> cmp::Ordering {
let res = (self.prev_hop.channel_id, self.prev_hop.htlc_id).cmp(
&(other.prev_hop.channel_id, other.prev_hop.htlc_id)
);
if res.is_eq() {
debug_assert!(self == other, "ClaimableHTLCs from the same source should be identical");
}
res
}
}

/// A trait defining behavior for creating and verifing the HMAC for authenticating a given data.
pub trait Verification {
/// Constructs an HMAC to include in [`OffersContext`] for the data along with the given
Expand Down Expand Up @@ -471,6 +490,22 @@ impl Verification for PaymentId {
}
}

impl PaymentId {
fn for_inbound_from_htlcs<I: Iterator<Item=(ChannelId, u64)>>(key: &[u8; 32], htlcs: I) -> PaymentId {
let mut prev_pair = None;
let mut hasher = HmacEngine::new(key);
for (channel_id, htlc_id) in htlcs {
hasher.input(&channel_id.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double-check my understanding of the current splicing spec: channel_id and short_channel_id will now be stable even after splices, and even after a 'channel upgrade', right? IIRC, this might have been an issue with earlier splicing/DF drafts, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is my understanding, yes.

hasher.input(&htlc_id.to_le_bytes());
if let Some(prev) = prev_pair {
debug_assert!(prev < (channel_id, htlc_id), "HTLCs should be sorted");
}
prev_pair = Some((channel_id, htlc_id));
}
PaymentId(Hmac::<Sha256>::from_engine(hasher).to_byte_array())
}
}

impl Writeable for PaymentId {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
self.0.write(w)
Expand Down Expand Up @@ -744,6 +779,7 @@ struct ClaimingPayment {
htlcs: Vec<events::ClaimedHTLC>,
sender_intended_value: Option<u64>,
onion_fields: Option<RecipientOnionFields>,
payment_id: Option<PaymentId>,
}
impl_writeable_tlv_based!(ClaimingPayment, {
(0, amount_msat, required),
Expand All @@ -752,6 +788,7 @@ impl_writeable_tlv_based!(ClaimingPayment, {
(5, htlcs, optional_vec),
(7, sender_intended_value, option),
(9, onion_fields, option),
(11, payment_id, option),
});

struct ClaimablePayment {
Expand All @@ -760,6 +797,15 @@ struct ClaimablePayment {
htlcs: Vec<ClaimableHTLC>,
}

impl ClaimablePayment {
fn inbound_payment_id(&self, secret: &[u8; 32]) -> PaymentId {
PaymentId::for_inbound_from_htlcs(
secret,
self.htlcs.iter().map(|htlc| (htlc.prev_hop.channel_id, htlc.prev_hop.htlc_id))
)
}
}

/// Represent the channel funding transaction type.
enum FundingType {
/// This variant is useful when we want LDK to validate the funding transaction and
Expand Down Expand Up @@ -2239,6 +2285,9 @@ where
/// keeping additional state.
probing_cookie_secret: [u8; 32],

/// When generating [`PaymentId`]s for inbound payments, we HMAC the HTLCs with this secret.
inbound_payment_id_secret: [u8; 32],
Copy link
Contributor

Choose a reason for hiding this comment

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

should we call this just payment_id_secret?
Just in case we want to generate payment_id similarly for forwarded payments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can rename it if we change what its used for (and if its for forwarded HTLCs I don't think we should call it "payment" at all - we don't forward payments we forward HTLCs :) )


/// The highest block timestamp we've seen, which is usually a good guess at the current time.
/// Assuming most miners are generating blocks with reasonable timestamps, this shouldn't be
/// very far in the past, and can only ever be up to two hours in the future.
Expand Down Expand Up @@ -3120,6 +3169,7 @@ where
fake_scid_rand_bytes: entropy_source.get_secure_random_bytes(),

probing_cookie_secret: entropy_source.get_secure_random_bytes(),
inbound_payment_id_secret: entropy_source.get_secure_random_bytes(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of these secrets being random, should these be derived from keysmanager?
That makes them deterministic i.e. no need to persist, no more irrecoverable data, and just determinism.

(It is more or less equivalent since we persist these currently and they will remain the same for the life of channelmanager.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did think about this for a minute, but not clear that adding yet another method on a public interface is an improvement here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we already have a method to getNodeSecret and we could just implement hkdf to derive keys in simple manner if possible outside of public interface.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, we could do it by HKDF'ing with some NUMS point, but that seems substantially over-engineered (we have to pick a NUMS point, for starters), and introduces a performance penalty (plus is weird in an async signer context, though we don't strictly speaking allow for an async node_secret currently, and its not clear we ever will). I'm not entirely sure its worth it, even if both of the above issues aren't really critical or insurmountable, just more work..mostly I'm not clear on how much gain there is here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fair enough.
I was not thinking about just this but other secrets in CM can be derived as well, this makes us more independent of channel-manager loss.


highest_seen_timestamp: AtomicUsize::new(current_timestamp as usize),

Expand Down Expand Up @@ -5623,10 +5673,9 @@ where
} else {
claimable_payment.onion_fields = Some(onion_fields);
}
let ref mut htlcs = &mut claimable_payment.htlcs;
let mut total_value = claimable_htlc.sender_intended_value;
let mut earliest_expiry = claimable_htlc.cltv_expiry;
for htlc in htlcs.iter() {
for htlc in claimable_payment.htlcs.iter() {
total_value += htlc.sender_intended_value;
earliest_expiry = cmp::min(earliest_expiry, htlc.cltv_expiry);
if htlc.total_msat != claimable_htlc.total_msat {
Expand All @@ -5648,13 +5697,18 @@ where
#[allow(unused_assignments)] {
committed_to_claimable = true;
}
htlcs.push(claimable_htlc);
let amount_msat = htlcs.iter().map(|htlc| htlc.value).sum();
htlcs.iter_mut().for_each(|htlc| htlc.total_value_received = Some(amount_msat));
let counterparty_skimmed_fee_msat = htlcs.iter()
claimable_payment.htlcs.push(claimable_htlc);
let amount_msat =
claimable_payment.htlcs.iter().map(|htlc| htlc.value).sum();
claimable_payment.htlcs.iter_mut()
.for_each(|htlc| htlc.total_value_received = Some(amount_msat));
let counterparty_skimmed_fee_msat = claimable_payment.htlcs.iter()
.map(|htlc| htlc.counterparty_skimmed_fee_msat.unwrap_or(0)).sum();
debug_assert!(total_value.saturating_sub(amount_msat) <=
counterparty_skimmed_fee_msat);
claimable_payment.htlcs.sort();
let payment_id =
claimable_payment.inbound_payment_id(&self.inbound_payment_id_secret);
new_events.push_back((events::Event::PaymentClaimable {
receiver_node_id: Some(receiver_node_id),
payment_hash,
Expand All @@ -5665,13 +5719,14 @@ where
via_user_channel_id: Some(prev_user_channel_id),
claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER),
onion_fields: claimable_payment.onion_fields.clone(),
payment_id: Some(payment_id),
}, None));
payment_claimable_generated = true;
} else {
// Nothing to do - we haven't reached the total
// payment value yet, wait until we receive more
// MPP parts.
htlcs.push(claimable_htlc);
claimable_payment.htlcs.push(claimable_htlc);
#[allow(unused_assignments)] {
committed_to_claimable = true;
}
Expand Down Expand Up @@ -6468,6 +6523,7 @@ where
}
}

let payment_id = payment.inbound_payment_id(&self.inbound_payment_id_secret);
let claiming_payment = claimable_payments.pending_claiming_payments
.entry(payment_hash)
.and_modify(|_| {
Expand All @@ -6485,6 +6541,7 @@ where
htlcs,
sender_intended_value,
onion_fields: payment.onion_fields,
payment_id: Some(payment_id),
}
});

Expand Down Expand Up @@ -7002,6 +7059,7 @@ where
htlcs,
sender_intended_value: sender_intended_total_msat,
onion_fields,
payment_id,
}) = payment {
self.pending_events.lock().unwrap().push_back((events::Event::PaymentClaimed {
payment_hash,
Expand All @@ -7011,6 +7069,7 @@ where
htlcs,
sender_intended_total_msat,
onion_fields,
payment_id,
}, None));
}
},
Expand Down Expand Up @@ -12232,6 +12291,7 @@ where
let mut events_override = None;
let mut in_flight_monitor_updates: Option<HashMap<(PublicKey, OutPoint), Vec<ChannelMonitorUpdate>>> = None;
let mut decode_update_add_htlcs: Option<HashMap<u64, Vec<msgs::UpdateAddHTLC>>> = None;
let mut inbound_payment_id_secret = None;
read_tlv_fields!(reader, {
(1, pending_outbound_payments_no_retry, option),
(2, pending_intercepted_htlcs, option),
Expand All @@ -12246,6 +12306,7 @@ where
(11, probing_cookie_secret, option),
(13, claimable_htlc_onion_fields, optional_vec),
(14, decode_update_add_htlcs, option),
(15, inbound_payment_id_secret, option),
});
let mut decode_update_add_htlcs = decode_update_add_htlcs.unwrap_or_else(|| new_hash_map());
if fake_scid_rand_bytes.is_none() {
Expand All @@ -12256,6 +12317,10 @@ where
probing_cookie_secret = Some(args.entropy_source.get_secure_random_bytes());
}

if inbound_payment_id_secret.is_none() {
inbound_payment_id_secret = Some(args.entropy_source.get_secure_random_bytes());
}

if let Some(events) = events_override {
pending_events_read = events;
}
Expand Down Expand Up @@ -12730,6 +12795,7 @@ where
previous_hop_monitor.provide_payment_preimage(&payment_hash, &payment_preimage, &args.tx_broadcaster, &bounded_fee_estimator, &args.logger);
}
}
let payment_id = payment.inbound_payment_id(&inbound_payment_id_secret.unwrap());
pending_events_read.push_back((events::Event::PaymentClaimed {
receiver_node_id,
payment_hash,
Expand All @@ -12738,6 +12804,7 @@ where
htlcs: payment.htlcs.iter().map(events::ClaimedHTLC::from).collect(),
sender_intended_total_msat: payment.htlcs.first().map(|htlc| htlc.total_msat),
onion_fields: payment.onion_fields,
payment_id: Some(payment_id),
}, None));
}
}
Expand Down Expand Up @@ -12807,6 +12874,7 @@ where
fake_scid_rand_bytes: fake_scid_rand_bytes.unwrap(),

probing_cookie_secret: probing_cookie_secret.unwrap(),
inbound_payment_id_secret: inbound_payment_id_secret.unwrap(),

our_network_pubkey,
secp_ctx,
Expand Down
Loading
Loading