-
Notifications
You must be signed in to change notification settings - Fork 345
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
Swap gossip message PublicKey
for NodeId
#2016
Swap gossip message PublicKey
for NodeId
#2016
Conversation
Note: for channel_announcement messages, public keys are parsed for 1. signature verification, but also 2. recreating the funding output to check if the output corresponding to the channel announcement matches. For 2. it’s possible to avoid doing this again after parsing for signature verification (by passing the previously parsed public keys as arguments to |
Codecov ReportBase: 90.89% // Head: 90.88% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2016 +/- ##
==========================================
- Coverage 90.89% 90.88% -0.02%
==========================================
Files 99 99
Lines 53026 53034 +8
Branches 53026 53034 +8
==========================================
+ Hits 48197 48198 +1
- Misses 4829 4836 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
2f2d167
to
5cbda98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This turned out to be pretty straightforward and a huge gain, as expected.
CI (which has tons of noise) says read_network_graph
is now test 527,259,818 ns/iter (+/- 44,143,324), down from 3,304,538,463 ns/iter (+/- 182,981,140)
Feel free to go ahead and squash the fixups down into the original commits. |
04c84ac
to
3c0152e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, big win! 🥳
Adds the macro `get_pubkey_from_node_id` to parse `PublicKey`s back from `NodeId`s for signature verification, as well as `make_funding_redeemscript_from_slices` to avoid parsing back and forth between types.
Also swaps `PublicKey` for `NodeId` in `get_next_node_announcement` and `InitSyncTracker` to avoid unnecessary deserialization that came from changing `UnsignedNodeAnnouncement`.
3c0152e
to
b156371
Compare
peer.their_node_id.as_ref() == Some(&msg.contents.node_id_2) { | ||
continue; | ||
if let Some(their_node_id) = peer.their_node_id { | ||
let their_node_id = NodeId::from_pubkey(&their_node_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, serializing the keys of all our peers every time we go to forward a gossip message blows, Let's keep a cached NodeId
copy for each peer (though it can go in a followup).
Closes #1973. Swaps
PublicKey
forNodeId
inUnsignedChannelAnnouncement
andUnsignedNodeAnnouncement
and updates any affected areas.