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: check transaction index when importing block #1909

Merged

Conversation

marcospb19-cw
Copy link
Contributor

@marcospb19-cw marcospb19-cw commented Dec 11, 2024

User description

when importing block from another node, and when importing-offline a block from postgres


PR Type

Enhancement


Description

  • Enhanced the block import process by adding transaction index validation:
    • Implemented sorting of transactions and receipts by transaction_index in both offline importer and regular importer
    • Added checks to ensure consecutive transaction indices in blocks and receipts
    • Introduced error logging for non-consecutive transaction indices in the regular importer
  • These changes improve data integrity and help detect potential issues during the block import process
  • The modifications affect both the offline importer (importer_offline.rs) and the regular importer (importer.rs)

Changes walkthrough 📝

Relevant files
Enhancement
importer_offline.rs
Enhance block import with transaction index validation     

src/bin/importer_offline.rs

  • Added sorting of transactions and receipts by transaction_index
  • Implemented checks for consecutive transaction indices
  • Modified the fetch_blocks_and_receipts function to include these new
    checks
  • +29/-1   
    importer.rs
    Improve importer with transaction index checks                     

    src/eth/follower/importer/importer.rs

  • Added sorting of transactions and receipts by transaction_index
  • Implemented checks for consecutive transaction indices
  • Added error logging for non-consecutive transaction indices
  • +21/-1   

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

    @marcospb19-cw marcospb19-cw enabled auto-merge (squash) December 11, 2024 16:55
    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 function uses assert! for validation, which may cause the program to panic. Consider using a more graceful error handling approach, such as returning a Result with a custom error type.

    Performance Concern
    Sorting transactions and receipts for every block might impact performance, especially for large blocks. Consider if this sorting is necessary or if it can be optimized.

    Error Handling
    The function logs errors for non-consecutive indices but continues execution. Consider if this should halt the import process or if additional error handling is needed.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Replace assertions with error handling to improve robustness and prevent panics

    Instead of using assert!, consider using a more graceful error handling approach.
    This will prevent the program from panicking and allow for better error reporting
    and recovery.

    src/bin/importer_offline.rs [304-309]

    -assert!(
    -    tx_index + 1 == next_tx_index,
    -    "two consecutive transactions must have consecutive indices: {} and {}",
    -    tx_index,
    -    next_tx_index,
    -);
    +if tx_index + 1 != next_tx_index {
    +    return Err(anyhow::anyhow!("Inconsistent transaction indices: {} and {}", tx_index, next_tx_index));
    +}
    Suggestion importance[1-10]: 8

    Why: The suggestion improves error handling by replacing assertions with a more graceful approach. This change enhances the robustness of the code by preventing panics and allowing for better error reporting and recovery.

    8
    Use Option type for missing transaction indices to improve safety and correctness

    Instead of using u32::MAX as a default value for missing transaction indices,
    consider using Option::None to represent missing indices more accurately and safely.

    src/eth/follower/importer/importer.rs [450-451]

    -let tx_index = window[0].transaction_index.map_or(u32::MAX, |index| index.as_u32());
    -let next_tx_index = window[1].transaction_index.map_or(u32::MAX, |index| index.as_u32());
    +let tx_index = window[0].transaction_index.map(|index| index.as_u32());
    +let next_tx_index = window[1].transaction_index.map(|index| index.as_u32());
    +if let (Some(tx_index), Some(next_tx_index)) = (tx_index, next_tx_index) {
    +    if tx_index + 1 != next_tx_index {
    +        tracing::error!(tx_index, next_tx_index, "two consecutive transactions must have consecutive indices");
    +    }
    +} else {
    +    tracing::error!("Missing transaction index");
    +}
    Suggestion importance[1-10]: 7

    Why: The suggestion enhances type safety by using Option instead of u32::MAX for missing indices. This change improves code correctness and prevents potential issues with unexpected maximum values.

    7

    @marcospb19-cw marcospb19-cw force-pushed the enha-check-transaction-index-when-importing-block branch from 6adc05f to 2375226 Compare December 11, 2024 17:00
    @marcospb19-cw marcospb19-cw merged commit 062fedd into main Dec 11, 2024
    34 checks passed
    @marcospb19-cw marcospb19-cw deleted the enha-check-transaction-index-when-importing-block branch December 11, 2024 17:17
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-3215293301

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1229.00, Min: 843.00, Avg: 1017.51, StdDev: 69.64
    TPS Stats: Max: 1199.00, Min: 843.00, Avg: 985.84, StdDev: 80.08

    Plot: View Plot

    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