feat(trin-execution): replace EraManager with E2HSManager #1855
Conversation
There was a problem hiding this comment.
nit: maybe rename file to era_binary_search.rs
bin/trin-execution/src/e2hs/utils.rs
Outdated
| attempts += 1; | ||
| if attempts > 10 { | ||
| anyhow::bail!("Failed to download e2store file after {attempts} attempts"); | ||
| } |
There was a problem hiding this comment.
| attempts += 1; | |
| if attempts > 10 { | |
| anyhow::bail!("Failed to download e2store file after {attempts} attempts"); | |
| } | |
| if attempts => 10 { | |
| anyhow::bail!("Failed to download e2store file after {attempts} attempts"); | |
| } | |
| attempts += 1; |
nit: I think log message is off by one
There was a problem hiding this comment.
I think we already have this file here: test_assets/era1/mainnet-00000-a6860fef.e2hs.
Maybe remove it, since it's in the wrong directory.
bin/trin-execution/src/e2hs/types.rs
Outdated
| let first_block_number = self.blocks[0].header.number; | ||
| (first_block_number..first_block_number + BLOCKS_PER_E2HS as u64).contains(&block_number) |
There was a problem hiding this comment.
nit: if we assume that ProcessedE2HS is created correctly (which we already do), we can just check that e2hs index for block_number is the same as self.index.
bin/trin-execution/src/e2hs/types.rs
Outdated
| } else { | ||
| Self::Era | ||
| } | ||
| &self.blocks[block_number as usize - self.blocks[0].header.number as usize] |
There was a problem hiding this comment.
nit: should we panic if block number doesn't match expected one? Up to you.
There was a problem hiding this comment.
nit: If I'm not mistaken, now we always call download_raw_e2store_file and follow it with process_e2hs_file.
Maybe create one more function download_e2hs_file that will download and return ProcessedE2HS
|
|
||
| pub async fn last_fetched_block(&self) -> anyhow::Result<&ProcessedBlock> { | ||
| let Some(current_e2hs) = &self.current_e2hs else { | ||
| panic!("current_e2hs should be initialized in E2HSManager::new"); |
There was a problem hiding this comment.
I think current message is a confusing/incorrect.
There was a problem hiding this comment.
I think you misunderstood my comment. Now it says:
current_e2hs should always be present, perhaps it wasn't initialized in E2HSManager::new()?
Looking at E2HSManager::new, it never initializes current_e2hs. The current_e2hs is set only during get_next_block.
There was a problem hiding this comment.
I think you misunderstood my comment. Now it says: current_e2hs should always be present, perhaps it wasn't initialized in E2HSManager::new()?
Looking at
E2HSManager::new, it never initializescurrent_e2hs. Thecurrent_e2hsis set only duringget_next_block.
I see what you mean now, I made a PR to address this #1857
What was wrong?
Trin Execution was using Era/Era1 files to execute to near the head of the chain. Handling Era/Era1 files adds a lot of complexity. Downloading Era files and processing them is slow and memory intensive. We want to transition to using E2HS files because they are smaller and we can use them to execute near the head of the chain.
Also Era files are generated with a week delay, where we generate E2HS files as soon as we can. The S3 bucket we used has also be very unreliable missing files multiple times last month.
How was it fixed?
Replacing Era/Era1 support for E2HS support