Skip to content
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(katana): compute genesis state root in a separate provider #3045

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

kariy
Copy link
Member

@kariy kariy commented Feb 17, 2025

When katana --chain <ID> is started with an already initialized database ie --db-dir <PATH>, the chain's genesis block hash corresponds will be compared to the existing block hash in the provided database. To perform this comparison, the genesis state root needs to computed first. Currently, the computation is done against the same database that we provide (remember the database has already been initialized, and may have other state apart from the genesis) and the trie computation logic (the call to trie_insert_declared_classes and trie_insert_contract_updates in the compute_new_state_root function) will automatically writes the updates into the db. Thus, OVERWRITING the already established tries and resulted in a completely different state root value.

So the fix is to perform the check first - checking whether there's a genesis block or not in the database: If yes, then we compute the genesis state using a separate provider to avoid overwriting the db. Else, (meaning the db is empty and hasn't been initialized yet) we compute it using the db storage provider.

Summary by CodeRabbit

  • New Features

    • Introduced enhanced block commitment operations, including specialized handling for initial blocks and improved block hash tracking.
    • Expanded contract state management with a new structure that provides more detailed data during trie updates.
    • Added a test function to verify backend reinitialization of the genesis state.
  • Refactor

    • Consolidated contract state definitions by adopting a shared implementation.
    • Streamlined state update routines and broadened module exports to offer more comprehensive functionality.

Copy link

coderabbitai bot commented Feb 17, 2025

Ohayo sensei!

Walkthrough

This pull request refactors and modularizes the blockchain commitment and trie update logic. In the backend module, new functions for committing blocks—including a genesis block—are introduced, along with a helper to update the block hash registry. A dedicated GenesisTrieWriter struct now handles trie updates. Additionally, the local definition of the ContractLeaf struct is replaced by a shared version imported from a common module, and the contracts module export has been expanded. These changes enhance clarity in state management and standardize trie-related operations.

Changes

Files Change Summary
crates/katana/.../mod.rs Refactored block commitment logic; added new functions commit_block, commit_genesis_block, and helper update_block_hash_registry_contract; introduced GenesisTrieWriter for trie updates.
crates/katana/storage/.../trie.rs &
crates/katana/trie/.../contracts.rs
Removed the local ContractLeaf definition and instead imported it from a shared module; introduced a new public ContractLeaf in the trie module with fields class_hash, storage_root, and nonce.
crates/katana/trie/.../lib.rs Changed public export from a specific ContractsTrie to export all public entities within the contracts module.
crates/katana/core/tests/.../backend.rs Added new test function can_reinitialize_genesis to verify backend behavior when reinitializing the genesis state with the same chain specification.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant B as Backend
    participant T as TrieWriter

    C->>B: commit_block(provider, header, txs, receipts, state_updates)
    B->>B: update_block_hash_registry_contract(state_updates, block_number)
    B->>T: Process trie updates via GenesisTrieWriter
    B->>C: Return sealed block
Loading
sequenceDiagram
    participant C as Client
    participant B as Backend
    participant G as GenesisTrieWriter

    C->>B: commit_genesis_block(provider, header, txs, receipts, state_updates)
    B->>G: Handle trie update for genesis block
    B->>C: Return sealed block
Loading

Possibly Related PRs


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
crates/katana/core/src/backend/mod.rs (3)

265-277: Ohayo sensei, let's be mindful of the "hacky" approach.
You're employing GenesisTrieWriter to avoid overwriting an existing database for the already initialized genesis. This solves the immediate problem, but consider a more elegant solution or a dedicated check in the future.


494-495: Ohayo sensei! Watch for concurrency or race conditions in update_block_hash_registry_contract.
Storing older block hashes with a buffer is a clever approach. The comment mentions “Fix quick!”, so let's keep an eye on potential concurrency with forked mode or when older blocks are absent.

Also applies to: 497-522


549-625: Ohayo sensei! GenesisTrieWriter effectively manages a volatile trie.
The approach of building contract leafs in memory and committing them helps avoid polluting the main DB. Just ensure no parallel usage by multiple threads without synchronization to avoid data races.

crates/katana/trie/src/lib.rs (2)

16-16: Ohayo! Consider being more selective with exports, sensei!

The wildcard export pub use contracts::*; exposes all public entities from the contracts module. While this provides access to the new ContractLeaf struct, it might expose more implementation details than necessary. Consider explicitly listing the required exports.

