Add invoice constructor with custom payment hash #1894

ssbright · 2022-12-01T18:46:17Z

TheBlueMatt

Thanks!

TheBlueMatt · 2022-12-02T00:30:07Z

lightning-invoice/src/utils.rs

@@ -332,6 +332,82 @@ where
 	)
 }

+pub fn create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(


All public methods are required to have documentation - this is causing CI to fail.

TheBlueMatt · 2022-12-02T00:31:53Z

lightning-invoice/src/utils.rs

+pub fn create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(
+	channelmanager: &ChannelManager<M, T, K, F, L>, keys_manager: K, logger: L,
+	network: Currency, amt_msat: Option<u64>, description: String, duration_since_epoch: Duration,
+	invoice_expiry_delta_secs: u32, payment_hash: PaymentHash, payment_secret: PaymentSecret


We shouldn't take the payment_secret but rather call https://docs.rs/lightning/latest/lightning/ln/channelmanager/struct.ChannelManager.html#method.create_inbound_payment_for_hash with the payment hash to get it.

TheBlueMatt · 2022-12-02T00:32:24Z

lightning-invoice/src/utils.rs

+	)
+}
+
+fn _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(


You should be able to DRY this up a lot by having _create_invoice_from_channelmanager_and_duration_since_epoch generate the payment hash/secret and then simply call this method. For git-diff-cleanliness you should probably then move _create_invoice_from_channelmanager_and_duration_since_epoch above this function.

+1 for DRYing this is up.

@TheBlueMatt , do you mean change _create_invoice_from_channelmanager_and_duration_since_epoch so that it also takes a payment_hash and have it also construct the payment_secret inside (as suggested in your other comment). That way, my main function calls this method instead of my new one?

You can split up the functions any way you want, but we'll need one util method that takes the payment hash + secret and builds the invoice (doing most of the work) and one like the existing util method that builds its own payment hash + secret. The method you're adding will need to take only the payment hash, building the payment secret itself and then delegating to the common big util function.

im sorry, I am having trouble understanding this request. I am relatively new to this. Can someone help me implement this? I believe I have solved the other problems here, will push when I do this last part.

So compared to your current code, you can replace _create_invoice_from_channelmanager_and_duration_since_epoch with, simply,

// `create_inbound_payment` only returns an error if the amount is greater than the total bitcoin // supply. let (payment_hash, payment_secret) = channelmanager .create_inbound_payment(amt_msat, invoice_expiry_delta_secs) .map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?; + _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(channelmanager, + keys_manager, logger, network, amt_msat, description, duration_since_epoch, + invoice_expiry_delta_secs, payment_hash, payment_secret)

If you do that, and swap the order of that function and the _with_payment_hash variant that you added the total diff in the patch will be substantially smaller.

thank you! I hope this new commit fixes this as envisioned and addresses the other comments here.

tnull · 2022-12-02T09:03:21Z

lightning-invoice/src/utils.rs

+	)
+}
+
+fn _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(


+1 for DRYing this is up.

tnull · 2022-12-02T09:03:58Z

lightning-invoice/src/utils.rs

@@ -665,6 +741,26 @@ mod test {
 		assert_eq!(invoice.description(), InvoiceDescription::Hash(&crate::Sha256(Sha256::hash("Testing description_hash".as_bytes()))));
 	}

+


nit: remove superfluous whitespace.

Suggested change

TheBlueMatt · 2022-12-05T21:15:00Z

tagged 113 because it fixes an issue tagged 113 and we have users who want this, but if it slips, it slips. Will need to land by early next week to make 113.

TheBlueMatt

Nice thanks. Can you squash down the fixup commit via a rebase? See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

TheBlueMatt · 2022-12-07T19:53:11Z

lightning-invoice/src/utils.rs

@@ -365,7 +336,7 @@ where
 		.duration_since_epoch(duration_since_epoch)
 		.payee_pub_key(our_node_pubkey)
 		.payment_hash(Hash::from_slice(&payment_hash.0).unwrap())
-		.payment_secret(payment_secret)
+		.payment_secret(payment_secret.unwrap())


We shouldnt unwrap here but rather map the err and return an err, like we do when we call .create_inbound_payment

noob question again... I am trying to do what you said as

.payment_secret(payment_secret) .map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?

but its yelling at me with the same thing where it doesn't like how payment_secret is throwing result. What am I not understanding? Once I do this will try to squash and then push again.

You have to map_err when constructing the payment_secret.

ok great, I think I have done that. Pushed again, hopefully I did the rebase correctly as well? Let me know if there are any issues or further changes needed.

There's still three commits in the PR, please squash them all down to one (I think I might have linked the wrong section in the doc), and make sure the commit message describes what it's doing (and not the fixups that are included).

codecov-commenter · 2022-12-07T20:47:18Z

Codecov Report

Base: 90.70% // Head: 90.67% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (5f71fe9) compared to base (5e577cb).
Patch coverage: 96.87% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1894      +/-   ##
==========================================
- Coverage   90.70%   90.67%   -0.03%     
==========================================
  Files          91       94       +3     
  Lines       48392    49559    +1167     
  Branches    48392    49559    +1167     
==========================================
+ Hits        43894    44940    +1046     
- Misses       4498     4619     +121

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`97.77% <96.87%> (-0.04%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <0.00%> (-7.82%)`	⬇️
lightning/src/util/test_utils.rs	`73.50% <0.00%> (-3.56%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`82.92% <0.00%> (-3.26%)`	⬇️
lightning/src/ln/onion_utils.rs	`93.56% <0.00%> (-1.37%)`	⬇️
lightning/src/chain/package.rs	`91.59% <0.00%> (-1.25%)`	⬇️
lightning-net-tokio/src/lib.rs	`77.01% <0.00%> (-0.30%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.93% <0.00%> (-0.22%)`	⬇️
lightning/src/ln/monitor_tests.rs	`99.46% <0.00%> (-0.11%)`	⬇️
lightning/src/util/ser.rs	`91.84% <0.00%> (-0.09%)`	⬇️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull · 2022-12-08T09:08:23Z

lightning-invoice/src/utils.rs

-/// This version can be used in a `no_std` environment, where [`std::time::SystemTime`] is not
-/// available and the current time is supplied by the caller.
-pub fn create_invoice_from_channelmanager_and_duration_since_epoch<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(
+///Function that allows for contructing invoices with custom payment hash


nit:

Suggested change

///Function that allows for contructing invoices with custom payment hash

/// Function that allows to construct invoices with the given payment hash.

tnull · 2022-12-08T09:23:48Z

lightning-invoice/src/utils.rs

 	let our_node_pubkey = channelmanager.get_our_node_id();
 	let channels = channelmanager.list_channels();
-
+	let payment_secret = channelmanager.create_inbound_payment_for_hash(payment_hash,amt_msat,invoice_expiry_delta_secs);


This should be

Suggested change

let payment_secret = channelmanager.create_inbound_payment_for_hash(payment_hash,amt_msat,invoice_expiry_delta_secs);

let payment_secret = channelmanager

.create_inbound_payment_for_hash(payment_hash, amt_msat, invoice_expiry_delta_secs)

.map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?;

tnull · 2022-12-08T09:24:33Z

lightning-invoice/src/utils.rs

@@ -365,7 +336,7 @@ where
 		.duration_since_epoch(duration_since_epoch)
 		.payee_pub_key(our_node_pubkey)
 		.payment_hash(Hash::from_slice(&payment_hash.0).unwrap())
-		.payment_secret(payment_secret)
+		.payment_secret(map_err(payment_secret))


This doesn't compile, should be simply payment_secret if you made the map_err()? change above.

TheBlueMatt · 2022-12-08T18:54:05Z

lightning-invoice/src/utils.rs

-/// This version can be used in a `no_std` environment, where [`std::time::SystemTime`] is not
-/// available and the current time is supplied by the caller.
-pub fn create_invoice_from_channelmanager_and_duration_since_epoch<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(
+///Function that allows to construct invoices with the given payment hash.


This doesn't really describe why a user would want to use this vs the other available utilities here, nor does it describe what half the parameters are. You might consider linking to an existing function as several other utilities in this file do, and then contrasting it, describing why a user would want a manual payment hash.

This one is difficult for me because I am not completely sure why a user would want a manual payment hash either. I also wasn't quite understanding the formats of the other functions descriptions, so was pretty unsure what the best description would be. Help here would be appreciated.

I think for the payment hash thing we should say, eg, "this may be useful if you're building an on-chain swap or integrating with another protocol where the payment hash is fixed outside of lightning". You can just link to another function with the [``] syntax, look at some of the other functions for examples.

ok, I hope what I did was good, and I hopefully reduced the diff for easier review.

TheBlueMatt · 2022-12-08T18:54:49Z

lightning-invoice/src/utils.rs

+/// See [`create_invoice_from_channelmanager`]
+/// This version can be used in a `no_std` environment, where [`std::time::SystemTime`] is not
+/// available and the current time is supplied by the caller.
+pub fn create_invoice_from_channelmanager_and_duration_since_epoch<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(


nit: if you move this and the above function up to their original positions the total diff will be smaller and thus easier to review. Its not a huge deal but its always nice to keep diffs small by not moving code around too much in commits that also add/change other code.

ok, will try to do this when I fix function description!

TheBlueMatt · 2022-12-09T21:41:49Z

lightning-invoice/src/utils.rs

 	let our_node_pubkey = channelmanager.get_our_node_id();
 	let channels = channelmanager.list_channels();
-
+	let payment_secret = channelmanager


Oops, missed this last time, can you move this hunk into create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash and pass the payment_secret into _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash (including from _create_invoice_from_channelmanager_and_duration_since_epoch)

dunxen

Thanks, @ssbright. Just left a few minor comments.

dunxen · 2022-12-12T17:36:11Z

lightning-invoice/src/utils.rs

+		T::Target: BroadcasterInterface,
+		K::Target: KeysInterface,
+		F::Target: FeeEstimator,
+		L::Target: Logger,


I think your formatter might be touching this unrelated code.

dunxen · 2022-12-12T17:37:22Z

lightning-invoice/src/utils.rs

-
 		let mut scid_aliases = HashSet::new();
 		scid_aliases.insert(chan_1_0_high_inbound_capacity.0.short_channel_id_alias.unwrap());
-
 		match_invoice_routes(Some(5000), &nodes[0], scid_aliases);


nit: Can also undo these unrelated changes.

lightning-invoice/src/utils.rs

dunxen · 2022-12-12T18:06:12Z

lightning-invoice/src/utils.rs

+	_create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash(
+		channelmanager, keys_manager, logger, network, amt_msat,
+		InvoiceDescription::Direct(
+			&Description::new(description).map_err(SignOrCreationError::CreationError)?,
+		),
+		duration_since_epoch, invoice_expiry_delta_secs, payment_hash, payment_secret
+	)


Looking neat!

tnull

Looks good from my side, ACK after outstanding comments are addressed.

ssbright · 2022-12-12T21:09:22Z

ok! I think I addressed the issues made by @dunxen . Hopefully this is all good.

dunxen

Thanks, @ssbright! I'll cover any possible nits left over in a follow-up!

TheBlueMatt

Gonna go ahead and land this since @tnull's last review included basically the same code with minimal changes. We can, of course, do any followups needed.

TheBlueMatt · 2022-12-12T23:23:55Z

CI failure is just #1914, unrelated to PR.

tnull self-requested a review December 1, 2022 18:46

dunxen self-requested a review December 1, 2022 18:54

TheBlueMatt reviewed Dec 2, 2022

View reviewed changes

tnull reviewed Dec 2, 2022

View reviewed changes

TheBlueMatt added this to the 0.0.113 milestone Dec 5, 2022

TheBlueMatt reviewed Dec 7, 2022

View reviewed changes

ssbright force-pushed the 2022-12-custom_payment_hash branch from faf4072 to 51136d5 Compare December 7, 2022 23:43

tnull reviewed Dec 8, 2022

View reviewed changes

ssbright force-pushed the 2022-12-custom_payment_hash branch from 51136d5 to 296c7f8 Compare December 8, 2022 17:16

TheBlueMatt reviewed Dec 8, 2022

View reviewed changes

ssbright force-pushed the 2022-12-custom_payment_hash branch from 296c7f8 to 4eddd20 Compare December 9, 2022 19:01

TheBlueMatt reviewed Dec 9, 2022

View reviewed changes

ssbright force-pushed the 2022-12-custom_payment_hash branch from 4eddd20 to 3622a04 Compare December 12, 2022 16:55

TheBlueMatt assigned tnull and TheBlueMatt Dec 12, 2022

TheBlueMatt previously approved these changes Dec 12, 2022

View reviewed changes

dunxen reviewed Dec 12, 2022

View reviewed changes

tnull reviewed Dec 12, 2022

View reviewed changes

ssbright dismissed TheBlueMatt’s stale review via 58cc5f0 December 12, 2022 21:04

ssbright force-pushed the 2022-12-custom_payment_hash branch from 3e41d74 to da79e03 Compare December 12, 2022 21:07

Add invoice constructor with custom payment hash

5f71fe9

ssbright force-pushed the 2022-12-custom_payment_hash branch from da79e03 to 5f71fe9 Compare December 12, 2022 21:08

dunxen approved these changes Dec 12, 2022

View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022

View reviewed changes

TheBlueMatt merged commit 5e14c24 into lightningdevkit:main Dec 13, 2022

		@@ -665,6 +741,26 @@ mod test {
		assert_eq!(invoice.description(), InvoiceDescription::Hash(&crate::Sha256(Sha256::hash("Testing description_hash".as_bytes()))));
		}

	///Function that allows for contructing invoices with custom payment hash
	/// Function that allows to construct invoices with the given payment hash.

Add invoice constructor with custom payment hash #1894

Add invoice constructor with custom payment hash #1894

Conversation

ssbright commented Dec 1, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Dec 5, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

ssbright commented Dec 12, 2022

dunxen left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Dec 12, 2022

codecov-commenter commented Dec 7, 2022 •

edited

Loading

tnull left a comment •

edited

Loading