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: migrate from rlp to alloy-rlp for transaction decoding #2026

Closed
wants to merge 1 commit into from

Conversation

gabriel-aranha-cw
Copy link
Contributor

@gabriel-aranha-cw gabriel-aranha-cw commented Feb 21, 2025

User description

Update transaction input decoding to use alloy-rlp instead of the standalone rlp crate:

  • Replace rlp dependency with alloy-rlp in Cargo.toml
  • Update transaction input decoding implementation to use alloy-rlp's Decodable trait
  • Modify RPC RLP parsing to use the new decoding method
  • Simplify transaction envelope decoding logic

PR Type

Enhancement


Description

  • Replace rlp crate with alloy-rlp

  • Update transaction input decoding implementation

  • Modify RPC RLP parsing method

  • Simplify transaction envelope decoding logic


Changes walkthrough 📝

Relevant files
Enhancement
transaction_input.rs
Update TransactionInput decoding to use alloy-rlp               

src/eth/primitives/transaction_input.rs

  • Replace rlp with alloy-rlp for Decodable trait
  • Update TransactionInput decoding implementation
  • Modify error handling to use RlpError
  • Simplify transaction envelope decoding logic
  • +15/-16 
    rpc_parser.rs
    Update RPC RLP parsing to use alloy-rlp                                   

    src/eth/rpc/rpc_parser.rs

  • Replace rlp with alloy-rlp for Decodable import
  • Update parse_rpc_rlp function to use alloy-rlp's Decodable trait
  • +3/-2     
    Dependencies
    Cargo.toml
    Update dependencies to replace rlp with alloy-rlp               

    Cargo.toml

    • Remove rlp dependency
    • Add alloy-rlp dependency version 0.3.11
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Update transaction input decoding to use alloy-rlp instead of the standalone rlp crate:
    - Replace rlp dependency with alloy-rlp in Cargo.toml
    - Update transaction input decoding implementation to use alloy-rlp's Decodable trait
    - Modify RPC RLP parsing to use the new decoding method
    - Simplify transaction envelope decoding logic
    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /flamegraph

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    /benchmark

    Copy link

    PR Reviewer Guide 🔍

    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 implementation uses custom error messages with RlpError::Custom. Consider using more specific error types or adding more context to these errors for better debugging and error handling.

        .map_err(|_| RlpError::Custom("failed to convert transaction"))
    }
    
    if buf.is_empty() {
        return Err(RlpError::Custom("empty transaction bytes"));
    }
    
    let first_byte = buf[0];
    if first_byte >= 0xc0 {
        // Legacy transaction
        TxEnvelope::fallback_decode(buf)
            .map_err(|_| RlpError::Custom("failed to decode legacy transaction"))
            .and_then(convert_tx)
    } else {
        // Typed transaction (EIP-2718)
        let first_byte = buf[0];
        *buf = &buf[1..];
        TxEnvelope::typed_decode(first_byte, buf)
            .map_err(|_| RlpError::Custom("failed to decode transaction envelope"))
    Unused Variable

    The buf variable is mutated but its final state is not used. Consider removing the mut keyword if the mutation is not necessary.

    let mut buf = value;

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Ensure complete buffer consumption

    Handle the case where the buffer is not fully consumed after decoding. This ensures
    that the entire input is processed and no unexpected data remains.

    src/eth/primitives/transaction_input.rs [86-117]

     fn decode(buf: &mut &[u8]) -> Result<Self, RlpError> {
         fn convert_tx(envelope: TxEnvelope) -> Result<TransactionInput, RlpError> {
             TransactionInput::try_from(alloy_rpc_types_eth::Transaction {
                 inner: envelope,
                 block_hash: None,
                 block_number: None,
                 ...
             })
             .map_err(|_| RlpError::Custom("failed to convert transaction"))
         }
         
         if buf.is_empty() {
             return Err(RlpError::Custom("empty transaction bytes"));
         }
         
         let first_byte = buf[0];
    -    if first_byte >= 0xc0 {
    +    let result = if first_byte >= 0xc0 {
             // Legacy transaction
             TxEnvelope::fallback_decode(buf)
                 .map_err(|_| RlpError::Custom("failed to decode legacy transaction"))
                 .and_then(convert_tx)
         } else {
             // Typed transaction (EIP-2718)
             let first_byte = buf[0];
             *buf = &buf[1..];
             TxEnvelope::typed_decode(first_byte, buf)
                 .map_err(|_| RlpError::Custom("failed to decode transaction envelope"))
                 .and_then(convert_tx)
    +    };
    +    
    +    if !buf.is_empty() {
    +        return Err(RlpError::Custom("input buffer not fully consumed"));
         }
    +    
    +    result
     }
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses an important edge case where the input buffer is not fully consumed after decoding, which could lead to subtle bugs or security issues. It improves the robustness of the decoding process.

    Medium
    Check for complete buffer consumption

    Add error handling for the case where the buffer is not fully consumed after
    decoding, similar to the change suggested for the TransactionInput decoding.

    src/eth/rpc/rpc_parser.rs [80-85]

     pub fn parse_rpc_rlp<T: Decodable>(value: &[u8]) -> Result<T, RpcError> {
         let mut buf = value;
    -    match T::decode(&mut buf) {
    +    let result = T::decode(&mut buf);
    +    if !buf.is_empty() {
    +        return Err(RpcError::TransactionInvalid { decode_error: "input buffer not fully consumed".to_string() });
    +    }
    +    match result {
             Ok(trx) => Ok(trx),
             Err(e) => Err(RpcError::TransactionInvalid { decode_error: e.to_string() }),
         }
     }
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion adds an important check for complete buffer consumption in the RPC parsing function, similar to the previous suggestion. It enhances the reliability of the RPC decoding process and maintains consistency across the codebase.

    Medium

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Flamegraphs:
    Run ID: bench-304241857

    Git Info:

    Flamegraphs:
    idle:

    write:

    read:

    follower:

    @gabriel-aranha-cw
    Copy link
    Contributor Author

    Benchmark:
    Run ID: bench-2312284310

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    Leader Stats:
    RPS Stats: Max: 1862.00, Min: 1233.00, Avg: 1680.57, StdDev: 73.37
    TPS Stats: Max: 1838.00, Min: 1508.00, Avg: 1630.87, StdDev: 86.64

    Follower Stats:
    Imported Blocks/s: Max: 2.00, Min: 1.00, Avg: 1.58, StdDev: 0.49
    Imported Transactions/s: Max: 3616.00, Min: 229.00, Avg: 2653.91, StdDev: 840.91

    Plots:

    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