Misc (mostly-)feature cleanups by TheBlueMatt · Pull Request #3250 · lightningdevkit/rust-lightning

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt · 2024-08-17T22:23:46Z

Built on #3249, random stuff I came across while writing that.

codecov · 2024-08-17T22:32:09Z

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (bbfa15e) to head (11ab302).
Report is 8 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/functional_tests.rs	50.00%	2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs	94.73%	1 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3250 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01% 
==========================================
Files 125 125 Lines 102867 102798 -69 Branches 102867 102798 -69 ==========================================
- Hits 92398 92327 -71 - Misses 7753 7756 +3 + Partials 2716 2715 -1

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

arik-so · 2024-08-19T23:41:32Z

Needs a rebase.

In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

To handle `std` and `no-std` concepts of time in scoring, we'd originally written a generic `Time` trait which we could use to fetch the current time, observe real (not wall-clock) elapsed time, and serialize the time. Eventually, scoring stopped using this `Time` trait but outbound payment retry expiry started using it instead to mock time in tests. Since scoring no longer uses the full features which required the `Time` trait, we can substantially simplify by just having the mocking option.

Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

We never actually build with `#![no_std]` in tests as Rust does not support it. Thus, many tests have spurious `std` feature gates when they run just fine without them. Here we remove those gates, though note that tests that depend on behavior elsewhere in the codebase which is `std`-gated obviously need to retain their feature gates.

TheBlueMatt · 2024-08-21T14:11:15Z

Rebased.

valentinewallace

Love to see it

valentinewallace · 2024-08-21T15:17:03Z

-	#[cfg(feature = "std")]
 	/// Returns true when a full routing table sync should be performed with a peer.
-	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
- //TODO: Determine whether to request a full sync based on the network map.


Did you mean to remove this todo?

Yea, I don't think we will/can ever make such a decision - instead we need the spec to include a set reconciliation protocol.

valentinewallace · 2024-08-21T15:28:25Z

+ #[allow(unused)]
+ let should_sync = self.should_request_full_sync();


nit: I don't see why this got moved and allow(unused)'d?

I removed the std bounds on should_request_full_sync, and in order to make it not-unused, I moved this out of the std block below. But that also means that in no-std builds this value is unused. In theory we should keep the bounds as they were, but I liked the diff stats 😅

ah, I had a similar issue once. Hate this aspect of conditional compilation

arik-so

Looks good, with the same questions that @valentinewallace raised.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 468d088 to d27e098Compare August 17, 2024 22:25

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from d27e098 to 5b615feCompare August 18, 2024 15:35

TheBlueMatt added 5 commits August 21, 2024 14:11

Remove note about std/no-std scorer serialization compat
1c83e61
In 81389de we removed a note about mixing the `std` and `no-std` feature when de/serializing `ProbabilisticScorer`s but forgot to note that there was a second copy of that note in the module documentation. This removes that note.

Drop std::Error impl for PeerHandleError
75a2089
Not sure why we ever really had this, no one really ever bounds anything on `std::Error` and its kinda a dead type, so there's no need for us to `impl` it for our types.

Reduce feature = std flags in gossip.rs somewhat
5412784
Fewer feature flags makes for more readable code, so we opt for that over very marginally more effecient code here.

TheBlueMatt force-pushed the 2024-08-feature-cleanup branch from 5b615fe to 11ab302Compare August 21, 2024 14:11

valentinewallace approved these changes Aug 21, 2024
View reviewed changes

arik-so reviewed Aug 21, 2024
View reviewed changes

arik-so approved these changes Aug 21, 2024
View reviewed changes

TheBlueMatt merged commit 3111508 into lightningdevkit:mainAug 21, 2024

		#[allow(unused)]
		let should_sync = self.should_request_full_sync();

Uh oh!

Conversation

TheBlueMatt commented Aug 17, 2024

Uh oh!

codecovBot commented Aug 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arik-so commented Aug 19, 2024

Uh oh!

TheBlueMatt commented Aug 21, 2024

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Aug 17, 2024

Uh oh!

codecovBot commented Aug 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arik-so commented Aug 19, 2024

Uh oh!

TheBlueMatt commented Aug 21, 2024

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Aug 17, 2024

Uh oh!

codecovBot commented Aug 17, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecovBot commented Aug 17, 2024•
edited
Loading

codecovBot commented Aug 17, 2024•
edited
Loading

codecovBot commented Aug 17, 2024•
edited
Loading

codecovBot commented Aug 17, 2024•
edited
Loading