Skip to content

Conversation

@taco-paco
Copy link
Contributor

@taco-paco taco-paco commented Aug 18, 2025

At the moment transaction execution is protected b y read lock, which allows different threads execute transactions on the same writable accounts at the same time. This leads to invalid account states.

Example: RPC executes TX on magic_context & at the same time slot-ticker executes AcceptScheduleCommits tx in magic_context.
outcome: Even though accept was executed - MagicContext still contains just accepted Commits

Greptile Summary

This PR addresses a critical race condition in transaction execution by changing the synchronization mechanism in the bank's transaction processing. The change modifies the lock acquisition in load_and_execute_sanitized_transactions from a read lock to a write lock on the transaction_processor.

The issue stems from the current implementation allowing multiple threads to concurrently execute transactions that modify the same writable accounts. This is particularly problematic for the MagicBlock validator's magic_context account, which manages scheduled commits. The race condition occurs when, for example, an RPC request executes a transaction on magic_context while simultaneously the slot-ticker executes an AcceptScheduleCommits transaction on the same account. Even though both transactions might appear to succeed individually, the concurrent execution leads to invalid account states where committed changes can be lost or overwritten.

The fix ensures proper serialization of transaction execution by requiring exclusive access during the execution phase. This prevents multiple threads from simultaneously modifying the same accounts, maintaining data consistency and preventing the loss of scheduled commits or other account state corruption. The change aligns with the broader architecture where the validator manages complex state transitions through scheduled commits and account delegations.

PR Description Notes:

  • Minor typo: "protected b y read lock" should be "protected by read lock"

Confidence score: 5/5

  • This PR is extremely safe to merge with minimal risk of causing production issues
  • Score reflects a simple, well-targeted fix that addresses a clear concurrency bug with a straightforward solution
  • No files require special attention as the change is localized and the fix is architecturally sound

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.

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

Copy link
Contributor

@bmuddha bmuddha 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 9cccb57 into master Aug 18, 2025
4 checks passed
@taco-paco taco-paco deleted the fix/transaction-executor/race-condition branch August 18, 2025 09:15
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