Create normal/phantom invoice with description hash #1361

luizParreira · 2022-03-13T15:37:56Z

luizParreira · 2022-03-13T15:44:31Z

@TheBlueMatt I implemented the first try on supporting description_hash. On my implementation, I created 2 variant functions create_phantom_invoice_with_description_hash and create_invoice_from_channelmanager_with_description_hash, both of which receive a normal description that is hashed within the function. I was not sure if this would be better than just receiving the description_hash directly. Please, let me know what you think. :)

codecov-commenter · 2022-03-13T16:06:59Z

Codecov Report

Merging #1361 (2ce3832) into main (c244c78) will increase coverage by 0.01%.
The diff coverage is 97.67%.

❗ Current head 2ce3832 differs from pull request most recent head 925e296. Consider uploading reports for the commit 925e296 to get more accurate results

@@            Coverage Diff             @@
##             main    #1361      +/-   ##
==========================================
+ Coverage   90.73%   90.74%   +0.01%     
==========================================
  Files          73       73              
  Lines       40808    40885      +77     
  Branches    40808    40885      +77     
==========================================
+ Hits        37027    37102      +75     
- Misses       3781     3783       +2

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`96.79% <97.67%> (+0.25%)`	⬆️
lightning/src/util/events.rs	`33.09% <0.00%> (-0.36%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️
lightning/src/routing/router.rs	`92.40% <0.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c244c78...925e296. Read the comment docs.

ViktorTigerstrom

In general this looks quite good in terms of solving the issue from what I can see :)

However, please avoid mixing code reformatting with code changes as this PR currently does. Else it becomes hard to see what is actual code changes, and what is only code reformatting. It also complicates using git log/blame in the future, to find out why certain code was changed. If your IDE auto formats the code, I would recommend that you disable that feature, and only make it hint reformatting tips.
If you really want to include the reformatting, I'd recommend that you at least push it into a separate PR/commit.

ViktorTigerstrom · 2022-03-14T15:29:57Z

lightning-invoice/src/utils.rs

+        InvoiceDescription::Hash(hash) => InvoiceBuilder::new(network).description_hash(hash.0),
+    };
+
+    let mut invoice = invoice


The code below here in this function is code reformatting.

ViktorTigerstrom · 2022-03-14T15:48:19Z

lightning-invoice/src/utils.rs

+        InvoiceDescription::Hash(hash) => InvoiceBuilder::new(network).description_hash(hash.0),
+    };
+
+    let mut invoice = invoice


The code below here in this function is mostly code reformatting.

In this case, I am also avoiding the route_hints double loop and am setting the invoice's private_route as we loop through the channels.

Ah I see! That specific optimization will conflict with #1325, but if you prefer keeping it, whichever PR gets merged after the other could just resolve that :)

lightning-invoice/src/utils.rs

luizParreira · 2022-03-14T16:26:25Z

@ViktorTigerstrom Thank you very much for the review. I managed to fix all of your points. Let me know if there is any other improvement you think I should make.

ViktorTigerstrom · 2022-03-14T19:30:33Z

Great, that looks much better :)! Some quick fixes I recommend that you fix are:

Use tabs rather than spaces, which the rest of the project does
Try looking at how the rest of the project formats calls/argument passing to functions like create_announced_chan_between_nodes_with_value, handle_channel_update and similar to make the code more consistent with the rest of the project :)

TheBlueMatt

Nice this looks pretty great. As @ViktorTigerstrom mentioned, could you replace spaces here with tabs, otherwise I'm basically happy with this. Note it'll conflict with #1325 which is probably about ready to land, so I'll let you and @ViktorTigerstrom hash out who has to rebase on which and the merge order.

TheBlueMatt · 2022-03-16T01:17:46Z

lightning-invoice/src/utils.rs

+            InitFeatures::known(),
+        );
+        nodes[0]
+            .node


I know this is the rustfmt way and many rust projects use this kind of super-vertical layout, but I find it completely unreadable (and its the primary reason we don't use rustfmt here and I believe the primary reason rust-bitcoin doesn't use it either). If possible, could you condense lines a bit so that its easier for the eye to scan between statements? Only break lines if the line is reaching 100 chars in length, roughly.

luizParreira · 2022-03-16T17:05:55Z

@TheBlueMatt @ViktorTigerstrom I tried to remove most of the line breaks and make the code layout more horizontal. Please, let me know if there is anything else you would like me to improve :)

TheBlueMatt · 2022-03-16T17:33:06Z

Thanks! Sadly this needs rebase since we landed #1325 but otherwise it looks basically good to me.

luizParreira · 2022-03-16T17:40:01Z

@TheBlueMatt Rebase done :)

TheBlueMatt · 2022-03-16T17:41:51Z

