Skip to content

fix(l1): added checks to newpayload and forkchoiceupdated #2831

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

Merged
merged 16 commits into from
May 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 44 additions & 2 deletions .github/workflows/pr-main_l1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,53 @@ jobs:
ethrex_flags: ""
- name: "Paris Engine tests"
simulation: ethereum/engine
test_pattern: "engine-api/RPC|Re-org to Previously Validated Sidechain Payload|Re-Org Back into Canonical Chain, Depth=5|Safe Re-Org|Transaction Re-Org|Inconsistent|Suggested Fee|PrevRandao Opcode Transactions|Fork ID|Unknown SafeBlockHash|Unknown FinalizedBlockHash|Unique Payload ID|Re-Execute Payload|Multiple New Payloads|NewPayload with|Payload Build|Build Payload|Invalid PayloadAttributes|Unknown HeadBlockHash|Valid NewPayload->ForkchoiceUpdated|Invalid NewPayload, ParentHash|Bad Hash on NewPayload|In-Order Consecutive Payload Execution|Re-Org Back to Canonical Chain" # |Invalid P9 -> flaky
test_pattern: "engine-api/RPC|
Bad Hash on NewPayload|
Build Payload|
Fork ID|
In-Order Consecutive Payload Execution|
Inconsistent|
Invalid Missing Ancestor ReOrg|
Invalid NewPayload|
Invalid PayloadAttributes|
Multiple New Payloads|
NewPayload with|
ParentHash equals BlockHash on NewPayload|
Payload Build|
PrevRandao Opcode Transactions|
Re-Execute Payload|
Re-Org Back|
Re-org to Previously Validated Sidechain Payload|
RPC:|
Safe Re-Org|
Suggested Fee|
Transaction Re-Org|
Unique Payload ID|
Unknown|
Valid NewPayload->ForkchoiceUpdated" # |Invalid P9 -> flaky
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

ethrex_flags: ""
- name: "Engine withdrawal tests"
simulation: ethereum/engine
test_pattern: "engine-withdrawals/engine-withdrawals test loader|GetPayloadV2 Block Value|Sync after 2 blocks - Withdrawals on Genesis|Max Initcode Size|Pre-Merge Fork Number > 0|Empty Withdrawals|Corrupted Block Hash Payload|Withdrawals Fork on Block 2|Withdrawals Fork on Block 3|GetPayloadBodies|Withdrawals Fork on Block 1 - 1 Block Re-Org|Withdrawals Fork on Block 1 - 8 Block Re-Org NewPayload|Withdrawals Fork on Block 8 - 10 Block Re-Org NewPayload|Withdrawals Fork on Canonical Block 8 / Side Block 7 - 10 Block Re-Org [^S]|Withdrawals Fork on Canonical Block 8 / Side Block 9 - 10 Block Re-Org [^S]"
test_pattern: "engine-withdrawals/
Corrupted Block Hash Payload|
Empty Withdrawals|
engine-withdrawals test loader|
GetPayloadBodies|
GetPayloadV2 Block Value|
Max Initcode Size|
Sync after 2 blocks - Withdrawals on Genesis|
Withdraw many accounts|
Withdraw to a single account|
Withdraw to two accounts|
Withdraw zero amount|
Withdraw many accounts|
Withdrawals Fork on Block 1 - 1 Block Re-Org|
Withdrawals Fork on Block 1 - 8 Block Re-Org NewPayload|
Withdrawals Fork on Block 2|
Withdrawals Fork on Block 3|
Withdrawals Fork on Block 8 - 10 Block Re-Org NewPayload|
Withdrawals Fork on Canonical Block 8 / Side Block 7 - 10 Block Re-Org [^S]|
Withdrawals Fork on Canonical Block 8 / Side Block 9 - 10 Block Re-Org [^S]"
ethrex_flags: ""
- name: "Sync full"
simulation: ethereum/sync
Expand Down
44 changes: 28 additions & 16 deletions crates/blockchain/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,21 @@ impl Blockchain {
block: &Block,
) -> Result<(BlockExecutionResult, Vec<AccountUpdate>), ChainError> {
// Validate if it can be the new head and find the parent
let Ok(parent_header) = find_parent_header(&block.header, &self.storage) else {
// If the parent is not present, we store it as pending.
self.storage.add_pending_block(block.clone()).await?;
return Err(ChainError::ParentNotFound);
// Feda (#2831): We search for the entire block because during full/batch sync
// we can have the header without the body indicating it's still syncing.
let parent = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

so let me get this straight: the only change is that you are searching for the whole block instead of just the header. Any reason you decided to do this inline instead of with a helper function? I think it was actually more readable before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see that the previous function was confusing and the comments outdated. Maybe the solution is a better name and a better comment instead of inlining and repeating code?

Copy link
Contributor Author

@fedacking fedacking May 20, 2025

Choose a reason for hiding this comment

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

I'll do that. Do I return the old function for the other places in the code where they were used? With a better comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, and maybe a better naming. But only if you think it makes the code more readable, this is a suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After implementing it I didn't think it was more readable for this section, but the other places it actually was better, so I have reintroduced the function.

.storage
.get_block_by_hash(block.header.parent_hash)
.await?;
let parent_header = match parent {
Some(parent_block) => parent_block.header,
None => {
// If the parent is not present, we store it as pending.
self.storage.add_pending_block(block.clone()).await?;
return Err(ChainError::ParentNotFound);
}
};

let chain_config = self.storage.get_chain_config()?;

// Validate the block pre-execution
Expand Down Expand Up @@ -235,16 +245,18 @@ impl Blockchain {
}
// for the first block, we need to query the store
let parent_header = if i == 0 {
let Ok(parent_header) = find_parent_header(&block.header, &self.storage) else {
return Err((
ChainError::ParentNotFound,
Some(BatchBlockProcessingFailure {
failed_block_hash: block.hash(),
last_valid_hash,
}),
));
};
parent_header
match find_parent_header(&block.header, &self.storage) {
Ok(parent_header) => parent_header,
Err(error) => {
return Err((
error,
Some(BatchBlockProcessingFailure {
failed_block_hash: block.hash(),
last_valid_hash,
}),
))
}
}
} else {
// for the subsequent ones, the parent is the previous block
blocks[i - 1].header.clone()
Expand Down Expand Up @@ -561,8 +573,8 @@ pub async fn latest_canonical_block_hash(storage: &Store) -> Result<H256, ChainE
)))
}

