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

Cancun eth tests #1135

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from
Open

Cancun eth tests #1135

wants to merge 40 commits into from

Conversation

cabrador
Copy link
Collaborator

@cabrador cabrador commented Jul 18, 2024

Description

This PR adds creating BlockContext before transaction execution and allows passing dynamic ChainConfig to transaction execution.
Both of these new features are necessary by Cancun tests inside eth-tests.

TODO:

Type of change

  • New feature (non-breaking change which adds functionality)

@cabrador cabrador marked this pull request as ready for review September 2, 2024 10:09
@cabrador cabrador requested review from wsodsong and HerbertJordan and removed request for wsodsong September 2, 2024 10:09
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

This is a huge PR that would have benefited from being decomposed into multiple smaller PRs. For instance, the change of the GetDirectoryFiles function now accepting a list of paths could be on its own. So could be the introduction of the chain configuration in the block environment.

Smaller, focused PRs pay off in improving the review process, and thus leading to better results in my experience.

Another observation is that for all those new features being added, very little unit tests are present covering those.

I can mostly provide feedback on coding style and a few additional observations. I think @wsodsong should make the final approval decision.

// GetChainConfig returns current setup for ChainConfig.
GetChainConfig() *params.ChainConfig

// GetRandom returns the random hash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "the" random hash? Just "a" random hash generated right at the spot or something more restricted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually dont know. The documentation is very limited but its used for creating Mixhash in genesis

Copy link
Collaborator

Choose a reason for hiding this comment

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

Random is from Block context https://github.com/ethereum/go-ethereum/blob/8032b63f167938e6441d4be3eb49b12014d9bfc8/core/vm/evm.go#L67

I don't fully know how it works, but it has to do with mining diffuculty. It is not a random number.

txcontext/substate/substate.go Outdated Show resolved Hide resolved
utils/config.go Outdated Show resolved Hide resolved
executor/transaction_processor.go Outdated Show resolved Hide resolved
executor/transaction_processor.go Outdated Show resolved Hide resolved
ethtest/open.go Outdated Show resolved Hide resolved
ethtest/open.go Outdated Show resolved Hide resolved
ethtest/open.go Outdated Show resolved Hide resolved
ethtest/open.go Outdated Show resolved Hide resolved
ethtest/block_enviroment.go Outdated Show resolved Hide resolved
# Conflicts:
#	ethtest/open.go
#	executor/rpc_request_provider.go
#	utils/directory.go
# Conflicts:
#	ethtest/block_enviroment.go
#	ethtest/open.go
#	ethtest/result.go
#	ethtest/state_test_context.go
#	ethtest/test_case_splitter.go
#	ethtest/test_case_splitter_test.go
cmd/aida-vm-sdb/run_tx_generator_test.go Show resolved Hide resolved
// GetChainConfig returns current setup for ChainConfig.
GetChainConfig() *params.ChainConfig

// GetRandom returns the random hash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Random is from Block context https://github.com/ethereum/go-ethereum/blob/8032b63f167938e6441d4be3eb49b12014d9bfc8/core/vm/evm.go#L67

I don't fully know how it works, but it has to do with mining diffuculty. It is not a random number.

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