Skip to content

Remove user_payment_id #1180

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

Merged
merged 1 commit into from
Nov 22, 2021
Merged

Remove user_payment_id #1180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ fn get_payment_secret_hash(dest: &ChanMan, payment_id: &mut u8) -> Option<(Payme
let mut payment_hash;
for _ in 0..256 {
payment_hash = PaymentHash(Sha256::hash(&[*payment_id; 1]).into_inner());
if let Ok(payment_secret) = dest.create_inbound_payment_for_hash(payment_hash, None, 3600, 0) {
if let Ok(payment_secret) = dest.create_inbound_payment_for_hash(payment_hash, None, 3600) {
return Some((payment_secret, payment_hash));
}
*payment_id = payment_id.wrapping_add(1);
Expand Down
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 @@ -529,7 +529,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
let payment_hash = PaymentHash(Sha256::from_engine(sha).into_inner());
// Note that this may fail - our hashes may collide and we'll end up trying to
// double-register the same payment_hash.
let _ = channelmanager.create_inbound_payment_for_hash(payment_hash, None, 1, 0);
let _ = channelmanager.create_inbound_payment_for_hash(payment_hash, None, 1);
},
9 => {
for payment in payments_received.drain(..) {
Expand Down
1 change: 0 additions & 1 deletion lightning-invoice/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ where
let (payment_hash, payment_secret) = channelmanager.create_inbound_payment(
amt_msat,
DEFAULT_EXPIRY_TIME.try_into().unwrap(),
0,
);
let our_node_pubkey = channelmanager.get_our_node_id();
let mut invoice = InvoiceBuilder::new(network)
Expand Down
26 changes: 11 additions & 15 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,9 @@ struct PeerState {
///
/// For users who don't want to bother doing their own payment preimage storage, we also store that
/// here.
///
/// Note that this struct will be removed entirely soon, in favor of storing no inbound payment data
/// and instead encoding it in the payment secret.
struct PendingInboundPayment {
/// The payment secret that the sender must use for us to accept this payment
payment_secret: PaymentSecret,
Expand Down Expand Up @@ -2887,7 +2890,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
purpose: events::PaymentPurpose::InvoicePayment {
payment_preimage: inbound_payment.get().payment_preimage,
payment_secret: payment_data.payment_secret,
user_payment_id: inbound_payment.get().user_payment_id,
},
amt: total_value,
});
Expand Down Expand Up @@ -4523,7 +4525,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
}

fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106

let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes());
Expand All @@ -4533,7 +4535,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
match payment_secrets.entry(payment_hash) {
hash_map::Entry::Vacant(e) => {
e.insert(PendingInboundPayment {
payment_secret, min_value_msat, user_payment_id, payment_preimage,
payment_secret, min_value_msat, payment_preimage,
user_payment_id: 0, // For compatibility with version 0.0.103 and earlier
// We assume that highest_seen_timestamp is pretty close to the current time -
// its updated when we receive a new block with the maximum time we've seen in
// a header. It should never be more than two hours in the future.
Expand Down Expand Up @@ -4565,12 +4568,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
/// [`PaymentReceived`]: events::Event::PaymentReceived
/// [`PaymentReceived::payment_preimage`]: events::Event::PaymentReceived::payment_preimage
/// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> (PaymentHash, PaymentSecret) {
pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> (PaymentHash, PaymentSecret) {
let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());

(payment_hash,
self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs, user_payment_id)
self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs)
.expect("RNG Generated Duplicate PaymentHash"))
}

Expand All @@ -4584,12 +4587,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
/// The [`PaymentHash`] (and corresponding [`PaymentPreimage`]) must be globally unique. This
/// method may return an Err if another payment with the same payment_hash is still pending.
///
/// `user_payment_id` will be provided back in [`PaymentPurpose::InvoicePayment::user_payment_id`] events to
/// allow tracking of which events correspond with which calls to this and
/// [`create_inbound_payment`]. `user_payment_id` has no meaning inside of LDK, it is simply
/// copied to events and otherwise ignored. It may be used to correlate PaymentReceived events
/// with invoice metadata stored elsewhere.
///
Comment on lines -4587 to -4592
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be correct to say that payment_secret will be used for this after the follow-up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

More payment_hash, I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah I guess we won't duplicate those

/// `min_value_msat` should be set if the invoice being generated contains a value. Any payment
/// received for the returned [`PaymentHash`] will be required to be at least `min_value_msat`
/// before a [`PaymentReceived`] event will be generated, ensuring that we do not provide the
Expand Down Expand Up @@ -4618,9 +4615,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
///
/// [`create_inbound_payment`]: Self::create_inbound_payment
/// [`PaymentReceived`]: events::Event::PaymentReceived
/// [`PaymentPurpose::InvoicePayment::user_payment_id`]: events::PaymentPurpose::InvoicePayment::user_payment_id
pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs, user_payment_id)
pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs)
}

