fix(kv-store): iterate LMDBMultiMap ranges past a numeric key 0 - #376
Conversation
LMDBMultiMap.entriesAsync treated a numeric key 0 as an absent bound or a decode failure: it built range bounds with range?.start/range?.end truthiness and broke iteration on !deserializedKey. A valid key 0 is falsey, so a range scan that reached key 0 stopped at the first key. The p2p attestation pool's slot-keyed proposal-index cleanup (deleteOlderThan) iterates these numeric multimaps, so once a node stored a slot-0 proposal index, cleanup stopped deleting old indexes and stale p2p state grew unbounded by finalized-block cleanup. Mirror LMDBMap: compare bounds with !== undefined and break only when deserializeKey returns false. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // (used by the p2p attestation pool's slot-keyed proposal-index cleanup) must include and iterate | ||
| // past it; otherwise cleanup that starts at slot 0 stops at the first key and old indexes leak. | ||
| it('iterates entries past a numeric key 0', async () => { | ||
| const store = await openTmpStore('test-numeric-entries'); |
There was a problem hiding this comment.
store.delete(), so test runs leave database files behind. Add teardown for both stores.
| await map.set(1, 'b'); | ||
| await map.set(2, 'c'); | ||
|
|
||
| const keys = await toArray(map.keysAsync({ start: 0 })); |
There was a problem hiding this comment.
start: 0 is honored or ignored. Neither new test checks end: 0, so both bound changes could regress without failing these tests. Add cases that distinguish an explicit zero bound from an absent one.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| await map.set(0, 'a'); | ||
| await map.set(1, 'b'); | ||
| await map.set(2, 'c'); | ||
|
|
||
| const keys = (await toArray(map.entriesAsync())).map(([key]) => key); |
There was a problem hiding this comment.
store.transactionAsync(). The repository requires related AztecAsyncKVStore reads and writes to be wrapped in a transaction for atomicity. Please satisfy that requirement in both tests before merging.
Context Used: yarn-project/CLAUDE.md (source)
- add a discriminating end:0 case: end is exclusive, so an honored zero bound returns [], while the pre-fix code reads 0 as falsey, drops the bound, and returns all keys - so this assertion fails when the zero bound regresses (start:0 forward cannot distinguish an honored bound from an absent one with non-negative keys, so it is not a regression guard on its own) - wrap the related writes and range reads in store.transactionAsync for atomicity - delete both temp stores in a finally block so the tests do not leak LMDB dirs
2 participants
LMDBMultiMap.entriesAsync treated a numeric key 0 as an absent bound or a decode failure. A valid key 0 is falsey, so a range scan that reached key 0 stopped at the first key. Once a node stored a slot-0 proposal index, cleanup stopped deleting old indices.
Mirror LMDBMap: compare bounds with !== undefined and break only when deserializeKey returns false.
Fixes A-2186