-
Notifications
You must be signed in to change notification settings - Fork 145
feat: create header proof using BeaconBlockElectra #1800
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
Conversation
bin/e2hs-writer/src/reader.rs
Outdated
@@ -241,6 +242,48 @@ impl EpochReader { | |||
}) | |||
} | |||
|
|||
#[allow(dead_code)] | |||
fn get_electra_block_data(&mut self, block_number: u64) -> anyhow::Result<AllBlockData> { |
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.
This logic is triggered after processing era file. Do you think this will be needed?
Also, at the moment, we decide which fork to use based on block number, and we don't have one for Pectra yet.
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.
We have the timestamp it will be enabled already
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.
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.
We would call it from here:
trin/bin/e2hs-writer/src/reader.rs
Lines 244 to 267 in afa57c1
pub fn iter_blocks(mut self) -> impl Iterator<Item = anyhow::Result<AllBlockData>> { | |
(self.starting_block..self.ending_block).map(move |current_block| { | |
if current_block | |
< EthereumHardfork::Paris | |
.activation_block(network_spec().network().into()) | |
.expect("Paris should be available") | |
{ | |
self.get_pre_merge_block_data(current_block) | |
} else if current_block | |
< EthereumHardfork::Shanghai | |
.activation_block(network_spec().network().into()) | |
.expect("Shanghai should be available") | |
{ | |
self.get_merge_to_capella_block_data(current_block) | |
} else if current_block | |
< EthereumHardfork::Cancun | |
.activation_block(network_spec().network().into()) | |
.expect("Cancun should be available") | |
{ | |
self.get_capella_to_deneb_block_data(current_block) | |
} else { | |
self.get_deneb_block_data(current_block) | |
} | |
}) |
I'm not sure if/how we would refactor it...
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.
There are post merge and pre-merge blocks, all post merge blocks use ERA, I don't think this current loop makes much sense tbh
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.
We could do a match case on what CL fork the BeaconBlock is for, instead of blindly parsing it like we are now using block number
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.
I refactored how it works.
I no longer check what fork we are on based on number, but rather based on variant of SignedBeaconBlock.
If you think it would be good, I can also add a check to make sure that variant is at the expected fork.
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.
Yeah the changes look good.
If you think it would be good, I can also add a check to make sure that variant is at the expected fork.
I think this would be overkill to be honest, but either way it isn't like we are using hardcoded values anymore which is nice
@KolbyML, ready for a review. |
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.
looks good
This is a followup of #1799
What was wrong?
We are missing logic for creating header proof using
BeaconBlockElectra
.How was it fixed?
Added function for creating proof.
Note that we don't have any test vectors to verify if this works as intended.
To-Do