#[cfg(any(test, feature = "fuzztarget", feature = "_test_utils"))]
Expand Down Expand Up @@ -6680,7 +6676,7 @@ pub mod bench {
payment_preimage.0[0..8].copy_from_slice(&payment_count.to_le_bytes());
payment_count += 1;
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner());
let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200, 0).unwrap();
let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200).unwrap();

$node_a.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
let payment_event = SendEvent::from_event($node_a.get_and_clear_pending_msg_events().pop().unwrap());
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ macro_rules! get_payment_preimage_hash {
let payment_preimage = PaymentPreimage([*payment_count; 32]);
*payment_count += 1;
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner());
let payment_secret = $dest_node.node.create_inbound_payment_for_hash(payment_hash, $min_value_msat, 7200, 0).unwrap();
let payment_secret = $dest_node.node.create_inbound_payment_for_hash(payment_hash, $min_value_msat, 7200).unwrap();
(payment_preimage, payment_hash, payment_secret)
}
}
Expand Down
38 changes: 18 additions & 20 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,7 @@ fn test_duplicate_htlc_different_direction_onchain() {
let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1])[..], 900_000);

let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[0], 800_000);
let node_a_payment_secret = nodes[0].node.create_inbound_payment_for_hash(payment_hash, None, 7200, 0).unwrap();
let node_a_payment_secret = nodes[0].node.create_inbound_payment_for_hash(payment_hash, None, 7200).unwrap();
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[0]]], 800_000, payment_hash, node_a_payment_secret);

// Provide preimage to node 0 by claiming payment
Expand Down Expand Up @@ -4957,7 +4957,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {

let (our_payment_preimage, duplicate_payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 900000);

let payment_secret = nodes[3].node.create_inbound_payment_for_hash(duplicate_payment_hash, None, 7200, 0).unwrap();
let payment_secret = nodes[3].node.create_inbound_payment_for_hash(duplicate_payment_hash, None, 7200).unwrap();
// We reduce the final CLTV here by a somewhat arbitrary constant to keep it under the one-byte
// script push size limit so that the below script length checks match
// ACCEPTED_HTLC_SCRIPT_WEIGHT.
Expand Down Expand Up @@ -5160,30 +5160,30 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
let (_, payment_hash_2, _) = route_payment(&nodes[0], &[&nodes[2], &nodes[3], &nodes[4]], ds_dust_limit*1000); // not added < dust limit + HTLC tx fee
let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], ds_dust_limit*1000);
// 2nd HTLC:
send_along_route_with_secret(&nodes[1], route.clone(), &[&[&nodes[2], &nodes[3], &nodes[5]]], ds_dust_limit*1000, payment_hash_1, nodes[5].node.create_inbound_payment_for_hash(payment_hash_1, None, 7200, 0).unwrap()); // not added < dust limit + HTLC tx fee
send_along_route_with_secret(&nodes[1], route.clone(), &[&[&nodes[2], &nodes[3], &nodes[5]]], ds_dust_limit*1000, payment_hash_1, nodes[5].node.create_inbound_payment_for_hash(payment_hash_1, None, 7200).unwrap()); // not added < dust limit + HTLC tx fee
// 3rd HTLC:
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], ds_dust_limit*1000, payment_hash_2, nodes[5].node.create_inbound_payment_for_hash(payment_hash_2, None, 7200, 0).unwrap()); // not added < dust limit + HTLC tx fee
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], ds_dust_limit*1000, payment_hash_2, nodes[5].node.create_inbound_payment_for_hash(payment_hash_2, None, 7200).unwrap()); // not added < dust limit + HTLC tx fee
// 4th HTLC:
let (_, payment_hash_3, _) = route_payment(&nodes[0], &[&nodes[2], &nodes[3], &nodes[4]], 1000000);
// 5th HTLC:
let (_, payment_hash_4, _) = route_payment(&nodes[0], &[&nodes[2], &nodes[3], &nodes[4]], 1000000);
let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], 1000000);
// 6th HTLC:
send_along_route_with_secret(&nodes[1], route.clone(), &[&[&nodes[2], &nodes[3], &nodes[5]]], 1000000, payment_hash_3, nodes[5].node.create_inbound_payment_for_hash(payment_hash_3, None, 7200, 0).unwrap());
send_along_route_with_secret(&nodes[1], route.clone(), &[&[&nodes[2], &nodes[3], &nodes[5]]], 1000000, payment_hash_3, nodes[5].node.create_inbound_payment_for_hash(payment_hash_3, None, 7200).unwrap());
// 7th HTLC:
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], 1000000, payment_hash_4, nodes[5].node.create_inbound_payment_for_hash(payment_hash_4, None, 7200, 0).unwrap());
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], 1000000, payment_hash_4, nodes[5].node.create_inbound_payment_for_hash(payment_hash_4, None, 7200).unwrap());

