-
Notifications
You must be signed in to change notification settings - Fork 2k
bump(revm
: step 2): bump alloy
+ revm
+ alloy-evm
+ other deps to latest
#10454
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
base: zerosnacks/revm-bump-2
Are you sure you want to change the base?
Conversation
apply patches for block-explorers and compilers
Use ChaChaRng as temporary measure since proptest is on rand 8
// Prop test uses rand 8 whereas alloy-core has been bumped to rand 9 | ||
// self.test_runner().rng() | ||
self.rng.get_or_insert_with(|| match self.config.seed { | ||
Some(seed) => ChaChaRng::from_seed(seed.to_be_bytes::<32>()), | ||
None => ChaChaRng::from_os_rng(), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds a temp rng field to Cheatcodes
to generate random values with the given seed
This is a workaround as proptest still uses rand 8.
lgtm! RE: rand 8 / rand 9
if we highlight this in the changelog this is fine |
crates/evm/core/src/backend/mod.rs
Outdated
// while active_journaled_state.journal.len() > target_fork.journaled_state.journal.len() { | ||
// target_fork.journaled_state.journal.push(Default::default()); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Revm 21+ flattens the journal
field to Vec<JournalEntry
from Vec<Vec<JournalEntry>>
Ref: https://github.com/bluealloy/revm/pull/2440/files
I'm trying to understand whether this has side effects for our multiforking
Do you remember the context of this code block @mattsse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, unsure previously forking messed up internal tracking causing unwrap panics
I think we no longer need this because this now tracks the entire journal and not nested by calls @rakita ?
so we can try removing this @zerosnacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimistically removed here: 5244a40
alloy
+ revm
+ alloy-evm
to latest
alloy
+ revm
+ alloy-evm
to latestrevm
: 2): bump alloy
+ revm
+ alloy-evm
to latest
revm
: 2): bump alloy
+ revm
+ alloy-evm
to latestrevm
: step 2): bump alloy
+ revm
+ alloy-evm
to latest
revm = { git = "https://github.com/bluealloy/revm.git", rev = "b5808253" } | ||
op-revm = { git = "https://github.com/bluealloy/revm.git", rev = "b5808253" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently pins to bluealloy/revm#2512 that we rely on, not yet included in a 23.* release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool,
some nits, regarding the journal change, I believe this okay if we remove the mocking which previously was required to prevent panics (due to unwraps in revm since swapping journal violated some invariats)
crates/evm/core/src/backend/mod.rs
Outdated
// while active_journaled_state.journal.len() > target_fork.journaled_state.journal.len() { | ||
// target_fork.journaled_state.journal.push(Default::default()); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, unsure previously forking messed up internal tracking causing unwrap panics
I think we no longer need this because this now tracks the entire journal and not nested by calls @rakita ?
so we can try removing this @zerosnacks
@@ -146,7 +146,7 @@ pub fn configure_tx_req_env( | |||
|
|||
// Type 4, EIP-7702 | |||
if let Some(authorization_list) = authorization_list { | |||
env.tx.authorization_list = authorization_list.clone(); | |||
env.tx.set_signed_authorization(authorization_list.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we also need to update the txtype of the tx?
not obvious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make sense that it would
Perhaps something along these lines? yash/revm-alloy-bumps...zerosnacks/derive-tx-type-during-tx-configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @klkvr on the above
…nal tracking, tests do not indicate it is longer required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, left couple of nits / questions
…f new context requirement for bytes but we refactor resolved this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending @mattsse comment to be clarified #10454 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending @klkvr
revm
: step 2): bump alloy
+ revm
+ alloy-evm
to latestrevm
: step 2): bump alloy
+ revm
+ alloy-evm
+ other deps to latest
Motivation
Builds on #10183
Bumps
Gas::memory
field bluealloy/revm#2512)Unblocks #10411
Solution
Previously used the following patches, no longer necessary:
PR Checklist