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

chore: bump revm #6281

Merged
merged 15 commits into from
Nov 16, 2023
Merged

chore: bump revm #6281

merged 15 commits into from
Nov 16, 2023

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Nov 10, 2023

bump revm to same rev as reth
fix breaking changes:

  • _ref suffix for DatabaseRef
  • SharedMemory
  • Inspector trait changes

the revm bump includes a fix for op transactions ref #6073

https://github.com/bluealloy/revm/blob/1609e07c68048909ad1682c98cf2b9baa76310b5/crates/revm/src/evm_impl.rs#L262

FYI @anikaraghu

@mattsse mattsse mentioned this pull request Nov 10, 2023
@mattsse
Copy link
Member Author

mattsse commented Nov 10, 2023

snekmate error unrelated I believe #6277

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

LGTM. 1 nit + I don't really like the memory field rename given its API is the same. Will raise a complaint with management on this.

crates/evm/traces/src/inspector.rs Outdated Show resolved Hide resolved
@mattsse
Copy link
Member Author

mattsse commented Nov 10, 2023

Will raise a complaint with management on this.

:D while you're at it, consider moving this to revm:

https://github.com/paradigmxyz/reth/blob/e26bc175316a77038900dd9d198608d300b7a545/crates/revm/revm-inspectors/src/tracing/types.rs#L627-L631

can also rename to Memory, I didn't bother with this in this PR, but this will only clone the context memory and not the entire SharedMemory

crates/evm/evm/src/inspectors/stack.rs Show resolved Hide resolved
crates/evm/traces/src/lib.rs Show resolved Hide resolved
crates/evm/traces/src/inspector.rs Outdated Show resolved Hide resolved
@DaniPopes
Copy link
Member

DaniPopes commented Nov 10, 2023

Don't think it's unrelated (snekmate)

2023-11-10T15:44:49.5234315Z Failing tests:
2023-11-10T15:44:49.5235019Z Encountered 2 failing tests in test/utils/Multicall.t.sol:MulticallTest
2023-11-10T15:44:49.5236405Z [FAIL. Reason: call reverted as expected, but without data] testMulticallSelfRevert() (gas: 8937393460525242070)
2023-11-10T15:44:49.5237849Z [FAIL. Reason: EvmError: Revert] testMulticallSelfSuccess() (gas: 8937393460525241948)

@mattsse
Copy link
Member Author

mattsse commented Nov 10, 2023

@DaniPopes
Copy link
Member

DaniPopes commented Nov 10, 2023

No : #6280

@DaniPopes
Copy link
Member

DaniPopes commented Nov 10, 2023

Don't think it's unrelated (snekmate)

2023-11-10T15:44:49.5234315Z Failing tests:
2023-11-10T15:44:49.5235019Z Encountered 2 failing tests in test/utils/Multicall.t.sol:MulticallTest
2023-11-10T15:44:49.5236405Z [FAIL. Reason: call reverted as expected, but without data] testMulticallSelfRevert() (gas: 8937393460525242070)
2023-11-10T15:44:49.5237849Z [FAIL. Reason: EvmError: Revert] testMulticallSelfSuccess() (gas: 8937393460525241948)

Maybe an overflow somewhere, I'm getting 1090162555 gas for testMulticallSelfRevert on master

These functions have a lot of recursion, might be a bug in revm

@mattsse
Copy link
Member Author

mattsse commented Nov 10, 2023

hmm, yeah this fails due to a memmory OOG, so I must have introduced recursion somewhere,

@mattsse mattsse removed the request for review from onbjerg November 14, 2023 14:36
Copy link
Member Author

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

we're back

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

let's send ittt

@DaniPopes DaniPopes merged commit 3e12d88 into foundry-rs:master Nov 16, 2023
20 checks passed
@pcaversaccio
Copy link
Contributor

FYI, since bluealloy/revm#865 is not yet fixed and I keep pushing to my main snekmate branch, I will ignore these specific failures in my own CI pipeline, eg pcaversaccio/snekmate@e2e2afe. Just in case you directly rely in any form on a green status of snekmate's main branch.

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