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

chore: move to alloy pt4 #1995

Merged
merged 21 commits into from
Feb 3, 2025
Merged

chore: move to alloy pt4 #1995

merged 21 commits into from
Feb 3, 2025

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Jan 31, 2025

PR Type

Enhancement, Bug fix


Description

  • Migrate to Alloy types for improved performance

  • Fix transaction handling in block processing

  • Enhance error handling and validation

  • Update type conversions for compatibility


Changes walkthrough 📝

Relevant files
Enhancement
10 files
alias.rs
Update Alloy type aliases and remove unused Ethers type   
+1/-2     
importer_offline.rs
Improve block and transaction handling with Alloy types   
+23/-3   
executor.rs
Update transaction execution for Alloy compatibility         
+1/-1     
importer.rs
Enhance block and transaction processing with Alloy types
+30/-4   
block_header.rs
Update BlockHeader conversion for Alloy compatibility       
+18/-18 
difficulty.rs
Add conversion from AlloyUint256 to Difficulty                     
+7/-0     
external_block.rs
Migrate ExternalBlock to use AlloyBlockExternalTransaction
+7/-7     
hash.rs
Add conversion from B256 to Hash                                                 
+6/-0     
logs_bloom.rs
Add conversion from AlloyBloom to LogsBloom                           
+7/-0     
size.rs
Add conversion from AlloyUint256 to Size                                 
+7/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    github-actions bot commented Jan 31, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 50bc209)

    Here are some key observations to aid the review process:

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

    Error Handling

    The new code for handling BlockTransactions::Full might benefit from more robust error handling and logging. Consider adding more context to the error messages and ensuring proper error propagation.

    if let BlockTransactions::Full(txs) = &mut block.transactions {
        txs.iter_mut().for_each(ExternalTransaction::fill_missing_transaction_type);
    } else {
        return log_and_err!(GlobalState::shutdown_from(TASK_NAME, "expected full transactions, got hashes or uncle"));
    }
    Transaction Validation

    The new code for sorting and validating transactions and receipts could be refactored into a separate function for better readability and reusability. Also, consider adding more detailed logging for mismatched transaction and receipt lengths.

    let block_number = block.number();
    let transactions = if let BlockTransactions::Full(txs) = &mut block.transactions {
        Ok(txs)
    } else {
        Err(anyhow!("expected full transactions, got hashes or uncle"))
    }?;
    
    if transactions.len() != receipts.len() {
        return Err(anyhow!(
            "block {} has mismatched transaction and receipt length: {} transactions but {} receipts",
            block_number,
            transactions.len(),
            receipts.len()
        ));
    }
    
    // Stably sort transactions and receipts by transaction_index
    transactions.sort_by(|a, b| a.transaction_index.cmp(&b.transaction_index));
    receipts.sort_by(|a, b| a.transaction_index.cmp(&b.transaction_index));
    
    // perform additional checks on the transaction index
    for window in transactions.windows(2) {
        let tx_index = window[0].transaction_index.ok_or(anyhow!("missing transaction index"))?.as_u32();
        let next_tx_index = window[1].transaction_index.ok_or(anyhow!("missing transaction index"))?.as_u32();
        if tx_index + 1 != next_tx_index {

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle all BlockTransactions variants

    Replace the TODO comment with a proper implementation for handling different
    BlockTransactions variants. Consider how to handle cases where transactions are not
    full or when dealing with uncles.

    src/eth/follower/importer/importer.rs [440-443]

    -// Extract transactions into a vector we can sort
     let transactions = match &mut block.transactions {
         BlockTransactions::Full(txs) => txs,
    -    _ => anyhow::bail!("Expected full transactions, got hashes or uncle"),
    +    BlockTransactions::Hashes(hashes) => {
    +        // Fetch full transactions using hashes
    +        // This is a placeholder and needs to be implemented
    +        return Err(anyhow::anyhow!("Fetching full transactions from hashes not implemented"));
    +    }
    +    BlockTransactions::Uncle => {
    +        // Handle uncle blocks appropriately
    +        return Err(anyhow::anyhow!("Uncle blocks not supported in this context"));
    +    }
     };
    Suggestion importance[1-10]: 8

    Why: This suggestion significantly improves the code by handling all variants of BlockTransactions, not just the Full variant. It provides placeholders for handling Hashes and Uncle variants, which is crucial for comprehensive error handling and future extensibility.

    8
    Improve error handling for transactions

    Implement proper error handling for the case when BlockTransactions is not Full.
    Consider propagating this error upwards or providing a meaningful fallback behavior.

    src/bin/importer_offline.rs [243-248]

    -if let BlockTransactions::Full(txs) = &mut block.transactions {
    -    // TODO: improve before merging
    -    txs.iter_mut().for_each(ExternalTransaction::fill_missing_transaction_type);
    -} else {
    -    anyhow::bail!("Expected full transactions, got hashes or uncle");
    +match &mut block.transactions {
    +    BlockTransactions::Full(txs) => {
    +        txs.iter_mut().for_each(ExternalTransaction::fill_missing_transaction_type);
    +    }
    +    _ => return Err(anyhow::anyhow!("Expected full transactions, got hashes or uncle")),
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by using a match statement instead of if-let, providing a more robust way to handle different BlockTransactions variants. It also removes the TODO comment, making the code more production-ready.

    7
    General
    Finalize BlockHeader conversion implementation

    Remove the TODO comment and ensure that all fields are correctly mapped from
    ExternalBlock to BlockHeader. Verify that the field names and types match between
    the two structs.

    src/eth/primitives/block_header.rs [176-200]

    -// TODO: improve before merging
     impl TryFrom<&ExternalBlock> for BlockHeader {
         type Error = anyhow::Error;
         fn try_from(value: &ExternalBlock) -> Result<Self, Self::Error> {
             Ok(Self {
                 number: BlockNumber::try_from(value.0.header.inner.number)?,
                 hash: Hash::from(value.0.header.hash),
                 transactions_root: Hash::from(value.0.header.inner.transactions_root),
                 gas_used: Gas::try_from(value.0.header.inner.gas_used)?,
                 gas_limit: Gas::try_from(value.0.header.inner.gas_limit)?,
                 bloom: LogsBloom::from(value.0.header.inner.logs_bloom),
                 timestamp: UnixTime::from(value.0.header.inner.timestamp),
                 parent_hash: Hash::from(value.0.header.inner.parent_hash),
                 author: Address::from(value.0.header.inner.beneficiary),
                 extra_data: Bytes::from(value.0.header.inner.extra_data.clone()),
                 miner: Address::from(value.0.header.inner.beneficiary),
                 difficulty: Difficulty::from(value.0.header.inner.difficulty),
                 receipts_root: Hash::from(value.0.header.inner.receipts_root),
                 uncle_hash: Hash::from(value.0.header.inner.ommers_hash),
                 size: Size::try_from(value.0.header.size.unwrap_or_default())?,
                 state_root: Hash::from(value.0.header.inner.state_root),
                 total_difficulty: Difficulty::from(value.0.header.total_difficulty.unwrap_or_default()),
                 nonce: MinerNonce::from(value.0.header.inner.nonce.0),
             })
         }
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion removes the TODO comment and finalizes the implementation. However, it doesn't introduce any significant changes to the existing code, only removing the comment. While it's good to clean up TODOs, the impact is relatively minor.

    6

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /benchmark

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /describe

    Copy link

    PR Description updated to latest commit (50bc209)

    @gabriel-aranha-cw gabriel-aranha-cw marked this pull request as ready for review January 31, 2025 17:48
    Copy link

    Persistent review updated to latest commit 50bc209

    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Benchmark:
    Run ID: bench-2272520371

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1946.00, Min: 1254.00, Avg: 1749.79, StdDev: 79.21
    TPS Stats: Max: 1944.00, Min: 1579.00, Avg: 1698.20, StdDev: 91.41

    Plot: View Plot

    @gabriel-aranha-cw gabriel-aranha-cw merged commit 89d8ea8 into main Feb 3, 2025
    38 checks passed
    @gabriel-aranha-cw gabriel-aranha-cw deleted the chore-move-to-alloy-pt4 branch February 3, 2025 12:20
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Final benchmark:
    Run ID: bench-585153546

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1886.00, Min: 1032.00, Avg: 1715.73, StdDev: 87.67
    TPS Stats: Max: 1868.00, Min: 1514.00, Avg: 1665.94, StdDev: 92.92

    Plot: View Plot

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    #1963

    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.

    2 participants