Skip to content

feat: Add Dedicated Local Launcher #14154

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

Open
wants to merge 1 commit 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

@Bisht13 Bisht13 force-pushed the feat/dev-mode-payload-attributes branch 5 times, most recently from ed84666 to 68290f0 Compare February 14, 2025 02:58
@Bisht13 Bisht13 force-pushed the feat/dev-mode-payload-attributes branch from b0b215a to f48eefb Compare February 14, 2025 03:31
@Bisht13
Copy link
Contributor Author

Bisht13 commented Feb 14, 2025

I have implemented a dedicated dev/local launcher which gets used if the node is run in dev mode, also added PayloadAttributesBuilderAddOn for providing PayloadAttributesBuilder for local launcher on demand. The impl for LocalEngineNodeLauncher is similar to EngineNodeLauncher leading to a lot of DRY code, this is intended as someone might want to tweak few things in one engine but not in another.

PS dropped specialization since it's not stable

@Bisht13 Bisht13 marked this pull request as ready for review February 14, 2025 03:49
@Bisht13 Bisht13 changed the title [WIP] feat: support custom payload attributes in dev mode feat: Add Dedicated Local Launcher Feb 14, 2025
@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Mar 14, 2025
@github-actions github-actions bot removed the S-stale This issue/PR is stale and will close with no further activity label Mar 21, 2025
@github-actions github-actions bot added S-stale This issue/PR is stale and will close with no further activity and removed S-stale This issue/PR is stale and will close with no further activity labels Apr 11, 2025
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