Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @martti007, 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 refactors the Smt class by removing its internal concurrency control mechanism and simplifying its API. The primary change involves making the rootHash and getPath operations asynchronous, as they now compute the Merkle tree root on demand, rather than maintaining a cached state. This change propagates throughout the codebase, updating all instances where the Smt class is used.
Highlights
- Smt Class Simplification: The internal locking mechanism (
smtUpdateLock,waitingPromises,LOCK_TIMEOUT_MS,acquireSmtLock,releaseSmtLock,withSmtLock) within theSmtclass has been completely removed. This simplifies the class by delegating concurrency concerns, if any, to the underlyingSparseMerkleTreeor higher-level application logic. - Asynchronous SMT Root Calculation: The
rootHashandgetPathmethods of theSmtclass are now asynchronous. They no longer rely on a cached_rootproperty but instead calculate the Sparse Merkle Tree root on demand by callingawait this.smt.calculateRoot(). - Smt Instantiation Changes: The static asynchronous
Smt.createfactory method has been removed. TheSmtclass can now be instantiated synchronously usingnew Smt(smt), reflecting the removal of the initial root calculation from the constructor/factory. - Codebase Adaptation: All call sites across the application, including
AggregatorGateway,RoundManager,AggregatorRouter, and various test files, have been updated to reflect the changes in theSmtclass. This primarily involves changingSmt.createcalls tonew Smt()and accessingrootHashandgetPathasawait smt.rootHash()orawait smt.getPath().
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR removes the locking mechanism from the Smt class wrapper around SparseMerkleTree. The changes eliminate the concurrency control that was previously implemented to ensure sequential execution of asynchronous operations on the SMT.
Key changes:
- Removed the internal locking mechanism (
smtUpdateLock,waitingPromises, timeout handling) - Changed
rootHashfrom a synchronous property to an asynchronous method - Simplified the constructor by removing the static
createmethod - Updated all callers to use the new constructor and async
rootHash()method
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/smt/Smt.ts | Removed locking mechanism, converted rootHash to async method, simplified constructor |
| tests/smt/SmtTest.ts | Completely removed test file that was testing the locking functionality |
| src/RoundManager.ts | Updated to use async rootHash() method |
| src/router/AggregatorRouter.ts | Updated to use async rootHash() method with proper error handling |
| src/AggregatorGateway.ts | Updated constructor usage and async rootHash() calls |
| tests/smt/SmtChunkedLoadingTest.ts | Updated to use async rootHash() method |
| tests/benchmarks/BlockCreationBenchmarkTest.ts | Updated constructor usage and removed extra blank line |
| tests/RoundManagerUnitTest.ts | Updated constructor usage and reordered imports |
| tests/AggregatorServiceTest.ts | Updated constructor usage |
There was a problem hiding this comment.
Code Review
This pull request refactors the Smt class to remove its internal locking mechanism. This simplifies the class significantly, but it also introduces a critical race condition, as concurrent updates to the SMT are no longer serialized. This could lead to data corruption. Additionally, there's a performance regression, as the SMT root is now recalculated on every read access instead of being cached. My review includes detailed comments on these two issues with suggestions for how to address them.
No description provided.