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

[WIP] feat: support custom payload attributes in dev mode #14154

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Bisht13
Copy link
Contributor

@Bisht13 Bisht13 commented Feb 1, 2025

Description

This PR adds support for custom payload attributes in development mode (--dev)

closes #14064

@Bisht13 Bisht13 force-pushed the feat/dev-mode-payload-attributes branch from 3a4c982 to 3f2472c Compare February 1, 2025 23:06
@Bisht13
Copy link
Contributor Author

Bisht13 commented Feb 2, 2025

@mattsse is it okay if the user has to implement PayloadAttributesBuilder for their CustomPayloadAttributes like this

impl PayloadAttributesBuilder<CustomPayloadAttributes> for LocalPayloadAttributesBuilder<ChainSpec>
where
    ChainSpec: Send + Sync + EthereumHardforks + 'static,
{
    fn build(&self, timestamp: u64) -> CustomPayloadAttributes {
        CustomPayloadAttributes {
            inner: EthPayloadAttributes {
                timestamp,
                prev_randao: B256::random(),
                suggested_fee_recipient: Address::random(),
                withdrawals: self
                    .chain_spec()
                    .is_shanghai_active_at_timestamp(timestamp)
                    .then(Default::default),
                parent_beacon_block_root: self
                    .chain_spec()
                    .is_cancun_active_at_timestamp(timestamp)
                    .then(B256::random),
            },
            custom: 0,
        }
    }
}

Having difficulty doing if this type implements a trait, do X, otherwise do Y, initially I thought that if a user defines custom payload attributes and tries to run dev mode without implementing the required trait then it should throw a runtime error like custom payload attributes not handled but since trait check happens in compile time, the compilation is failing (even the user doesn't use the dev mode because the entire binary compiles). Let me know if you have a better solution or idea for handling this.

let launcher =
EngineNodeLauncher::new(task_executor, builder.config.datadir(), engine_tree_config);
builder.launch_with(launcher).await
if builder.config.dev.dev {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also @mattsse was wondering that something like this can be done to choose the launcher, but again because of rust compilation time both branches are trait checked during compilation and for someone to use CustomPayloadAttributes, they'd need to impl PayloadAttributesBuilder for the corresponding struct even if they don't wish to run the node in dev mode

@emhane emhane added A-cli Related to the reth CLI A-sdk Related to reth's use as a library and removed A-cli Related to the reth CLI labels Feb 4, 2025
@mattsse
Copy link
Collaborator

mattsse commented Feb 4, 2025

is it okay if the user has to implement PayloadAttributesBuilder for their CustomPayloadAttributes like this

yep that would be expected, in case this shouldn't be supported, this can return an error for example

@Bisht13 Bisht13 force-pushed the feat/dev-mode-payload-attributes branch from 1d6355e to bc7fdd2 Compare February 4, 2025 18:17
@Bisht13
Copy link
Contributor Author

Bisht13 commented Feb 4, 2025

So I think one way to approach this problem is via specialization, 2 things to note with that,

  • First solution: we can use #![feature(specialization)] which is an incomplete feature right now but is the most straightforward implementation.
  • Second solution: move the PayloadAttributesBuilder implementation for LocalPayloadAttributesBuilder in reth-ethereum-forks crate and use #![feature(min_specialization)] instead (which is recommended over specialization right now) and remove the Clone trait bound (assuming this is not getting used anywhere) from EthereumHardfork trait. This requires a not so ideal refactor, keeping payload code in reth-ethereum-forks crate.

I, personally, think the first solution is a better one and the current PR reflects that but would love to hear your thoughts! @mattsse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sdk Related to reth's use as a library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dedicated dev/local launcher
3 participants