Uh oh!
There was an error while loading. Please reload this page.
Add a PaymentId for inbound payments#3303
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -23,7 +23,7 @@ use bitcoin::constants::ChainHash; | ||
| use bitcoin::key::constants::SECRET_KEY_SIZE; | ||
| use bitcoin::network::Network; | ||
| use bitcoin::hashes::{Hash, HashEngine, HmacEngine}; | ||
| use bitcoin::hashes::hmac::Hmac; | ||
| use bitcoin::hashes::sha256::Hash as Sha256; | ||
| use bitcoin::hash_types::{BlockHash, Txid}; | ||
| @@ -368,6 +368,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 { | ||
| @@ -380,6 +381,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, | ||
| @@ -411,6 +413,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 | ||
| @@ -491,6 +510,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); | ||
| 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) | ||
| @@ -764,6 +799,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), | ||
| @@ -772,6 +808,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 { | ||
| @@ -780,6 +817,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 | ||
| @@ -2261,6 +2307,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], | ||
G8XSU marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /// 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. | ||
| @@ -3152,6 +3201,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(), | ||
Contributor There was a problem hiding this comment. Instead of these secrets being random, should these be derived from keysmanager? (It is more or less equivalent since we persist these currently and they will remain the same for the life of channelmanager.) CollaboratorAuthor There was a problem hiding this comment. I did think about this for a minute, but not clear that adding yet another method on a public interface is an improvement here? Contributor There was a problem hiding this comment. 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. CollaboratorAuthor There was a problem hiding this comment. 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. Contributor There was a problem hiding this comment. Ok, fair enough. | ||
| highest_seen_timestamp: AtomicUsize::new(current_timestamp as usize), | ||
| @@ -5745,10 +5795,9 @@ where | ||
| } else { | ||
| claimable_payment.onion_fields = Some(onion_fields); | ||
| } | ||
| let mut total_value = claimable_htlc.sender_intended_value; | ||
| let mut earliest_expiry = claimable_htlc.cltv_expiry; | ||
| 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 { | ||
| @@ -5770,13 +5819,18 @@ where | ||
| #[allow(unused_assignments)] { | ||
| committed_to_claimable = true; | ||
| } | ||
| 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, | ||
| @@ -5787,13 +5841,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. | ||
| claimable_payment.htlcs.push(claimable_htlc); | ||
| #[allow(unused_assignments)] { | ||
| committed_to_claimable = true; | ||
| } | ||
| @@ -6590,6 +6645,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(|_| { | ||
| @@ -6607,6 +6663,7 @@ where | ||
| htlcs, | ||
| sender_intended_value, | ||
| onion_fields: payment.onion_fields, | ||
| payment_id: Some(payment_id), | ||
| } | ||
| }); | ||
| @@ -7124,6 +7181,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, | ||
| @@ -7133,6 +7191,7 @@ where | ||
| htlcs, | ||
| sender_intended_total_msat, | ||
| onion_fields, | ||
| payment_id, | ||
| }, None)); | ||
| } | ||
| }, | ||
| @@ -12381,6 +12440,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), | ||
| @@ -12395,6 +12455,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() { | ||
| @@ -12405,6 +12466,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; | ||
| } | ||
| @@ -12853,6 +12918,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, | ||
| @@ -12861,6 +12927,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)); | ||
| } | ||
| } | ||
| @@ -12930,6 +12997,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, | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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_idandshort_channel_idwill 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?There was a problem hiding this comment.
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.