Feat/upgrade 0 9 #1
Conversation
Serhii-Braavos
force-pushed
the
feat/upgrade_0_9
branch
from
51c3d25 to
981da71
Compare
| { | ||
| let state = state.clone(); | ||
| let period = Duration::from_secs(config.poll_secs); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Seems like when the main loop preparation process fails, the program stalls and doesn't exit
There was a problem hiding this comment.
added panic
| } | ||
|
|
||
| impl SqlStorageProvider { | ||
| pub async fn new(database_url: &str) -> Result<Self> { |
There was a problem hiding this comment.
Not sure, but perhaps these functions need more robust error handling so the main loop won't quit due to a db error?
There was a problem hiding this comment.
db is essential for LC to work, if it fails the client should not start
| /// | ||
| /// L1 range | ||
| /// | ||
| async fn read_l1_range(&self, block_number: i64) -> Result<L1Range> { |
There was a problem hiding this comment.
isn't it possible to have 2 values returned here?
There was a problem hiding this comment.
the ranges are overwritten if we found a new one, so we'll have
[A; B], [B; C] [C; D], etc
this code can potentially have 2 rows if the value is on the edge, but values on the edge already L2 sync, and we don't need L1 ranges for them
| self.storage().write_l1_ranges(&new_l1_ranges).await?; | ||
|
|
||
| // Fetch the starting state for the minimal range. This will always be present, otherwise we received invalid data from L1. | ||
| let end_state_from_storage = |
There was a problem hiding this comment.
Why not fetch this data before the loop above? is there anything about the loop above that can update the state in storage?
There was a problem hiding this comment.
we need to know the range.
the comment was outdated
| tracing::info!(state=?current, "updated"); | ||
| } | ||
| let _guard = async_blocker.block_tasks(); | ||
| match execute_sync( |
There was a problem hiding this comment.
cant this part be in the background worker as well?
There was a problem hiding this comment.
background worker is a low priority historical sync. This is where we sync the head of the chain. It determines which block light client sees as the 'latest'
| /// | ||
| /// # Panics | ||
| /// Panics if the stored L2 block hash does not match the newly fetched L1 state. | ||
| async fn execute_sync( |
There was a problem hiding this comment.
From what I can tell, it seems the syncing process misses the first range 1000000-1000056, do you see this as well?
There was a problem hiding this comment.
1000000 and 1000056 are added as default data. if you don't request blocks in-between they will not be synced
| let beerus = Arc::new(Client::new(&config.client, http, storage).await?); | ||
| let server = | ||
| beerus::rpc::Server::new(beerus.clone(), async_blocker.clone()); | ||
| let background_loader = |
There was a problem hiding this comment.
won't the two background processes here interfere with each other? it seems they are using different instances of the asynchronous blocker? (because it is cloned below)
There was a problem hiding this comment.
it is Arc::clone which points to the same instance
| let stored_state = | ||
| beerus.storage().read_state(l1_state.block_number).await?; | ||
|
|
||
| assert_eq!( |
There was a problem hiding this comment.
Is this lack of consistency really that uncommon? Isn't there always some difference between what we have on l1 and the latest block on l2? won't this exit the entire process?
There was a problem hiding this comment.
this is fundamental. The blocks that we sync on L2 will have commitments on L1 after some time.
If we found that the block that we processed doesn't match it's commitement on L1 - we have invalid chain in LC
| /// | ||
| /// # Panics | ||
| /// Panics if the stored L2 block hash does not match the newly fetched L1 state. | ||
| async fn execute_sync( |
There was a problem hiding this comment.
Is it implied here that gateway_state.block_number == verified_state.block_number?
There was a problem hiding this comment.
yes, they updated simultaneously
src/client.rs
Outdated
| /// # Errors | ||
| /// Returns an error if any RPC or data conversion fails, the parent hash does not match (if provided), | ||
| /// the Starknet version is unsupported, or validation of the block hash relationship fails. | ||
| pub async fn get_verified_state( |
There was a problem hiding this comment.
Since this function also updates storage, maybe it should have a name that implies it's not just a getter?
| let block_header_copy = block_header.clone(); | ||
| // Validate in a blocking thread since it may be CPU-heavy | ||
| tokio::task::spawn_blocking(move || { | ||
| validate_block_hash_from_header( |
There was a problem hiding this comment.
Is there any reason to have this validation, given that both parameters are extracted from block_header which we got from the rpc? Are we checking if the math is correct?
There was a problem hiding this comment.
we are validating proofs against 'root' that is a part of block hash.
block hashes are needed to validate the chain (because prev block hash is a part of calculations).
Serhii-Braavos
force-pushed
the
feat/upgrade_0_9
branch
from
71f41a4 to
1123f16
Compare
No description provided.