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

fix(op): Base Goerli op-reth sync patches #824

Merged
merged 9 commits into from
Oct 25, 2023

Conversation

clabby
Copy link
Contributor

@clabby clabby commented Oct 21, 2023

Overview

Contains several fixes for the Optimism state transition code.

Context

After hooking up the OP execution changes to op-reth and performing a sync test on the Base Goerli network, a few bugs were found.

Patches

  1. The Optimism EL has a critical invariant that deposit transactions may never fail. This implies that even upon a total failure of the state transition, for any reason, the mint value should be persisted to the sender account's balance to prevent loss of funds, and the sender's nonce should be bumped as well.
    1. Solution: Pull out the default implementation of the transact function in the Transact trait, and define a catch-all failure handler that persists the mint value to and bumps the nonce of the caller account before returning a special Halt variant.
  2. When a deposit Halts, a few special rules apply. These happen to be identical to the logic in the above catch-all.
    1. Solution: Wipe the state of the journal and fall back to the catch-all that persists the mint value and bumps the nonce of the caller. This will return a Halt::FailedDeposit status.

Reference Branch (cherry-picked #781 onto 3.5.0 for integrating w/ op-reth: https://github.com/anton-rs/op-revm/tree/cl/patch-op)

@clabby clabby marked this pull request as ready for review October 21, 2023 18:16
Copy link
Collaborator

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

changes lgtm, pending @rakita

crates/revm/src/handler.rs Outdated Show resolved Hide resolved
self.preverify_transaction()
.and_then(|()| self.transact_preverified())
}
fn transact(&mut self) -> EVMResult<DBError>;
Copy link
Member

Choose a reason for hiding this comment

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

Should stay as this and logic should be changes for used function.

{
crate::handler::optimism::default_transact(self)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This should not go here, but inside transact_preverified logic will be skipped otherwise.

@rakita
Copy link
Member

rakita commented Oct 23, 2023

@clabby if you are okay with it, to speed things up i can create the PR on this branch

@rakita
Copy link
Member

rakita commented Oct 25, 2023

failed test unrelated to PR

@rakita rakita merged commit f1f174e into bluealloy:main Oct 25, 2023
7 of 8 checks passed
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
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