Skip to content

Use a real (probing-generated) scorer in benchmarks #3103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

TheBlueMatt
Copy link
Collaborator

Until now, our routing benchmarks used a synthetic scorer, generated by scoring random paths to build up some history. This is pretty far removed from real-world routing conditions, as alternative paths generally have no scoring information and even the paths we do take have only one or two past scoring results.

Instead, we fetch a static serialized scorer, generated using minutely probes. This means future changes to the scorer's data may be harder to benchmark, but makes for substantially more realistic benchmarks for changes which don't impact the serialized state.

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 93.04348% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.19%. Comparing base (2701bc5) to head (7ae311d).
Report is 66 commits behind head on main.

Files Patch % Lines
lightning/src/routing/router.rs 92.92% 5 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3103      +/-   ##
==========================================
+ Coverage   89.88%   91.19%   +1.31%     
==========================================
  Files         119      119              
  Lines       97551   106869    +9318     
  Branches    97551   106869    +9318     
==========================================
+ Hits        87681    97459    +9778     
+ Misses       7304     6921     -383     
+ Partials     2566     2489      -77     

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

tnull
tnull previously approved these changes Jun 12, 2024
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Would be fine to land as-is from my side, still a few questions:

tnull
tnull previously approved these changes Jun 12, 2024
@TheBlueMatt
Copy link
Collaborator Author

Fixed the printed path:

$ git diff-tree -U1 cbe24bcb 3a462ef0
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index ca1281ec6..b904b7777 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -8494,3 +8494,3 @@ pub(crate) mod bench_utils {
 			"scorer-2023-12-10.bin",
-			"Please fetch https://bitcoin.ninja/ldk-scorer-v0.0.118-2023-12-10.bin and place it at scorer-2023-12-10.bin"
+			"Please fetch https://bitcoin.ninja/ldk-scorer-v0.0.118-2023-12-10.bin and place it at lightning/scorer-2023-12-10.bin"
 		);

Until now, our routing benchmarks used a synthetic scorer,
generated by scoring random paths to build up some history. This is
pretty far removed from real-world routing conditions, as
alternative paths generally have no scoring information and even
the paths we do take have only one or two past scoring results.

Instead, we fetch a static serialized scorer, generated using
minutely probes. This means future changes to the scorer's data may
be harder to benchmark, but makes for substantially more realistic
benchmarks for changes which don't impact the serialized state.
This ensures the route benchmarks themselves will appear with a
distinct callgraph, making router profiling somewhat easier.
In our route tests we need some "random" bytes for the router to
randomize amounts using. We generate this by building an actual
`KeysManager` and then deriving some random bytes using the
`EntropySource` trait. However, `get_route` (what we're normally
testing) doesn't actually use the random bytes, and even if it did,
using a `KeysManager` is just a fancy way of creating a constant,
so there's really no reason to do all the fancy crypto.

Instead, here, we change our routing tests and benchmarks to simply
use `[42; 32]` as the "random" bytes.
@TheBlueMatt
Copy link
Collaborator Author

Dropped further redundant code:

$ git diff-tree -U10 3a462ef0 7ae311d6
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index b904b7777..f464c096e 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -8584,31 +8584,20 @@ pub(crate) mod bench_utils {
 				let path_exists =
 					get_route(&payer, &route_params, &graph.read_only(), Some(&[&first_hop]),
 						&TestLogger::new(), scorer, score_params, &random_seed_bytes).is_ok();
 				if path_exists {
 					route_endpoints.push((first_hop, params, amt_msat));
 					break;
 				}
 			}
 		}

-		// Because we've changed channel scores, it's possible we'll take different routes to the
-		// selected destinations, possibly causing us to fail because, eg, the newly-selected path
-		// requires a too-high CLTV delta.
-		route_endpoints.retain(|(first_hop, params, amt_msat)| {
-			let route_params = RouteParameters::from_payment_params_and_value(
-				params.clone(), *amt_msat);
-			get_route(&payer, &route_params, &graph.read_only(), Some(&[first_hop]),
-				&TestLogger::new(), scorer, score_params, &random_seed_bytes).is_ok()
-		});
-		route_endpoints.truncate(route_count);
-		assert_eq!(route_endpoints.len(), route_count);
 		route_endpoints
 	}
 }

 #[cfg(ldk_bench)]
 pub mod benches {
 	use super::*;
 	use crate::routing::scoring::{ScoreUpdate, ScoreLookUp};
 	use crate::ln::channelmanager;
 	use crate::ln::features::Bolt11InvoiceFeatures;

@TheBlueMatt TheBlueMatt merged commit 08c566c into lightningdevkit:main Jun 12, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants