Replace PublicKey with [u8; 33] in NetworkGraph #1107

dunxen · 2021-10-05T06:23:29Z

Closes #960

dunxen · 2021-10-05T06:41:12Z

Going to take this out of draft once I've fixed the linting and fuzzing.

TheBlueMatt

This is awesome, excited to see benchmark results once bench compiles :)

lightning-invoice/src/de.rs

lightning/src/routing/network_graph.rs

TheBlueMatt

Nice, current bench shows a pretty substantial gain in read_network_graph (1.7 seconds to 1.1 seconds on GH Actions) but a very substantial regression in route performance (89ms to 500ms on GH Actions), I believe due to calls to serialize or from_slice in the tight inner loop, see comment.

lightning/src/routing/router.rs

dunxen · 2021-10-06T08:25:42Z

I believe due to calls to serialize or from_slice in the tight inner loop, see comment.

I'm also going to minimise serialize() in the rest of get_route() as much as possible.

dunxen · 2021-10-06T09:12:24Z

Seems to fix for earlier Rust versions not supporting const generics, I'll need to use another way to compare NodeIds without introducing more allocations or other overhead.

Edit: Sort of wrong assumption here. It works on 1.47. Rust versions 1.51 and later just use const generics. Would have worked if these were Schnorr pubkeys lol. I'll provide the impl for [T; 33], then I'll promote the PR from draft to ready.

Edit 2: Darn, doesn't seem like I can do this until rust-lang/rust#31844 anyway. And I'm not sure if we'd manage to do it via conditional compilation.

dunxen · 2021-10-06T09:32:57Z

Just comparing here

#1079

test ln::channelmanager::bench::bench_sends               ... bench:   7,765,229 ns/iter (+/- 1,232,859)
test routing::network_graph::benches::read_network_graph  ... bench: 2,303,387,362 ns/iter (+/- 80,066,866)
test routing::network_graph::benches::write_network_graph ... bench: 152,065,789 ns/iter (+/- 7,420,318)
test routing::router::benches::generate_mpp_routes        ... bench: 104,163,061 ns/iter (+/- 64,813,214)
test routing::router::benches::generate_routes            ... bench:  97,599,490 ns/iter (+/- 69,822,412)

vs c36d6e7

test ln::channelmanager::bench::bench_sends               ... bench:   7,565,717 ns/iter (+/- 1,752,280)
test routing::network_graph::benches::read_network_graph  ... bench: 1,407,918,285 ns/iter (+/- 124,747,638)
test routing::network_graph::benches::write_network_graph ... bench: 134,930,617 ns/iter (+/- 10,801,383)
test routing::router::benches::generate_mpp_routes        ... bench:  42,800,183 ns/iter (+/- 28,587,831)
test routing::router::benches::generate_routes            ... bench:  42,839,538 ns/iter (+/- 33,412,245)

The write performance is not a huge improvement (expected this probably) but read and routes are nice.

TheBlueMatt · 2021-10-06T18:25:29Z

Seems to fix for earlier Rust versions not supporting const generics, I'll need to use another way to compare NodeIds without introducing more allocations or other overhead.

I think the way to do this is to replace NodeId type alias with pub struct NodeId([u8; 33]) and then you'll be able to impl core::cmp::PartialOrd for NodeId directly.

TheBlueMatt · 2021-10-06T18:28:15Z

The rest of the patch looks good, I think.

lightning/src/routing/network_graph.rs

codecov · 2021-10-06T21:28:42Z

Codecov Report

Merging #1107 (e9059e0) into main (6582aae) will decrease coverage by 0.01%.
The diff coverage is 82.75%.

❗ Current head e9059e0 differs from pull request most recent head fce631c. Consider uploading reports for the commit fce631c to get more accurate results

@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
- Coverage   90.67%   90.65%   -0.02%     
==========================================
  Files          66       65       -1     
  Lines       34608    34621      +13     
==========================================
+ Hits        31381    31387       +6     
- Misses       3227     3234       +7

