From c07576e9964d0c2b0799a58126f4a51c7f03fe88 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Thu, 30 Nov 2023 18:47:44 +0800 Subject: [PATCH 1/5] reset provider --- fendermint/vm/topdown/src/sync/pointers.rs | 5 +++++ fendermint/vm/topdown/src/sync/syncer.rs | 17 +++++++++-------- fendermint/vm/topdown/src/sync/tendermint.rs | 1 + 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/fendermint/vm/topdown/src/sync/pointers.rs b/fendermint/vm/topdown/src/sync/pointers.rs index 6dd6e0c1..34b342f2 100644 --- a/fendermint/vm/topdown/src/sync/pointers.rs +++ b/fendermint/vm/topdown/src/sync/pointers.rs @@ -31,6 +31,11 @@ impl SyncPointers { pub fn set_tail(&mut self, height: BlockHeight, hash: BlockHash) { self.tail = Some((height, hash)); } + + pub fn reset(&mut self, head: BlockHeight) { + self.tail = None; + self.head = head; + } } impl Display for SyncPointers { diff --git a/fendermint/vm/topdown/src/sync/syncer.rs b/fendermint/vm/topdown/src/sync/syncer.rs index 0e3b8784..f7dd1426 100644 --- a/fendermint/vm/topdown/src/sync/syncer.rs +++ b/fendermint/vm/topdown/src/sync/syncer.rs @@ -89,7 +89,7 @@ where chain_head, "reorg detected from height" ); - return self.reset_cache().await; + return self.reset().await; } if !self.has_new_blocks(chain_head) { @@ -106,6 +106,14 @@ where Ok(()) } + + /// Reset the cache in the face of a reorg + pub async fn reset(&mut self) -> anyhow::Result<()> { + let finality = query_starting_finality(&self.query, &self.parent_proxy).await?; + atomically(|| self.provider.reset(finality.clone())).await; + self.sync_pointers.reset(finality.height); + Ok(()) + } } impl LotusParentSyncer @@ -282,13 +290,6 @@ where parent_chain_head_height - self.config.chain_head_delay, )) } - - /// Reset the cache in the face of a reorg - async fn reset_cache(&self) -> anyhow::Result<()> { - let finality = query_starting_finality(&self.query, &self.parent_proxy).await?; - atomically(|| self.provider.reset(finality.clone())).await; - Ok(()) - } } #[cfg(test)] diff --git a/fendermint/vm/topdown/src/sync/tendermint.rs b/fendermint/vm/topdown/src/sync/tendermint.rs index 9101a42a..ac0003d0 100644 --- a/fendermint/vm/topdown/src/sync/tendermint.rs +++ b/fendermint/vm/topdown/src/sync/tendermint.rs @@ -29,6 +29,7 @@ where pub async fn sync(&mut self) -> anyhow::Result<()> { if self.is_syncing_peer().await? { tracing::debug!("syncing with peer, skip parent finality syncing this round"); + self.inner.reset().await?; return Ok(()); } self.inner.sync().await From 012454906b7efd5ab907185d19166311adc1b59b Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Thu, 30 Nov 2023 19:33:29 +0800 Subject: [PATCH 2/5] reset cache when reorg happens --- fendermint/vm/topdown/src/sync/syncer.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fendermint/vm/topdown/src/sync/syncer.rs b/fendermint/vm/topdown/src/sync/syncer.rs index f7dd1426..a4febf3c 100644 --- a/fendermint/vm/topdown/src/sync/syncer.rs +++ b/fendermint/vm/topdown/src/sync/syncer.rs @@ -102,7 +102,14 @@ where return Ok(()); } - self.poll_next().await?; + match self.poll_next().await { + Ok(_) => {} + Err(Error::ParentChainReorgDetected) => { + tracing::warn!("potential reorg detected, clear cache and retry"); + self.reset().await?; + } + Err(e) => return Err(anyhow!(e)), + } Ok(()) } From 73ca827fdcc14422fbd4bb67ada287e3a1b10c70 Mon Sep 17 00:00:00 2001 From: cryptoAtwill <108330426+cryptoAtwill@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:16:17 +0800 Subject: [PATCH 3/5] more env var in infra scirpts (#450) --- infra/scripts/fendermint.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/infra/scripts/fendermint.toml b/infra/scripts/fendermint.toml index 0910c5f0..f20d484e 100644 --- a/infra/scripts/fendermint.toml +++ b/infra/scripts/fendermint.toml @@ -100,6 +100,8 @@ docker run \ --env FM_IPC__TOPDOWN__EXPONENTIAL_BACK_OFF=5 \ --env FM_IPC__TOPDOWN__EXPONENTIAL_RETRY_LIMIT=5 \ --env FM_IPC__TOPDOWN__POLLING_INTERVAL=10 \ + --env FM_IPC__TOPDOWN__PROPOSAL_DELAY=${TOPDOWN_PROPOSAL_DELAY} \ + --env FM_IPC__TOPDOWN__MAX_PROPOSAL_RANGE=${TOPDOWN_MAX_PROPOSAL_RANGE} \ --env FM_ABCI__LISTEN__HOST=0.0.0.0 \ --env FM_ETH__LISTEN__HOST=0.0.0.0 \ --env FM_TENDERMINT_RPC_URL=http://${CMT_CONTAINER_NAME}:26657 \ From 8d2ff3a0ebb2c6a44537189a8aeaf4b31ec0840a Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Mon, 4 Dec 2023 22:12:37 +0800 Subject: [PATCH 4/5] remove head and tail --- fendermint/vm/topdown/src/error.rs | 4 +- fendermint/vm/topdown/src/finality/fetch.rs | 4 + fendermint/vm/topdown/src/finality/null.rs | 18 +- fendermint/vm/topdown/src/sync/mod.rs | 5 +- fendermint/vm/topdown/src/sync/pointers.rs | 55 ------ fendermint/vm/topdown/src/sync/syncer.rs | 204 ++++++++------------ fendermint/vm/topdown/src/toggle.rs | 4 + 7 files changed, 102 insertions(+), 192 deletions(-) delete mode 100644 fendermint/vm/topdown/src/sync/pointers.rs diff --git a/fendermint/vm/topdown/src/error.rs b/fendermint/vm/topdown/src/error.rs index b0780599..f3dc8c0b 100644 --- a/fendermint/vm/topdown/src/error.rs +++ b/fendermint/vm/topdown/src/error.rs @@ -9,7 +9,9 @@ use thiserror::Error; pub enum Error { #[error("Incoming items are not order sequentially")] NotSequential, - #[error("The parent view update with block height is not sequential: {0:?}")] + #[error( + "Cache update wrt height is not sequential, cache might be changed during fetching: {0:?}" + )] NonSequentialParentViewInsert(SequentialAppendError), #[error("Parent chain reorg detected")] ParentChainReorgDetected, diff --git a/fendermint/vm/topdown/src/finality/fetch.rs b/fendermint/vm/topdown/src/finality/fetch.rs index 018317d3..91c53096 100644 --- a/fendermint/vm/topdown/src/finality/fetch.rs +++ b/fendermint/vm/topdown/src/finality/fetch.rs @@ -233,6 +233,10 @@ impl CachedFinalityProvider { pub fn cached_blocks(&self) -> Stm { self.inner.cached_blocks() } + + pub fn first_non_null_block_before(&self, height: BlockHeight) -> Stm> { + self.inner.first_non_null_block_before(height) + } } #[cfg(test)] diff --git a/fendermint/vm/topdown/src/finality/null.rs b/fendermint/vm/topdown/src/finality/null.rs index 5bc778c0..7b6c0ab6 100644 --- a/fendermint/vm/topdown/src/finality/null.rs +++ b/fendermint/vm/topdown/src/finality/null.rs @@ -145,20 +145,25 @@ impl FinalityWithNull { /// Get the latest height tracked in the provider, includes both cache and last committed finality pub(crate) fn latest_height(&self) -> Stm> { let h = if let Some(h) = self.latest_height_in_cache()? { + tracing::debug!(height = h, "latest height found in cache"); h } else if let Some(p) = self.last_committed_finality()? { + tracing::debug!( + height = p.height, + "no cache, use previous last committed finality" + ); p.height } else { return Ok(None); }; Ok(Some(h)) } -} -/// All the private functions -impl FinalityWithNull { - /// Get the first non-null block in the range [start, end]. - fn first_non_null_block_before(&self, height: BlockHeight) -> Stm> { + /// Get the first non-null block in the range of earliest cache block till the height specified. + pub(crate) fn first_non_null_block_before( + &self, + height: BlockHeight, + ) -> Stm> { let cache = self.cached_data.read()?; Ok(cache.lower_bound().and_then(|lower_bound| { for h in (lower_bound..height).rev() { @@ -169,7 +174,10 @@ impl FinalityWithNull { None })) } +} +/// All the private functions +impl FinalityWithNull { fn propose_next_height(&self) -> Stm> { let latest_height = if let Some(h) = self.latest_height_in_cache()? { h diff --git a/fendermint/vm/topdown/src/sync/mod.rs b/fendermint/vm/topdown/src/sync/mod.rs index 9d30aea3..e5bd0776 100644 --- a/fendermint/vm/topdown/src/sync/mod.rs +++ b/fendermint/vm/topdown/src/sync/mod.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0, MIT //! A constant running process that fetch or listener to parent state -mod pointers; mod syncer; mod tendermint; @@ -155,9 +154,7 @@ where let mut interval = tokio::time::interval(config.polling_interval); tokio::spawn(async move { - let lotus_syncer = LotusParentSyncer::new(config, parent_client, provider, query) - .await - .expect(""); + let lotus_syncer = LotusParentSyncer::new(config, parent_client, provider, query); let mut tendermint_syncer = TendermintAwareSyncer::new(lotus_syncer, tendermint_client); loop { diff --git a/fendermint/vm/topdown/src/sync/pointers.rs b/fendermint/vm/topdown/src/sync/pointers.rs deleted file mode 100644 index 34b342f2..00000000 --- a/fendermint/vm/topdown/src/sync/pointers.rs +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright 2022-2023 Protocol Labs -// SPDX-License-Identifier: Apache-2.0, MIT - -use crate::{BlockHash, BlockHeight}; -use ethers::utils::hex; -use std::fmt::{Display, Formatter}; - -#[derive(Clone, Debug)] -pub(crate) struct SyncPointers { - tail: Option<(BlockHeight, BlockHash)>, - head: BlockHeight, -} - -impl SyncPointers { - pub fn new(head: BlockHeight) -> Self { - Self { tail: None, head } - } - - pub fn head(&self) -> BlockHeight { - self.head - } - - pub fn tail(&self) -> Option<(BlockHeight, BlockHash)> { - self.tail.clone() - } - - pub fn advance_head(&mut self) { - self.head += 1; - } - - pub fn set_tail(&mut self, height: BlockHeight, hash: BlockHash) { - self.tail = Some((height, hash)); - } - - pub fn reset(&mut self, head: BlockHeight) { - self.tail = None; - self.head = head; - } -} - -impl Display for SyncPointers { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - if let Some((height, hash)) = &self.tail { - write!( - f, - "{{tail: {{height: {}, hash: {}}}, head: {}}}", - height, - hex::encode(hash), - self.head - ) - } else { - write!(f, "{{tail: None, head: {}}}", self.head) - } - } -} diff --git a/fendermint/vm/topdown/src/sync/syncer.rs b/fendermint/vm/topdown/src/sync/syncer.rs index a4febf3c..3ffdf800 100644 --- a/fendermint/vm/topdown/src/sync/syncer.rs +++ b/fendermint/vm/topdown/src/sync/syncer.rs @@ -4,7 +4,6 @@ use crate::finality::ParentViewPayload; use crate::proxy::ParentQueryProxy; -use crate::sync::pointers::SyncPointers; use crate::sync::{query_starting_finality, ParentFinalityStateQuery}; use crate::{ is_null_round_str, BlockHash, BlockHeight, CachedFinalityProvider, Config, Error, Toggle, @@ -21,9 +20,6 @@ pub(crate) struct LotusParentSyncer { parent_proxy: Arc

