feat(l1-recover): DO NOT MERGE, work in progress #3174
Conversation
| fn commit_functions() -> Result<Vec<Function>> { | ||
| let contract = Self::hyperchain_contract(); | ||
| Ok(vec![ | ||
| contract.functions_by_name("commitBatches").unwrap()[0].clone(), |
| RollupEventType::Execute => "BlockExecution", | ||
| RollupEventType::NewPriorityTx => "NewPriorityRequest", | ||
| }; | ||
| Ok(Self::hyperchain_contract().events_by_name(event_name)?[0].clone()) |
| RollupEventType::Commit => 1, | ||
| RollupEventType::Prove => 2, | ||
| RollupEventType::Execute => 1, | ||
| _ => panic!("{event_type:?} event doesn't have l1_batch_number"), |
| event_type: RollupEventType, | ||
| log: &Log, | ||
| ) -> PriorityOpId { | ||
| assert_eq!(event_type, RollupEventType::NewPriorityTx); |
There was a problem hiding this comment.
why do you need the event_type argument in the first place, if you are just asserting the value?
| eth_client | ||
| } | ||
| /// Get the last published L1 block. | ||
| async fn get_last_l1_block_number(eth_client: &Box<DynClient<L1>>) -> Result<U64> { |
There was a problem hiding this comment.
why it doesn't use &self instead?
| .await?; | ||
|
|
||
| last_block | ||
| .expect("last block must be present") |
| @@ -0,0 +1,1083 @@ | |||
| use std::{cmp, cmp::PartialEq, collections::HashMap, future::Future, sync::Arc}; | |||
|
|
|||
| use anyhow::{anyhow, Result}; | |||
There was a problem hiding this comment.
nit: use "format_err" (alias of anyhow) instead. It is a more meaningful name
| @@ -0,0 +1,1083 @@ | |||
| use std::{cmp, cmp::PartialEq, collections::HashMap, future::Future, sync::Arc}; | |||
|
|
|||
| use anyhow::{anyhow, Result}; | |||
There was a problem hiding this comment.
use qualified anyhow::Result instead, so as not to shadow the Result from prelude, especially since you define custom errors in this file as well.
|
|
||
| fn hyperchain_contract() -> Contract { | ||
| hyperchain_contract() | ||
| } |
There was a problem hiding this comment.
not very useful method, isn't it :P
| log: &Log, | ||
| ) -> PriorityOpId { | ||
| assert_eq!(event_type, RollupEventType::NewPriorityTx); | ||
| L1Tx::try_from(log.clone()).unwrap().common_data.serial_id |
| .await | ||
| .unwrap(); | ||
| let mut end_block = last_l1_block_number; | ||
| // we use step override as such big step value guarantees that we don't miss |
There was a problem hiding this comment.
I don't understand this comment. Can you elaborate?
| end_block = last_l1_block_number; | ||
| let mut current_block = start_block; | ||
| loop { | ||
| let filter_to_block = cmp::min(current_block + step - 1, end_block); |
There was a problem hiding this comment.
fyi, instead of cmp::min you can use min method (no imports required), for example
let filter_to_block = end_block.min(...).
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct CommitBlock { | ||
| /// ZKSync batch number. | ||
| pub l1_batch_number: u64, |
| { | ||
| let commit_block_info = F::try_from(value)?; | ||
| Ok(Self::from_commit_block(commit_block_info.to_enum_variant())) | ||
| } |
There was a problem hiding this comment.
wouldn't https://github.com/matter-labs/zksync-era/tree/main/core/lib/l1_contract_interface be a better place for this logic?
| ) | ||
| .await; | ||
| let tx = | ||
| L1Fetcher::get_transaction_by_hash(&self.eth_client, logs[0].transaction_hash.unwrap()) |
There was a problem hiding this comment.
if you don't want to make these methods of L1Fetcher, then consider wrapping eth_client into yet another type instead and making functions like get_transaction_by_hash methods of that new type instead.
| hyperchain_contract() | ||
| } | ||
|
|
||
| fn commit_functions() -> Result<Vec<Function>> { |
There was a problem hiding this comment.
any specific reason why those functions are members of L1Fetcher class?
| ) -> Vec<CommitBlock> { | ||
| let end_block = L1Fetcher::get_last_l1_block_number(&self.eth_client) | ||
| .await | ||
| .unwrap(); |
| RollupEventsFilter::L1BatchNumber(l1_batch_number + 1), | ||
| ) | ||
| .await | ||
| .unwrap() |
There was a problem hiding this comment.
Return an error. In general in places that you can statically guarantee that an error will never occur (and therefore unwrap call is legitimate), document in a comment why it is the case.
| // This code is compatible with both Infura and Alchemy API providers. | ||
| // Note: we don't handle rate-limits here - assumption is that we're never going to hit them. | ||
| if let Err(err) = &result { | ||
| tracing::warn!("Provider returned error message: {err}"); |
There was a problem hiding this comment.
in general, by default choose "{err:#}" for error printing, as "{err}" tends to not display the cause.
| }; | ||
|
|
||
| // check whether the error is related to having too many results | ||
| if err_message.contains(TOO_MANY_RESULTS_INFURA) |
There was a problem hiding this comment.
just to confirm: there is no error code for these?
| // safety check to prevent infinite recursion (quite unlikely) | ||
| if from_number >= mid { | ||
| tracing::warn!("Infinite recursion detected while getting events: from_number={from_number:?}, mid={mid:?}"); | ||
| return result; |
There was a problem hiding this comment.
if to_number == from_number+1, then this case would trigger, while afaict it would be perfectly fine to split the range further into two 1-element ranges. Also the message is misleading, because the real problem is that a single block has too many events, not that our binary search has infinite recursion.
| } else if should_retry(err_code, err_message) && retries_left > 0 { | ||
| tracing::warn!("Retrying. Retries left: {retries_left}"); | ||
| result = self | ||
| .get_events_inner(from, to, topics1, topics2, addresses, retries_left - 1) |
There was a problem hiding this comment.
nit: it seems wasteful to use recursion for retries instead of a loop, no?
Also the layout/block nesting of this function could be improved, by inverting success and failure branches, i.e.:
let err = match result {
Ok(res) => return res;
Err(err) => err
};
// error handling.
|
|
||
| async fn get_logs( | ||
| &self, | ||
| start_block: U64, |
There was a problem hiding this comment.
nit: it would be nice to have a dedicated type for L1 block numbers (or at least an alias), instead of plain U64
| E: std::fmt::Display, | ||
| { | ||
| sleep(Duration::from_millis(100)).await; | ||
| for attempt in 1..=100 { |
There was a problem hiding this comment.
that's a lot of retries. Why 100?
| Fut: Future<Output = Result<T, E>>, | ||
| E: std::fmt::Display, | ||
| { | ||
| sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
this is an artificial delay. Why is it needed?
| .ok_or_else(|| anyhow!("found latest block, but it contained no block number")) | ||
| } | ||
|
|
||
| async fn retry_call<T, E, Fut>(callback: impl Fn() -> Fut, err: L1FetchError) -> Result<T> |
There was a problem hiding this comment.
why not map_err() at the call site?
| match callback().await { | ||
| Ok(x) => return Ok(x), | ||
| Err(e) => { | ||
| tracing::error!("attempt {attempt}: failed to fetch from L1: {e}"); |
| protocol_versioning: &ProtocolVersioning, | ||
| l1_block_number: u64, | ||
| commit_candidates: &[Function], | ||
| calldata: &[u8], |
There was a problem hiding this comment.
this parsing looks very manual. Can't we make it more robust by using ethabi library somehow? Also l1_contracts_interface could be a better place for this logic.
| /// `MAX_RETRIES` is the maximum number of retries on failed blob retrieval. | ||
| const MAX_RETRIES: u8 = 5; | ||
| /// The interval in seconds to wait before retrying to fetch a blob. | ||
| const FAILED_FETCH_RETRY_INTERVAL_S: u64 = 10; |
There was a problem hiding this comment.
use time::Duration for this constant directly
| }, | ||
| Err(e) => { | ||
| tracing::error!("attempt {}: GET {} failed: {:?}", attempt, full_url, e); | ||
| sleep(Duration::from_secs(FAILED_FETCH_RETRY_INTERVAL_S)).await; |
There was a problem hiding this comment.
invert control flow to avoid spurious indentation, deduplicate sleeps
| if l1_batch_number == log_batch_number { | ||
| return Some(log); | ||
| } else { | ||
| continue; |
There was a problem hiding this comment.
"else { continue }" is not needed
| } | ||
|
|
||
| fn query_client(eth_client: &Box<DynClient<L1>>) -> &dyn EthInterface { | ||
| eth_client |
There was a problem hiding this comment.
this function does explicit casting to an interface. Does rust compiler have problems without it?
No description provided.