Skip to content

Conversation

@taco-paco
Copy link
Contributor

@taco-paco taco-paco commented Sep 25, 2025

In ExternalAccountsManager ignore nondelegated accounts during periodic commits
Additionally fixes sqlite INTEGER overflow

Greptile Overview

Updated On: 2025-09-25 12:32:37 UTC

Summary

This PR fixes a critical issue in the periodic commit process by preventing attempts to commit undelegated accounts, which was causing failures in the ExternalAccountsManager.

Key Changes:

  • Fixed MESSAGE_ID overflow: Changed initialization from u64::MAX - 1 to (i64::MAX - 1) as u64 to prevent potential overflow issues
  • Added delegation filter: Introduced .filter(|(_, _, _, acc)| acc.is_delegated()) in the periodic commit pipeline to only process accounts that are actually delegated to the validator
  • Enhanced error logging: Added intent ID to error messages in the intent execution engine for better debugging of failed executions
  • Dependency updates: Updated Cargo.lock with delegation program and async dependencies

Confidence Score: 4/5

  • This PR is safe to merge with careful attention to the delegation filter logic
  • Score reflects a well-targeted fix for a specific delegation issue with good intent ID logging improvement. The MESSAGE_ID fix addresses a potential overflow, and the delegation filter prevents inappropriate commit attempts. Only minor risk is ensuring the is_delegated() method correctly identifies delegation status
  • Pay attention to magicblock-accounts/src/external_accounts_manager.rs to ensure the delegation filter works correctly in all scenarios

Important Files Changed

File Analysis

Filename        Score        Overview
magicblock-accounts/src/external_accounts_manager.rs 4/5 Fixed MESSAGE_ID overflow issue and added delegation filter to prevent committing undelegated accounts during periodic commits
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs 5/5 Enhanced error logging by adding intent ID to error messages for better debugging of failed BaseIntent executions

Sequence Diagram

sequenceDiagram
    participant EAM as ExternalAccountsManager
    participant IAP as InternalAccountProvider
    participant Filter as DelegationFilter
    participant Committer as CommitService

    Note over EAM: Periodic commit cycle begins
    EAM->>EAM: create_scheduled_base_intents()
    EAM->>EAM: Initialize MESSAGE_ID with (i64::MAX - 1) as u64
    
    loop For each account to be committed
        EAM->>IAP: get_account(pubkey)
        IAP-->>EAM: AccountSharedData
        
        Note over EAM,Filter: NEW: Check delegation status
        EAM->>Filter: acc.is_delegated()
        Filter-->>EAM: true/false
        
        alt Account is delegated
            EAM->>EAM: Check if account hash changed
            alt Hash changed
                EAM->>EAM: Create AccountCommittee
                EAM->>Committer: Schedule for commit
            end
        else Account not delegated
            Note over EAM: Skip account (prevents undelegated commit attempts)
        end
    end
    
    EAM-->>Committer: List of delegated accounts to commit
Loading

@github-actions
Copy link

github-actions bot commented Sep 25, 2025

Manual Deploy Available

You can trigger a manual deploy of this PR branch to testnet:

Deploy to Testnet 🚀

Alternative: Comment /deploy on this PR to trigger deployment directly.

⚠️ Note: Manual deploy requires authorization. Only authorized users can trigger deployments.

Comment updated automatically when the PR is synchronized.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@taco-paco taco-paco changed the title fix: periodic committer attempts to commit undelegated accounts fix: periodic committer attempts to commit undelegated accounts & sqlite INTEGER overflow Sep 25, 2025
Copy link
Contributor

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for the delegation flag changes.

@taco-paco
Copy link
Contributor Author

@thlorenz Could you take a look?

Copy link
Contributor

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@taco-paco taco-paco merged commit d259367 into master Oct 10, 2025
6 checks passed
@taco-paco taco-paco deleted the fix/external-account-manager/ignore-nondelegated branch October 10, 2025 11:09
thlorenz added a commit that referenced this pull request Oct 13, 2025
* master:
  release: 0.2.3 (#571)
  fix: release package with CI (#570)
  release: 0.2.2 (#568)
  feat: add cli command to run the solana-test-validator with ER setup (#567)
  fix: periodic committer attempts to commit undelegated accounts & sqlite INTEGER overflow (#556)
  feat: schedule tasks (#493)
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.

5 participants