-pub use contracts::*;
+pub use contracts::{ContractLeaf, ContractsTrie};

20-25: Ohayo! Documentation needs updating, sensei!

The documentation references ContractsTrie directly, but with the expanded exports, it should be updated to mention other important types that are now publicly available, such as ContractLeaf.

 /// This struct is not meant to be used directly, and instead use the specific tries that have
-/// been derived from it, [`ClassesTrie`], [`ContractsTrie`], or [`StoragesTrie`].
+/// been derived from it, [`ClassesTrie`], [`ContractsTrie`], [`ContractLeaf`], or [`StoragesTrie`].
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 040c1d6 and 67cde31.

📒 Files selected for processing (4)
  • crates/katana/core/src/backend/mod.rs (6 hunks)
  • crates/katana/storage/provider/src/providers/db/trie.rs (1 hunks)
  • crates/katana/trie/src/contracts.rs (1 hunks)
  • crates/katana/trie/src/lib.rs (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: build
  • GitHub Check: docs
  • GitHub Check: ensure-wasm
🔇 Additional comments (9)
crates/katana/core/src/backend/mod.rs (6)

1-1: Ohayo sensei! Good expansion of imports here.
These new imports (BTreeMap, HashMap, ClassHash, CompiledClassHash, and the katana_trie references) neatly set the stage for the enhanced trie-handling logic. No issues spotted.

Also applies to: 12-12, 24-28


118-119: Ohayo sensei! Nice refactor to call commit_block with a dedicated provider.
This approach centralizes block finalization logic and ensures the block is computed consistently. No concerns here.


278-282: Ohayo sensei! Good mismatch check for existing vs. provided genesis hash.
Ensuring these hashes match prevents corrupting local state. Great safety net.


288-294: Ohayo sensei! Fallback logic for empty DB is neat.
Switching to commit_genesis_block with the real provider if no local genesis block is found ensures a proper chain start without rewriting the DB.


524-537: Ohayo sensei! commit_block function looks well-structured.
Registering the block hash (contract address 0x1) before computing the rest clarifies the flow. Great job.


539-547: Ohayo sensei! Separating commit_genesis_block is a clean solution.
Keeping genesis logic distinct reduces the risk of accidental overwrites in normal block commits. Nicely done.

crates/katana/trie/src/contracts.rs (1)

49-54: Ohayo sensei! ContractLeaf struct is a welcome addition.
Having class hash, storage root, and nonce in one structure centralizes contract state. Looks good to me.

crates/katana/storage/provider/src/providers/db/trie.rs (1)

10-12: Ohayo sensei! Neat refactor to import ContractLeaf from katana_trie.
Removing local duplication reduces maintenance overhead. Good code reuse.

crates/katana/trie/src/lib.rs (1)

1-178: Ohayo! Changes look good, sensei!

The modifications support the PR's objective of fixing genesis state root computation by making necessary types available while maintaining the core trie functionality intact.

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 98.29060% with 2 lines in your changes missing coverage. Please review.

Project coverage is 57.76%. Comparing base (040c1d6) to head (4a0b8eb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/katana/core/src/backend/mod.rs 98.29% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3045      +/-   ##
==========================================
+ Coverage   57.68%   57.76%   +0.07%     
==========================================
  Files         437      437              
  Lines       59231    59305      +74     
==========================================
+ Hits        34169    34257      +88     
+ Misses      25062    25048      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
crates/katana/core/tests/backend.rs (1)

83-91: Consider adding error case coverage, sensei!

While the happy path is well tested, consider adding test cases for potential error scenarios such as:

  • Database corruption between initializations
  • Invalid chain specifications
  • Network issues during initialization
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67cde31 and 4a0b8eb.

📒 Files selected for processing (1)
  • crates/katana/core/tests/backend.rs (1 hunks)
🔇 Additional comments (1)
crates/katana/core/tests/backend.rs (1)

80-91: Ohayo! Well-structured test implementation, sensei!

The new test case effectively validates the fix for computing genesis state root with an existing database. The test structure is clean and uses parameterized testing appropriately.

@kariy kariy enabled auto-merge (squash) February 18, 2025 02:09
@kariy kariy disabled auto-merge February 18, 2025 02:09
@kariy kariy merged commit 1065468 into main Feb 18, 2025
15 checks passed
@kariy kariy deleted the katana/genesis-trie-overwritten branch February 18, 2025 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant