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

feat: engine driver #12

Merged
merged 11 commits into from
Jan 22, 2025
Merged

feat: engine driver #12

merged 11 commits into from
Jan 22, 2025

Conversation

greged93
Copy link
Collaborator

@greged93 greged93 commented Jan 17, 2025

Add the engine driver component to the rollup node. The Engine driver is the interface to the Engine API of the EN and handles communicating new safe and unsafe payloads to the EN.

Resolves #5


/// The payload attributes for block building tailored for Scroll.
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
pub struct ScrollPayloadAttributes {
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 will move this to scroll-alloy-rpc-types-engine once the alloy PR is merge on Reth.

/// - all transactions match
/// - timestamps are equal
/// - `prev_randaos` are equal
/// - TODO: should we also compare the `fee_recipient` with the `suggested_fee_recipient`?
Copy link
Collaborator Author

@greged93 greged93 Jan 17, 2025

Choose a reason for hiding this comment

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

the fee recipient is checked for equality in OP, but I think I remember @Thegaram mentioning that the fee_recipient field was used for something else currently?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this header.coinbase? It is currently used by Clique.

crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/payload.rs Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/payload.rs Show resolved Hide resolved
@greged93 greged93 requested a review from frisitano January 17, 2025 15:21
Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Looks good! Added a few comments inline.

Cargo.toml Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Outdated Show resolved Hide resolved
crates/engine/src/payload.rs Outdated Show resolved Hide resolved
crates/engine/src/engine.rs Show resolved Hide resolved
safe_head: BlockInfo,
finalized_head: BlockInfo,
) -> Self {
let fcu = ForkchoiceState {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let fcu = ForkchoiceState {
let fcs = ForkchoiceState {

Comment on lines 57 to 68
loop {
match client.fork_choice_updated_v1(fcu, None).await {
Err(err) => {
debug!(target: "engine::driver", ?err, "waiting on engine client");
tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
}
Ok(status) => {
info!(target: "engine::driver", payload_status = ?status.payload_status.status, "engine ready");
break
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loop {
match client.fork_choice_updated_v1(fcu, None).await {
Err(err) => {
debug!(target: "engine::driver", ?err, "waiting on engine client");
tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
}
Ok(status) => {
info!(target: "engine::driver", payload_status = ?status.payload_status.status, "engine ready");
break
}
}
}
// wait on engine
while let Err(err) = client.fork_choice_updated_v1(fcu, None).await {
debug!(target: "engine::driver", ?err, "waiting on engine client");
tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
}
info!(target: "engine::driver", "engine ready");

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to keep Ok(status) to log it

/// The finalized L2 block info.
finalized_block_info: BlockInfo,
/// Marker
_types: std::marker::PhantomData<ET>,
Copy link
Member

Choose a reason for hiding this comment

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

just curious about why this variable need a _

Copy link
Contributor

Choose a reason for hiding this comment

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

PhantomData is a zero-sized type that is only there to give a hint about ET to the compiler. So this field will never be accessed. The _ prefix just tells the compiler that this field is not used and prevents warnings (see here).

Copy link
Collaborator

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Looks good! Left a single comment inline. Let me know what you think.

crates/engine/src/engine.rs Outdated Show resolved Hide resolved
@frisitano frisitano merged commit a59549e into main Jan 22, 2025
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.

Design and Implement Engine Driver
4 participants