From c4e20af74d5096cd737c61b5211b8de517d59b2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 6 Jan 2020 14:58:43 +0000 Subject: [PATCH] client: allow reverting finalized blocks (#4535) * client: allow reverting blocks past finality * client: fix leaves reversion * client: extend docs on revert * client: add comment on leaves revert --- substrate/client/api/src/backend.rs | 10 ++- substrate/client/db/src/lib.rs | 89 ++++++++++++++++++--------- substrate/client/src/client.rs | 20 +++++- substrate/client/src/in_mem.rs | 6 +- substrate/client/src/leaves.rs | 35 +++++++++-- substrate/client/src/light/backend.rs | 6 +- 6 files changed, 124 insertions(+), 42 deletions(-) diff --git a/substrate/client/api/src/backend.rs b/substrate/client/api/src/backend.rs index 632c5d5b62..90a0c9aa7b 100644 --- a/substrate/client/api/src/backend.rs +++ b/substrate/client/api/src/backend.rs @@ -283,10 +283,16 @@ pub trait Backend: AuxStore + Send + Sync where Ok(()) } - /// Attempts to revert the chain by `n` blocks. + /// 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. /// /// Returns the number of blocks that were successfully reverted. - fn revert(&self, n: NumberFor) -> sp_blockchain::Result>; + fn revert( + &self, + n: NumberFor, + revert_finalized: bool, + ) -> sp_blockchain::Result>; /// Insert auxiliary data into key-value store. fn insert_aux< diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index 80d71ec149..d8dcf3c0af 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -1484,40 +1484,71 @@ impl sc_client_api::backend::Backend for Backend) -> ClientResult> { - let mut best = self.blockchain.info().best_number; + fn revert(&self, n: NumberFor, revert_finalized: bool) -> ClientResult> { + let mut best_number = self.blockchain.info().best_number; + let mut best_hash = self.blockchain.info().best_hash; let finalized = self.blockchain.info().finalized_number; - let revertible = best - finalized; - let n = if revertible < n { revertible } else { n }; - for c in 0 .. n.saturated_into::() { - if best.is_zero() { - return Ok(c.saturated_into::>()) - } - let mut transaction = DBTransaction::new(); - match self.storage.state_db.revert_one() { - Some(commit) => { - apply_state_commit(&mut transaction, commit); - let removed = self.blockchain.header(BlockId::Number(best))?.ok_or_else( - || sp_blockchain::Error::UnknownBlock( - format!("Error reverting to {}. Block hash not found.", best)))?; + let revertible = best_number - finalized; + let n = if !revert_finalized && revertible < n { + revertible + } else { + n + }; - best -= One::one(); // prev block - let hash = self.blockchain.hash(best)?.ok_or_else( - || sp_blockchain::Error::UnknownBlock( - format!("Error reverting to {}. Block hash not found.", best)))?; - let key = utils::number_and_hash_to_lookup_key(best.clone(), &hash)?; - transaction.put(columns::META, meta_keys::BEST_BLOCK, &key); - transaction.delete(columns::KEY_LOOKUP, removed.hash().as_ref()); - children::remove_children(&mut transaction, columns::META, meta_keys::CHILDREN_PREFIX, hash); - self.storage.db.write(transaction).map_err(db_err)?; - self.blockchain.update_meta(hash, best, true, false); - self.blockchain.leaves.write().revert(removed.hash().clone(), removed.number().clone(), removed.parent_hash().clone()); + let mut revert_blocks = || -> ClientResult> { + for c in 0 .. n.saturated_into::() { + if best_number.is_zero() { + return Ok(c.saturated_into::>()) + } + let mut transaction = DBTransaction::new(); + match self.storage.state_db.revert_one() { + Some(commit) => { + apply_state_commit(&mut transaction, commit); + let removed = self.blockchain.header(BlockId::Number(best_number))?.ok_or_else( + || sp_blockchain::Error::UnknownBlock( + format!("Error reverting to {}. Block hash not found.", best_number)))?; + + best_number -= One::one(); // prev block + best_hash = self.blockchain.hash(best_number)?.ok_or_else( + || sp_blockchain::Error::UnknownBlock( + format!("Error reverting to {}. Block hash not found.", best_number)))?; + + let update_finalized = best_number < finalized; + + let key = utils::number_and_hash_to_lookup_key(best_number.clone(), &best_hash)?; + transaction.put(columns::META, meta_keys::BEST_BLOCK, &key); + if update_finalized { + transaction.put(columns::META, meta_keys::FINALIZED_BLOCK, &key); + } + transaction.delete(columns::KEY_LOOKUP, removed.hash().as_ref()); + children::remove_children(&mut transaction, columns::META, meta_keys::CHILDREN_PREFIX, best_hash); + self.storage.db.write(transaction).map_err(db_err)?; + self.blockchain.update_meta(best_hash, best_number, true, update_finalized); + } + None => return Ok(c.saturated_into::>()) } - None => return Ok(c.saturated_into::>()) } - } - Ok(n) + + Ok(n) + }; + + let reverted = revert_blocks()?; + + let revert_leaves = || -> ClientResult<()> { + let mut transaction = DBTransaction::new(); + let mut leaves = self.blockchain.leaves.write(); + + leaves.revert(best_hash, best_number); + leaves.prepare_transaction(&mut transaction, columns::META, meta_keys::LEAF_PREFIX); + self.storage.db.write(transaction).map_err(db_err)?; + + Ok(()) + }; + + revert_leaves()?; + + Ok(reverted) } fn blockchain(&self) -> &BlockchainDb { diff --git a/substrate/client/src/client.rs b/substrate/client/src/client.rs index 349cd816b2..66e031db9f 100644 --- a/substrate/client/src/client.rs +++ b/substrate/client/src/client.rs @@ -1142,10 +1142,24 @@ impl Client where Ok(()) } - /// Attempts to revert the chain by `n` blocks. Returns the number of blocks that were - /// successfully reverted. + /// Attempts to revert the chain by `n` blocks guaranteeing that no block is + /// reverted past the last finalized block. Returns the number of blocks + /// that were successfully reverted. pub fn revert(&self, n: NumberFor) -> sp_blockchain::Result> { - Ok(self.backend.revert(n)?) + Ok(self.backend.revert(n, false)?) + } + + /// Attempts to revert the chain by `n` blocks disregarding finality. This + /// method will revert any finalized blocks as requested and can potentially + /// lead the node in an inconsistent state. Other modules in the system that + /// persist data and that rely on finality (e.g. consensus parts) will be + /// unaffected by the revert. Use this method with caution and making sure + /// that no other data needs to be reverted for consistency aside from the + /// block data. + /// + /// Returns the number of blocks that were successfully reverted. + pub fn unsafe_revert(&self, n: NumberFor) -> sp_blockchain::Result> { + Ok(self.backend.revert(n, true)?) } /// Get usage info about current client. diff --git a/substrate/client/src/in_mem.rs b/substrate/client/src/in_mem.rs index af3adbbb5c..020f2b9e4e 100644 --- a/substrate/client/src/in_mem.rs +++ b/substrate/client/src/in_mem.rs @@ -707,7 +707,11 @@ where } } - fn revert(&self, _n: NumberFor) -> sp_blockchain::Result> { + fn revert( + &self, + _n: NumberFor, + _revert_finalized: bool, + ) -> sp_blockchain::Result> { Ok(Zero::zero()) } diff --git a/substrate/client/src/leaves.rs b/substrate/client/src/leaves.rs index 8a9df0f071..bb556da83a 100644 --- a/substrate/client/src/leaves.rs +++ b/substrate/client/src/leaves.rs @@ -158,12 +158,35 @@ impl LeafSet where Undo { inner: self } } - /// currently since revert only affects the canonical chain - /// we assume that parent has no further children - /// and we add it as leaf again - pub fn revert(&mut self, hash: H, number: N, parent_hash: H) { - self.insert_leaf(Reverse(number.clone() - N::one()), parent_hash); - self.remove_leaf(&Reverse(number), &hash); + /// Revert to the given block height by dropping all leaves in the leaf set + /// with a block number higher than the target. + pub fn revert(&mut self, best_hash: H, best_number: N) { + let items = self.storage.iter() + .flat_map(|(number, hashes)| hashes.iter().map(move |h| (h.clone(), number.clone()))) + .collect::>(); + + for (hash, number) in &items { + if number.0 > best_number { + assert!( + self.remove_leaf(number, hash), + "item comes from an iterator over storage; qed", + ); + + self.pending_removed.push(hash.clone()); + } + } + + let best_number = Reverse(best_number); + let leaves_contains_best = self.storage + .get(&best_number) + .map_or(false, |hashes| hashes.contains(&best_hash)); + + // we need to make sure that the best block exists in the leaf set as + // this is an invariant of regular block import. + if !leaves_contains_best { + self.insert_leaf(best_number.clone(), best_hash.clone()); + self.pending_added.push(LeafSetItem { hash: best_hash, number: best_number }); + } } /// returns an iterator over all hashes in the leaf set diff --git a/substrate/client/src/light/backend.rs b/substrate/client/src/light/backend.rs index 58151ce99a..568d290834 100644 --- a/substrate/client/src/light/backend.rs +++ b/substrate/client/src/light/backend.rs @@ -213,7 +213,11 @@ impl ClientBackend for Backend where Ok(GenesisOrUnavailableState::Unavailable) } - fn revert(&self, _n: NumberFor) -> ClientResult> { + fn revert( + &self, + _n: NumberFor, + _revert_finalized: bool, + ) -> ClientResult> { Err(ClientError::NotAvailableOnLightClient) }