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

feature: add e2hs mode to portal-bridge #1735

Merged
merged 2 commits into from
Mar 31, 2025

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Mar 28, 2025

What was wrong?

With the introduction of the new e2hs file format, we need a bridge mode to support these files.

This is a very basic bridge mode, that won't really work unless you have all necessary e2hs files in a local test-e2hs dir, inside the root level of the trin dir. But the intention is that this will be updated to use a centralized server that hosts all e2hs files, once they are generated and made available.

The existing BridgeMode type has turned into a bit of a frankenstein, as a result of us trying to push too much into a single cli arg. After this new bridge mode matures, I expect a fair amount of legacy bridge code to be deleted, that's why I decided to create new, isolated cli args for the e2hs mode to use, rather than try and squeeze them into the old BridgeMode type.

There's another trade-off to consider. I opted to require the user to provide the entire block range, rather than using a live el provider to detect the head of the chain. I'm not married to this, but I think it's easier if a user just wants to casually run the bridge, without an infura / pandaops account.

But both of these decisions are fairly trivial, and can be changed as new use-cases for the bridge are implemented.

Usage

"--mode e2hs --e2hs-range 1000-10000 --e2hs-randomize": randomize the order in which epochs from block range are gossiped

How was it fixed?

  • added e2hs mode to portal-bridge

To-Do

@njgheorghita njgheorghita marked this pull request as ready for review March 28, 2025 18:22
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: looks good. I think it is fine if we iterate on this like you explained in the PR's description.

Comment on lines 400 to 417
fn validate_block_tuple(&self, block_tuple: &BlockTuple) -> anyhow::Result<()> {
self.header_validator
.validate_header_with_proof(&block_tuple.header_with_proof.header_with_proof)?;
let receipts = &block_tuple.receipts.receipts;
let receipts_root = receipts.root();
ensure!(
receipts_root
== block_tuple
.header_with_proof
.header_with_proof
.header
.receipts_root,
"Receipts root mismatch"
);
let body = &block_tuple.body.body;
body.validate_against_header(&block_tuple.header_with_proof.header_with_proof.header)?;
Ok(())
}
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
fn validate_block_tuple(&self, block_tuple: &BlockTuple) -> anyhow::Result<()> {
self.header_validator
.validate_header_with_proof(&block_tuple.header_with_proof.header_with_proof)?;
let receipts = &block_tuple.receipts.receipts;
let receipts_root = receipts.root();
ensure!(
receipts_root
== block_tuple
.header_with_proof
.header_with_proof
.header
.receipts_root,
"Receipts root mismatch"
);
let body = &block_tuple.body.body;
body.validate_against_header(&block_tuple.header_with_proof.header_with_proof.header)?;
Ok(())
}
fn validate_block_tuple(&self, block_tuple: &BlockTuple) -> anyhow::Result<()> {
let header_with_proof = &block_tuple.header_with_proof.header_with_proof;
self.header_validator
.validate_header_with_proof(header_with_proof)?;
let body = &block_tuple.body.body;
body.validate_against_header(&header_with_proof.header)?;
let receipts = &block_tuple.receipts.receipts;
ensure!(
receipts.root()
== header_with_proof
.header
.receipts_root,
"Receipts root mismatch"
);
Ok(())
}

I think ordering it like this would be a bit more readable. Also I don't think we need the extra variable receipts_root

Idk up to you

block_stats: Arc<Mutex<HistoryBlockStats>>,
metrics: BridgeMetricsReporter,
) -> anyhow::Result<()> {
let header_number = block_tuple
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 header_number = block_tuple
let block_number = block_tuple

I think this is more clear


info!("Serving block tuple for block #{header_number}");

let header_hash = block_tuple
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 header_hash = block_tuple
let block_hash = block_tuple

I think this is more clear

Comment on lines +122 to +126
let block_number = block_tuple
.header_with_proof
.header_with_proof
.header
.number;
Copy link
Member

Choose a reason for hiding this comment

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

We are doing this 3 times with 3 different variable names

  • block_number
  • number
  • header_number

Could we only do it once, then just pass it as a variable and use block_number as I think that is the standard terminology.

Comment on lines 107 to 114
match block_range {
Some(_) => {
info!("Gossiping block range: {block_range:?} from epoch {epoch_index}",);
}
None => {
info!("Gossiping entire epoch {epoch_index}");
}
}
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
match block_range {
Some(_) => {
info!("Gossiping block range: {block_range:?} from epoch {epoch_index}",);
}
None => {
info!("Gossiping entire epoch {epoch_index}");
}
}
match block_range {
Some(_) => info!("Gossiping block range: {block_range:?} from epoch {epoch_index}"),
None => info!("Gossiping entire epoch {epoch_index}"),
}

Perhaps we could condense it now like this?

Comment on lines 344 to 347
error!(
"Failed to gossip history content key: {:?} - {:?}",
hwp_by_number_content_key, err
);
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
error!(
"Failed to gossip history content key: {:?} - {:?}",
hwp_by_number_content_key, err
);
error!("Failed to gossip history content key: {hwp_by_number_content_key:?} - {err:?}");

Comment on lines 368 to 371
error!(
"Failed to gossip history content key: {:?} - {:?}",
body_content_key, err
);
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
error!(
"Failed to gossip history content key: {:?} - {:?}",
body_content_key, err
);
error!("Failed to gossip history content key: {body_content_key:?} - {err:?}");

Comment on lines 392 to 395
error!(
"Failed to gossip history content key: {:?} - {:?}",
receipts_content_key, err
);
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
error!(
"Failed to gossip history content key: {:?} - {:?}",
receipts_content_key, err
);
error!("Failed to gossip history content key: {receipts_content_key:?} - {err:?}");

Comment on lines 432 to 434
if parts.len() != 2 {
return Err(anyhow!("Invalid block range format"));
}
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
if parts.len() != 2 {
return Err(anyhow!("Invalid block range format"));
}
ensure!(parts.len() == 2, "Invalid block range format");

/// HashMap mapping epoch indexes to either None or Some((start_block, end_block))
pub fn block_range_to_epochs(start_block: u64, end_block: u64) -> HashMap<u64, Option<(u64, u64)>> {
let mut epoch_map = HashMap::new();
const EPOCH_SIZE: u64 = 8192;
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
const EPOCH_SIZE: u64 = 8192;

Isn't this a pretty common constant couldn't we import this from somewhere else?

@njgheorghita njgheorghita merged commit afd685a into ethereum:master Mar 31, 2025
14 checks passed
@njgheorghita njgheorghita deleted the add-e2hs-bridge branch March 31, 2025 15:41
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.

2 participants