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

Move cfg_env and block_env configuration from PayloadBuilderAttributes into PayloadBuilder #10510

Merged
merged 20 commits into from
Sep 17, 2024

Conversation

garwahl
Copy link
Contributor

@garwahl garwahl commented Aug 25, 2024

This PR moves the configuration environment and block environment setup from PayloadBuilderAttributes out into their respective payload builders.

Context
Both OptimismPayloadBuilder and EthereumPayloadBuilder have an evm_config that defines a fill_cfg_and_block_env that we can use to populate.

This closes #10446 which has additional context.

Comment on lines 817 to 825
/// Returns the configured [`CfgEnvWithHandlerCfg`] and [`BlockEnv`] for the targeted payload
/// (that has the `parent` as its parent).
///
/// The `chain_spec` is used to determine the correct chain id and hardfork for the payload
/// based on its timestamp.
///
/// Block related settings are derived from the `parent` block and the configured attributes.
///
/// NOTE: This is only intended for beacon consensus (after merge).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have enough context to meaningfully update this comment so please suggest alternatives.

@garwahl garwahl marked this pull request as ready for review August 25, 2024 11:18
@garwahl
Copy link
Contributor Author

garwahl commented Aug 25, 2024

@mattsse @onbjerg PTAL.

I am still fixing imports etc due to /crates/optimism/ not importing correctly into my VSCode but I will fix that in parallel as this gets reviewed.

crates/optimism/payload/src/builder.rs Outdated Show resolved Hide resolved
crates/ethereum/payload/src/lib.rs Outdated Show resolved Hide resolved
crates/ethereum/payload/src/lib.rs Outdated Show resolved Hide resolved
crates/optimism/payload/src/builder.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg added C-debt Refactor of code section that is hard to understand or maintain A-block-building Related to block building labels Aug 26, 2024
@garwahl
Copy link
Contributor Author

garwahl commented Aug 27, 2024

Hey @mattsse @onbjerg one thing I'd love some guidance on is now that initialized_block_env and initialized_cfg have been removed from PayloadConfig

/// Pre-configured block environment.
pub initialized_block_env: BlockEnv,
/// Configuration for the environment.
pub initialized_cfg: CfgEnvWithHandlerCfg,

Both the OptimismPayloadBuilder and EthereumPayloadBuilder require these. I assume a good place to move these fields would be to OptimismPayloadBuilderAttributes and EthPayloadBuilderAttributes respectively, unless there is somewhere better/shared?

@garwahl
Copy link
Contributor Author

garwahl commented Aug 28, 2024

ping @mattsse would love your thoughts here

@garwahl garwahl requested a review from onbjerg August 28, 2024 11:20
Copy link
Collaborator

@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.

I realized we need to do some additional changes first so we can easily do this

@mattsse mattsse requested a review from rakita as a code owner September 17, 2024 10:57
@mattsse
Copy link
Collaborator

mattsse commented Sep 17, 2024

@onbjerg I removed the cfg_and_block_env from the trait and instead moved this into the payloadbuilder impl.

this isn't perfect yet, because ideally we can use the evmconfig for this entirely, but this is currently tricky because we'd need to construct the new header right away

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

can work with this in alphanet for now

@mattsse mattsse enabled auto-merge September 17, 2024 11:37
@mattsse mattsse added this pull request to the merge queue Sep 17, 2024
Merged via the queue into paradigmxyz:main with commit 2a04b1c Sep 17, 2024
36 checks passed
@garwahl
Copy link
Contributor Author

garwahl commented Sep 17, 2024

Thanks for getting this across the line @mattsse @onbjerg much appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-block-building Related to block building C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move revm config from PayloadConfig down to PayloadBuilder
3 participants