Set return type to Iterator for functions in file: `lightning-invoice/src/utils.rs` : issue #2240 #2290

upjohnc · 2023-05-11T19:07:59Z

GH Issue 2290 intent is to use iterators in the place of building up vectors. For example, select_phantom_hints and sort_and_filter_channels return vectors. This pr refactors them to return iterators.

upjohnc · 2023-05-11T19:08:17Z

I assumed this was acceptable. It was faster than figuring out tests for the select_phantom_hints function
In PR #2044, the function select_phantom_hints returns a vector. This PR changes the return type
to an Iterator rather than building up a vector.

note that the hints were limited to 3 so the zip_vectors has a limit
I set zip_vectors as its own function, so that I could add tests to around it.
I assumed this was acceptable. It was faster than figuring out tests for the select_phantom_hints function

TheBlueMatt

There are a handful of other places in this file where we allocate vecs and could swap them for iterators.

Note that if we have to collect() and create another intermediate vec there's no reason to move to iterators. It's only where we don't need to that it's useful.

lightning-invoice/src/utils.rs

jkczyz · 2023-05-11T20:06:28Z

lightning-invoice/src/utils.rs

+    const TEST_RESULT_SIZE: usize = 3;
+
+    #[test]
+    fn test_zip_vecs_one() {


Why add more tests? Do the current set of tests not cover the previously implemented behavior?

I am not sure which tests you are referencing.

I added multiple tests for the different cases of inputs with expected results. If there is a way to parameterize a single test function by defining a set of inputs with their paired expected output, I can change the test to that.

If you are referencing an existing set of tests that I overlook, let me know.

If you are referencing an existing set of tests that I overlook, let me know.

Yeah, I'm referring to the existing set of tests. Presumably, they would be testing the previous logic so should be sufficient to test the refactored logic. Just wondering if this is actually the case or if instead you thought the previous testing was insufficient.

After I worked through where the select_phantom_hints was being called, I realized what I missed about your question. You were asking what about the existing tests and I misunderstood it as "existing unit tests" rather than the "existing integration tests".

I think there is value in having unit tests around this function. My reasoning is that it tests the different inputs without adding those different cases to the larger integration test. This does two things, in my opinion, allow for devs to make improvements to the function with the unit tests as an assurance of correctness and also allow devs to see what the expected functionality is through example (tests being the example of input -> output).

If there is a convention in this project that I am new to and I should remove these unit tests, I can take them out.

codecov-commenter · 2023-05-13T15:47:48Z

Codecov Report

Patch coverage: 99.03% and project coverage change: -0.06 ⚠️

Comparison is base (64c58a5) 90.79% compared to head (3e6b9ae) 90.73%.

❗ Current head 3e6b9ae differs from pull request most recent head 8f00ba5. Consider uploading reports for the commit 8f00ba5 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2290      +/-   ##
==========================================
- Coverage   90.79%   90.73%   -0.06%     
==========================================
  Files         104      104              
  Lines       53033    53256     +223     
  Branches    53033    53256     +223     
==========================================
+ Hits        48151    48324     +173     
- Misses       4882     4932      +50

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`97.29% <99.03%> (+0.11%)`	⬆️

... and 9 files with indirect coverage changes

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

jkczyz · 2023-05-16T18:42:06Z

lightning-invoice/src/utils.rs

+// Similar to the zip method on a vector, it combines the vectors by index - zero index comes first
+// then second, etc.


zip returns 2-tuples whereas this iterator returns single elements. I don't think we should use "zip" in the name, as that implies tupling elements from each input.

Do you have a suggestion for what to name it?

My reasoning for having zip in the name is because it does zip multiple arrays together by their index. I couldn't think of another name for that. Like in Python, it is also called zip when combine two iterables by their index.

Do you have a suggestion for what to name it?

How about rotate?

My reasoning for having zip in the name is because it does zip multiple arrays together by their index. I couldn't think of another name for that. Like in Python, it is also called zip when combine two iterables by their index.

zip has the same behavior in both Python and Rust: they both yield tuples and terminate when any of the inputs are exhausted. However, here, the iterator yields single elements, rotating between each input, and continues to yield elements when any of the inputs are exhausted.

lightning-invoice/src/utils.rs

upjohnc · 2023-05-24T12:46:45Z

There are a handful of other places in this file where we allocate vecs and could swap them for iterators.

@TheBlueMatt I went through the code to find other places in the file lightning-invoice/src/utils.rs, I did not find any more - other than the ones refactored in this pr. If I am overlooking some or did not search correctly, let me know.

I did change the iterators as you had suggested. Thank you for that. I will be ruminating on those for a while.

I am going to change the pr from draft to "ready for review" as it seems that the pr completes the intent of the gh issue.

lightning-invoice/src/utils.rs

TheBlueMatt

One comment, but otherwise looks good. Feel free to squash the commits down to individual standalone commit(s) without bugfixes for previous commits in later ones.

TheBlueMatt · 2023-05-26T20:12:53Z

lightning-invoice/src/utils.rs

+			// exhausted_vectors increase when the "next_idx" vector is exhausted
+			exhausted_iterators += 1;
+			// return None when all of the nested vectors are exhausted
+			if exhausted_iterators == vecs.len() {


Does this work if, for example, there's one iterator with zero elements and another iterator with 5? On each loop iteration/function call we check the first iterator, see its empty, increment exhausted_iterators, and then return Some for the next iterator. Once we've done that once we'll return None, even though we have more elements from another iterator.

Once we yield Some, the next call will reset exhausted_iterators to zero since it is a local variable, IIUC.

Oops indeed, you're right.

I'll add to the comment to help with the explanation of how the loop function. Each of us has needed to think through it, so maybe a comment with help with future reading of the code.

TheBlueMatt

Please don't have two-line-title commit messages - commit messages should (a) consist entirely of lines no longer than 70 chars (which I think you did, thanks!) and (b) have a single up-to-70-char title line first, followed by an empty line, then more details.

TheBlueMatt · 2023-05-29T19:56:42Z

lightning-invoice/src/utils.rs

+// then second, etc.
+// The difference is that this function handles many or just one vector, whereas the builtin method
+// works on just two vectors.
+fn zip_nested_vectors<T: Clone>(vecs: Vec<Vec<T>>, result_size: usize) -> impl Iterator<Item = T> {


Mmm, I'm not a fan of introducing a function in one commit and then completely rewriting it later in the same PR. Instead, you might consider splitting the second commit into adding the rotate_nested_vectors function, making that a standalone first commit, and then going from there.

TheBlueMatt · 2023-05-29T19:57:32Z

lightning-invoice/src/utils.rs

-            return hint_value;
-        };
-    })
+	let max_vector_length: usize = vecs.iter().map(|x| x.len()).max().unwrap();


