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: save analysed bytecodes #1905

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

carneiro-cw
Copy link
Contributor

@carneiro-cw carneiro-cw commented Dec 6, 2024

PR Type

Enhancement


Description

  • Refactored the codebase to use the Bytecode type instead of raw bytes for representing contract bytecode
  • Updated Account, ExecutionAccountChanges, and related structures to work with the new Bytecode type
  • Introduced BytecodeRocksdb for persistent storage of bytecode, supporting different bytecode formats (Legacy, Analyzed, EOF)
  • Modified EVM configuration to use AnalysisKind::Analyse for created bytecodes
  • Updated RPC methods and storage implementations to work with the new bytecode representation
  • Added necessary conversions and implementations for seamless integration of the new bytecode types
  • Adjusted test cases to reflect the changes in bytecode handling
  • Enabled the "serde" feature for the revm dependency to support serialization of new types

Changes walkthrough 📝

Relevant files
Configuration changes
1 files
evm.rs
Update EVM bytecode analysis configuration                             

src/eth/executor/evm.rs

  • Changed perf_analyse_created_bytecodes from AnalysisKind::Raw to
    AnalysisKind::Analyse
  • +1/-1     
    Enhancement
    9 files
    account.rs
    Refactor Account struct to use Bytecode type                         

    src/eth/primitives/account.rs

  • Replaced Bytes with Bytecode for the bytecode field in Account struct
  • Updated is_contract method to check bytecode.bytecode() instead of
    bytecode
  • Modified From implementation to use to_analysed function for bytecode
    conversion
  • +7/-4     
    execution_account_changes.rs
    Update ExecutionAccountChanges to use Bytecode                     

    src/eth/primitives/execution_account_changes.rs

  • Changed bytecode field type from ExecutionValueChange> to
    ExecutionValueChange>
  • +3/-2     
    execution_value_change.rs
    Add Default implementations for value change types             

    src/eth/primitives/execution_value_change.rs

  • Added Default derive for ExecutionValueChange and ValueState structs
  • Set NotSet as the default variant for ValueState enum
  • +3/-2     
    rpc_server.rs
    Modify eth_get_code RPC method for new Bytecode type         

    src/eth/rpc/rpc_server.rs

  • Updated eth_get_code function to use bytecode.original_bytes() instead
    of direct bytecode
  • +1/-1     
    inmemory.rs
    Update InMemoryPermanentAccount to use Bytecode                   

    src/eth/storage/permanent/inmemory.rs

  • Changed bytecode field type from InMemoryHistory> to InMemoryHistory>
    in InMemoryPermanentAccount struct
  • +2/-2     
    account.rs
    Refactor AccountRocksdb to use BytecodeRocksdb                     

    src/eth/storage/permanent/rocks/types/account.rs

  • Changed import from bytes::BytesRocksdb to bytecode::BytecodeRocksdb
  • Updated bytecode field type from Option to Option in AccountRocksdb
    struct
  • +2/-2     
    bytecode.rs
    Introduce BytecodeRocksdb for persistent storage                 

    src/eth/storage/permanent/rocks/types/bytecode.rs

  • Added new file with implementations for BytecodeRocksdb,
    LegacyAnalyzedBytecodeRocksdb, EofRocksdb, and related types
  • Implemented conversions between Bytecode and BytecodeRocksdb
  • +137/-0 
    bytes.rs
    Extend BytesRocksdb conversions                                                   

    src/eth/storage/permanent/rocks/types/bytes.rs

  • Added conversions between BytesRocksdb and RevmBytes
  • Implemented From> for BytesRocksdb
  • +20/-0   
    mod.rs
    Add bytecode module to types                                                         

    src/eth/storage/permanent/rocks/types/mod.rs

    • Added mod bytecode; to include the new bytecode module
    +1/-0     
    Tests
    1 files
    rocks_state.rs
    Adjust test cases for new Bytecode type                                   

    src/eth/storage/permanent/rocks/rocks_state.rs

  • Updated test cases to use Bytecode::new_raw instead of raw bytes for
    bytecode
  • +5/-4     
    Dependencies
    1 files
    Cargo.toml
    Enable serde feature for revm dependency                                 

    Cargo.toml

  • Added "serde" feature to revm dependency for both target
    configurations
  • +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Dec 6, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance Impact
    The change from AnalysisKind::Raw to AnalysisKind::Analyse for created bytecodes may have performance implications. This should be carefully evaluated for its impact on execution time and resource usage.

    Code Complexity
    The introduction of the Bytecode type and the changes to the Account struct increase the complexity of the code. This might make future maintenance more challenging and could potentially introduce subtle bugs.

    New File Complexity
    This new file introduces complex structures and conversions for handling different bytecode formats. The complexity of these structures and conversions should be carefully reviewed for correctness and potential performance issues.

    Copy link

    github-actions bot commented Dec 6, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Implement bidirectional conversion between TypesSection and TypesSectionRocksdb for consistency and ease of use

    Implement the From trait for TypesSection to ensure bidirectional conversion between
    these types.

    src/eth/storage/permanent/rocks/types/bytecode.rs [54-62]

     impl From<TypesSection> for TypesSectionRocksdb {
         fn from(value: TypesSection) -> Self {
             Self {
                 inputs: value.inputs,
                 outputs: value.outputs,
                 max_stack_size: value.max_stack_size,
             }
         }
     }
     
    +impl From<TypesSectionRocksdb> for TypesSection {
    +    fn from(value: TypesSectionRocksdb) -> Self {
    +        Self {
    +            inputs: value.inputs,
    +            outputs: value.outputs,
    +            max_stack_size: value.max_stack_size,
    +        }
    +    }
    +}
    +
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances code consistency and usability by implementing bidirectional conversion between TypesSection and TypesSectionRocksdb. It completes the conversion logic, making it easier to work with these types in both directions.

    8
    Simplify block retrieval logic to improve code readability and reduce nesting

    Refactor the block retrieval logic to use a match expression with a single arm for
    BlockFilter::Hash, avoiding nested if-let statements.

    src/eth/storage/permanent/rocks/rocks_state.rs [383-389]

    -BlockFilter::Hash(block_hash) => {
    -            if let Some(block_number) = self.blocks_by_hash.get(&block_hash.into())? {
    -                self.blocks_by_number.get(&block_number)
    -            } else {
    -                Ok(None)
    -            }
    -        }
    +BlockFilter::Hash(block_hash) => self.blocks_by_hash.get(&block_hash.into())?.and_then(|block_number| self.blocks_by_number.get(&block_number)).unwrap_or(Ok(None)),
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and reduces nesting by using a more concise and idiomatic Rust approach. It eliminates the need for an explicit if-let statement, making the code more maintainable.

    7

    @carneiro-cw carneiro-cw changed the base branch from main to breaking-enha-rocksdb December 10, 2024 20:47
    @carneiro-cw carneiro-cw merged commit 946dbf8 into breaking-enha-rocksdb Dec 10, 2024
    34 checks passed
    @carneiro-cw carneiro-cw deleted the save_analysed_bytecode branch December 10, 2024 20:48
    marcospb19-cw pushed a commit that referenced this pull request Dec 15, 2024
    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