Skip to content

Conversation

@GabrielePicco
Copy link
Collaborator

@GabrielePicco GabrielePicco commented Oct 2, 2025

Greptile Overview

Updated On: 2025-10-02 10:45:46 UTC

Summary

This hotfix addresses a memory optimization issue in the account cloning system by preventing the cloning and caching of non-existent accounts. The changes introduce a new `DoesNotExist` variant to the `AccountClonerUnclonableReason` enum and implement logic to detect when accounts don't exist on-chain (empty system accounts with 0 lamports owned by the system program).

The implementation adds an early exit in the cloning logic that checks if an account is essentially empty (0 lamports + system program ownership) and marks it as unclonable rather than processing it. Additionally, the caching mechanism is updated to exclude these non-existent accounts from being stored in the cache, which should reduce memory usage and prevent unnecessary storage of default/empty account states.

This change fits well within the existing account cloning architecture, leveraging the established AccountClonerUnclonableReason enum pattern to categorize why certain accounts cannot be cloned. The modification maintains the existing API contract while adding more intelligent handling of edge cases that can occur when the RPC returns default system account data for accounts that don't actually exist on-chain.

Important Files Changed

Changed Files
Filename Score Overview
magicblock-account-cloner/src/account_cloner.rs 5/5 Added new DoesNotExist enum variant to categorize non-existent accounts
magicblock-account-cloner/src/remote_account_cloner_worker.rs 4/5 Implemented logic to detect and skip cloning of non-existent accounts, plus caching exclusion

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it adds defensive logic without breaking existing functionality
  • Score reflects well-structured changes that follow existing patterns, though the u64::MAX slot value could be more explicit
  • Pay close attention to the remote worker implementation to ensure the non-existence detection logic is robust

Sequence Diagram

sequenceDiagram
    participant User
    participant RemoteAccountClonerWorker
    participant AccountFetcher
    participant AccountUpdates
    participant AccountDumper
    participant InternalAccountProvider
    participant Cache

    User->>RemoteAccountClonerWorker: "clone_account(pubkey)"
    RemoteAccountClonerWorker->>RemoteAccountClonerWorker: "can_clone() check permissions"
    
    alt cloning not allowed
        RemoteAccountClonerWorker->>User: "Unclonable: NoCloningAllowed"
    else cloning allowed
        RemoteAccountClonerWorker->>AccountUpdates: "get_last_known_update_slot(pubkey)"
        AccountUpdates->>RemoteAccountClonerWorker: "last_update_slot"
        
        RemoteAccountClonerWorker->>Cache: "get_last_clone_output_from_pubkey(pubkey)"
        Cache->>RemoteAccountClonerWorker: "cached_output (optional)"
        
        alt cache hit and recent
            RemoteAccountClonerWorker->>User: "return cached output"
        else cache miss or stale
            RemoteAccountClonerWorker->>InternalAccountProvider: "get_account(pubkey)"
            InternalAccountProvider->>RemoteAccountClonerWorker: "local_account (optional)"
            
            RemoteAccountClonerWorker->>RemoteAccountClonerWorker: "do_clone(pubkey, stage)"
            RemoteAccountClonerWorker->>RemoteAccountClonerWorker: "check blacklist"
            
            alt account blacklisted
                RemoteAccountClonerWorker->>User: "Unclonable: IsBlacklisted"
            else account not blacklisted
                RemoteAccountClonerWorker->>AccountFetcher: "fetch_account_chain_snapshot(pubkey)"
                AccountFetcher->>RemoteAccountClonerWorker: "account_chain_snapshot"
                
                alt account does not exist (empty system account)
                    RemoteAccountClonerWorker->>RemoteAccountClonerWorker: "check if lamports=0 and owner=system_program"
                    RemoteAccountClonerWorker->>User: "Unclonable: DoesNotExist"
                else account exists
                    alt account is FeePayer
                        RemoteAccountClonerWorker->>AccountDumper: "dump_feepayer_account(pubkey, lamports, owner)"
                        AccountDumper->>RemoteAccountClonerWorker: "signature"
                    else account is Undelegated
                        RemoteAccountClonerWorker->>AccountDumper: "dump_undelegated_account(pubkey, account)"
                        AccountDumper->>RemoteAccountClonerWorker: "signature"
                    else account is Delegated
                        RemoteAccountClonerWorker->>AccountDumper: "dump_delegated_account(pubkey, account, owner)"
                        AccountDumper->>RemoteAccountClonerWorker: "signature"
                    end
                    
                    RemoteAccountClonerWorker->>RemoteAccountClonerWorker: "do_clone_and_update_cache()"
                    
                    alt should_cache (not DoesNotExist)
                        RemoteAccountClonerWorker->>Cache: "insert(pubkey, clone_output)"
                    end
                    
                    RemoteAccountClonerWorker->>User: "Cloned: {account_chain_snapshot, signature}"
                end
            end
        end
    end
Loading

@github-actions
Copy link

github-actions bot commented Oct 2, 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

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 even though I'm not sure how we'd ever encounter the case that this is fixing as I explained in my comment.

DoesNotHaveDelegatedEscrowAccount,
DoesNotAllowEscrowedPda,
DoesNotAllowFeepayerWithEscrowedPda,
/// The account does not exist on-chain (RPC returned empty/system default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you verify that the RPC returns this? I thought it returns some user error if the account is not found or None see here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@GabrielePicco GabrielePicco merged commit d41dd8f into master Oct 3, 2025
8 of 9 checks passed
@GabrielePicco GabrielePicco deleted the hotfix/no-change-not-existing-accounts branch October 3, 2025 07:14
thlorenz added a commit that referenced this pull request Oct 6, 2025
* thlorenz/chainlink: (59 commits)
  fix: sync accountsdb slot after ledger replay
  fix: for failed txns use meta defaults that are explorer compatible
  chore: update progress
  chore: rename test to proper name
  chore: allow overriding ephem port while running single test
  chore: all non-skipped ledger tests passing in parallel
  chore: true random port picking for ledger tests
  chore: initial changes to allow running ledger tests in parallel
  feat: ledger test validators start at different ports
  chore: fix compile issue due to merge
  chore: fix merge issues for test-integration
  chore: fix more merge issues to build validator
  chore: fix merge conflicts in cargo lock
  chore: ignore undelegate between restarts test for now
  chore: fix resume strategies test
  chore: fix restore ledger with new keypair test
  chore: fix timestamp ledger test
  chore: fix ledger account flush test
  fix: allow users to directly set AccountMeta(shorter version) for base layer action (#561)
  Hotfix: don't clone and cache not existing accounts (#560)
  ...
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.

4 participants