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

enha: stratus_reset always resets to zero #1561

Merged
merged 10 commits into from
Jul 30, 2024
Merged

enha: stratus_reset always resets to zero #1561

merged 10 commits into from
Jul 30, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 30, 2024

PR Type

Enhancement, Other


Description

  • Removed enable-genesis flag and related logic from miner configuration.
  • Added reset_to_genesis method to StratusStorage and updated related calls.
  • Simplified reset methods in various storage implementations to reset all state to default.
  • Added DebugAsJson derive to Block struct.
  • Updated and simplified various shell scripts and justfile commands.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
miner_config.rs
Refactor miner configuration and genesis block handling   

src/eth/miner/miner_config.rs

  • Removed enable-genesis flag and related logic.
  • Added logic to reset storage to genesis state if necessary.
  • Added can_mine_new_blocks method to MinerMode.
  • +16/-22 
    block.rs
    Add DebugAsJson derive to Block struct                                     

    src/eth/primitives/block.rs

    • Added DebugAsJson derive to Block struct.
    +2/-1     
    rpc_server.rs
    Update stratus_reset to use reset_to_genesis                         

    src/eth/rpc/rpc_server.rs

    • Changed stratus_reset to use reset_to_genesis method.
    +1/-1     
    inmemory_permanent.rs
    Simplify in-memory storage reset method                                   

    src/eth/storage/inmemory/inmemory_permanent.rs

    • Simplified reset method to reset all state to default.
    +3/-22   
    permanent_storage.rs
    Update permanent storage reset method signature                   

    src/eth/storage/permanent_storage.rs

    • Changed reset method to no longer take a block number.
    +1/-1     
    redis_permanent.rs
    Update Redis storage reset method signature                           

    src/eth/storage/redis/redis_permanent.rs

    • Changed reset method to no longer take a block number.
    +1/-1     
    rocks_permanent.rs
    Simplify RocksDB permanent storage reset method                   

    src/eth/storage/rocks/rocks_permanent.rs

    • Simplified reset method to reset all state to default.
    +3/-13   
    rocks_state.rs
    Simplify RocksDB state reset method                                           

    src/eth/storage/rocks/rocks_state.rs

    • Simplified reset method to clear all state.
    +8/-73   
    stratus_storage.rs
    Add reset_to_genesis method to StratusStorage                       

    src/eth/storage/stratus_storage.rs

  • Renamed reset method to reset_to_genesis.
  • Added logic to reset storage to genesis state and save test accounts.
  • +19/-4   
    justfile
    Simplify justfile commands and remove enable-genesis flag

    justfile

  • Removed --enable-genesis flag from various commands.
  • Simplified build and run commands.
  • +9/-19   
    Miscellaneous
    4 files
    unix_time.rs
    Update imports in unix_time module                                             

    src/eth/primitives/unix_time.rs

    • Added specific imports for UnixTime and Utc.
    +2/-1     
    contracts-test.sh
    Update log messages in contracts-test.sh                                 

    e2e/cloudwalk-contracts/contracts-test.sh

    • Updated log messages for test execution.
    +2/-2     
    block-time-check.sh
    Update earliest block parameter in block-time-check.sh     

    utils/block-time-check.sh

    • Changed earliest block parameter to 0x1.
    +2/-2     
    justfile_helpers
    Update log message in justfile_helpers                                     

    justfile_helpers

    • Updated log message for waiting for Stratus to start.
    +1/-1     
    Cleanup
    1 files
    rocks_cf.rs
    Remove unused methods in RocksDB column family                     

    src/eth/storage/rocks/rocks_cf.rs

  • Removed unused methods related to deleting and iterating over entries.

  • +0/-27   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Missing Logging
    The initialization of the Miner component should be logged with all relevant configurations to ensure proper traceability and debugging.

    Missing Span Fields
    The reset function is instrumented with tracing::info_span, but no identifiers are recorded as span fields. Consider adding relevant fields for better traceability.

    Dynamic Fields in Tracing
    Avoid formatting dynamic fields in tracing events. Instead, add these dynamic fields as tracing fields. For example, in the tracing::debug! and tracing::error! calls.

    Unwrap Usage
    The code uses unwrap which can cause a panic. Consider using expect with a meaningful message to handle potential errors gracefully.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Wrap the timed function call in a closure to ensure proper execution order

    The timed function call should be wrapped in a closure to ensure that the
    metrics::inc_storage_reset function is called only after the reset operation
    completes.

    src/eth/storage/stratus_storage.rs [488]

    -timed(|| self.perm.reset()).with(|m| {
    +timed(|| { self.perm.reset() }).with(|m| {
     
    Suggestion importance[1-10]: 8

    Why: Wrapping the timed function call in a closure ensures that the metrics are updated only after the reset operation completes, which is a good practice for ensuring accurate metrics.

    8
    Performance
    Use a more relaxed memory ordering for atomic operations if strict ordering is not necessary

    Instead of using Ordering::SeqCst for the atomic store operation, consider using
    Ordering::Relaxed if the strictest memory ordering is not required. This can improve
    performance.

    src/eth/storage/inmemory/inmemory_permanent.rs [284]

    -self.block_number.store(0u64, Ordering::SeqCst);
    +self.block_number.store(0u64, Ordering::Relaxed);
     
    Suggestion importance[1-10]: 7

    Why: The suggestion is valid and can improve performance by using a more relaxed memory ordering. However, it should be verified that strict ordering is indeed not necessary in this context.

    7

    @dinhani-cw
    Copy link
    Contributor Author

    /describe

    Copy link

    PR Description updated to latest commit (033ceef)

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 05:55
    @dinhani-cw
    Copy link
    Contributor Author

    /describe

    Copy link

    PR Description updated to latest commit (8a7b8b3)

    @dinhani-cw
    Copy link
    Contributor Author

    /describe

    Copy link

    PR Description updated to latest commit (baf8663)

    @dinhani-cw dinhani-cw merged commit dbe2cb2 into main Jul 30, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the perm-reset branch July 30, 2024 07: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