, provider: Arc>>, query: Arc, - - /// The pointers that indicate which height to poll parent next - sync_pointers: SyncPointers, } impl LotusParentSyncer @@ -31,69 +27,45 @@ where T: ParentFinalityStateQuery + Send + Sync + 'static, P: ParentQueryProxy + Send + Sync + 'static, { - pub async fn new( + pub fn new( config: Config, parent_proxy: Arc

, provider: Arc>>, query: Arc, - ) -> anyhow::Result { - let last_committed_finality = atomically(|| provider.last_committed_finality()) - .await - .ok_or_else(|| anyhow!("parent finality not ready"))?; - - Ok(Self { + ) -> Self { + Self { config, parent_proxy, provider, query, - sync_pointers: SyncPointers::new(last_committed_finality.height), - }) + } } - /// There are 2 pointers, each refers to a block height, when syncing with parent. As Lotus has - /// delayed execution and null round, we need to ensure the topdown messages and validator - /// changes polled are indeed finalized and executed. The following three pointers are introduced: - /// - tail: The next block height in cache to be confirmed executed, could be None - /// - head: The latest block height fetched in cache, finalized but may not be executed. - /// - /// Say we have block chain as follows: - /// NonNullBlock(1) -> NonNullBlock(2) -> NullBlock(3) -> NonNullBlock(4) -> NullBlock(5) -> NonNullBlock(6) - /// and block height 1 is the previously finalized and executed block height. - /// - /// At the beginning, head == 1 and tail == None. With a new block height fetched, - /// `head = 2`. Since height at 2 is not a null block, `tail = Some(2)`, because we cannot be sure - /// block 2 has executed yet. When a new block is fetched, `head = 3`. Since head is a null block, we - /// cannot confirm block height 2. When `head = 4`, it's not a null block, we can confirm block 2 is - /// executed (also with some checks to ensure no reorg has occurred). We fetch block 2's data and set - /// `tail = Some(4)`. - /// The data fetch at block height 2 is pushed to cache and height 2 is ready to be proposed. - /// - /// At height 6, it's block height 4 will be confirmed and its data pushed to cache. At the same - /// time, since block 3 is a null block, empty data will also be pushed to cache. Block 4 is ready - /// to be proposed. + /// Insert the height into cache when we see a new non null block pub async fn sync(&mut self) -> anyhow::Result<()> { let chain_head = if let Some(h) = self.finalized_chain_head().await? { h } else { return Ok(()); }; - tracing::debug!( - chain_head, - pointers = self.sync_pointers.to_string(), - "syncing heights" - ); + let (latest_height_fetched, first_non_null_parent_hash) = self.get_from_cache().await; + tracing::debug!(chain_head, latest_height_fetched, "syncing heights"); - if self.detected_reorg_by_height(chain_head) { + if latest_height_fetched > chain_head { tracing::warn!( - pointers = self.sync_pointers.to_string(), chain_head, - "reorg detected from height" + latest_height_fetched, + "chain head went backwards, potential detected from height" ); return self.reset().await; } - if !self.has_new_blocks(chain_head) { - tracing::debug!("the parent has yet to produce a new block"); + if latest_height_fetched == chain_head { + tracing::debug!( + chain_head, + latest_height_fetched, + "the parent has yet to produce a new block" + ); return Ok(()); } @@ -102,7 +74,10 @@ where return Ok(()); } - match self.poll_next().await { + match self + .poll_next(latest_height_fetched + 1, first_non_null_parent_hash) + .await + { Ok(_) => {} Err(Error::ParentChainReorgDetected) => { tracing::warn!("potential reorg detected, clear cache and retry"); @@ -118,7 +93,6 @@ where pub async fn reset(&mut self) -> anyhow::Result<()> { let finality = query_starting_finality(&self.query, &self.parent_proxy).await?; atomically(|| self.provider.reset(finality.clone())).await; - self.sync_pointers.reset(finality.height); Ok(()) } } @@ -133,11 +107,48 @@ where atomically(|| self.provider.cached_blocks()).await > max_cache_blocks } - /// Poll the next block height. Returns finalized and executed block data. - async fn poll_next(&mut self) -> Result<(), Error> { - let height = self.sync_pointers.head() + 1; - let parent_block_hash = self.non_null_parent_hash().await; + /// Get the data needed to pull the next block + async fn get_from_cache(&self) -> (BlockHeight, BlockHash) { + // we are getting the latest height fetched in cache along with the first non null block + // that is stored in cache. + // we are doing two fetches in one `atomically` as if we get the data in two `atomically`, + // the cache might be updated in between the two calls. `atomically` should guarantee atomicity. + atomically(|| { + let latest_height = if let Some(h) = self.provider.latest_height()? { + h + } else { + unreachable!("guaranteed to have last committed finality, report bug please") + }; + + let prev_non_null_height = + if let Some(h) = self.provider.first_non_null_block_before(latest_height)? { + h + } else { + // guaranteed to have non null parent height because it's guaranteed to have + // last committed finality + unreachable!("guaranteed to have non null parent height, report bug please") + }; + + let hash = if let Some(h) = self.provider.block_hash(prev_non_null_height)? { + h + } else { + unreachable!( + "guaranteed to have hash as the height {} is found", + prev_non_null_height + ) + }; + + Ok((latest_height, hash)) + }) + .await + } + /// Poll the next block height. Returns finalized and executed block data. + async fn poll_next( + &mut self, + height: BlockHeight, + parent_block_hash: BlockHash, + ) -> Result<(), Error> { tracing::debug!( height, parent_block_hash = hex::encode(&parent_block_hash), @@ -149,9 +160,16 @@ where Err(e) => { let err = e.to_string(); if is_null_round_str(&err) { - tracing::debug!(height, "detected null round at height"); + tracing::debug!( + height, + "detected null round at height, inserted None to cache" + ); - self.sync_pointers.advance_head(); + atomically_or_err::<_, Error, _>(|| { + self.provider.new_parent_view(height, None)?; + Ok(()) + }) + .await?; return Ok(()); } @@ -169,38 +187,13 @@ where return Err(Error::ParentChainReorgDetected); } - if let Some((to_confirm_height, to_confirm_hash)) = self.sync_pointers.tail() { - tracing::debug!( - height, - confirm = to_confirm_height, - "non-null round at height, confirmed previous height" - ); - - let data = self.fetch_data(to_confirm_height, to_confirm_hash).await?; - atomically_or_err::<_, Error, _>(|| { - // we only push the null block in cache when we confirmed a block so that in cache - // the latest height is always a confirmed non null block. - let latest_height = self - .provider - .latest_height()? - .expect("provider contains data at this point"); - for h in (latest_height + 1)..to_confirm_height { - self.provider.new_parent_view(h, None)?; - tracing::debug!(height = h, "found null block pushed to cache"); - } - self.provider - .new_parent_view(to_confirm_height, Some(data.clone()))?; - tracing::debug!(height = to_confirm_height, "non-null block pushed to cache"); - Ok(()) - }) - .await?; - } else { - tracing::debug!(height, "non-null round at height, waiting for confirmation"); - }; - - self.sync_pointers - .set_tail(height, block_hash_res.block_hash); - self.sync_pointers.advance_head(); + let data = self.fetch_data(height, block_hash_res.block_hash).await?; + atomically_or_err::<_, Error, _>(|| { + self.provider.new_parent_view(height, Some(data.clone()))?; + tracing::debug!(height, "non-null block pushed to cache"); + Ok(()) + }) + .await?; Ok(()) } @@ -243,47 +236,6 @@ where Ok((block_hash, changes_res.value, topdown_msgs_res.value)) } - /// We only want the non-null parent block's hash - async fn non_null_parent_hash(&self) -> BlockHash { - if let Some((height, hash)) = self.sync_pointers.tail() { - tracing::debug!( - pending_height = height, - "previous non null parent is the pending confirmation block" - ); - return hash; - }; - - atomically(|| { - Ok(if let Some(h) = self.provider.latest_height_in_cache()? { - tracing::debug!( - previous_confirmed_height = h, - "found previous non null block in cache" - ); - // safe to unwrap as we have height recorded - self.provider.block_hash(h)?.unwrap() - } else if let Some(p) = self.provider.last_committed_finality()? { - tracing::debug!( - previous_confirmed_height = p.height, - "no cache, found previous non null block as last committed finality" - ); - p.block_hash - } else { - unreachable!("guaranteed to non null block hash, report bug please") - }) - }) - .await - } - - fn has_new_blocks(&self, height: BlockHeight) -> bool { - self.sync_pointers.head() < height - } - - fn detected_reorg_by_height(&self, height: BlockHeight) -> bool { - // If the below is true, we are going backwards in terms of block height, the latest block - // height is lower than our previously fetched head. It could be a chain reorg. - self.sync_pointers.head() > height - } - async fn finalized_chain_head(&self) -> anyhow::Result> { let parent_chain_head_height = self.parent_proxy.get_chain_head_height().await?; // sanity check @@ -413,8 +365,6 @@ mod tests { latest_finality: committed_finality, }), ) - .await - .unwrap() } /// Creates a mock of a new parent blockchain view. The key is the height and the value is the diff --git a/fendermint/vm/topdown/src/toggle.rs b/fendermint/vm/topdown/src/toggle.rs index b6650266..c6d86bc4 100644 --- a/fendermint/vm/topdown/src/toggle.rs +++ b/fendermint/vm/topdown/src/toggle.rs @@ -123,4 +123,8 @@ impl

Toggle> { pub fn cached_blocks(&self) -> Stm { self.perform_or_else(|p| p.cached_blocks(), BlockHeight::MAX) } + + pub fn first_non_null_block_before(&self, height: BlockHeight) -> Stm> { + self.perform_or_else(|p| p.first_non_null_block_before(height), None) + } } From f74e3e0581290c57afc05173eb76ee6b9f4a7521 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Mon, 4 Dec 2023 23:13:42 +0800 Subject: [PATCH 5/5] fix unit tests --- fendermint/vm/topdown/src/finality/fetch.rs | 4 +- fendermint/vm/topdown/src/finality/null.rs | 144 +++++++++++++------- fendermint/vm/topdown/src/sync/syncer.rs | 94 ++++--------- fendermint/vm/topdown/src/toggle.rs | 4 +- 4 files changed, 127 insertions(+), 119 deletions(-) diff --git a/fendermint/vm/topdown/src/finality/fetch.rs b/fendermint/vm/topdown/src/finality/fetch.rs index 91c53096..714e7003 100644 --- a/fendermint/vm/topdown/src/finality/fetch.rs +++ b/fendermint/vm/topdown/src/finality/fetch.rs @@ -234,8 +234,8 @@ impl CachedFinalityProvider { self.inner.cached_blocks() } - pub fn first_non_null_block_before(&self, height: BlockHeight) -> Stm> { - self.inner.first_non_null_block_before(height) + pub fn first_non_null_block(&self, height: BlockHeight) -> Stm> { + self.inner.first_non_null_block(height) } } diff --git a/fendermint/vm/topdown/src/finality/null.rs b/fendermint/vm/topdown/src/finality/null.rs index 7b6c0ab6..5540fc3a 100644 --- a/fendermint/vm/topdown/src/finality/null.rs +++ b/fendermint/vm/topdown/src/finality/null.rs @@ -159,14 +159,11 @@ impl FinalityWithNull { Ok(Some(h)) } - /// Get the first non-null block in the range of earliest cache block till the height specified. - pub(crate) fn first_non_null_block_before( - &self, - height: BlockHeight, - ) -> Stm> { + /// Get the first non-null block in the range of earliest cache block till the height specified, inclusive. + pub(crate) fn first_non_null_block(&self, height: BlockHeight) -> Stm> { let cache = self.cached_data.read()?; Ok(cache.lower_bound().and_then(|lower_bound| { - for h in (lower_bound..height).rev() { + for h in (lower_bound..=height).rev() { if let Some(Some(_)) = cache.get_value(h) { return Some(h); } @@ -196,18 +193,17 @@ impl FinalityWithNull { let candidate_height = min(max_proposal_height, latest_height); tracing::debug!(max_proposal_height, candidate_height, "propose heights"); - let first_non_null_height = - if let Some(h) = self.first_non_null_block_before(candidate_height)? { - h - } else { - tracing::debug!(height = candidate_height, "no non-null block found before"); - return Ok(None); - }; + let first_non_null_height = if let Some(h) = self.first_non_null_block(candidate_height)? { + h + } else { + tracing::debug!(height = candidate_height, "no non-null block found before"); + return Ok(None); + }; tracing::debug!(first_non_null_height, candidate_height); // an extra layer of delay let maybe_proposal_height = - self.first_non_null_block_before(first_non_null_height - self.config.proposal_delay())?; + self.first_non_null_block(first_non_null_height - self.config.proposal_delay())?; tracing::debug!( delayed_height = maybe_proposal_height, delay = self.config.proposal_delay() @@ -395,19 +391,18 @@ mod tests { let parent_blocks = vec![ (100, Some((vec![0; 32], vec![], vec![]))), // last committed block (101, Some((vec![1; 32], vec![], vec![]))), // cache start - (102, Some((vec![2; 32], vec![], vec![]))), // final proposal height - (103, Some((vec![3; 32], vec![], vec![]))), // final delayed height - (104, Some((vec![4; 32], vec![], vec![]))), - (105, Some((vec![5; 32], vec![], vec![]))), // first non null block - (106, Some((vec![6; 32], vec![], vec![]))), // max proposal height (last committed + 6) - (107, Some((vec![7; 32], vec![], vec![]))), - (108, Some((vec![8; 32], vec![], vec![]))), // cache latest height + (102, Some((vec![2; 32], vec![], vec![]))), + (103, Some((vec![3; 32], vec![], vec![]))), + (104, Some((vec![4; 32], vec![], vec![]))), // final delayed height + proposal height + (105, Some((vec![5; 32], vec![], vec![]))), + (106, Some((vec![6; 32], vec![], vec![]))), // max proposal height (last committed + 6), first non null block + (107, Some((vec![7; 32], vec![], vec![]))), // cache latest height ]; let provider = new_provider(parent_blocks).await; let f = IPCParentFinality { - height: 102, - block_hash: vec![2; 32], + height: 104, + block_hash: vec![4; 32], }; assert_eq!( atomically(|| provider.next_proposal()).await, @@ -427,7 +422,7 @@ mod tests { ); // this ensures sequential insertion is still valid - atomically_or_err(|| provider.new_parent_view(109, None)) + atomically_or_err(|| provider.new_parent_view(108, None)) .await .unwrap(); } @@ -437,11 +432,11 @@ mod tests { // max_proposal_range is 6. proposal_delay is 2 let parent_blocks = vec![ (100, Some((vec![0; 32], vec![], vec![]))), // last committed block - (101, Some((vec![1; 32], vec![], vec![]))), // cache start and final height - (102, Some((vec![2; 32], vec![], vec![]))), // delayed height - (103, Some((vec![3; 32], vec![], vec![]))), - (104, Some((vec![4; 32], vec![], vec![]))), // first non null block - (105, Some((vec![4; 32], vec![], vec![]))), // cache latest height + (101, Some((vec![1; 32], vec![], vec![]))), + (102, Some((vec![2; 32], vec![], vec![]))), + (103, Some((vec![3; 32], vec![], vec![]))), // delayed height + final height + (104, Some((vec![4; 32], vec![], vec![]))), + (105, Some((vec![4; 32], vec![], vec![]))), // cache latest height, first non null block // max proposal height is 106 ]; let provider = new_provider(parent_blocks).await; @@ -449,8 +444,8 @@ mod tests { assert_eq!( atomically(|| provider.next_proposal()).await, Some(IPCParentFinality { - height: 101, - block_hash: vec![1; 32] + height: 103, + block_hash: vec![3; 32] }) ); } @@ -467,15 +462,44 @@ mod tests { (107, None), (108, None), (109, None), - (110, Some((vec![4; 32], vec![], vec![]))), // cache latest height + (110, Some((vec![4; 32], vec![], vec![]))), // first non null block + (111, None), // cache latest height // max proposal height is 112 ]; let mut provider = new_provider(parent_blocks).await; - provider.config.max_proposal_range = Some(8); + provider.config.max_proposal_range = Some(10); assert_eq!(atomically(|| provider.next_proposal()).await, None); } + #[tokio::test] + async fn test_with_some_null_blocks() { + // max_proposal_range is 10. proposal_delay is 2 + let parent_blocks = vec![ + (102, Some((vec![2; 32], vec![], vec![]))), // last committed block + (103, None), + (104, None), + (105, Some((vec![5; 32], vec![], vec![]))), // final block + (106, None), + (107, None), + (108, None), // delayed block + (109, None), + (110, Some((vec![10; 32], vec![], vec![]))), // first non null block + (111, None), // cache latest height + // max proposal height is 112 + ]; + let mut provider = new_provider(parent_blocks).await; + provider.config.max_proposal_range = Some(10); + + assert_eq!( + atomically(|| provider.next_proposal()).await, + Some(IPCParentFinality { + height: 105, + block_hash: vec![5; 32] + }) + ); + } + #[tokio::test] async fn test_with_partially_null_blocks_i() { // max_proposal_range is 10. proposal_delay is 2 @@ -485,14 +509,14 @@ mod tests { (104, None), // we wont have a proposal because after delay, there is no more non-null proposal (105, None), (106, None), - (107, Some((vec![7; 32], vec![], vec![]))), // first non null block - (108, None), - (109, None), - (110, Some((vec![10; 32], vec![], vec![]))), // cache latest height + (107, None), + (108, None), // delayed block + (109, Some((vec![8; 32], vec![], vec![]))), + (110, Some((vec![10; 32], vec![], vec![]))), // cache latest height, first non null block // max proposal height is 112 ]; let mut provider = new_provider(parent_blocks).await; - provider.config.max_proposal_range = Some(8); + provider.config.max_proposal_range = Some(10); assert_eq!(atomically(|| provider.next_proposal()).await, None); } @@ -502,24 +526,52 @@ mod tests { // max_proposal_range is 10. proposal_delay is 2 let parent_blocks = vec![ (102, Some((vec![2; 32], vec![], vec![]))), // last committed block - (103, Some((vec![3; 32], vec![], vec![]))), // first non null delayed block, final + (103, Some((vec![3; 32], vec![], vec![]))), (104, None), - (105, None), // delayed block + (105, None), (106, None), - (107, Some((vec![7; 32], vec![], vec![]))), // first non null block - (108, None), + (107, Some((vec![7; 32], vec![], vec![]))), // first non null after delay + (108, None), // delayed block (109, None), - (110, Some((vec![10; 32], vec![], vec![]))), // cache latest height + (110, Some((vec![10; 32], vec![], vec![]))), // cache latest height, first non null block // max proposal height is 112 ]; let mut provider = new_provider(parent_blocks).await; - provider.config.max_proposal_range = Some(8); + provider.config.max_proposal_range = Some(10); assert_eq!( atomically(|| provider.next_proposal()).await, Some(IPCParentFinality { - height: 103, - block_hash: vec![3; 32] + height: 107, + block_hash: vec![7; 32] + }) + ); + } + + #[tokio::test] + async fn test_with_partially_null_blocks_iii() { + let parent_blocks = vec![ + (102, Some((vec![2; 32], vec![], vec![]))), // last committed block + (103, Some((vec![3; 32], vec![], vec![]))), + (104, None), + (105, None), + (106, None), + (107, Some((vec![7; 32], vec![], vec![]))), // first non null delayed block, final + (108, None), // delayed block + (109, None), + (110, Some((vec![10; 32], vec![], vec![]))), // first non null block + (111, None), + (112, None), + // max proposal height is 122 + ]; + let mut provider = new_provider(parent_blocks).await; + provider.config.max_proposal_range = Some(20); + + assert_eq!( + atomically(|| provider.next_proposal()).await, + Some(IPCParentFinality { + height: 107, + block_hash: vec![7; 32] }) ); } diff --git a/fendermint/vm/topdown/src/sync/syncer.rs b/fendermint/vm/topdown/src/sync/syncer.rs index 3ffdf800..d9a6f10b 100644 --- a/fendermint/vm/topdown/src/sync/syncer.rs +++ b/fendermint/vm/topdown/src/sync/syncer.rs @@ -120,13 +120,19 @@ where unreachable!("guaranteed to have last committed finality, report bug please") }; + // first try to get the first non null block before latest_height + 1, i.e. from cache let prev_non_null_height = - if let Some(h) = self.provider.first_non_null_block_before(latest_height)? { - h + if let Some(height) = self.provider.first_non_null_block(latest_height)? { + tracing::debug!(height, "first non null block in cache"); + height + } else if let Some(p) = self.provider.last_committed_finality()? { + tracing::debug!( + height = p.height, + "first non null block not in cache, use latest finality" + ); + p.height } else { - // guaranteed to have non null parent height because it's guaranteed to have - // last committed finality - unreachable!("guaranteed to have non null parent height, report bug please") + unreachable!("guaranteed to have last committed finality, report bug please") }; let hash = if let Some(h) = self.provider.block_hash(prev_non_null_height)? { @@ -387,36 +393,18 @@ mod tests { 101 => Some(vec![1; 32]), 102 => Some(vec![2; 32]), 103 => Some(vec![3; 32]), - 104 => Some(vec![4; 32]), - 105 => Some(vec![5; 32]) // chain head + 104 => Some(vec![4; 32]), // after chain head delay, we fetch only to here + 105 => Some(vec![5; 32]), + 106 => Some(vec![6; 32]) // chain head ); let mut syncer = new_syncer(parent_blocks).await; - assert_eq!(syncer.sync_pointers.head(), 100); - assert_eq!(syncer.sync_pointers.tail(), None); - - // sync block 101, which is a non-null block - let r = syncer.sync().await; - assert!(r.is_ok()); - assert_eq!(syncer.sync_pointers.head(), 101); - assert_eq!(syncer.sync_pointers.tail(), Some((101, vec![1; 32]))); - // latest height is None as we are yet to confirm block 101, so latest height should equal - // to the last committed finality initialized, which is the genesis block 100 - assert_eq!( - atomically(|| syncer.provider.latest_height()).await, - Some(100) - ); - - // sync block 101, which is a non-null block - let r = syncer.sync().await; - assert!(r.is_ok()); - assert_eq!(syncer.sync_pointers.head(), 102); - assert_eq!(syncer.sync_pointers.tail(), Some((102, vec![2; 32]))); - assert_eq!( - atomically(|| syncer.provider.latest_height()).await, - Some(101) - ); + for h in 101..=104 { + syncer.sync().await.unwrap(); + let p = atomically(|| syncer.provider.latest_height()).await; + assert_eq!(p, Some(h)); + } } #[tokio::test] @@ -438,45 +426,13 @@ mod tests { let mut syncer = new_syncer(parent_blocks).await; - assert_eq!(syncer.sync_pointers.head(), 100); - assert_eq!(syncer.sync_pointers.tail(), None); - // sync block 101 to 103, which are null blocks - for h in 101..=103 { - let r = syncer.sync().await; - assert!(r.is_ok()); - assert_eq!(syncer.sync_pointers.head(), h); - assert_eq!(syncer.sync_pointers.tail(), None); - } - - // sync block 104, which is a non-null block - syncer.sync().await.unwrap(); - assert_eq!(syncer.sync_pointers.head(), 104); - assert_eq!(syncer.sync_pointers.tail(), Some((104, vec![4; 32]))); - // latest height is None as we are yet to confirm block 104, so latest height should equal - // to the last committed finality initialized, which is the genesis block 100 - assert_eq!( - atomically(|| syncer.provider.latest_height()).await, - Some(100) - ); - - // sync block 105 to 107, which are null blocks - for h in 105..=107 { - let r = syncer.sync().await; - assert!(r.is_ok()); - assert_eq!(syncer.sync_pointers.head(), h); - assert_eq!(syncer.sync_pointers.tail(), Some((104, vec![4; 32]))); + for h in 101..=109 { + syncer.sync().await.unwrap(); + assert_eq!( + atomically(|| syncer.provider.latest_height()).await, + Some(h) + ); } - - // sync block 108, which is a non-null block - syncer.sync().await.unwrap(); - assert_eq!(syncer.sync_pointers.head(), 108); - assert_eq!(syncer.sync_pointers.tail(), Some((108, vec![5; 32]))); - // latest height is None as we are yet to confirm block 108, so latest height should equal - // to the previous confirmed block, which is 104 - assert_eq!( - atomically(|| syncer.provider.latest_height()).await, - Some(104) - ); } } diff --git a/fendermint/vm/topdown/src/toggle.rs b/fendermint/vm/topdown/src/toggle.rs index c6d86bc4..68a70bbf 100644 --- a/fendermint/vm/topdown/src/toggle.rs +++ b/fendermint/vm/topdown/src/toggle.rs @@ -124,7 +124,7 @@ impl

Toggle> { self.perform_or_else(|p| p.cached_blocks(), BlockHeight::MAX) } - pub fn first_non_null_block_before(&self, height: BlockHeight) -> Stm> { - self.perform_or_else(|p| p.first_non_null_block_before(height), None) + pub fn first_non_null_block(&self, height: BlockHeight) -> Stm> { + self.perform_or_else(|p| p.first_non_null_block(height), None) } }