lightning-invoice/src/utils.rs

+{
+    _create_phantom_invoice::<Signer, K>(
+        amt_msat,
+        InvoiceDescription::Hash(&crate::Sha256(Hash::hash(description.as_bytes()))),


Oh, actually, sorry, can we just take the hash itself as the argument? That's strictly more generic.

You mean receive the Sha256 struct as a param to create_phantom_invoice_with_description_hash, instead of the desccription as as String? 🤔

@TheBlueMatt Got it. 2ce3832

TheBlueMatt · 2022-03-16T17:42:45Z

lightning-invoice/src/utils.rs

@@ -1015,4 +1201,82 @@ mod test {
 		}
 		assert!(chan_ids_to_match.is_empty(), "Unmatched short channel ids: {:?}", chan_ids_to_match);
 	}
+    #[test]


nit: there's still a lot of spaces instead of tabs in the file in the tests here :).

@TheBlueMatt My bad, I forgot to fix the spaces issue. Is it ok now? On my editor it seems to be on tabs size 4 🤔

ViktorTigerstrom

The rebase looks good! Just some quick recommendations.

Also, unfortunately there's still spaces instead of tabs in all of functions and tests you've added. In some functions/tests, there's a mix between both tabs and spaces. You should set your editor to specifically use tabs, not 4 spaces for indentations.

ViktorTigerstrom · 2022-03-16T18:16:08Z

lightning-invoice/src/utils.rs

@@ -1015,4 +1201,81 @@ mod test {
 		}
 		assert!(chan_ids_to_match.is_empty(), "Unmatched short channel ids: {:?}", chan_ids_to_match);
 	}
+
+    #[test]
+    fn test_create_invoice_with_description_hash() {


I'd personally place this test with the other tests that doesn't require std, just so it becomes clearer what's actually loaded under which configuration. I suggest you place it after the test_from_channelmanager test.

ViktorTigerstrom · 2022-03-16T18:17:21Z

lightning-invoice/src/utils.rs

+
+	#[test]
+    #[cfg(feature = "std")]
+    fn create_phantom_invoice_with_description_hash() {


Same here, I suggest that you place this right after the do_test_multi_node_receive test.

luizParreira · 2022-03-16T18:49:40Z

@ViktorTigerstrom @TheBlueMatt I believe now I am properly formatting with tabs :)

TheBlueMatt

Left a few comments on examples of places where I think the formatting needlessly leads to reviewers/readers having no context - the classic issue with rustfmt's poor formatting. Could you take a pass through and remove newlines that don't improve readability (feel free to add blank lines to break up hunks if the code looks too dense).

TheBlueMatt · 2022-03-16T22:16:11Z

lightning-invoice/src/utils.rs

-	amt_msat: Option<u64>, description: String, payment_hash: PaymentHash, payment_secret:
-	PaymentSecret, phantom_route_hints: Vec<PhantomRouteHints>, keys_manager: K, network: Currency
-) -> Result<Invoice, SignOrCreationError<()>> where K::Target: KeysInterface {
+	amt_msat: Option<u64>,


I'm also really not a huge fan of making this layout super vertical just to call a utility method.

TheBlueMatt · 2022-03-16T22:16:54Z

lightning-invoice/src/utils.rs

+			Some(payment_amt), description_hash, payment_hash, payment_secret, route_hints,
+			&nodes[1].keys_manager, Currency::BitcoinTestnet,
+		)
+		.unwrap();


Why did you \n here?

No particular reason. Removed with 24ed8af

TheBlueMatt · 2022-03-16T22:29:53Z

lightning-invoice/src/utils.rs

+/// that the payment secret is valid when the invoice is paid.
+/// Use this variant if you want to pass the `description_hash` to the invoice.
+pub fn create_invoice_from_channelmanager_with_description_hash<
+	Signer: Sign,


Same here - we're now 21 LoC to list 5 arguments, with two or three of these suddenly even large terminals have no context for what's going on..this is a great example of why rustfmt is unreadable. Can you condense these somewhat (I know some of the existing code was this way)

TheBlueMatt · 2022-03-16T22:30:12Z

lightning-invoice/src/utils.rs

+			&nodes[1].node, nodes[1].keys_manager, Currency::BitcoinTestnet, Some(10_000),
+			description_hash, Duration::from_secs(1234567),
+		)
+		.unwrap();


same here - why \n?

No particular reason. Removed with 24ed8af

TheBlueMatt · 2022-03-16T22:30:38Z

lightning-invoice/src/utils.rs

