Skip to content
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 PublicKeys for NodeIds #1973

Closed
TheBlueMatt opened this issue Jan 19, 2023 · 3 comments · Fixed by #2016
Closed

Swap Gossip Message PublicKeys for NodeIds #1973

TheBlueMatt opened this issue Jan 19, 2023 · 3 comments · Fixed by #2016
Labels
good first issue Good for newcomers
Milestone

Comments

@TheBlueMatt
Copy link
Collaborator

To improve the deserialization time of our network graph and address #960, we swapped the PublicKeys describing node ids directly in the graph with a new NodeId struct in #1107. This improved performance greatly, but missed that we're still deserializing PublicKeys a ton when reading a NetworkGraph - we store the original gossip messages in the graph itself with PublicKeys and have to do a bunch of key checking when reading the graph again.

Replacing the PublicKeys in gossip messages with NodeIds should give us yet again a huge speedup in the time it takes to read our network grpah.

@TheBlueMatt TheBlueMatt added the good first issue Good for newcomers label Jan 19, 2023
@TheBlueMatt TheBlueMatt added this to the 0.0.115 milestone Jan 21, 2023
@alecchendev
Copy link
Contributor

I can try and pick this up! I see that the structs UnsignedChannelAnnouncement and UnsignedNodeAnnouncement use PublicKey and are embedded in NetworkGraph. Would this mainly involve changing these structs' PublicKey fields to NodeId and adapting the associated serialization?

@TheBlueMatt
Copy link
Collaborator Author

Yep, basically just that. We'll need to parse them to PublicKeys when checking signatures (and Err for invalid keys), but that also shouldn't be too complicated (just try to avoid parsing each key more than once in the processing of any given message).

@alecchendev
Copy link
Contributor

Cool, I'll get started on this tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants