From 5ebf17bfe1a1262716aed07c57246e697724610a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 3 Aug 2022 10:46:10 +0200 Subject: [PATCH] Revert non-best block (#11716) * Revert non-best block This makes `revert` also revert non-best blocks. * Update client/db/src/lib.rs * Do not count leaves against the maximum number to revert * Add some explanation * Fix bug * Apply suggestions from code review Co-authored-by: Davide Galassi Co-authored-by: Davide Galassi --- substrate/client/api/src/backend.rs | 3 +- substrate/client/api/src/leaves.rs | 5 + substrate/client/db/src/lib.rs | 149 ++++++++++++++---- .../service/src/chain_ops/revert_chain.rs | 7 + 4 files changed, 135 insertions(+), 29 deletions(-) diff --git a/substrate/client/api/src/backend.rs b/substrate/client/api/src/backend.rs index af8552886b..54784a2f27 100644 --- a/substrate/client/api/src/backend.rs +++ b/substrate/client/api/src/backend.rs @@ -507,7 +507,8 @@ pub trait Backend: AuxStore + Send + Sync { /// Attempts to revert the chain by `n` blocks. If `revert_finalized` is set it will attempt to /// revert past any finalized block, this is unsafe and can potentially leave the node in an - /// inconsistent state. + /// inconsistent state. All blocks higher than the best block are also reverted and not counting + /// towards `n`. /// /// Returns the number of blocks that were successfully reverted and the list of finalized /// blocks that has been reverted. diff --git a/substrate/client/api/src/leaves.rs b/substrate/client/api/src/leaves.rs index 2e5d4be3a5..26eda46e6f 100644 --- a/substrate/client/api/src/leaves.rs +++ b/substrate/client/api/src/leaves.rs @@ -259,6 +259,11 @@ where removed } + + /// Returns the highest leaf and all hashes associated to it. + pub fn highest_leaf(&self) -> Option<(N, &[H])> { + self.storage.iter().next().map(|(k, v)| (k.0.clone(), &v[..])) + } } /// Helper for undoing operations. diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index 7dd49f9831..e14d3a26aa 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -2057,36 +2057,46 @@ impl sc_client_api::backend::Backend for Backend { ) -> ClientResult<(NumberFor, HashSet)> { let mut reverted_finalized = HashSet::new(); - let mut best_number = self.blockchain.info().best_number; - let mut best_hash = self.blockchain.info().best_hash; + let info = self.blockchain.info(); - let finalized = self.blockchain.info().finalized_number; + let highest_leaf = self + .blockchain + .leaves + .read() + .highest_leaf() + .and_then(|(n, h)| h.last().map(|h| (n, *h))); + + let best_number = info.best_number; + let best_hash = info.best_hash; + + let finalized = info.finalized_number; let revertible = best_number - finalized; let n = if !revert_finalized && revertible < n { revertible } else { n }; + let (n, mut number_to_revert, mut hash_to_revert) = match highest_leaf { + Some((l_n, l_h)) => (n + (l_n - best_number), l_n, l_h), + None => (n, best_number, best_hash), + }; + let mut revert_blocks = || -> ClientResult> { for c in 0..n.saturated_into::() { - if best_number.is_zero() { + if number_to_revert.is_zero() { return Ok(c.saturated_into::>()) } let mut transaction = Transaction::new(); let removed = - self.blockchain.header(BlockId::Number(best_number))?.ok_or_else(|| { + self.blockchain.header(BlockId::Hash(hash_to_revert))?.ok_or_else(|| { sp_blockchain::Error::UnknownBlock(format!( - "Error reverting to {}. Block hash not found.", - best_number + "Error reverting to {}. Block header not found.", + hash_to_revert, )) })?; let removed_hash = removed.hash(); - let prev_number = best_number.saturating_sub(One::one()); - let prev_hash = self.blockchain.hash(prev_number)?.ok_or_else(|| { - sp_blockchain::Error::UnknownBlock(format!( - "Error reverting to {}. Block hash not found.", - best_number - )) - })?; + let prev_number = number_to_revert.saturating_sub(One::one()); + let prev_hash = + if prev_number == best_number { best_hash } else { *removed.parent_hash() }; if !self.have_state_at(&prev_hash, prev_number) { return Ok(c.saturated_into::>()) @@ -2096,12 +2106,15 @@ impl sc_client_api::backend::Backend for Backend { Some(commit) => { apply_state_commit(&mut transaction, commit); - best_number = prev_number; - best_hash = prev_hash; + number_to_revert = prev_number; + hash_to_revert = prev_hash; - let update_finalized = best_number < finalized; + let update_finalized = number_to_revert < finalized; - let key = utils::number_and_hash_to_lookup_key(best_number, &best_hash)?; + let key = utils::number_and_hash_to_lookup_key( + number_to_revert, + &hash_to_revert, + )?; if update_finalized { transaction.set_from_vec( columns::META, @@ -2111,12 +2124,14 @@ impl sc_client_api::backend::Backend for Backend { reverted_finalized.insert(removed_hash); if let Some((hash, _)) = self.blockchain.info().finalized_state { - if hash == best_hash { - if !best_number.is_zero() && - self.have_state_at(&prev_hash, best_number - One::one()) - { + if hash == hash_to_revert { + if !number_to_revert.is_zero() && + self.have_state_at( + &prev_hash, + number_to_revert - One::one(), + ) { let lookup_key = utils::number_and_hash_to_lookup_key( - best_number - One::one(), + number_to_revert - One::one(), prev_hash, )?; transaction.set_from_vec( @@ -2137,13 +2152,16 @@ impl sc_client_api::backend::Backend for Backend { &mut transaction, columns::META, meta_keys::CHILDREN_PREFIX, - best_hash, + hash_to_revert, ); self.storage.db.commit(transaction)?; + + let is_best = number_to_revert < best_number; + self.blockchain.update_meta(MetaUpdate { - hash: best_hash, - number: best_number, - is_best: true, + hash: hash_to_revert, + number: number_to_revert, + is_best, is_finalized: update_finalized, with_state: false, }); @@ -2161,7 +2179,7 @@ impl sc_client_api::backend::Backend for Backend { let mut transaction = Transaction::new(); let mut leaves = self.blockchain.leaves.write(); - leaves.revert(best_hash, best_number); + leaves.revert(hash_to_revert, number_to_revert); leaves.prepare_transaction(&mut transaction, columns::META, meta_keys::LEAF_PREFIX); self.storage.db.commit(transaction)?; @@ -3463,4 +3481,79 @@ pub(crate) mod tests { insert_header_no_head(&backend, 1, block0, [1; 32].into()); assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a]); } + + #[test] + fn revert_non_best_blocks() { + let backend = Backend::::new_test(10, 10); + + let genesis = + insert_block(&backend, 0, Default::default(), None, Default::default(), vec![], None) + .unwrap(); + + let block1 = + insert_block(&backend, 1, genesis, None, Default::default(), vec![], None).unwrap(); + + let block2 = + insert_block(&backend, 2, block1, None, Default::default(), vec![], None).unwrap(); + + let block3 = { + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, BlockId::Number(1)).unwrap(); + let header = Header { + number: 3, + parent_hash: block2, + state_root: BlakeTwo256::trie_root(Vec::new(), StateVersion::V1), + digest: Default::default(), + extrinsics_root: Default::default(), + }; + + op.set_block_data(header.clone(), Some(Vec::new()), None, None, NewBlockState::Normal) + .unwrap(); + + backend.commit_operation(op).unwrap(); + + header.hash() + }; + + let block4 = { + let mut op = backend.begin_operation().unwrap(); + backend.begin_state_operation(&mut op, BlockId::Hash(block2)).unwrap(); + let header = Header { + number: 4, + parent_hash: block3, + state_root: BlakeTwo256::trie_root(Vec::new(), StateVersion::V1), + digest: Default::default(), + extrinsics_root: Default::default(), + }; + + op.set_block_data(header.clone(), Some(Vec::new()), None, None, NewBlockState::Normal) + .unwrap(); + + backend.commit_operation(op).unwrap(); + + header.hash() + }; + + let block3_fork = insert_header_no_head(&backend, 3, block2, Default::default()); + + assert!(backend.have_state_at(&block1, 1)); + assert!(backend.have_state_at(&block2, 2)); + assert!(backend.have_state_at(&block3, 3)); + assert!(backend.have_state_at(&block4, 4)); + assert!(backend.have_state_at(&block3_fork, 3)); + + assert_eq!(backend.blockchain.leaves().unwrap(), vec![block4, block3_fork]); + assert_eq!(4, backend.blockchain.leaves.read().highest_leaf().unwrap().0); + + assert_eq!(3, backend.revert(1, false).unwrap().0); + + assert!(backend.have_state_at(&block1, 1)); + assert!(!backend.have_state_at(&block2, 2)); + assert!(!backend.have_state_at(&block3, 3)); + assert!(!backend.have_state_at(&block4, 4)); + assert!(!backend.have_state_at(&block3_fork, 3)); + + assert_eq!(backend.blockchain.leaves().unwrap(), vec![block1]); + assert_eq!(1, backend.blockchain.leaves.read().highest_leaf().unwrap().0); + } } diff --git a/substrate/client/service/src/chain_ops/revert_chain.rs b/substrate/client/service/src/chain_ops/revert_chain.rs index 9a3ce6024e..3ee4399d06 100644 --- a/substrate/client/service/src/chain_ops/revert_chain.rs +++ b/substrate/client/service/src/chain_ops/revert_chain.rs @@ -40,6 +40,13 @@ where info!("There aren't any non-finalized blocks to revert."); } else { info!("Reverted {} blocks. Best: #{} ({})", reverted.0, info.best_number, info.best_hash); + + if reverted.0 > blocks { + info!( + "Number of reverted blocks is higher than requested \ + because of reverted leaves higher than the best block." + ) + } } Ok(()) }