fix(FileStore): deduplicate merkle leaves in readMerkleFile to preven…#12
Closed
es-kt wants to merge 3 commits into
Closed
fix(FileStore): deduplicate merkle leaves in readMerkleFile to preven…#12es-kt wants to merge 3 commits into
es-kt wants to merge 3 commits into
Conversation
…t wrong leaf lookup The JSONL merkle leaves file can accumulate duplicate CID entries when appendMerkleLeaves is called after a process restart re-syncs an overlapping range. Since getMerkleLeaf uses the CID as an array index (rows[cid]), duplicates shift all subsequent lookups — causing buildLocalProofPath to retrieve wrong leaf commitments and produce invalid merkle proofs. After sorting by CID, deduplicate adjacent entries keeping the last occurrence of each CID. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces deduplication logic for entries in the FileStore to prevent incorrect leaf retrieval caused by duplicate CID entries. The reviewer suggested refactoring the manual loop implementation into a more idiomatic reduce-based approach to improve readability.
Comment on lines
+125
to
+136
| if (out.length > 0) { | ||
| const deduped: typeof out = [out[0]!]; | ||
| for (let i = 1; i < out.length; i++) { | ||
| if (out[i]!.cid === deduped[deduped.length - 1]!.cid) { | ||
| deduped[deduped.length - 1] = out[i]!; | ||
| } else { | ||
| deduped.push(out[i]!); | ||
| } | ||
| } | ||
| return deduped; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
The deduplication logic is correct, but it can be implemented more concisely using the reduce method. This would make the code more idiomatic and potentially easier to read for developers familiar with functional programming patterns.
Suggested change
| if (out.length > 0) { | |
| const deduped: typeof out = [out[0]!]; | |
| for (let i = 1; i < out.length; i++) { | |
| if (out[i]!.cid === deduped[deduped.length - 1]!.cid) { | |
| deduped[deduped.length - 1] = out[i]!; | |
| } else { | |
| deduped.push(out[i]!); | |
| } | |
| } | |
| return deduped; | |
| } | |
| return undefined; | |
| if (out.length === 0) { | |
| return undefined; | |
| } | |
| return out.reduce((acc, current) => { | |
| const last = acc[acc.length - 1]; | |
| if (last?.cid === current.cid) { | |
| acc[acc.length - 1] = current; | |
| } else { | |
| acc.push(current); | |
| } | |
| return acc; | |
| }, [] as typeof out); |
…NextCid getMerkleNextCid catches all errors and returns 0, which tells appendMerkleLeaves the file is empty. On a transient I/O error this causes every incoming leaf to be re-appended, creating duplicate CID entries in the JSONL file. Since getMerkleLeaf uses cid as an array index (rows[cid]), the duplicates shift all lookups — buildLocalProofPath retrieves wrong commitments and produces invalid merkle proofs. Only return 0 for ENOENT (file genuinely missing). Propagate all other errors so appendMerkleLeaves skips the write instead of corrupting the file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hub.com/PolyhedraZK/ocash-sdk into fix/filestore-merkle-leaf-duplicate-cid
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…t wrong leaf lookup
The JSONL merkle leaves file can accumulate duplicate CID entries when appendMerkleLeaves is called after a process restart re-syncs an overlapping range. Since getMerkleLeaf uses the CID as an array index (rows[cid]), duplicates shift all subsequent lookups — causing buildLocalProofPath to retrieve wrong leaf commitments and produce invalid merkle proofs.
After sorting by CID, deduplicate adjacent entries keeping the last occurrence of each CID.
Summary
Brief description of changes.
Changes
Testing
pnpm run testpassespnpm run buildsucceedspnpm run type-checkpassesRelated Issues
Closes #