feat: forward and validate beacon block - #618
Conversation
|
I will wait for @unnawut comments to be resolved before reviewing this |
| pub cached_proposer_signature: RwLock<HashMap<(PublicKey, u64), BLSSignature>>, | ||
| pub cached_bls_to_execution_signature: RwLock<HashMap<(PublicKey, u64), BLSToExecutionChange>>, |
There was a problem hiding this comment.
shouldn't this be moved out of the BeaconChain struct? as they aren't used here
| @@ -0,0 +1,123 @@ | |||
| use anyhow::{Ok, anyhow}; | |||
There was a problem hiding this comment.
| use anyhow::{Ok, anyhow}; | |
| use anyhow::{anyhow}; |
we shouldn't be importing OK
| use anyhow::{Ok, anyhow}; | ||
| use ream_beacon_chain::beacon_chain::BeaconChain; | ||
| use ream_consensus::{ | ||
| constants::MAX_BLOBS_PER_BLOCK_ELECTRA, electra::beacon_block::SignedBeaconBlock, | ||
| misc::compute_start_slot_at_epoch, | ||
| }; | ||
| use ream_storage::tables::{Field, Table}; | ||
|
|
||
| #[derive(Debug)] | ||
| pub enum ValidationResult { | ||
| Accept, | ||
| Ignore, | ||
| Reject, |
There was a problem hiding this comment.
can you make gossipsub.rs into a folder then move this file their, but then make a folder called validate, and make a file called beacon_block and put validate_beacon_block there
| let latest_block_in_db = store.db.get_latest_block()?; | ||
| let latest_state_in_db = store.db.get_latest_state()?; | ||
|
|
||
| if block.message.slot < latest_block_in_db.message.slot { |
There was a problem hiding this comment.
[IGNORE] The block is not from a future slot (with a MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. validate that signed_beacon_block.message.slot <= current_slot (a client MAY queue future blocks for processing at the appropriate slot).
Are you sure that it is the latest slot we have or it it the actual latest slot? because you can do math to calculate what the current_slot is as there is 1 slot every 12 seconds. Because it mentions MAXIMUM_GOSSIP_CLOCK_DISPARITY, I am assuming we are calcualting the actual current slot, but you should double check
There was a problem hiding this comment.
Hmm good question
Here's my thought process on this:
if a node has latest block =10
actual latest block = 15
the node can never now that the actual latest block number(or by extension, slot) is 15...i dont think there's a way to calculate. Of course slot=time / 12. But in the context of the node, the time is that of block=10. i.e. it does not the actual chain has progresses till 15. Please correct me if im wrong or the explanation is not clear.
Now,
-
case 1: node receives block from gossip network for block=11.
it passes all validation and the node is updated. -
case 2: node receives block = 13
currently it will ignore it because we are not queuing it. I think this should be done in a separate issue since there could possibly require discussions on how to implement it and then use the queued block later at the correct slot. Also, wrapping this PR will help others working on other validate and forward gossip issues to make progress.
There was a problem hiding this comment.
Will create an issue for this
There was a problem hiding this comment.
the node can never now that the actual latest block number(or by extension, slot) is 15...i dont think there's a way to calculate.
I think it's this one: https://ethereum.github.io/consensus-specs/specs/phase0/fork-choice/#get_current_slot. So:
-
I think we can base off the global slot number. Another reason why I think we should base on the global slot number is because if our node is stalling (stuck processing a block),
latest_block_in_db.message.slotwill get stuck. Then, more and more messages will not be ignored byblock.message.slot < latest_block_in_db.message.slotand spamming the gossip network. Ablock.message.slot < store.get_current_slot()will solve this. -
But the function above also doesn't take into account
MAXIMUM_GOSSIP_CLOCK_DISPARITYso I agree we can take thisMAXIMUM_GOSSIP_CLOCK_DISPARITYpart in another PR so please create a new issue!
There was a problem hiding this comment.
looks like i missed this comment...will push a fix
There was a problem hiding this comment.
Also on rechecking this comparison was wrong;
it should be
if block.message.slot > current_global_slot {
return Ok(ValidationResult::Ignore(
"Block is from a future slot".to_string(),
));
}
Taking the example above:
if our node has latest block =10
actual latest block = 15
Cases:
- incoming block=11: this will pass since
11>15is not true - incoming block=17: this will ignore since
17>15.
Also for case 2, this is where queuing will come in: Add Queuing mechanism to gossip pipeline #636
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
I need to read validate_beacon_block more deeply, but here is a quick review to get started before I go to bed
| let start_slot_at_epoch = | ||
| compute_start_slot_at_epoch(store.db.finalized_checkpoint_provider().get()?.epoch); | ||
|
|
||
| if block.message.slot < start_slot_at_epoch { | ||
| return Ok(ValidationResult::Ignore); | ||
| } |
There was a problem hiding this comment.
we might as well inline this, as the variable isn't reused
| pub cached_proposer_signature: RwLock<HashMap<(PublicKey, u64), BLSSignature>>, | ||
| pub cached_bls_to_execution_signature: RwLock<HashMap<(PublicKey, u64), BLSToExecutionChange>>, |
There was a problem hiding this comment.
where is the logic which clears this cache shouldn't we use an LRU here?
| if let Ok(validation_result) = | ||
| validate_beacon_block(beacon_chain, &signed_block).await | ||
| { | ||
| match validation_result { | ||
| ValidationResult::Accept => { | ||
| if let Err(err) = | ||
| beacon_chain.process_block(*signed_block.clone()).await | ||
| { | ||
| error!("Failed to process gossipsub beacon block: {err}"); | ||
| } | ||
| p2psender.send_gossip(GossipMessage { | ||
| topic: GossipTopic::from_topic_hash(&message.topic) | ||
| .expect("invalid topic hash"), | ||
| data: signed_block.as_ssz_bytes(), | ||
| }); | ||
| } | ||
| ValidationResult::Ignore => warn!("Ignoring gossipsub beacon block"), | ||
| ValidationResult::Reject => { | ||
| warn!("Rejecting gossipsub beacon block. Peer should be penalized") | ||
| } | ||
| } |
There was a problem hiding this comment.
| if let Ok(validation_result) = | |
| validate_beacon_block(beacon_chain, &signed_block).await | |
| { | |
| match validation_result { | |
| ValidationResult::Accept => { | |
| if let Err(err) = | |
| beacon_chain.process_block(*signed_block.clone()).await | |
| { | |
| error!("Failed to process gossipsub beacon block: {err}"); | |
| } | |
| p2psender.send_gossip(GossipMessage { | |
| topic: GossipTopic::from_topic_hash(&message.topic) | |
| .expect("invalid topic hash"), | |
| data: signed_block.as_ssz_bytes(), | |
| }); | |
| } | |
| ValidationResult::Ignore => warn!("Ignoring gossipsub beacon block"), | |
| ValidationResult::Reject => { | |
| warn!("Rejecting gossipsub beacon block. Peer should be penalized") | |
| } | |
| } | |
| let Ok(validation_result) = validate_beacon_block(beacon_chain, &signed_block).await else { | |
| return; | |
| } | |
| match validation_result { | |
| ValidationResult::Accept => { | |
| if let Err(err) = | |
| beacon_chain.process_block(*signed_block.clone()).await | |
| { | |
| error!("Failed to process gossipsub beacon block: {err}"); | |
| } | |
| p2psender.send_gossip(GossipMessage { | |
| topic: GossipTopic::from_topic_hash(&message.topic) | |
| .expect("invalid topic hash"), | |
| data: signed_block.as_ssz_bytes(), | |
| }); | |
| } | |
| ValidationResult::Ignore => warn!("Ignoring gossipsub beacon block"), | |
| ValidationResult::Reject => { | |
| warn!("Rejecting gossipsub beacon block. Peer should be penalized") | |
| } | |
| } |
| #[derive(Debug)] | ||
| pub enum ValidationResult { | ||
| Accept, | ||
| Ignore, | ||
| Reject, | ||
| } | ||
|
|
||
| pub async fn validate_beacon_block( | ||
| beacon_chain: &BeaconChain, | ||
| block: &SignedBeaconBlock, | ||
| ) -> anyhow::Result<ValidationResult> { |
There was a problem hiding this comment.
if we are defining ValidationResult, shouldn't we have Error as a part of the result, instead of having to unwrap 2 results
There was a problem hiding this comment.
actually reutrning a result in validate_beacon_block is helpful since there are many calls to other functions that return a Result.
if validate_beacon_block only returns ValidationResult, this will mean there are many if..let or match operations in this function.
I dont think its an issue if we are checking once for error and then once again
| } | ||
| pub fn get_latest_block(&self) -> anyhow::Result<SignedBeaconBlock> { | ||
| let highest_root = self |
|
Could you add a comment with the first sentence of every check being done, just so it is easier to distinguish |
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
Leaving feedback quick, I need to find some time to more thoroughly review the validate beacon block function
unnawut
left a comment
unnawut
left a comment
There was a problem hiding this comment.
Left some initial comments below.
Unless I'm totally mistaken, I think we really need to be careful getting the validation rules right, both in terms of implementation and testing.
| } | ||
|
|
||
| // [REJECT] The block's execution payload timestamp is correct with respect to the slot. | ||
| if !block.message.body.execution_payload.timestamp |
There was a problem hiding this comment.
Is this the intended way to compare timestamps (u64) using the bitwise negation? For example:
println!("{:?}", !1u64 == 1u64); // false
println!("{:?}", !1u64 == 2u64); // falseAlso applies to other similar uses with other return types as well
There was a problem hiding this comment.
Ahh yes good catch
| // [IGNORE] The block's parent (defined by block.parent_root) has been seen. | ||
| if let Some(parent) = store | ||
| .db | ||
| .beacon_block_provider() | ||
| .get(block.message.parent_root)? | ||
| { | ||
| // [REJECT] The block is from a higher slot than its parent. | ||
| if block.message.slot > parent.message.slot + 1 { | ||
| return Ok(ValidationResult::Reject( | ||
| "Block is from a higher slot than expected".to_string(), | ||
| )); | ||
| } | ||
| } else { | ||
| return Ok(ValidationResult::Ignore( | ||
| "Parent block not found".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
| // [IGNORE] The block's parent (defined by block.parent_root) has been seen. | |
| if let Some(parent) = store | |
| .db | |
| .beacon_block_provider() | |
| .get(block.message.parent_root)? | |
| { | |
| // [REJECT] The block is from a higher slot than its parent. | |
| if block.message.slot > parent.message.slot + 1 { | |
| return Ok(ValidationResult::Reject( | |
| "Block is from a higher slot than expected".to_string(), | |
| )); | |
| } | |
| } else { | |
| return Ok(ValidationResult::Ignore( | |
| "Parent block not found".to_string(), | |
| )); | |
| } | |
| match store | |
| .db | |
| .beacon_block_provider() | |
| .get(block.message.parent_root)? { | |
| Some(parent_block) => { | |
| // [REJECT] The block is from a higher slot than its parent. | |
| if block.message.slot > parent.message.slot + 1 { | |
| return Ok(ValidationResult::Reject( | |
| "Block is from a higher slot than expected".to_string(), | |
| )); | |
| } | |
| } | |
| None => { | |
| // [IGNORE] The block's parent (defined by block.parent_root) has been seen. | |
| return Ok(ValidationResult::Ignore( | |
| "Parent block not found".to_string(), | |
| )); | |
| } | |
| } |
I think a match case would be a lot more readable here, also moving // [IGNORE] The block's parent (defined by block.parent_root) has been seen. down
|
|
||
| let finalized_checkpoint = store.db.finalized_checkpoint_provider().get()?; |
There was a problem hiding this comment.
Aren't we missing the
[REJECT] The block's parent (defined by block.parent_root) passes validation. check? I can't see it
| // [REJECT] The current finalized_checkpoint is an ancestor of block. | ||
| if !store.get_checkpoint_block(block.message.parent_root, finalized_checkpoint.epoch)? | ||
| == finalized_checkpoint.root |
There was a problem hiding this comment.
| // [REJECT] The current finalized_checkpoint is an ancestor of block. | |
| if !store.get_checkpoint_block(block.message.parent_root, finalized_checkpoint.epoch)? | |
| == finalized_checkpoint.root | |
| // [REJECT] The current finalized_checkpoint is an ancestor of block. | |
| if store.get_checkpoint_block(block.message.parent_root, finalized_checkpoint.epoch)? | |
| != finalized_checkpoint.root |
shouldn't it be like this?
| // [REJECT] The block is proposed by the expected proposer_index for the block's slot. | ||
| if !latest_state_in_db.get_beacon_proposer_index(Some(block.message.slot))? | ||
| == block.message.proposer_index |
There was a problem hiding this comment.
| // [REJECT] The block is proposed by the expected proposer_index for the block's slot. | |
| if !latest_state_in_db.get_beacon_proposer_index(Some(block.message.slot))? | |
| == block.message.proposer_index | |
| // [REJECT] The block is proposed by the expected proposer_index for the block's slot. | |
| if latest_state_in_db.get_beacon_proposer_index(Some(block.message.slot))? | |
| != block.message.proposer_index |
same comment, having ! at the start looks weird without (), but also it isn't needed
There was a problem hiding this comment.
Yes this was fixed in latest commit...just saw your review now will implement the suggestions next
| "Execution payload timestamp is incorrect".to_string(), | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we are missing the highlighted cases
| }) | ||
| { | ||
| return Ok(ValidationResult::Ignore( | ||
| "Signature already received".to_string(), | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
[IGNORE] current_epoch >= CAPELLA_FORK_EPOCH, where current_epoch is defined by the current wall-clock time.
[IGNORE] The signed_bls_to_execution_change is the first valid signed bls to execution change received for the validator with index signed_bls_to_execution_change.message.validator_index.
[REJECT] All of the conditions within process_bls_to_execution_change pass validation.
aren't we missing the last case here?
|
Latest changes does the following things:
|
459e3eb to
f5dcc6a
Compare
2a369c5 to
295b524
Compare
295b524 to
116249e
Compare
116249e to
31cb932
Compare
|
lgtm now. Please recheck formatting & @KolbyML's 👍 before merging |
31cb932 to
21cb977
Compare
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
The PR is looking better, but a I have a few concerns that should be resolved
There was a problem hiding this comment.
gossipsub/handle_gossipsub.rs
this should just be gossipsub/handle.rs then, having gossipsub written twice is redundent
| ValidationResult::Accept => { | ||
| if let Err(err) = beacon_chain.process_block(*signed_block.clone()).await { | ||
| error!("Failed to process gossipsub beacon block: {err}"); | ||
| } | ||
| p2psender.send_gossip(GossipMessage { | ||
| topic: GossipTopic::from_topic_hash(&message.topic) | ||
| .expect("invalid topic hash"), | ||
| data: signed_block.as_ssz_bytes(), |
There was a problem hiding this comment.
| ValidationResult::Accept => { | |
| if let Err(err) = beacon_chain.process_block(*signed_block.clone()).await { | |
| error!("Failed to process gossipsub beacon block: {err}"); | |
| } | |
| p2psender.send_gossip(GossipMessage { | |
| topic: GossipTopic::from_topic_hash(&message.topic) | |
| .expect("invalid topic hash"), | |
| data: signed_block.as_ssz_bytes(), | |
| ValidationResult::Accept => { | |
| let signed_block_bytes = signed_block.as_ssz_bytes(); | |
| if let Err(err) = beacon_chain.process_block(signed_block).await { | |
| error!("Failed to process gossipsub beacon block: {err}"); | |
| } | |
| p2psender.send_gossip(GossipMessage { | |
| topic: GossipTopic::from_topic_hash(&message.topic) | |
| .expect("invalid topic hash"), | |
| data: signed_block_bytes, |
we can avoid the clone right?
| let Some(parent_block) = store | ||
| .db | ||
| .beacon_block_provider() | ||
| .get(block.message.parent_root)? | ||
| else { | ||
| return Err(anyhow!("failed to get parent block")); | ||
| }; | ||
|
|
||
| let Some(parent_state) = store | ||
| .db | ||
| .beacon_state_provider() | ||
| .get(block.message.parent_root)? | ||
| else { | ||
| return Err(anyhow!("failed to get parent state")); | ||
| }; | ||
|
|
||
| // Validate incoming block | ||
| match validate_beacon_block(beacon_chain, cached_db, block, &latest_state_in_db, false).await? { | ||
| ValidationResult::Accept => {} | ||
| ValidationResult::Ignore(reason) => { | ||
| return Ok(ValidationResult::Ignore(reason)); | ||
| } | ||
| ValidationResult::Reject(reason) => { | ||
| return Ok(ValidationResult::Reject(reason)); | ||
| } | ||
| } | ||
| // Validate parent block [block.message.parent_root] | ||
| match validate_beacon_block(beacon_chain, cached_db, &parent_block, &parent_state, true).await? | ||
| { | ||
| ValidationResult::Accept => {} | ||
| ValidationResult::Ignore(reason) => { | ||
| return Ok(ValidationResult::Ignore(reason)); | ||
| } | ||
| ValidationResult::Reject(reason) => { | ||
| return Ok(ValidationResult::Reject(reason)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
| let Some(parent_block) = store | |
| .db | |
| .beacon_block_provider() | |
| .get(block.message.parent_root)? | |
| else { | |
| return Err(anyhow!("failed to get parent block")); | |
| }; | |
| let Some(parent_state) = store | |
| .db | |
| .beacon_state_provider() | |
| .get(block.message.parent_root)? | |
| else { | |
| return Err(anyhow!("failed to get parent state")); | |
| }; | |
| // Validate incoming block | |
| match validate_beacon_block(beacon_chain, cached_db, block, &latest_state_in_db, false).await? { | |
| ValidationResult::Accept => {} | |
| ValidationResult::Ignore(reason) => { | |
| return Ok(ValidationResult::Ignore(reason)); | |
| } | |
| ValidationResult::Reject(reason) => { | |
| return Ok(ValidationResult::Reject(reason)); | |
| } | |
| } | |
| // Validate parent block [block.message.parent_root] | |
| match validate_beacon_block(beacon_chain, cached_db, &parent_block, &parent_state, true).await? | |
| { | |
| ValidationResult::Accept => {} | |
| ValidationResult::Ignore(reason) => { | |
| return Ok(ValidationResult::Ignore(reason)); | |
| } | |
| ValidationResult::Reject(reason) => { | |
| return Ok(ValidationResult::Reject(reason)); | |
| } | |
| }; | |
| // Validate incoming block | |
| match validate_beacon_block(beacon_chain, cached_db, block, &latest_state_in_db, false).await? { | |
| ValidationResult::Accept => {} | |
| ValidationResult::Ignore(reason) => { | |
| return Ok(ValidationResult::Ignore(reason)); | |
| } | |
| ValidationResult::Reject(reason) => { | |
| return Ok(ValidationResult::Reject(reason)); | |
| } | |
| } | |
| let Some(parent_block) = store | |
| .db | |
| .beacon_block_provider() | |
| .get(block.message.parent_root)? | |
| else { | |
| bail!("failed to get parent block")); | |
| }; | |
| let Some(parent_state) = store | |
| .db | |
| .beacon_state_provider() | |
| .get(block.message.parent_root)? | |
| else { | |
| bail!("failed to get parent state")); | |
| }; | |
| // Validate parent block [block.message.parent_root] | |
| match validate_beacon_block(beacon_chain, cached_db, &parent_block, &parent_state, true).await? | |
| { | |
| ValidationResult::Accept => {} | |
| ValidationResult::Ignore(reason) => { | |
| return Ok(ValidationResult::Ignore(reason)); | |
| } | |
| ValidationResult::Reject(reason) => { | |
| return Ok(ValidationResult::Reject(reason)); | |
| } | |
| }; |
we should probably move those 2 getter calls down as it isn't needed above
| let signed_proposer_bls_execution_change = block | ||
| .message | ||
| .body | ||
| .bls_to_execution_changes | ||
| .get(block.message.proposer_index as usize) | ||
| .ok_or(anyhow!("Invalid index for signed bls to execution change"))?; |
There was a problem hiding this comment.
I think you should choose one style and be consistent either do let some or ok_or in this function, but not both
| pub async fn validate_beacon_block( | ||
| beacon_chain: &BeaconChain, | ||
| cached_db: &CachedDB, | ||
| block: &SignedBeaconBlock, | ||
| state: &BeaconState, | ||
| is_parent: bool, | ||
| ) -> anyhow::Result<ValidationResult> { | ||
| let store = beacon_chain.store.lock().await; |
There was a problem hiding this comment.
This code will not work, as it will deadlock no?
We call let store = beacon_chain.store.lock().await; in the function above. Have you tested if this code works?
There was a problem hiding this comment.
You're right about the deadlock here...will be fixed in latest
As discussed with O, testing this will be top priority after this PR. Issue here: #635
There was a problem hiding this comment.
You shouldn't be submitting broken code, you need to make sure it works. If that expectation can't be met maybe tests should be done in this PR.
Even if tests aren't added in a PR you should test if the code works.
There was a problem hiding this comment.
Of course edge cases can be missed and that is fine, it is just the deadlock problem could be avoided fairly easily, also deadlocks could cost the team a week or two, depending on when we find them.
| let current_global_slot = store.get_current_slot()?; | ||
|
|
||
| // [IGNORE] The block is not from a future slot. | ||
| if block.message.slot > current_global_slot { |
There was a problem hiding this comment.
| let current_global_slot = store.get_current_slot()?; | |
| // [IGNORE] The block is not from a future slot. | |
| if block.message.slot > current_global_slot { | |
| // [IGNORE] The block is not from a future slot. | |
| if block.message.slot > store.get_current_slot()? { |
we might as well inline this, as we don't reuse the variable
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
@varun-doshi could you make a PR with just the scaffolding everything but, validate_gossip_beacon_block and validate_beacon_block. Then rebase this PR ontop of the scaffolding PR. We should be able to merge the scaffolding in quick, then would you be able to add some tests for the validation logic being added in the PR which adds validate_gossip_beacon_block and validate_beacon_block. Just because a few tests can save us time in the future, also I think they are high value in this case, as this code path is fairly critical and a hot path.
The tests should create dummy states/blocks, mainnet blocks would be to big of course.
| let latest_state_in_db = { | ||
| let store = beacon_chain.store.lock().await; | ||
|
|
||
| store.db.get_latest_state()? | ||
| }; |
There was a problem hiding this comment.
| let latest_state_in_db = { | |
| let store = beacon_chain.store.lock().await; | |
| store.db.get_latest_state()? | |
| }; | |
| let latest_state = beacon_chain.store.lock().await.db.get_latest_state()?; |
this can be inlined
There was a problem hiding this comment.
This feedback wasn't addressed
1f537d5 to
5d0c9e8
Compare
|
Tests added for block validation. There's 1 catch here regarding the test though. Specifically the following validation rule: The issue is this goes back recursively backwords to the last finalized checkpoint so which means we need atleast the last 64 blocks (which means we need to save 64 blocks as json). Now I've tested it going backwards 3 blocks and it words as expected. |
Going forward we shouldn't include test data in json files like this, they should be encoded into ssz_snappy Like was done in this PR https://github.com/ReamLabs/ream/pull/609/files and in ef-tests How big in terms of MB is the testdata? |
we shouldn't be uncommenting validation cases, maybe mark this section with a cfg to not compile it in test code
I need to read more into the case before I can fully comment on this. I left some early feedback above |
|
@varun-doshi can you rebase? |
c9da2e3 to
bcc3cda
Compare
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
Overall the PR is looking very good now, just a few little issues
| let latest_state_in_db = { | ||
| let store = beacon_chain.store.lock().await; | ||
|
|
||
| store.db.get_latest_state()? | ||
| }; |
There was a problem hiding this comment.
This feedback wasn't addressed
| let validator = | ||
| if let Some(validator) = state.validators.get(block.message.proposer_index as usize) { | ||
| validator | ||
| } else { | ||
| return Ok(ValidationResult::Reject("Validator not found".to_string())); | ||
| }; |
There was a problem hiding this comment.
| let validator = | |
| if let Some(validator) = state.validators.get(block.message.proposer_index as usize) { | |
| validator | |
| } else { | |
| return Ok(ValidationResult::Reject("Validator not found".to_string())); | |
| }; | |
| let Some(validator) = state.validators.get(block.message.proposer_index as usize) else { | |
| return Ok(ValidationResult::Reject("Validator not found".to_string())); | |
| }; |
^ we can simplify the code to this
| let validator = if let Some(validator) = latest_state_in_db | ||
| .validators | ||
| .get(block.message.proposer_index as usize) | ||
| { | ||
| validator | ||
| } else { | ||
| return Ok(ValidationResult::Reject("Validator not found".to_string())); | ||
| }; |
| match store | ||
| .db | ||
| .beacon_block_provider() | ||
| .get(block.message.parent_root)? | ||
| { | ||
| Some(parent_block) => { | ||
| // [REJECT] The block is from a higher slot than its parent. | ||
| if block.message.slot > parent_block.message.slot + 1 { | ||
| return Ok(ValidationResult::Reject( | ||
| "Block is from a higher slot than expected".to_string(), | ||
| )); |
There was a problem hiding this comment.
| match store | |
| .db | |
| .beacon_block_provider() | |
| .get(block.message.parent_root)? | |
| { | |
| Some(parent_block) => { | |
| // [REJECT] The block is from a higher slot than its parent. | |
| if block.message.slot > parent_block.message.slot + 1 { | |
| return Ok(ValidationResult::Reject( | |
| "Block is from a higher slot than expected".to_string(), | |
| )); | |
| match store | |
| .db | |
| .beacon_block_provider() | |
| .get(block.message.parent_root)? | |
| { | |
| Some(parent_block) => { | |
| // [REJECT] The block is from a higher slot than its parent. | |
| if block.message.slot <= parent_block.message.slot { | |
| return Ok(ValidationResult::Reject( | |
| "Block is from a higher slot than expected".to_string(), | |
| )); |
^ shouldn't the condition be this?
| #[cfg(feature = "allow_ancestor_validation")] | ||
| { | ||
| let finalized_checkpoint = store.db.finalized_checkpoint_provider().get()?; | ||
| // [REJECT] The current finalized_checkpoint is an ancestor of block. | ||
| if store.get_checkpoint_block(block.message.parent_root, finalized_checkpoint.epoch)? | ||
| != finalized_checkpoint.root | ||
| { | ||
| return Ok(ValidationResult::Reject( | ||
| "Finalized checkpoint is not an ancestor".to_string(), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
can you negate this so it is on by default, then disable it just for the test
bcc3cda to
c33c25c
Compare
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
looks good. I will merge the PR now as I want it in 😤
There was a problem hiding this comment.
the PR is pretty much ready to merge, could you move these tests to the crate they are for, as this test isn't related to the beacon-api
There was a problem hiding this comment.
or make a new testing crate
4 participants
What are you trying to achieve?
Fixes #589
How was it implemented/fixed?
https://ethereum.github.io/consensus-specs/specs/phase0/p2p-interface/#beacon_block
https://ethereum.github.io/consensus-specs/specs/altair/p2p-interface/#beacon_block
https://ethereum.github.io/consensus-specs/specs/bellatrix/p2p-interface/#beacon_block
https://ethereum.github.io/consensus-specs/specs/capella/p2p-interface/#beacon_block
https://ethereum.github.io/consensus-specs/specs/deneb/p2p-interface/#beacon_block
https://ethereum.github.io/consensus-specs/specs/electra/p2p-interface/#beacon_block
To-Do