// 8th HTLC:
let (_, payment_hash_5, _) = route_payment(&nodes[0], &[&nodes[2], &nodes[3], &nodes[4]], 1000000);
// 9th HTLC:
let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], ds_dust_limit*1000);
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], ds_dust_limit*1000, payment_hash_5, nodes[5].node.create_inbound_payment_for_hash(payment_hash_5, None, 7200, 0).unwrap()); // not added < dust limit + HTLC tx fee
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], ds_dust_limit*1000, payment_hash_5, nodes[5].node.create_inbound_payment_for_hash(payment_hash_5, None, 7200).unwrap()); // not added < dust limit + HTLC tx fee

// 10th HTLC:
let (_, payment_hash_6, _) = route_payment(&nodes[0], &[&nodes[2], &nodes[3], &nodes[4]], ds_dust_limit*1000); // not added < dust limit + HTLC tx fee
// 11th HTLC:
let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], 1000000);
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], 1000000, payment_hash_6, nodes[5].node.create_inbound_payment_for_hash(payment_hash_6, None, 7200, 0).unwrap());
send_along_route_with_secret(&nodes[1], route, &[&[&nodes[2], &nodes[3], &nodes[5]]], 1000000, payment_hash_6, nodes[5].node.create_inbound_payment_for_hash(payment_hash_6, None, 7200).unwrap());

// Double-check that six of the new HTLC were added
// We now have six HTLCs pending over the dust limit and six HTLCs under the dust limit (ie,
Expand Down Expand Up @@ -7164,7 +7164,7 @@ fn test_check_htlc_underpaying() {
let payee = Payee::from_node_id(nodes[1].node.get_our_node_id()).with_features(InvoiceFeatures::known());
let route = get_route(&nodes[0].node.get_our_node_id(), &payee, nodes[0].network_graph, None, 10_000, TEST_FINAL_CLTV, nodes[0].logger, &scorer).unwrap();
let (_, our_payment_hash, _) = get_payment_preimage_hash!(nodes[0]);
let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(our_payment_hash, Some(100_000), 7200, 0).unwrap();
let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(our_payment_hash, Some(100_000), 7200).unwrap();
nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap();
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -8018,7 +8018,7 @@ fn test_preimage_storage() {
create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;

{
let (payment_hash, payment_secret) = nodes[1].node.create_inbound_payment(Some(100_000), 7200, 42);
let (payment_hash, payment_secret) = nodes[1].node.create_inbound_payment(Some(100_000), 7200);
let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000);
nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
check_added_monitors!(nodes[0], 1);
Expand All @@ -8035,8 +8035,7 @@ fn test_preimage_storage() {
match events[0] {
Event::PaymentReceived { ref purpose, .. } => {
match &purpose {
PaymentPurpose::InvoicePayment { payment_preimage, user_payment_id, .. } => {
assert_eq!(*user_payment_id, 42);
PaymentPurpose::InvoicePayment { payment_preimage, .. } => {
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage.unwrap());
},
_ => panic!("expected PaymentPurpose::InvoicePayment")
Expand All @@ -8056,11 +8055,11 @@ fn test_secret_timeout() {

create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;

let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment(Some(100_000), 2, 0);
let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment(Some(100_000), 2);

// We should fail to register the same payment hash twice, at least until we've connected a
// block with time 7200 + CHAN_CONFIRM_DEPTH + 1.
if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2, 0) {
if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) {
assert_eq!(err, "Duplicate payment hash");
} else { panic!(); }
let mut block = {
Expand All @@ -8075,16 +8074,16 @@ fn test_secret_timeout() {
}
};
connect_block(&nodes[1], &block);
if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2, 0) {
if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) {
assert_eq!(err, "Duplicate payment hash");
} else { panic!(); }

// If we then connect the second block, we should be able to register the same payment hash
// again with a different user_payment_id (this time getting a new payment secret).
// again (this time getting a new payment secret).
block.header.prev_blockhash = block.header.block_hash();
block.header.time += 1;
connect_block(&nodes[1], &block);
let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2, 42).unwrap();
let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2).unwrap();
assert_ne!(payment_secret_1, our_payment_secret);

{
Expand All @@ -8102,9 +8101,8 @@ fn test_secret_timeout() {
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentReceived { purpose: PaymentPurpose::InvoicePayment { payment_preimage, payment_secret, user_payment_id }, .. } => {
Event::PaymentReceived { purpose: PaymentPurpose::InvoicePayment { payment_preimage, payment_secret }, .. } => {
assert!(payment_preimage.is_none());
assert_eq!(user_payment_id, 42);
assert_eq!(payment_secret, our_payment_secret);
// We don't actually have the payment preimage with which to claim this payment!
},
Expand All @@ -8124,7 +8122,7 @@ fn test_bad_secret_hash() {

let random_payment_hash = PaymentHash([42; 32]);
let random_payment_secret = PaymentSecret([43; 32]);
let (our_payment_hash, our_payment_secret) = nodes[1].node.create_inbound_payment(Some(100_000), 2, 0);
let (our_payment_hash, our_payment_secret) = nodes[1].node.create_inbound_payment(Some(100_000), 2);
let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000);

// All the below cases should end up being handled exactly identically, so we macro the
Expand Down
Loading