Skip to content

fix(store): bound MemPublicKeyStore cache to prevent memory exhaustion#93

Open
emrul wants to merge 1 commit into
n0-computer:mainfrom
aster-rpc:fix/bound-mempublickeystore
Open

fix(store): bound MemPublicKeyStore cache to prevent memory exhaustion#93
emrul wants to merge 1 commit into
n0-computer:mainfrom
aster-rpc:fix/bound-mempublickeystore

Conversation

@emrul
Copy link
Copy Markdown

@emrul emrul commented Apr 12, 2026

Description

The in-memory public-key cache on MemPublicKeyStore was unbounded — every distinct 32-byte public key passed through public_key() added a cache entry that persisted for the lifetime of the store. The existing source already carried a TODO: Make max number of keys stored configurable. comment acknowledging this. This PR addresses that low-hanging fruit.

The cache is fed from sync-peer input via insert_remote_entry → validate_entry → SignedEntry::verify → PublicKeyStore::public_key. A peer that sends entries signed by many distinct authors grows the victim's map one entry per distinct 32-byte author/namespace key, forever.

This PR caps the cache at MAX_CACHED_KEYS = 10_000 using lru::LruCache. The lru crate is already transitively present via iroh-relay, so chose to add this as a direct dependency to avoid adding something new to the project.

Two tests cover the change: cache_is_bounded_under_unique_key_flood feeds 15,000 distinct valid ed25519 keys through the cache and asserts the size stays ≤ MAX_CACHED_KEYS; recently_used_keys_survive_eviction fills the cache to capacity, touches the first key, inserts 100 more distinct keys, and asserts the touched key is still cached while the oldest untouched one has been evicted.

Breaking Changes

None at the API surface. MemPublicKeyStore's internal representation changed from HashMap to LruCache; the public PublicKeyStore trait is unchanged. Behaviour change: once the cache reaches 10,000 entries, previously-cached keys may be evicted on subsequent misses (re-verification cost is a single VerifyingKey::from_bytes call).

Notes & open questions

  • 10,000 is a judgment call; asking reviewers whether it's the right default for iroh-docs?

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@n0bot n0bot Bot added this to iroh Apr 12, 2026
@github-project-automation github-project-automation Bot moved this to 🚑 Needs Triage in iroh Apr 12, 2026
@Frando
Copy link
Copy Markdown
Member

Frando commented Apr 20, 2026

I think this should use an LRU cache instead of blindly clearing the map once the size limit is reached.

The in-memory public key cache was unbounded — every distinct 32-byte
public key passed through `public_key()` added an entry that persisted
for the lifetime of the store. The existing source already carried a
`TODO: Make max number of keys stored configurable.` comment
acknowledging this.

This matters because the cache is fed from sync-peer input via the
`insert_remote_entry → validate_entry → SignedEntry::verify → public_key`
call chain. A peer that sends entries signed by many distinct authors
grows the victim's map one entry per distinct author forever.

Fix: cap the cache at `MAX_CACHED_KEYS = 10_000` using an `lru::LruCache`.
When the cache is full, the least-recently-used entry is evicted. The
`lru` crate is already transitively present in the dependency tree via
`iroh-relay`, so this adds no new compile.
@emrul emrul force-pushed the fix/bound-mempublickeystore branch from 6bb86ff to 73bab94 Compare April 21, 2026 08:45
@emrul
Copy link
Copy Markdown
Author

emrul commented Apr 21, 2026

Thanks @Frando - agreed that makes sense. Have done the same as for #94 (used your PR template + did formatting checks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

2 participants