+		.unwrap();
+		assert_eq!(invoice.amount_pico_btc(), Some(100_000));
+		assert_eq!(
+			invoice.min_final_cltv_expiry(),


No need to break this into multiple lines, the line written out isn't very long.

TheBlueMatt · 2022-03-16T22:30:58Z

lightning-invoice/src/utils.rs

+			MIN_FINAL_CLTV_EXPIRY as u64
+		);
+		assert_eq!(
+			invoice.description(),


I don't see a reason why this needs to be on a new line.

luizParreira · 2022-03-18T13:29:21Z

@TheBlueMatt Fixed your code-style comments. Anything else?

ViktorTigerstrom

Starting to look really good :)! Just left a few comments of further cleanup that can be made, and also improvements to the code formatting that I think you should implement to make the code more consistent with the rest of the project, even though lines will exceed 100 chars.

ViktorTigerstrom · 2022-03-18T17:25:31Z

lightning-invoice/src/utils.rs

+pub fn create_invoice_from_channelmanager_with_description_hash<Signer: Sign, M: Deref, T: Deref,
+	K: Deref, F: Deref, L: Deref,
+>(


I think you should make this into a single line, even though it's over 100 chars to make it more consistent with the rest of the project.

ViktorTigerstrom · 2022-03-18T17:25:53Z

lightning-invoice/src/utils.rs

+pub fn create_invoice_from_channelmanager_with_description_hash_and_duration_since_epoch<
+	Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref,
+>(


Same here, make this into a single line.

ViktorTigerstrom · 2022-03-18T17:26:28Z

lightning-invoice/src/utils.rs

+		let invoice = ::utils::create_phantom_invoice_with_description_hash::<
+			EnforcingSigner,
+			&test_utils::TestKeysInterface,
+		>(


Make this into a single line.

ViktorTigerstrom · 2022-03-18T17:27:41Z

lightning-invoice/src/utils.rs

+		let chan_0_1 = create_announced_chan_between_nodes_with_value(
+			&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known(),
+		);
+		nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &chan_0_1.1);
+		nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &chan_0_1.0);
+		let chan_0_2 = create_announced_chan_between_nodes_with_value(
+			&nodes, 0, 2, 100000, 10001, InitFeatures::known(), InitFeatures::known(),
+		);
+		nodes[0].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan_0_2.1);
+		nodes[2].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &chan_0_2.0);


Sorry for not picking up on this earlier, but there's actually no need for you to create these channels in order to create the invoice for this test. So you can just remove them to make the test more focused :). Also while you're at it, you might as well remove the extra unused node created in test_create_invoice_with_description_hash and create_phantom_invoice_with_description_hash to avoid any confusion.

luizParreira · 2022-03-18T18:01:42Z

@ViktorTigerstrom Fixed your points on 7714393

TheBlueMatt

LGTM! Thanks for tacking some of the style stuff, two trivial nits, but no need to rush to fix them unless there's other stuff to fix or you feel like it.

lightning-invoice/src/utils.rs

ViktorTigerstrom · 2022-03-18T18:13:20Z

@ViktorTigerstrom Fixed your points on 7714393

Great!

jkczyz · 2022-03-18T18:41:48Z

Rather than have a handful of functions with long names and parameter lists, should we consider having one function that takes an InvoiceBuilder?

ViktorTigerstrom · 2022-03-18T18:55:10Z

Rather than have a handful of functions with long names and parameter lists, should we consider having one function that takes an InvoiceBuilder?

If that's something we'd like to implement, I could work on that in a follow up if you'd like, to not bloat this PR.

TheBlueMatt · 2022-03-18T18:56:33Z

Yea, passing an InvoiceBuilder instead of a function would be nice, but its kinda an awkward API to communicate to the user exactly what is or is not required to be set before calling the function. More importantly, the InvoiceBuidler API isn't something we can describe in the bindings, so its currently not exported :(.

jkczyz · 2022-03-18T19:15:07Z

Hmmm... yeah, bindings would be a problem. Would be nice to at least have the phantom and non-phantom versions take an InvoiceDescription enum instead of having a version for each variant, but looks like we don't export that either. 🙁

jkczyz · 2022-03-18T20:06:38Z

lightning-invoice/src/utils.rs

+}
+
+#[cfg(feature = "std")]
+/// Utility to create an invoice that can be paid to one of multiple nodes, or a "phantom invoice."


Perhaps better to reference the other docs rather than repeating them here. But could be left to a follow-up.

TheBlueMatt · 2022-03-18T20:28:46Z

Would be nice to at least have the phantom and non-phantom versions take an InvoiceDescription enum instead of having a version for each variant, but looks like we don't export that either.

Yea, there's probably an easy-ish way to do something like that, the issue with it currently is its really an enum-holding-references type, which we can't map, but we can create a second enum that isn't holding a reference which would let us do this.

TheBlueMatt · 2022-03-18T20:29:19Z

Figure its worth landing as-is, we can condense the functions as a follow-up, or not, its not like its bad in its current state.

luizParreira force-pushed the rust-lightning/issues/1280 branch 3 times, most recently from 55e696f to 813a965 Compare March 14, 2022 10:22

ViktorTigerstrom reviewed Mar 14, 2022

View reviewed changes

luizParreira force-pushed the rust-lightning/issues/1280 branch 3 times, most recently from 7789ea7 to 66a33fa Compare March 14, 2022 16:24

luizParreira force-pushed the rust-lightning/issues/1280 branch from 66a33fa to 8c27b47 Compare March 14, 2022 16:28

TheBlueMatt reviewed Mar 16, 2022

View reviewed changes

luizParreira force-pushed the rust-lightning/issues/1280 branch from 8c27b47 to 7cef6b6 Compare March 16, 2022 17:04

luizParreira force-pushed the rust-lightning/issues/1280 branch from 7cef6b6 to e618f7b Compare March 16, 2022 17:39

TheBlueMatt reviewed Mar 16, 2022

View reviewed changes

luizParreira force-pushed the rust-lightning/issues/1280 branch from e618f7b to 411d827 Compare March 16, 2022 17:50

ViktorTigerstrom reviewed Mar 16, 2022

View reviewed changes

luizParreira force-pushed the rust-lightning/issues/1280 branch 2 times, most recently from 5c7c5b4 to d1f4536 Compare March 16, 2022 18:48

luizParreira force-pushed the rust-lightning/issues/1280 branch from d1f4536 to 2ce3832 Compare March 16, 2022 19:33

TheBlueMatt reviewed Mar 16, 2022

View reviewed changes

luizParreira force-pushed the rust-lightning/issues/1280 branch from 2ce3832 to 5852bbe Compare March 18, 2022 13:24

luizParreira force-pushed the rust-lightning/issues/1280 branch from 5852bbe to 24ed8af Compare March 18, 2022 13:34

ViktorTigerstrom reviewed Mar 18, 2022

View reviewed changes

luizParreira force-pushed the rust-lightning/issues/1280 branch 2 times, most recently from 439c6ed to 925e296 Compare March 18, 2022 18:01

TheBlueMatt previously approved these changes Mar 18, 2022

View reviewed changes

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Show resolved Hide resolved

Create normal/phantom invoice with description hash

7714393

luizParreira dismissed TheBlueMatt’s stale review via 7714393 March 18, 2022 18:08

luizParreira force-pushed the rust-lightning/issues/1280 branch from 925e296 to 7714393 Compare March 18, 2022 18:08

jkczyz approved these changes Mar 18, 2022

View reviewed changes

TheBlueMatt approved these changes Mar 18, 2022

View reviewed changes

TheBlueMatt merged commit 5ed2985 into lightningdevkit:main Mar 18, 2022

Create normal/phantom invoice with description hash #1361

Create normal/phantom invoice with description hash #1361

Conversation

luizParreira commented Mar 13, 2022 • edited Loading

luizParreira commented Mar 13, 2022

codecov-commenter commented Mar 13, 2022 • edited Loading

Codecov Report

ViktorTigerstrom left a comment • edited Loading

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

luizParreira commented Mar 14, 2022

ViktorTigerstrom commented Mar 14, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luizParreira commented Mar 16, 2022

TheBlueMatt commented Mar 16, 2022

luizParreira commented Mar 16, 2022

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

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

ViktorTigerstrom Mar 16, 2022 • edited Loading

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

luizParreira commented Mar 16, 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

luizParreira commented Mar 18, 2022

ViktorTigerstrom left a comment • edited Loading

Choose a reason for hiding this comment

ViktorTigerstrom Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

luizParreira commented Mar 18, 2022 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Mar 18, 2022

jkczyz commented Mar 18, 2022

ViktorTigerstrom commented Mar 18, 2022 • edited Loading

TheBlueMatt commented Mar 18, 2022

jkczyz commented Mar 18, 2022

Choose a reason for hiding this comment

TheBlueMatt commented Mar 18, 2022

TheBlueMatt commented Mar 18, 2022

luizParreira commented Mar 13, 2022 •

edited

Loading

codecov-commenter commented Mar 13, 2022 •

edited

Loading

ViktorTigerstrom left a comment •

edited

Loading

ViktorTigerstrom Mar 16, 2022 •

edited

Loading

ViktorTigerstrom left a comment •

edited

Loading

ViktorTigerstrom Mar 18, 2022 •

edited

Loading

ViktorTigerstrom Mar 18, 2022 •

edited

Loading

luizParreira commented Mar 18, 2022 •

edited

Loading

ViktorTigerstrom commented Mar 18, 2022 •

edited

Loading