-
Notifications
You must be signed in to change notification settings - Fork 11
Fix IMT skipping #390
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
Fix IMT skipping #390
Conversation
Summary of ChangesHello @MartinOndejka, 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 introduces significant enhancements to the Indexed Merkle Tree (IMT) and the Data Availability (DA) layer, focusing on data integrity and verifiability. It strengthens circuit-level guarantees for IMT operations, integrates the IMT's state into the DA layer's cryptographic signatures, and adds robust checks to ensure the consistent ordering of accounts. These changes are supported by necessary data model updates, RPC versioning, a new database migration, and comprehensive logging improvements across the system. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces several changes to fix an issue with IMT skipping. The changes span across circuits and the DA layer. In the circuits, a new check is added to ensure the leaf before a new entry y is non-empty, and adjacency is verified. In the DA layer, the account set sparse ledger is now sent to the DA node to verify the order of new accounts, preventing skipping. The signature for DA checks now also covers the account set.
The changes are quite extensive and touch many files to propagate necessary data like acc_set_openings. The introduction of versioned RPC fallbacks is a good improvement for backward compatibility.
I've found two critical issues that need to be addressed:
- An incorrect assertion in
indexed_merkle_tree.mlthat compares two different Merkle roots that should not be equal. - A typo in
da_layer/lib/core.mlthat makes a security check always pass.
Apart from these issues, the changes look good and consistent.
Fixes of #350
fix ZK-341
Circuit changes:
yin IMT is non-emptyDA changes: