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

[PLA-2154] Block processor issue #320

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

leonardocustodio
Copy link
Contributor

@leonardocustodio leonardocustodio commented Jan 28, 2025

PR Type

Bug fix


Description

  • Fixed incorrect variable usage in block syncing logic.

  • Added exception handling with RestartIngestException.

  • Improved logging for block synchronization process.


Changes walkthrough 📝

Relevant files
Bug fix
BlockProcessor.php
Fix and enhance block synchronization logic                           

src/Services/Processor/Substrate/BlockProcessor.php

  • Replaced lastBlockSynced with lastSyncedHeight for clarity.
  • Added RestartIngestException to method documentation.
  • Updated logging messages for better debugging.
  • Adjusted logic to use the correct variable in block processing.
  • +7/-4     

    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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Possible Issue

    The variable $lastSyncedHeight is used to replace $lastBlockSynced, but its calculation defaults to 0 if no block is found. This might lead to unintended behavior if no blocks are synced yet. Verify if this logic is correct and handles edge cases appropriately.

    $lastSyncedHeight = $this->lastSyncedBlock()?->number ?? 0;
    $blockBeforeSubscription = HexConverter::hexToUInt($heightHexed) - 1;
    $this->warn("Last block synced: {$lastSyncedHeight}");
    $this->warn("Block before subscription: {$blockBeforeSubscription}");
    
    if ($blockBeforeSubscription > $lastSyncedHeight) {
        $this->warn('Processing blocks left behind');
        $this->fetchPastHeads($lastSyncedHeight, $blockBeforeSubscription);
    Exception Handling

    The @throws RestartIngestException annotation is added, but there is no clear indication in the code where this exception might be thrown. Ensure that this is either implemented or the annotation is removed.

    /**
     * @throws RestartIngestException
     */

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate input before hexadecimal conversion

    Validate that $heightHexed is a valid hexadecimal string before converting it with
    HexConverter::hexToUInt to prevent potential runtime errors.

    src/Services/Processor/Substrate/BlockProcessor.php [72]

    +if (!ctype_xdigit($heightHexed)) {
    +    throw new InvalidArgumentException("Invalid hexadecimal string: {$heightHexed}");
    +}
     $blockBeforeSubscription = HexConverter::hexToUInt($heightHexed) - 1;
    Suggestion importance[1-10]: 9

    Why: Validating the input before conversion prevents potential runtime errors and ensures robustness, addressing a critical issue that could lead to unexpected failures.

    9
    General
    Handle cases with no blocks left

    Ensure that the fetchPastHeads method handles cases where $lastSyncedHeight is
    greater than $blockBeforeSubscription to avoid unexpected behavior or errors.

    src/Services/Processor/Substrate/BlockProcessor.php [76-79]

     if ($blockBeforeSubscription > $lastSyncedHeight) {
         $this->warn('Processing blocks left behind');
         $this->fetchPastHeads($lastSyncedHeight, $blockBeforeSubscription);
         $this->warn('Finished processing blocks left behind');
    +} else {
    +    $this->warn('No blocks left behind to process');
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion adds a check for cases where no blocks are left to process, improving code clarity and handling edge cases. However, the impact is moderate as it does not address a critical bug or issue.

    7

    @leonardocustodio leonardocustodio merged commit e65743f into master Jan 28, 2025
    7 checks passed
    @leonardocustodio leonardocustodio deleted the bugfix/PLA-2154/block-issue branch January 28, 2025 17:38
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants