Skip to content

fix: check fetch write race causes invalid unexpected accounts - #738

Closed
Arrowana wants to merge 1 commit into
solana-foundation:mainfrom
Arrowana:fix/check-fetch-write-race
Closed

fix: check fetch write race causes invalid unexpected accounts#738
Arrowana wants to merge 1 commit into
solana-foundation:mainfrom
Arrowana:fix/check-fetch-write-race

Conversation

@Arrowana

@Arrowana Arrowana commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

check–fetch–write race: Surfpool checked local state, released the lock for the mainnet request, then later wrote that stale response unconditionally. A transaction could update the account during that gap.

The fix:

  • Remote fetches run without holding locks.
  • Responses are merged under a short write lock.
  • Local state always wins; remote data only hydrates accounts still missing.
  • Token mint and program-data companion accounts are merged independently.
  • DB-backed accounts are materialized atomically.

Background: I was testing my own system and observed very bizarre behaviour, despite a tx mutating a given account i would observe the older state afterwards, which is a revert to mainnet state.

@greptile-apps

greptile-apps Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR changes remote account hydration so network requests run without SVM locks and fetched responses are merged under a write lock, preserving local and DB-backed state.

  • Adds contextualized write-lock access for atomic account materialization.
  • Rechecks requested and companion accounts before inserting remote data.
  • Adds coverage for preserving local requested-account and companion-account state.

Confidence Score: 4/5

The PR is not yet safe to merge because a fetch completed after an account becomes offline can still expose the blocked remote state.

The hydration guard prevents the fetched account from being inserted after it becomes offline, but the final fallback returns the same remote account when the local re-read remains empty, and both single- and multi-account lookup paths can expose that value.

Files Needing Attention: crates/core/src/surfnet/svm.rs, crates/core/src/surfnet/locker.rs

Important Files Changed

Filename Overview
crates/core/src/surfnet/locker.rs Moves remote-response merging into short contextualized write-lock sections and materializes DB-backed local reads atomically.
crates/core/src/surfnet/svm.rs Adds conditional remote hydration that preserves existing requested and companion accounts, with tests across configured storage backends.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Locker
    participant Remote
    participant SVM
    Caller->>Locker: Read account
    Locker->>SVM: Check local state
    SVM-->>Locker: Missing
    Locker->>Remote: Fetch without lock
    Remote-->>Locker: Remote account
    Locker->>SVM: Acquire write lock and hydrate
    SVM->>SVM: Recheck local and DB-backed state
    alt Local state exists
        SVM-->>Locker: Return local state
    else Account remains eligible for hydration
        SVM->>SVM: Insert remote state
        SVM-->>Locker: Return hydrated state
    end
    Locker-->>Caller: Contextualized result
Loading

Reviews (2): Last reviewed commit: "fix: check fetch write race" | Re-trigger Greptile

Comment on lines +2425 to +2428
match self.inner.get_account_result(&requested_pubkey) {
Ok(local_result) if !local_result.is_none() => local_result,
_ => remote_read_result,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Blocked hydration returns remote state

When an account is marked offline while its remote fetch is in flight, hydrate_account_if_missing rejects insertion but this fallback still returns remote_read_result, causing the caller to observe remote state that the offline policy requires suppressing.

Knowledge Base Used: Surfnet Engine (SVM, Locker, Remote Fetch)

@Arrowana
Arrowana force-pushed the fix/check-fetch-write-race branch from 00afcbf to 55e9825 Compare August 5, 2026 03:12
@cds-amal

cds-amal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Drive by offering to say: good find! The progression here is exactly the right shape for this kind of race. It also feels like a good concurrency pattern to reuse elsewhere.

Following the "where there's one, there's three" rule, I don't think this is isolated. get_account_info, get_multiple_accounts, get_token_account_balance, get_token_supply, get_balance, simulate_transaction, and likely a few others all follow a similar check/fetch/write flow where data is fetched and then written back under a second lock. They could all benefit from routing through hydrate_account_update (or applying the same merge strategy).

That's follow-up work rather than this PR's scope, though. Nice fix!

@MicaiahReid

Copy link
Copy Markdown
Collaborator

This is a great find, @Arrowana! I had a hard time in reviewing this - there are so many calls to get_account, get_account_inner and seeming circular writes. This isn't your fault, it was the fault of my initial architecture of account writing.

I wanted to take a crack at cleaning that up a bit, so I've opened #763. It takes a policy-driven approach to inserts - there's a more centralized place to handle writes, and you have to decide ahead of time if this write is Authoritative, or if hydration should occur.

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.

3 participants