/// Validates if the provided block could be the new head of the chain, and returns the
/// parent_header in that case. If not found, the new block is saved as pending.
/// Searchs the header of the parent block header. If the parent header is missing,
/// Returns a ChainError::ParentNotFound. If the storage has an error it propagates it
pub fn find_parent_header(
block_header: &BlockHeader,
storage: &Store,
Expand Down
20 changes: 12 additions & 8 deletions crates/blockchain/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,23 @@ pub async fn apply_fork_choice(
None
};

let head_res = store.get_block_header_by_hash(head_hash)?;
// Feda (#2831): We search for the entire block because during full/batch sync
// we can have the header without the body indicating it's still syncing.
let head_block_res = store.get_block_by_hash(head_hash).await?;

let Some(head_block) = head_block_res else {
return Err(InvalidForkChoice::Syncing);
};

if !safe_hash.is_zero() {
check_order(&safe_res, &head_res)?;
check_order(safe_res.as_ref(), Some(&head_block.header))?;
}

if !finalized_hash.is_zero() && !safe_hash.is_zero() {
check_order(&finalized_res, &safe_res)?;
check_order(finalized_res.as_ref(), safe_res.as_ref())?;
}

let Some(head) = head_res else {
return Err(InvalidForkChoice::Syncing);
};
let head = head_block.header;

let latest = store.get_latest_block_number().await?;

Expand Down Expand Up @@ -130,8 +134,8 @@ pub async fn apply_fork_choice(

// Checks that block 1 is prior to block 2 and that if the second is present, the first one is too.
fn check_order(
block_1: &Option<BlockHeader>,
block_2: &Option<BlockHeader>,
block_1: Option<&BlockHeader>,
block_2: Option<&BlockHeader>,
) -> Result<(), InvalidForkChoice> {
// We don't need to perform the check if the hashes are null
match (block_1, block_2) {
Expand Down
2 changes: 1 addition & 1 deletion crates/networking/rpc/eth/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ impl RpcHandler for GetBlobBaseFee {
_ => return Err(RpcErr::Internal("Could not get block header".to_owned())),
};
let parent_header = match find_parent_header(&header, &context.storage) {
Ok(header) => header,
Ok(option_header) => option_header,
Err(error) => return Err(RpcErr::Internal(error.to_string())),
};

Expand Down