-
Notifications
You must be signed in to change notification settings - Fork 97
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
Changes from all commits
3a0737b
6775cce
182ccc2
908da90
4dc3421
569ca2a
eb14b33
2dabf49
d270912
e78704a
2ffd4a0
d77303c
2244dda
c6e4ed4
d8096c4
04f5e3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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() | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!