Reduce attribution data hold time resolution to 100 ms

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.

[joostjager]

Implementing latest iteration of the spec lightning/bolts#1044

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[TheBlueMatt]

LGTM, but need to update the message in onion_utils.rs: "Htlc hold time at pos {}: {} ms".

[tnull]

LGTM, mod one nit and pending comments.

[joostjager]

I did a bit of rustfmt in the first commit. Unfortunately onion_route_tests don't have the fn level skips.

[codecov]

Codecov Report

Attention: Patch coverage is 93.11594% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (b5bfa85) to head (1fc1a14).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.82%	7 Missing and 3 partials ⚠️
lightning/src/ln/outbound_payment.rs	94.28%	6 Missing ⚠️
lightning/src/events/mod.rs	66.66%	0 Missing and 2 partials ⚠️
lightning/src/ln/msgs.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3868 +/- ##
==========================================
+ Coverage 89.75% 90.60% +0.84% 
==========================================
Files 164 164 Lines 132834 142084 +9250 Branches 132834 142084 +9250 ==========================================
+ Hits 119229 128737 +9508 + Misses 10926 10660 -266 - Partials 2679 2687 +8 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

CI is quite sad

[TheBlueMatt]

There's a few patterns that look worth cleaning up, otherwise LGTM.

[TheBlueMatt]

Wanna pull the sub into a variable? Or maybe the sub / the granularity?

[joostjager]

Done. Renamed to 'unit' / 'units' also.

[TheBlueMatt]

Mind cleaning this up?

[joostjager]

Extracted var. I don't think it was necessary.

[TheBlueMatt]

bleh wth is this formatting, rustfmt. Vertical or not, this would be way more readable split into variables.

[joostjager]

Done. Not my preference to fix this in this context though. It was already bad.

[TheBlueMatt]

If we import build_onion_payloads directly we can hide some of the visual noise here (same applies to a few other tests below):

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index b590b123d4..69b63e331c 100644
--- a/lightning/src/ln/onion_route_tests.rs+++ b/lightning/src/ln/onion_route_tests.rs@@ -538,16 +538,10 @@ fn test_onion_failure() {
&route.paths[0],
&session_priv,
);
- let recipient_onion_fields = RecipientOnionFields::spontaneous_empty();- let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(- &route.paths[0],- 40000,- &recipient_onion_fields,- cur_height,- &None,- None,- None,- )+ let path = &route.paths[0];+ let recipient_fields = RecipientOnionFields::spontaneous_empty();+ let (mut onion_payloads, _htlc_msat, _htlc_cltv) =+ build_onion_payloads(path, 40000, recipient_fields, cur_height, &None, None, None)
.unwrap();
let mut new_payloads = Vec::new();
for payload in onion_payloads.drain(..) {

[joostjager]

Done. Couldn't get them all on one line by extracting vars.

[TheBlueMatt]

Lol, can we make this a few variables

[joostjager]

Got a 'Tried to violate existing lockorder.'

[TheBlueMatt]

Probably need a scope, but actually we have a helper for this that we really should use (in case ChannelManager changes how it stores things so we don't have to update a million places in the tests):

@@ -1087,21 +1074,13 @@ fn test_onion_failure() {
);
let short_channel_id = channels[1].0.contents.short_channel_id;
- let amt_to_forward = nodes[1]- .node- .per_peer_state- .read()- .unwrap()- .get(&nodes[2].node.get_our_node_id())- .unwrap()- .lock()- .unwrap()- .channel_by_id- .get(&channels[1].2)- .unwrap()- .context()- .get_counterparty_htlc_minimum_msat()- - 1;+ let amt_to_forward = {+ let (per_peer_state, peer_state);+ let node_c_id = nodes[2].node.get_our_node_id();+ let chan = get_channel_ref!(nodes[1], node_c_id, per_peer_state, peer_state, channels[1].2);+ chan.context().get_counterparty_htlc_minimum_msat() - 1+ };+
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].hops.len();
bogus_route.paths[0].hops[route_len - 1].fee_msat = amt_to_forward;

[joostjager]

Ah I now remember something similar with a test, a few months ago. That these vars are released in the order in which they are declared, not the reverse. I think it was something like that?

[joostjager]

Fixed. Code needed to be slightly different.

[TheBlueMatt]

Worth pulling the hex out here and in the next few diff hunks?

[joostjager]

Pulled out some constants. Don't think I like it very much.

[joostjager]

It once again became clear to me that preferences differ for code formatting. I think we should go ahead with fn-level skips also for the test files, and format only on demand.

[tnull]

Changes look mostly good mod some formatting nits.

+1886/-684 change suddenly. Funny how that goes.

[tnull]

nit: Could leave a comment (and/or appropriately rename the variable) to convey why the hold time has to be multiplied here.

[joostjager]

Updated to use the HOLD_TIME_UNIT_MILLIS constant.

[tnull]

Who are these people?

[joostjager]

The names of the nodes. Open to suggestions for alternative naming.

[TheBlueMatt]

Oh lol we can drop the second clause - if two unsigned ints sum to one, one of them has to be equal to one :)

[joostjager]

Fix pushed. Advantage of var extraction.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

LGTM

[joostjager]

@TheBlueMatt already acked all of this PR except that last trivial commit. Going to merge this.