Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion e2store/src/era.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Era {
let slot_index_state = SlotIndexStateEntry::try_from(&file.entries[entries_length - 1])?;
let slot_indexes = Era::get_block_slot_indexes(&slot_index_block);

// an era file should has 4 entries which are not blocks
// an era file has 4 entries which are not blocks
ensure!(
slot_indexes.len() == entries_length - 4,
"invalid slot index block: incorrect count"
Expand Down
4 changes: 4 additions & 0 deletions e2store/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ pub async fn get_era1_files(http_client: &Client) -> anyhow::Result<HashMap<u64,
era1_files.len()
)
);
ensure!(
(0..ERA1_FILE_COUNT).all(|epoch| era1_files.contains_key(&(epoch as u64))),
"Epoch indices are not starting from zero or not consecutive",
);
Ok(era1_files)
}

Expand Down
6 changes: 1 addition & 5 deletions portal-bridge/src/bridge/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,7 @@ impl StateBridge {
for block_number in starting_block..=last_block {
info!("Gossipping state for block at height: {block_number}");

let block = if block_number == starting_block {
era_manager.get_current_block().await?
} else {
era_manager.get_next_block().await?
};
let block = era_manager.get_next_block().await?;

// process block
let RootWithTrieDiff {
Expand Down
118 changes: 55 additions & 63 deletions trin-execution/src/era/manager.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Current implementation doesn't support fetching blocks out of order, right?
If that's the case, I would make that more explicit, for example:

  • pass starting_block in constructor
  • rename get_block_by_number to get_next_block

Copy link
Member Author

Choose a reason for hiding this comment

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

Current implementation doesn't support fetching blocks out of order, right?

Fetching epochs out of order isn't supported

fetching blocks within the current epoch is supported,

If we wanted to make it more explicit I would want
get_current_block and get_next_block

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case yes, I would be more explicit:

  • change constructor to pub fn new(starting_block) and start fetching corresponding era in the background.
  • add current_block and next_block (or get_* variants, whatever you prefer)
  • maybe even add current_block_number

fetching blocks within the current epoch is supported

We would lose this feature, but without fetching epochs our of order doesn't feel like a big loss.

Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::collections::HashMap;

use e2store::{
era1::Era1,
utils::{get_era_files, get_shuffled_era1_files, ERA1_FILE_COUNT},
utils::{get_era1_files, get_era_files, ERA1_FILE_COUNT},
};
use surf::{Client, Config};
use tokio::task::JoinHandle;
Expand All @@ -17,81 +19,64 @@ use super::{
};

pub struct EraManager {
current_block_number: u64,
next_block_number: u64,
current_era: Option<ProcessedEra>,
next_era: Option<JoinHandle<ProcessedEra>>,
next_era: Option<JoinHandle<anyhow::Result<ProcessedEra>>>,
http_client: Client,
era1_files: Vec<String>,
era1_files: HashMap<u64, String>,
era_files: HashMap<u64, String>,
}

impl EraManager {
pub async fn new(starting_block: u64) -> anyhow::Result<Self> {
pub async fn new(next_block_number: u64) -> anyhow::Result<Self> {
let http_client: Client = Config::new()
.add_header("Content-Type", "application/xml")
.expect("to be able to add header")
.try_into()?;
let era1_files = get_shuffled_era1_files(&http_client).await?;
let era1_files = get_era1_files(&http_client).await?;
let era_files = get_era_files(&http_client).await?;

let mut era_manager = Self {
current_block_number: starting_block,
next_block_number,
current_era: None,
next_era: None,
http_client,
era1_files,
era_files,
};

// initialize the current era
let current_era = era_manager.fetch_era_file().await?;
let _ = era_manager.current_era.insert(current_era);
// initialize the next era file
// The first time using era_manager we need to find the current era file we are starting
// from If the block is from a era file we will need to binary search to find which
// era file contains the block we want
let era1_files = era_manager.era1_files.clone();
let http_client = era_manager.http_client.clone();
let join_handle = tokio::spawn(async move {
Self::init_current_era(era1_files, http_client, next_block_number).await
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It is a bit weird to call function init_current_era and then assign it to the self.next_era (while we have self.current_era). How about we rename function to something like fetch_and_process_era_by_block_number()?

smallest nit: up to you, I don't think join_handle adds much to readability, so I would just have

era_manager.next_era = Some(tokio::spawn(async move {

or at least rename join_handle to next_era or next_era_handle.

});
era_manager.next_era = Some(join_handle);

Ok(era_manager)
}

pub fn current_block_number(&self) -> u64 {
self.current_block_number
}

pub async fn get_current_block(&mut self) -> anyhow::Result<&ProcessedBlock> {
let Some(current_era) = &self.current_era else {
panic!("current_era should be initialized in EraManager::new");
};
Ok(current_era.get_block(self.current_block_number))
pub fn next_block_number(&self) -> u64 {
self.next_block_number
}

pub async fn get_next_block(&mut self) -> anyhow::Result<&ProcessedBlock> {
self.current_block_number += 1;
let processed_era = match &self.current_era {
Some(processed_era) if processed_era.contains_block(self.current_block_number) => {
Some(processed_era) if processed_era.contains_block(self.next_block_number) => {
self.current_era.as_ref().expect("current_era to be some")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: isn't this the same as processed_era?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a little confused what this is implying

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, can't this be simplified to:

let processed_era = match &self.current_era {
    Some(processed_era) if processed_era.contains_block(block_number) => {
        processed_era
    }
    ...
};

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this but it gets mad at me saying *self is used, but then mut self is used

}
_ => {
let current_era = self.fetch_era_file().await?;
self.current_era.insert(current_era)
}
};
Ok(processed_era.get_block(self.current_block_number))
}
let block = processed_era.get_block(self.next_block_number);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It should, but I think we can't be sure that processed_era actually contains the next_block_number (because of the _ => { case above).

It's better to check than panic (in my opinion), so I would add ensure! just before this line.

self.next_block_number += 1;

async fn fetch_era_file_link(
&self,
block_number: u64,
epoch_index: u64,
) -> anyhow::Result<String> {
match EraType::for_block_number(block_number) {
EraType::Era1 => {
let era1_file = self
.era1_files
.iter()
.find(|file| file.contains(&format!("mainnet-{epoch_index:05}-")))
.expect("to be able to find era file")
.clone();
Ok(era1_file)
}
EraType::Era => {
let era_files = get_era_files(&self.http_client).await?;
Ok(era_files[&(epoch_index)].clone())
}
}
Ok(block)
}

async fn fetch_era_file(&mut self) -> anyhow::Result<ProcessedEra> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this function name doesn't make much sense to me anymore. I would rename it to something like get_next_processed_era and add comment describing what it actually does (getting processed era, and start processing next one).

Expand All @@ -101,30 +86,29 @@ impl EraManager {
let current_era = self.next_era.take();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I find this naming a bit confusing. How about:

let processed_era = match self.next_era.take() {
    Some(era_handle) => era_handle.await??,
    None => bail!("The next_era handle can't be None"),
};

Or at least new_current_era if you don't like processed_era?

Also, comment above this line is no longer valid.

let current_era = match current_era {
Some(era_handle) => era_handle.await?,
None => self.init_current_era(self.current_block_number).await?,
};
None => return Err(anyhow::anyhow!("EraManager not initialized")),
}?;

// Download the next era file
let mut next_era_type = current_era.era_type;
let mut next_epoch_index = current_era.epoch_index + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this whole block (lines 108:117) a bit hard to read and understand. I think we can make it very simple:

let mut next_era_type = current_era.era_type;
let mut next_epoch_index = current_era.epoch_index + 1;
// Handle transition from era1 to era
if next_era_type == EraType::Era1 && next_epoch_index == ERA1_FILE_COUNT {
    next_era_type = EraType::Era;
    next_epoch_index = FIRST_ERA_EPOCH_WITH_EXECUTION_PAYLOAD;
}
let next_era_path = self.fetch_era_file_link(next_era_type, next_epoch_index).await?;

// Handle transition from era1 to era
if current_era.era_type == EraType::Era1 && next_epoch_index == ERA1_FILE_COUNT as u64 {
if next_era_type == EraType::Era1 && next_epoch_index == ERA1_FILE_COUNT as u64 {
next_era_type = EraType::Era;
next_epoch_index = FIRST_ERA_EPOCH_WITH_EXECUTION_PAYLOAD;
}
let next_block_number = current_era.first_block_number + current_era.len() as u64;
let next_era_path = self
.fetch_era_file_link(next_block_number, next_epoch_index)
.await?;
let era_type = EraType::for_block_number(next_block_number);
let Some(next_era_path) = (match next_era_type {
EraType::Era1 => self.era1_files.get(&next_epoch_index).cloned(),
EraType::Era => self.era_files.get(&next_epoch_index).cloned(),
}) else {
return Err(anyhow::anyhow!("Unable to find next era file's path: index {next_epoch_index} type {next_era_type:?}"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we should return error here. This is valid thing to happen if we are processing most recent era file.

I would say, we have two approaches:

  1. pass next_era_path as Option<String> to the spawn task below, which would return error
    • or maybe even better, in case of an EraType::Era, it can try to download most recent era_files
  2. if next_era_path is None, skip next few lines and leave self.next_era as empty (and update message at line 89)

I think I like option 1 more, but up to you.

};
let http_client = self.http_client.clone();
let join_handle = tokio::spawn(async move {
let raw_era = download_raw_era(next_era_path, http_client.clone())
.await
.expect("to be able to download raw era");
match era_type {
EraType::Era1 => process_era1_file(raw_era, next_epoch_index)
.expect("to be able to process era1 file"),
EraType::Era => process_era_file(raw_era, next_epoch_index)
.expect("to be able to process era file"),
let raw_era = download_raw_era(next_era_path, http_client.clone()).await?;
match next_era_type {
EraType::Era1 => process_era1_file(raw_era, next_epoch_index),
EraType::Era => process_era_file(raw_era, next_epoch_index),
}
});
self.next_era = Some(join_handle);
Expand All @@ -135,15 +119,23 @@ impl EraManager {
/// The first time using era_manager we need to find the current era file we are starting from
/// If the block is from a era file we will need to binary search to find which era file
/// contains the block we want
async fn init_current_era(&mut self, block_number: u64) -> anyhow::Result<ProcessedEra> {
async fn init_current_era(
era1_files: HashMap<u64, String>,
http_client: Client,
block_number: u64,
) -> anyhow::Result<ProcessedEra> {
if let EraType::Era1 = EraType::for_block_number(block_number) {
let epoch_index = Era1::epoch_number_from_block_number(block_number);
let era_path = self.fetch_era_file_link(block_number, epoch_index).await?;
let raw_era1 = download_raw_era(era_path, self.http_client.clone()).await?;
let Some(era1_path) = era1_files.get(&epoch_index).cloned() else {
return Err(anyhow::anyhow!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think you can replace return Err(anyhow::anyhow!( with anyhow::bail!(

"Unable to find era1 file's path during initialization: index {epoch_index}"
));
};
let raw_era1 = download_raw_era(era1_path, http_client.clone()).await?;

return process_era1_file(raw_era1, epoch_index);
}

EraBinarySearch::find_era_file(self.http_client.clone(), block_number).await
EraBinarySearch::find_era_file(http_client.clone(), block_number).await
}
}
2 changes: 1 addition & 1 deletion trin-execution/src/era/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ impl ProcessedEra {
}
}

#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum EraType {
Era,
Era1,
Expand Down
2 changes: 2 additions & 0 deletions trin-execution/src/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,8 @@ mod tests {
let raw_era1 = fs::read("../test_assets/era1/mainnet-00000-5ec1ffb8.era1").unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should be using both raw file and EraManager (it should be one or the other).

I would say we shouldn't use EraManager in tests if we can avoid it (because EraManager would download from public sources).

I think we can obtain ProcessedEra directly from raw_era1, and remove usage of EraManager in this test.

era::utils::process_era1_file(raw_era1, /* epoch_index= */ 0)

for block_tuple in Era1::iter_tuples(raw_era1) {
if block_tuple.header.header.number == 0 {
// skip genesis block
era_manager.get_next_block().await.unwrap();
continue;
}
state
Expand Down
6 changes: 1 addition & 5 deletions trin-execution/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
}

let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["fetching_block_from_era"]);
let block = if block_number == starting_block_number {
era_manager.get_current_block().await?
} else {
era_manager.get_next_block().await?
};
let block = era_manager.get_next_block().await?;
stop_timer(timer);
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["processing_block"]);
if block.header.number == 0 {
Expand Down
6 changes: 1 addition & 5 deletions trin-execution/tests/content_generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,7 @@ async fn test_we_can_generate_content_key_values_up_to_x() -> Result<()> {

for block_number in 0..=blocks {
println!("Starting block: {block_number}");
let block = if block_number == 0 {
era_manager.get_current_block().await?
} else {
era_manager.get_next_block().await?
};
let block = era_manager.get_next_block().await?;
ensure!(
block_number == block.header.number,
"Block number doesn't match!"
Expand Down