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

feat(iroh-net): cache for crypto keys #1677

Merged
merged 10 commits into from
Oct 20, 2023
Merged

feat(iroh-net): cache for crypto keys #1677

merged 10 commits into from
Oct 20, 2023

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Oct 19, 2023

Description

An attempt to get down the size of a key to something more manageable, see #1673

Notes & open questions

Size of the cache and TTL. I think 1 minute is fine, if you don't perform any crypto ops on a key for 1 minute it means you are not connected to it. Size is more difficult, but the items are not that big so there is not much harm in having a big size.

Note two: this should help a lot for the derper.

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

This way the key itself remains nice and small
iroh-net/src/key.rs Outdated Show resolved Hide resolved
iroh-net/Cargo.toml Outdated Show resolved Hide resolved
- use OnceCell instead of lazy_static
- use serde_bytes to make sure the serialization format is right
iroh-net/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

Have you considered putting the cache into the magicsock struct rather than having it global? It would make the change more invasive probably, but you have more flexibility. E.g. you could potentially allow customising it etc.

@dignifiedquire dignifiedquire added this to the v0.8.0 milestone Oct 20, 2023
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 20, 2023

Have you considered putting the cache into the magicsock struct rather than having it global? It would make the change more invasive probably, but you have more flexibility. E.g. you could potentially allow customising it etc.

You could just remove crypto ops from the keys entirely and then have a keymap struct that has signatures like this:

fn verify(&self, key: PublicKey, message: &[u8], signature: &Signature) -> Result<(), SignatureError>;
fn whatever_the_crypto_op_is_that_needs_cryptobox(&self, ...);

You would then keep this keymap struct somewhere in the magicsock. Downside is that people using the keys as is would have a less discoverable API and would have to make their own cryptomap. But it would be cleaner, yes.

But it would also be much more invasive. E.g. you could not do validation directly in deserialize like you do now. So let's maybe do it as a second step.

@rklaehn rklaehn added this pull request to the merge queue Oct 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 20, 2023
@rklaehn rklaehn added this pull request to the merge queue Oct 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 20, 2023
@rklaehn rklaehn added this pull request to the merge queue Oct 20, 2023
Merged via the queue into main with commit f8f08a0 Oct 20, 2023
15 checks passed
@dignifiedquire dignifiedquire deleted the small-keys-2 branch October 20, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants