fix: sort cell indices before calling recoverCellsAndKzgProofs
#8450
Conversation
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
Contributor
Summary of ChangesHello @ensi321, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical compliance requirement from the latest specification for KZG operations. It ensures that cell indices are correctly sorted before being processed by the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where cell indices were not sorted before being passed to recoverCellsAndKzgProofs, as required by the latest spec. The change correctly sorts the partialSidecars in dataColumnMatrixRecovery. However, I've found a critical bug in how the sorted array is used to gather cellIndices, which would lead to incorrect data recovery. I've also pointed out a minor redundancy. The changes in the test file to validate sorted indices are a good addition.
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
Member
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request correctly implements the spec requirement to sort cell indices before calling recoverCellsAndKzgProofs. The changes in dataColumnMatrixRecovery ensure that sidecars are sorted by index, guaranteeing that cell indices passed to the KZG function are in ascending order. The corresponding update to the test helper in kzg.ts to validate the sorted order of indices is a good addition for test correctness. The implementation is clean and effectively addresses the issue. No issues found.
nflaig
reviewed
Sep 23, 2025
| function recoverCellsAndKzgProofs(input: RecoverCellsAndKzgProofsInput): [string[], string[]] | null { | ||
| const isSorted = input.cell_indices.every((val, i, arr) => i === 0 || arr[i - 1] < val); | ||
| // If cell indices are not in ascending order, they are deemed invalid and cannot pass it to kzg | ||
| if (!isSorted) return null; |
Member
There was a problem hiding this comment.
is the spec test invalid, or what happens if you pass them to kzg? I see we already have a try/catch below and would also return null
Contributor
Author
There was a problem hiding this comment.
The problem is kzg.recoverCellsAndKzgProofs doesn't enforce the order of cell indices. So it will return list of cells without sorting them instead of throwing error.
So it's up to the client to enforce the order before calling kzg
Contributor
Author
There was a problem hiding this comment.
Here's what it looks like if we run the related spec test without validating the order
Member
There was a problem hiding this comment.
should we create a wrapper function that ensures sorting and call that one instead?
Member
There was a problem hiding this comment.
i think that is what jtraglia suggested
Contributor
Author
There was a problem hiding this comment.
should we create a wrapper function that ensures sorting and call that one instead?
But spec test calls recoverCellsAndKzgProofs where as dataColumnMatrixRecovery calls asyncRecoverCellsAndKzgProofs. So even if we have a wrapper, we would need a flag to indicate async or not, and there will still be two code paths.
Member
There was a problem hiding this comment.
The code paths are actually the same under the hood...
https://github.com/crate-crypto/rust-eth-kzg/blob/master/bindings/node/src/lib.rs#L202
Member
There was a problem hiding this comment.
I think its fine to return null like this. We know we are handling the case correctly in the main codebase and short circuiting the spec tests is done elsewhere due to code conditions.
I think its worth just adding a comment stating that we are doing it for spec compliance and then point to this PR for future reference of where in the actual code we are handling the condition and that should cover us for spirit of intent and correctness with regards to the spec
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #8450 +/- ##
============================================
- Coverage 52.24% 52.22% -0.03%
============================================
Files 853 853
Lines 64770 64899 +129
Branches 4764 4769 +5
============================================
+ Hits 33841 33894 +53
- Misses 30859 30935 +76
Partials 70 70 🚀 New features to boost your workflow:
|
Member
|
🎉 This PR is included in v1.35.0 🎉 |
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.
For context please view https://discord.com/channels/593655374469660673/1229813597501395028/1419794786121945141 .
The latest spec requires cell indices to be sorted in
recover_cells_and_kzg_proofsso we need to sort them before passing it to kzg.