Impacted Files	Coverage Δ
lightning/src/routing/network_graph.rs	`91.22% <74.50%> (-0.32%)`	⬇️
lightning/src/routing/router.rs	`96.04% <89.23%> (+<0.01%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.87% <0.00%> (-0.01%)`	⬇️
lightning/src/ln/mod.rs	`90.00% <0.00%> (ø)`
lightning/src/ln/payment_tests.rs
lightning/src/ln/functional_tests.rs	`97.40% <0.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

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

TheBlueMatt

Basically LGTM, note you'll have to squash for CI to pass.

lightning/src/routing/network_graph.rs

dunxen · 2021-10-06T21:56:04Z

Basically LGTM, note you'll have to squash for CI to pass.

The one time I forget to squash before pushing 🥲

lightning/src/routing/network_graph.rs

Kixunil · 2021-10-08T17:30:48Z

lightning/src/routing/router.rs

@@ -4409,9 +4413,9 @@ mod tests {
 		'load_endpoints: for _ in 0..10 {
 			loop {
 				seed = seed.overflowing_mul(0xdeadbeef).0;
-				let src = nodes.keys().skip(seed % nodes.len()).next().unwrap();
+				let src = &PublicKey::from_slice(nodes.keys().skip(seed % nodes.len()).next().unwrap().as_slice()).unwrap();


How about impl From<NodeId> for PublicKey (and vice-versa)?

Yeah, I did consider that right after I pushed the last change. It'll clean things up quite a bit, I agree 🙂

I don't believe we can implement From for a struct outside our crate. There's a workaround with using a tuple struct wrapping the PublicKey. But also note that the conversion functions consume the struct being converted, so in many cases we'd have to make a copy since we only have a reference, which I think we'd want to avoid.

We also can't implement From<NodeId> for PublicKey because its not infallible, it'd have to be TryFrom, at which point its arguably a similar amount of code to read (and I strongly prefer to see from_slice over try_from, because its much more explicit for the same thing).

@jkczyz since NodeId is in this crate From<T> for NodeId can always be implemented for any T we want. Also since Rust 1.41 From<NodeId> for T is possible. In older versions at least impl Into<T> for NodeId.

Also From and Into can be implemented for references, so copying should be non-issue.

I think we can leave it as is for now then?

Is there going to be a bump in MSRV soon or are we trying to avoid that? I agree things look fine as is right now but would be nice to be able to implement From / TryFrom for external structs for maybe other things in future?

Is there going to be a bump in MSRV soon or are we trying to avoid that?

Unlikely - we generally try to ensure you can compile LDK using commonly-available Rust toolchains. Distros don't usually ship rust updates except when they have to (ie when a new Firefox ESR comes out and they have to bump to meet the Firefox MSRV).

@Kixunil Happy to review any follow-ups if this is possible within our MSRV constraints.

I don't care much, just note that Debian oldstable contains Rust 1.41, which is sufficient to solve the From issue. Debian Bullseye has 1.48 which can even support modern Tokio. Debian stable is probably the most conservative distro that's actually widely used, so doesn't seem too bad to me.

dunxen marked this pull request as draft October 5, 2021 06:40

TheBlueMatt reviewed Oct 5, 2021

View reviewed changes

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

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2021-10-swap-pubkey-for-bytearray branch from e60d06f to 01e67a4 Compare October 5, 2021 20:49

TheBlueMatt reviewed Oct 5, 2021

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2021-10-swap-pubkey-for-bytearray branch from 01e67a4 to c36d6e7 Compare October 6, 2021 08:58

dunxen force-pushed the 2021-10-swap-pubkey-for-bytearray branch from c36d6e7 to 2a3c25a Compare October 6, 2021 20:50

TheBlueMatt reviewed Oct 6, 2021

View reviewed changes

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

dunxen marked this pull request as ready for review October 6, 2021 21:16

TheBlueMatt approved these changes Oct 6, 2021

View reviewed changes

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2021-10-swap-pubkey-for-bytearray branch 2 times, most recently from bede58e to 5e204d2 Compare October 7, 2021 05:07

jkczyz reviewed Oct 7, 2021

View reviewed changes

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

lightning/src/routing/network_graph.rs Show resolved Hide resolved

dunxen force-pushed the 2021-10-swap-pubkey-for-bytearray branch from 5e204d2 to 059bf1a Compare October 8, 2021 15:55

Replace PublicKey with [u8; 33] in NetworkGraph

fce631c

dunxen force-pushed the 2021-10-swap-pubkey-for-bytearray branch from 059bf1a to fce631c Compare October 8, 2021 16:38

Kixunil reviewed Oct 8, 2021

View reviewed changes

TheBlueMatt approved these changes Oct 8, 2021

View reviewed changes

jkczyz approved these changes Oct 8, 2021

View reviewed changes

TheBlueMatt merged commit 843d25d into lightningdevkit:main Oct 8, 2021

TheBlueMatt mentioned this pull request Oct 8, 2021

Swap NetworkGraph PublicKeys for [u8; 33] #960

Closed

dunxen deleted the 2021-10-swap-pubkey-for-bytearray branch October 9, 2021 06:14

TheBlueMatt mentioned this pull request Jan 19, 2023

Swap Gossip Message PublicKeys for NodeIds #1973

Closed

jkczyz mentioned this pull request Apr 6, 2024

Compact blinded path handling #2961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace PublicKey with [u8; 33] in NetworkGraph #1107

Replace PublicKey with [u8; 33] in NetworkGraph #1107

dunxen commented Oct 5, 2021

dunxen commented Oct 5, 2021

TheBlueMatt left a comment

TheBlueMatt left a comment

dunxen commented Oct 6, 2021

dunxen commented Oct 6, 2021 •

edited

Loading

dunxen commented Oct 6, 2021 •

edited

Loading

TheBlueMatt commented Oct 6, 2021

TheBlueMatt commented Oct 6, 2021

codecov bot commented Oct 6, 2021 •

edited

Loading

TheBlueMatt left a comment

dunxen commented Oct 6, 2021

Kixunil Oct 8, 2021

dunxen Oct 8, 2021

jkczyz Oct 8, 2021

TheBlueMatt Oct 8, 2021 •

edited

Loading

Kixunil Oct 8, 2021

dunxen Oct 8, 2021

dunxen Oct 8, 2021 •

edited

Loading

TheBlueMatt Oct 8, 2021

jkczyz Oct 8, 2021

Kixunil Oct 8, 2021

Replace PublicKey with [u8; 33] in NetworkGraph #1107

Replace PublicKey with [u8; 33] in NetworkGraph #1107

Conversation

dunxen commented Oct 5, 2021

dunxen commented Oct 5, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

dunxen commented Oct 6, 2021

dunxen commented Oct 6, 2021 • edited Loading

dunxen commented Oct 6, 2021 • edited Loading

TheBlueMatt commented Oct 6, 2021

TheBlueMatt commented Oct 6, 2021

codecov bot commented Oct 6, 2021 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

dunxen commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Oct 6, 2021 •

edited

Loading

dunxen commented Oct 6, 2021 •

edited

Loading

codecov bot commented Oct 6, 2021 •

edited

Loading

TheBlueMatt Oct 8, 2021 •

edited

Loading

dunxen Oct 8, 2021 •

edited

Loading