You introduce this code in the second commit, and then convert spaces to tabs in the third. The space -> tabs should go in the commit where the code is added :)

I reworked the commits to be two. They should be more clear of two separate refactors to the phantom hints and then to the sort_and_filter_channels.

Hmm, you're still introducing a bunch of code in the first commit to implement rotate_nested_vectors and then ripping it all out and replacing it wholesale in the second commit. Instead, maybe consider having a first commit that adds rotate_through_iterators and the test for it, without using it, then have the next commit do the rest?

Ok. I have changed the commits to be one for the rotate_through_iterators and the other for the rest of the refactor.

The mental hurdle for me in rebasing the commits was that there were changes to one function that meant a refactor of another - a domino effect of changing previous changes.

Let me know if these commits fit better with the project patterns.

Yea, that looks totally great to me. One issue, you're missing the function-closing } in the first commit, which CI is barfing on cause the first commit doesn't compile at all.

lightning-invoice/src/utils.rs

jkczyz · 2023-05-31T15:14:33Z

lightning-invoice/src/utils.rs

@@ -630,7 +641,7 @@ fn sort_and_filter_channels<L: Deref>(
 				// look at the public channels instead.
 				log_trace!(logger, "Not including channels in invoice route hints on account of public channel {}",
 					log_bytes!(channel.channel_id));
-				return vec![]
+				return vec![].into_iter().take(3).map(route_hint_from_channel);


Do we need the take(3) here and below? We already do this within _create_phantom_invoice and if removed here should also do it in _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash with a common constant. Otherwise, it's hidden that we are taking only 3.

From what I understand we need the take(3) in sort_and_filter_channels. We could move out to the file scope const MAX_HINTS: usize = 3; for reuse in the functions that build up to _create_phantom_invoice. We could rename it to MAX_CHANNEL_HINTS for clarity.

My understanding of the need for take(3):
The reason for the take(3) in sort_and_filter_channels is to have a ExactSizeIterator as the return type and the reason for that is the check on the returned length in select_phantom_hints.

rust-lightning/lightning-invoice/src/utils.rs

Line 250 in 3e6b9ae

let empty_route_hints = route_hints.len() == 0;

Ah, I see. Yeah, could you make a file-level constant for all uses?

- adding function to allow for select_phantom_hints to yield an iterator

jkczyz

LGTM. Please squash the last two commits into one commit.

- two functions refatored: `select_phantom_hints`, `sort_and_filter_channels`

TheBlueMatt reviewed May 11, 2023

View reviewed changes

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

jkczyz reviewed May 11, 2023

View reviewed changes

upjohnc marked this pull request as draft May 11, 2023 20:07

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch 2 times, most recently from 95f3a54 to 245bd93 Compare May 13, 2023 14:26

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 245bd93 to d099e66 Compare May 16, 2023 15:43

jkczyz reviewed May 16, 2023

View reviewed changes

TheBlueMatt reviewed May 21, 2023

View reviewed changes

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

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

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

upjohnc changed the title ~~Set return type to Iterator for select_phantom_hints : issue #2240~~ Set return type to Iterator for functions in file: lightning-invoice/src/utils.rs : issue #2240 May 24, 2023

upjohnc marked this pull request as ready for review May 24, 2023 12:46

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 4635ee0 to d4d8b81 Compare May 24, 2023 15:20

jkczyz reviewed May 24, 2023

View reviewed changes

jkczyz reviewed May 25, 2023

View reviewed changes

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 6e0fc89 to 42c0f2c Compare May 25, 2023 11:25

TheBlueMatt reviewed May 26, 2023

View reviewed changes

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch 2 times, most recently from 0923472 to 883d255 Compare May 29, 2023 14:39

upjohnc changed the base branch from main to 0.0.103-bindings May 29, 2023 14:51

upjohnc changed the base branch from 0.0.103-bindings to main May 29, 2023 14:51

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 22a271f to 8c79680 Compare May 29, 2023 17:17

TheBlueMatt reviewed May 29, 2023

View reviewed changes

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch 6 times, most recently from ee109e6 to 21e146a Compare May 30, 2023 22:59

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 21e146a to 9151962 Compare May 30, 2023 23:00

TheBlueMatt previously approved these changes May 31, 2023

View reviewed changes

jkczyz reviewed May 31, 2023

View reviewed changes

upjohnc dismissed TheBlueMatt’s stale review via 5e7e01b May 31, 2023 16:38

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 9151962 to 5e7e01b Compare May 31, 2023 16:38

Adding rotate_through_iterators for select_phantom_hints refactor

31807bc

- adding function to allow for select_phantom_hints to yield an iterator

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 5e7e01b to 3e6b9ae Compare May 31, 2023 16:46

jkczyz reviewed Jun 1, 2023

View reviewed changes

Refactor lightning-invoice/src/utils.rs to yield iterators

ec67ee7

- two functions refatored: `select_phantom_hints`, `sort_and_filter_channels`

upjohnc force-pushed the 2240_replace_vectors_with_iterators branch from 8f00ba5 to ec67ee7 Compare June 1, 2023 19:46

upjohnc mentioned this pull request Jun 4, 2023

Replace intermediate Vecs with iterators #2240

Closed

TheBlueMatt linked an issue Jun 4, 2023 that may be closed by this pull request

Replace intermediate Vecs with iterators #2240

Closed

TheBlueMatt approved these changes Jun 5, 2023

View reviewed changes

jkczyz approved these changes Jun 5, 2023

View reviewed changes

jkczyz merged commit 486c16a into lightningdevkit:main Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set return type to Iterator for functions in file: `lightning-invoice/src/utils.rs` : issue #2240 #2290

Set return type to Iterator for functions in file: `lightning-invoice/src/utils.rs` : issue #2240 #2290

upjohnc commented May 11, 2023 •

edited

Loading

upjohnc commented May 11, 2023

TheBlueMatt left a comment

jkczyz May 11, 2023

upjohnc May 11, 2023

jkczyz May 11, 2023

upjohnc May 14, 2023

codecov-commenter commented May 13, 2023 •

edited

Loading

jkczyz May 16, 2023

upjohnc May 16, 2023

jkczyz May 17, 2023

upjohnc commented May 24, 2023

TheBlueMatt left a comment

TheBlueMatt May 26, 2023

jkczyz May 26, 2023

TheBlueMatt May 27, 2023

upjohnc May 28, 2023

TheBlueMatt left a comment

TheBlueMatt May 29, 2023

TheBlueMatt May 29, 2023

upjohnc May 30, 2023

TheBlueMatt May 30, 2023

upjohnc May 30, 2023

TheBlueMatt May 30, 2023

jkczyz May 31, 2023

upjohnc Jun 1, 2023

jkczyz Jun 1, 2023

jkczyz left a comment

		// Similar to the zip method on a vector, it combines the vectors by index - zero index comes first
		// then second, etc.

Set return type to Iterator for functions in file: lightning-invoice/src/utils.rs : issue #2240 #2290

Set return type to Iterator for functions in file: lightning-invoice/src/utils.rs : issue #2240 #2290

Conversation

upjohnc commented May 11, 2023 • edited Loading

upjohnc commented May 11, 2023

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

codecov-commenter commented May 13, 2023 • 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

upjohnc commented May 24, 2023

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

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

jkczyz left a comment

Choose a reason for hiding this comment

Set return type to Iterator for functions in file: `lightning-invoice/src/utils.rs` : issue #2240 #2290

Set return type to Iterator for functions in file: `lightning-invoice/src/utils.rs` : issue #2240 #2290

upjohnc commented May 11, 2023 •

edited

Loading

codecov-commenter commented May 13, 2023 •

edited

Loading