From fb7e0cc0b31dd8364dd602ac167e1735942f9789 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Mon, 2 May 2022 20:45:16 +0200 Subject: [PATCH 01/11] Revert approval-voting subsystem --- node/core/approval-voting/src/lib.rs | 99 ++++++++++++++++++++++++++ node/core/chain-selection/src/lib.rs | 6 +- node/service/src/lib.rs | 102 +++++++++++++++++++-------- 3 files changed, 175 insertions(+), 32 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index c3892a160eef..b096fd7817b1 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -340,6 +340,105 @@ impl ApprovalVotingSubsystem { metrics, } } + + /// Revert to the block corresponding to the specified `hash`. + /// The revert is not allowed for blocks older than the last finalized one. + pub fn revert_to(&self, hash: Hash) -> Result<(), SubsystemError> { + let config = approval_db::v1::Config { col_data: self.db_config.col_data }; + let mut backend = approval_db::v1::DbBackend::new(self.db.clone(), config); + let mut overlay = OverlayedBackend::new(&backend); + + let mut entry = match overlay.load_block_entry(&hash)? { + Some(entry) => entry, + None => { + // This may only happen if hash corresponds to genesis or if + // we're trying to revert below the last finalized block. + // Revert to genesis block should be handled specially since no + // information about it is persisted as a block entry. + let blocks = overlay.load_blocks_at_height(&1).unwrap_or_default(); + let is_genesis = blocks + .first() + .and_then(|hash| overlay.load_block_entry(hash).ok()) + .flatten() + .map(|entry| entry.parent_hash() == hash) + .unwrap_or_default(); + if !is_genesis { + return Err(SubsystemError::Context( + "Trying to revert below last finalized block".to_string(), + )) + } + + // TODO: check if dummy entry is removed + // We use part of the information contained in the finalized block + // children (that are expected to be in the tree) to construct a + // dummy block entry for the last finalized block. This will be + // wiped as soon as the next block is finalized. + let entry = approval_db::v1::BlockEntry { + block_hash: hash, + block_number: 0, + parent_hash: Hash::default(), + session: 1, + slot: Slot::from(1), + relay_vrf_story: [0u8; 32], + candidates: Vec::new(), + approved_bitfield: approval_db::v1::Bitfield::from_slice(&[]), + children: blocks, + }; + + // This becomes the first entry according to the block number. + //backend.write_blocks_by_number(block_number, vec![hash]); + + entry.into() + }, + }; + + let mut stack: Vec<_> = std::mem::take(&mut entry.children) + .into_iter() + .map(|h| (h, entry.block_number() + 1)) + .collect(); + + // Write revert point block entry without the children. + overlay.write_block_entry(entry); + + while let Some((hash, number)) = stack.pop() { + let mut blocks_at_height = overlay.load_blocks_at_height(&number)?; + blocks_at_height.retain(|h| h != &hash); + overlay.write_blocks_at_height(number, blocks_at_height); + + if let Some(entry) = overlay.load_block_entry(&hash)? { + // Before going through the candidates loop is important to first remove the block + // entry. + overlay.delete_block_entry(&hash); + + // >>> NOTE / TODO / FIXME << + // THIS EXTREMELLY VERBOSE COMMENTS PURPOSE IS ONLY TO SIMPLIFY CODE REVIEW + // REMOVE COMMENTS BEFORE MERGE + // + // For each candidate in `candidates` of `BlockEntry`: + // 1. Use the `CandidateHash` with `load_candidate_entry` to load the `CandidateEntry`. + // 2. Remove from the `block_assignments` the entries corresponding to `BlockEntry` hash. + // 3. If the candidate is not referenced by any other block (i.e. `block_assignments` + // is empty) then we can remove the entry using `delete_candidate_entry`. + for (_, candidate_hash) in entry.candidates() { + if let Some(mut candidate_entry) = + overlay.load_candidate_entry(candidate_hash)? + { + candidate_entry.block_assignments.remove(&hash); + if candidate_entry.block_assignments.is_empty() { + overlay.delete_candidate_entry(candidate_hash); + } else { + overlay.write_candidate_entry(candidate_entry); + } + } + } + + stack.extend(entry.children.into_iter().map(|h| (h, number + 1))); + } + } + + let ops = overlay.into_write_ops(); + backend.write(ops) + } } impl overseer::Subsystem for ApprovalVotingSubsystem diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 64ee73b9e1a9..50a5eace4b43 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -318,9 +318,9 @@ impl ChainSelectionSubsystem { /// Revert to the block corresponding to the specified `hash`. /// The revert is not allowed for blocks older than the last finalized one. - pub fn revert(&self, hash: Hash) -> Result<(), Error> { - let backend_config = db_backend::v1::Config { col_data: self.config.col_data }; - let mut backend = db_backend::v1::DbBackend::new(self.db.clone(), backend_config); + pub fn revert_to(&self, hash: Hash) -> Result<(), Error> { + let config = db_backend::v1::Config { col_data: self.config.col_data }; + let mut backend = db_backend::v1::DbBackend::new(self.db.clone(), config); let ops = tree::revert_to(&backend, hash)?.into_write_ops(); diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index 43897cb8c7d8..4fdd449af833 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -37,7 +37,9 @@ use { beefy_gadget::notification::{BeefyBestBlockSender, BeefySignedCommitmentSender}, grandpa::{self, FinalityProofProvider as GrandpaFinalityProofProvider}, gum::info, - polkadot_node_core_approval_voting::Config as ApprovalVotingConfig, + polkadot_node_core_approval_voting::{ + self as approval_voting_subsystem, Config as ApprovalVotingConfig, + }, polkadot_node_core_av_store::Config as AvailabilityConfig, polkadot_node_core_av_store::Error as AvailabilityError, polkadot_node_core_candidate_validation::Config as CandidateValidationConfig, @@ -1403,28 +1405,6 @@ pub fn build_full( Err(Error::NoRuntime) } -struct RevertConsensus { - blocks: BlockNumber, - backend: Arc, -} - -impl ExecuteWithClient for RevertConsensus { - type Output = sp_blockchain::Result<()>; - - fn execute_with_client(self, client: Arc) -> Self::Output - where - >::StateBackend: sp_api::StateBackend, - Backend: sc_client_api::Backend + 'static, - Backend::State: sp_api::StateBackend, - Api: polkadot_client::RuntimeApiCollection, - Client: AbstractClient + 'static, - { - babe::revert(client.clone(), self.backend, self.blocks)?; - grandpa::revert(client, self.blocks)?; - Ok(()) - } -} - /// Reverts the node state down to at most the last finalized block. /// /// In particular this reverts: @@ -1441,6 +1421,10 @@ pub fn revert_backend( let finalized = client.info().finalized_number; let revertible = blocks.min(best_number - finalized); + if revertible == 0 { + return Ok(()) + } + let number = best_number - revertible; let hash = client.block_hash_from_id(&BlockId::Number(number))?.ok_or( sp_blockchain::Error::Backend(format!( @@ -1452,19 +1436,79 @@ pub fn revert_backend( let parachains_db = open_database(&config.database) .map_err(|err| sp_blockchain::Error::Backend(err.to_string()))?; + // Revert approval voting subsystem + revert_approval_voting(parachains_db.clone(), hash)?; + + // Revert chain-selection subsystem + revert_chain_selection(parachains_db, hash)?; + + // Revert Substrate consensus related components + client.execute_with(RevertConsensus { blocks, backend })?; + + Ok(()) +} + +fn revert_chain_selection(db: Arc, hash: Hash) -> sp_blockchain::Result<()> { let config = chain_selection_subsystem::Config { col_data: parachains_db::REAL_COLUMNS.col_chain_selection_data, stagnant_check_interval: chain_selection_subsystem::StagnantCheckInterval::never(), }; - let chain_selection = - chain_selection_subsystem::ChainSelectionSubsystem::new(config, parachains_db); + let chain_selection = chain_selection_subsystem::ChainSelectionSubsystem::new(config, db); chain_selection - .revert(hash) - .map_err(|err| sp_blockchain::Error::Backend(err.to_string()))?; + .revert_to(hash) + .map_err(|err| sp_blockchain::Error::Backend(err.to_string())) +} - client.execute_with(RevertConsensus { blocks, backend })?; +fn revert_approval_voting(db: Arc, hash: Hash) -> sp_blockchain::Result<()> { + let config = approval_voting_subsystem::Config { + col_data: parachains_db::REAL_COLUMNS.col_approval_data, + slot_duration_millis: Default::default(), + }; - Ok(()) + struct DummyOracle; + impl consensus_common::SyncOracle for DummyOracle { + fn is_major_syncing(&mut self) -> bool { + false + } + fn is_offline(&mut self) -> bool { + false + } + } + + // Construct a usable approval voting subsystem with a dummy sync oracle and keystore. + let approval_voting = approval_voting_subsystem::ApprovalVotingSubsystem::with_config( + config, + db, + Arc::new(sc_keystore::LocalKeystore::in_memory()), + Box::new(DummyOracle), + approval_voting_subsystem::Metrics::default(), + ); + + approval_voting + .revert_to(hash) + .map_err(|err| sp_blockchain::Error::Backend(err.to_string())) +} + +struct RevertConsensus { + blocks: BlockNumber, + backend: Arc, +} + +impl ExecuteWithClient for RevertConsensus { + type Output = sp_blockchain::Result<()>; + + fn execute_with_client(self, client: Arc) -> Self::Output + where + >::StateBackend: sp_api::StateBackend, + Backend: sc_client_api::Backend + 'static, + Backend::State: sp_api::StateBackend, + Api: polkadot_client::RuntimeApiCollection, + Client: AbstractClient + 'static, + { + babe::revert(client.clone(), self.backend, self.blocks)?; + grandpa::revert(client, self.blocks)?; + Ok(()) + } } From e68a3d772275e60c093b519b42dfac32c8ca205d Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 3 May 2022 11:36:44 +0200 Subject: [PATCH 02/11] Approval voting revert encapsulated within 'ops' module --- node/core/approval-voting/src/lib.rs | 90 +--------------------------- node/core/approval-voting/src/ops.rs | 73 +++++++++++++++++++++- 2 files changed, 74 insertions(+), 89 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index b096fd7817b1..cb583ac1a991 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -342,99 +342,13 @@ impl ApprovalVotingSubsystem { } /// Revert to the block corresponding to the specified `hash`. - /// The revert is not allowed for blocks older than the last finalized one. + /// The operation is not allowed for blocks older than the last finalized one. pub fn revert_to(&self, hash: Hash) -> Result<(), SubsystemError> { let config = approval_db::v1::Config { col_data: self.db_config.col_data }; let mut backend = approval_db::v1::DbBackend::new(self.db.clone(), config); let mut overlay = OverlayedBackend::new(&backend); - let mut entry = match overlay.load_block_entry(&hash)? { - Some(entry) => entry, - None => { - // This may only happen if hash corresponds to genesis or if - // we're trying to revert below the last finalized block. - // Revert to genesis block should be handled specially since no - // information about it is persisted as a block entry. - let blocks = overlay.load_blocks_at_height(&1).unwrap_or_default(); - let is_genesis = blocks - .first() - .and_then(|hash| overlay.load_block_entry(hash).ok()) - .flatten() - .map(|entry| entry.parent_hash() == hash) - .unwrap_or_default(); - if !is_genesis { - return Err(SubsystemError::Context( - "Trying to revert below last finalized block".to_string(), - )) - } - - // TODO: check if dummy entry is removed - // We use part of the information contained in the finalized block - // children (that are expected to be in the tree) to construct a - // dummy block entry for the last finalized block. This will be - // wiped as soon as the next block is finalized. - let entry = approval_db::v1::BlockEntry { - block_hash: hash, - block_number: 0, - parent_hash: Hash::default(), - session: 1, - slot: Slot::from(1), - relay_vrf_story: [0u8; 32], - candidates: Vec::new(), - approved_bitfield: approval_db::v1::Bitfield::from_slice(&[]), - children: blocks, - }; - - // This becomes the first entry according to the block number. - //backend.write_blocks_by_number(block_number, vec![hash]); - - entry.into() - }, - }; - - let mut stack: Vec<_> = std::mem::take(&mut entry.children) - .into_iter() - .map(|h| (h, entry.block_number() + 1)) - .collect(); - - // Write revert point block entry without the children. - overlay.write_block_entry(entry); - - while let Some((hash, number)) = stack.pop() { - let mut blocks_at_height = overlay.load_blocks_at_height(&number)?; - blocks_at_height.retain(|h| h != &hash); - overlay.write_blocks_at_height(number, blocks_at_height); - - if let Some(entry) = overlay.load_block_entry(&hash)? { - // Before going through the candidates loop is important to first remove the block - // entry. - overlay.delete_block_entry(&hash); - - // >>> NOTE / TODO / FIXME << - // THIS EXTREMELLY VERBOSE COMMENTS PURPOSE IS ONLY TO SIMPLIFY CODE REVIEW - // REMOVE COMMENTS BEFORE MERGE - // - // For each candidate in `candidates` of `BlockEntry`: - // 1. Use the `CandidateHash` with `load_candidate_entry` to load the `CandidateEntry`. - // 2. Remove from the `block_assignments` the entries corresponding to `BlockEntry` hash. - // 3. If the candidate is not referenced by any other block (i.e. `block_assignments` - // is empty) then we can remove the entry using `delete_candidate_entry`. - for (_, candidate_hash) in entry.candidates() { - if let Some(mut candidate_entry) = - overlay.load_candidate_entry(candidate_hash)? - { - candidate_entry.block_assignments.remove(&hash); - if candidate_entry.block_assignments.is_empty() { - overlay.delete_candidate_entry(candidate_hash); - } else { - overlay.write_candidate_entry(candidate_entry); - } - } - } - - stack.extend(entry.children.into_iter().map(|h| (h, number + 1))); - } - } + ops::revert_to(&mut overlay, hash)?; let ops = overlay.into_write_ops(); backend.write(ops) diff --git a/node/core/approval-voting/src/ops.rs b/node/core/approval-voting/src/ops.rs index d0699e8026ed..62267ddfc7f1 100644 --- a/node/core/approval-voting/src/ops.rs +++ b/node/core/approval-voting/src/ops.rs @@ -17,7 +17,7 @@ //! Middleware interface that leverages low-level database operations //! to provide a clean API for processing block and candidate imports. -use polkadot_node_subsystem::SubsystemResult; +use polkadot_node_subsystem::{SubsystemError, SubsystemResult}; use bitvec::order::Lsb0 as BitOrderLsb0; use polkadot_primitives::v2::{BlockNumber, CandidateHash, CandidateReceipt, GroupIndex, Hash}; @@ -311,3 +311,74 @@ pub fn force_approve( Ok(approved_hashes) } + +/// Revert to the block corresponding to the specified `hash`. +/// The operation is not allowed for blocks older than the last finalized one. +pub fn revert_to( + overlay: &mut OverlayedBackend<'_, impl Backend>, + hash: Hash, +) -> SubsystemResult<()> { + let (children, height) = match overlay.load_block_entry(&hash)? { + Some(mut entry) => { + let children_height = entry.block_number() + 1; + let children = std::mem::take(&mut entry.children); + // Write revert point block entry without the children. + overlay.write_block_entry(entry); + (children, children_height) + }, + None => { + // May be a revert to the last finalized block. + // Loading all blocks just to get the first one may be expensive + // but we should consider that revert is a "one-shot" and very + // sporadic operation. + let blocks = overlay.load_all_blocks()?; + let entry = blocks + .first() + .and_then(|hash| overlay.load_block_entry(hash).ok()) + .flatten() + .ok_or_else(|| { + SubsystemError::Context(format!("Unexpected lookup failure for block {}", hash)) + })?; + + // The parent is expected to be the revert point + if entry.parent_hash() != hash { + return Err(SubsystemError::Context( + "revert below last finalized block or corrupted storage".to_string(), + )) + } + + let children_height = entry.block_number(); + let children = overlay.load_blocks_at_height(&children_height)?; + (children, children_height) + }, + }; + + let mut stack: Vec<_> = children.into_iter().map(|h| (h, height)).collect(); + + while let Some((hash, number)) = stack.pop() { + let mut blocks_at_height = overlay.load_blocks_at_height(&number)?; + blocks_at_height.retain(|h| h != &hash); + overlay.write_blocks_at_height(number, blocks_at_height); + + if let Some(entry) = overlay.load_block_entry(&hash)? { + overlay.delete_block_entry(&hash); + + // Cleanup the candidate entries by removing any reference to the + // removed block. If for a candidate entry the block block_assignments + // drops to zero then we remove the entry. + for (_, candidate_hash) in entry.candidates() { + if let Some(mut candidate_entry) = overlay.load_candidate_entry(candidate_hash)? { + candidate_entry.block_assignments.remove(&hash); + if candidate_entry.block_assignments.is_empty() { + overlay.delete_candidate_entry(candidate_hash); + } else { + overlay.write_candidate_entry(candidate_entry); + } + } + } + + stack.extend(entry.children.into_iter().map(|h| (h, number + 1))); + } + } + Ok(()) +} From f1cdf633691e80b2cd4408a929e4f61d46da702e Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 3 May 2022 11:48:33 +0200 Subject: [PATCH 03/11] Nits --- node/core/approval-voting/src/ops.rs | 8 ++++---- node/core/chain-selection/src/lib.rs | 2 +- node/service/src/lib.rs | 11 +++++------ 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/node/core/approval-voting/src/ops.rs b/node/core/approval-voting/src/ops.rs index 62267ddfc7f1..b3b2cabc9fb1 100644 --- a/node/core/approval-voting/src/ops.rs +++ b/node/core/approval-voting/src/ops.rs @@ -332,22 +332,22 @@ pub fn revert_to( // but we should consider that revert is a "one-shot" and very // sporadic operation. let blocks = overlay.load_all_blocks()?; - let entry = blocks + let child_entry = blocks .first() .and_then(|hash| overlay.load_block_entry(hash).ok()) .flatten() .ok_or_else(|| { - SubsystemError::Context(format!("Unexpected lookup failure for block {}", hash)) + SubsystemError::Context(format!("lookup failure for block {}", hash)) })?; // The parent is expected to be the revert point - if entry.parent_hash() != hash { + if child_entry.parent_hash() != hash { return Err(SubsystemError::Context( "revert below last finalized block or corrupted storage".to_string(), )) } - let children_height = entry.block_number(); + let children_height = child_entry.block_number(); let children = overlay.load_blocks_at_height(&children_height)?; (children, children_height) }, diff --git a/node/core/chain-selection/src/lib.rs b/node/core/chain-selection/src/lib.rs index 50a5eace4b43..0728ed23c043 100644 --- a/node/core/chain-selection/src/lib.rs +++ b/node/core/chain-selection/src/lib.rs @@ -317,7 +317,7 @@ impl ChainSelectionSubsystem { } /// Revert to the block corresponding to the specified `hash`. - /// The revert is not allowed for blocks older than the last finalized one. + /// The operation is not allowed for blocks older than the last finalized one. pub fn revert_to(&self, hash: Hash) -> Result<(), Error> { let config = db_backend::v1::Config { col_data: self.config.col_data }; let mut backend = db_backend::v1::DbBackend::new(self.db.clone(), config); diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index 4fdd449af833..3067d0e79a13 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -1408,7 +1408,8 @@ pub fn build_full( /// Reverts the node state down to at most the last finalized block. /// /// In particular this reverts: -/// - `ChainSelectionSubsystem` data in the parachains-db. +/// - `ApprovalVotingSubsystem` data in the parachains-db; +/// - `ChainSelectionSubsystem` data in the parachains-db; /// - Low level Babe and Grandpa consensus data. #[cfg(feature = "full-node")] pub fn revert_backend( @@ -1436,12 +1437,8 @@ pub fn revert_backend( let parachains_db = open_database(&config.database) .map_err(|err| sp_blockchain::Error::Backend(err.to_string()))?; - // Revert approval voting subsystem revert_approval_voting(parachains_db.clone(), hash)?; - - // Revert chain-selection subsystem revert_chain_selection(parachains_db, hash)?; - // Revert Substrate consensus related components client.execute_with(RevertConsensus { blocks, backend })?; @@ -1477,7 +1474,9 @@ fn revert_approval_voting(db: Arc, hash: Hash) -> sp_blockchain::R } } - // Construct a usable approval voting subsystem with a dummy sync oracle and keystore. + // Construct an approval voting subsystem with a dummy sync oracle and keystore. + // This dummy components are not be used by the revert and are only necessary + // for construction. let approval_voting = approval_voting_subsystem::ApprovalVotingSubsystem::with_config( config, db, From 65cec53fb835386afcd81bb46d0a6178b142d0b1 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 3 May 2022 14:18:19 +0200 Subject: [PATCH 04/11] use 'get_stored_blocks' to get lower block height --- node/core/approval-voting/src/ops.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/node/core/approval-voting/src/ops.rs b/node/core/approval-voting/src/ops.rs index b3b2cabc9fb1..678f4bdd24ee 100644 --- a/node/core/approval-voting/src/ops.rs +++ b/node/core/approval-voting/src/ops.rs @@ -327,17 +327,20 @@ pub fn revert_to( (children, children_height) }, None => { - // May be a revert to the last finalized block. - // Loading all blocks just to get the first one may be expensive - // but we should consider that revert is a "one-shot" and very - // sporadic operation. - let blocks = overlay.load_all_blocks()?; - let child_entry = blocks + let children_height = overlay.load_stored_blocks()? + .map(|range| range.0) + .ok_or_else(|| { + SubsystemError::Context("no available blocks to infer revert point heightlookup failure for first block".to_string()) + })?; + + let children = overlay.load_blocks_at_height(&children_height)?; + + let child_entry = children .first() .and_then(|hash| overlay.load_block_entry(hash).ok()) .flatten() .ok_or_else(|| { - SubsystemError::Context(format!("lookup failure for block {}", hash)) + SubsystemError::Context("lookup failure for first block".to_string()) })?; // The parent is expected to be the revert point @@ -347,8 +350,6 @@ pub fn revert_to( )) } - let children_height = child_entry.block_number(); - let children = overlay.load_blocks_at_height(&children_height)?; (children, children_height) }, }; From b8323fa9d153bfd69f41ae186cce2b0183079887 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 3 May 2022 14:21:29 +0200 Subject: [PATCH 05/11] Fix error message --- node/core/approval-voting/src/ops.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/node/core/approval-voting/src/ops.rs b/node/core/approval-voting/src/ops.rs index 678f4bdd24ee..4ee1b04661df 100644 --- a/node/core/approval-voting/src/ops.rs +++ b/node/core/approval-voting/src/ops.rs @@ -327,11 +327,12 @@ pub fn revert_to( (children, children_height) }, None => { - let children_height = overlay.load_stored_blocks()? - .map(|range| range.0) - .ok_or_else(|| { - SubsystemError::Context("no available blocks to infer revert point heightlookup failure for first block".to_string()) - })?; + let children_height = + overlay.load_stored_blocks()?.map(|range| range.0).ok_or_else(|| { + SubsystemError::Context( + "no available blocks to infer revert point height".to_string(), + ) + })?; let children = overlay.load_blocks_at_height(&children_height)?; From 3744a68bfd315607d0c8a83ecd74967d454c7257 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 3 May 2022 20:04:32 +0200 Subject: [PATCH 06/11] Optionally shrink/delete stored blocks range --- .../approval-voting/src/approval_db/v1/mod.rs | 3 ++ node/core/approval-voting/src/backend.rs | 23 ++++++++----- node/core/approval-voting/src/ops.rs | 34 ++++++++++++++----- node/core/approval-voting/src/tests.rs | 3 ++ 4 files changed, 46 insertions(+), 17 deletions(-) diff --git a/node/core/approval-voting/src/approval_db/v1/mod.rs b/node/core/approval-voting/src/approval_db/v1/mod.rs index fa39819977ff..03b7aa68f134 100644 --- a/node/core/approval-voting/src/approval_db/v1/mod.rs +++ b/node/core/approval-voting/src/approval_db/v1/mod.rs @@ -95,6 +95,9 @@ impl Backend for DbBackend { stored_block_range.encode(), ); }, + BackendWriteOp::DeleteStoredBlockRange => { + tx.delete(self.config.col_data, &STORED_BLOCKS_KEY); + }, BackendWriteOp::WriteBlocksAtHeight(h, blocks) => { tx.put_vec(self.config.col_data, &blocks_at_height_key(h), blocks.encode()); }, diff --git a/node/core/approval-voting/src/backend.rs b/node/core/approval-voting/src/backend.rs index 3a21fba34875..d9d335f752c8 100644 --- a/node/core/approval-voting/src/backend.rs +++ b/node/core/approval-voting/src/backend.rs @@ -37,6 +37,7 @@ pub enum BackendWriteOp { WriteBlocksAtHeight(BlockNumber, Vec), WriteBlockEntry(BlockEntry), WriteCandidateEntry(CandidateEntry), + DeleteStoredBlockRange, DeleteBlocksAtHeight(BlockNumber), DeleteBlockEntry(Hash), DeleteCandidateEntry(CandidateHash), @@ -70,9 +71,8 @@ pub trait Backend { /// the underlying backend, give the same view as the state of the overlay. pub struct OverlayedBackend<'a, B: 'a> { inner: &'a B, - - // `None` means unchanged - stored_block_range: Option, + // `Some(None)` means deleted. Missing (`None`) means query inner. + stored_block_range: Option>, // `None` means 'deleted', missing means query inner. blocks_at_height: HashMap>>, // `None` means 'deleted', missing means query inner. @@ -112,7 +112,7 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { pub fn load_stored_blocks(&self) -> SubsystemResult> { if let Some(val) = self.stored_block_range.clone() { - return Ok(Some(val)) + return Ok(val) } self.inner.load_stored_blocks() @@ -148,7 +148,11 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { // The assumption is that stored block range is only None on initialization. // Therefore, there is no need to delete_stored_block_range. pub fn write_stored_block_range(&mut self, range: StoredBlockRange) { - self.stored_block_range = Some(range); + self.stored_block_range = Some(Some(range)); + } + + pub fn delete_stored_block_range(&mut self) { + self.stored_block_range = Some(None); } pub fn write_blocks_at_height(&mut self, height: BlockNumber, blocks: Vec) { @@ -193,9 +197,12 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { None => BackendWriteOp::DeleteCandidateEntry(h), }); - self.stored_block_range - .map(|v| BackendWriteOp::WriteStoredBlockRange(v)) - .into_iter() + let stored_block_range_ops = self.stored_block_range.into_iter().map(|v| match v { + Some(v) => BackendWriteOp::WriteStoredBlockRange(v), + None => BackendWriteOp::DeleteStoredBlockRange, + }); + + stored_block_range_ops .chain(blocks_at_height_ops) .chain(block_entry_ops) .chain(candidate_entry_ops) diff --git a/node/core/approval-voting/src/ops.rs b/node/core/approval-voting/src/ops.rs index 4ee1b04661df..acde964e4bbd 100644 --- a/node/core/approval-voting/src/ops.rs +++ b/node/core/approval-voting/src/ops.rs @@ -318,7 +318,11 @@ pub fn revert_to( overlay: &mut OverlayedBackend<'_, impl Backend>, hash: Hash, ) -> SubsystemResult<()> { - let (children, height) = match overlay.load_block_entry(&hash)? { + let mut stored_range = overlay.load_stored_blocks()?.ok_or_else(|| { + SubsystemError::Context("no available blocks to infer revert point height".to_string()) + })?; + + let (children, children_height) = match overlay.load_block_entry(&hash)? { Some(mut entry) => { let children_height = entry.block_number() + 1; let children = std::mem::take(&mut entry.children); @@ -327,13 +331,7 @@ pub fn revert_to( (children, children_height) }, None => { - let children_height = - overlay.load_stored_blocks()?.map(|range| range.0).ok_or_else(|| { - SubsystemError::Context( - "no available blocks to infer revert point height".to_string(), - ) - })?; - + let children_height = stored_range.0; let children = overlay.load_blocks_at_height(&children_height)?; let child_entry = children @@ -355,11 +353,18 @@ pub fn revert_to( }, }; - let mut stack: Vec<_> = children.into_iter().map(|h| (h, height)).collect(); + let mut stack: Vec<_> = children.into_iter().map(|h| (h, children_height)).collect(); + let mut range_top = stored_range.1; while let Some((hash, number)) = stack.pop() { let mut blocks_at_height = overlay.load_blocks_at_height(&number)?; blocks_at_height.retain(|h| h != &hash); + + // Check if we need to update the range top + if blocks_at_height.is_empty() && number <= range_top { + range_top = number.saturating_sub(1); + } + overlay.write_blocks_at_height(number, blocks_at_height); if let Some(entry) = overlay.load_block_entry(&hash)? { @@ -382,5 +387,16 @@ pub fn revert_to( stack.extend(entry.children.into_iter().map(|h| (h, number + 1))); } } + + // Check if our modifications to the dag has reduced the range top + if range_top != stored_range.1 { + if stored_range.0 <= range_top { + stored_range.1 = range_top; + overlay.write_stored_block_range(stored_range); + } else { + overlay.delete_stored_block_range(); + } + } + Ok(()) } diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 165b66828ae6..c568a433299a 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -306,6 +306,9 @@ impl Backend for TestStoreInner { BackendWriteOp::WriteStoredBlockRange(stored_block_range) => { self.stored_block_range = Some(stored_block_range); }, + BackendWriteOp::DeleteStoredBlockRange => { + self.stored_block_range = None; + }, BackendWriteOp::WriteBlocksAtHeight(h, blocks) => { self.blocks_at_height.insert(h, blocks); }, From b891d87f00d8785b968b8e3d3e3fef64258bc6be Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 4 May 2022 12:10:12 +0200 Subject: [PATCH 07/11] range end number is last block number plus 1 --- node/core/approval-voting/src/ops.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/node/core/approval-voting/src/ops.rs b/node/core/approval-voting/src/ops.rs index acde964e4bbd..c4c64aeaf4e7 100644 --- a/node/core/approval-voting/src/ops.rs +++ b/node/core/approval-voting/src/ops.rs @@ -354,15 +354,15 @@ pub fn revert_to( }; let mut stack: Vec<_> = children.into_iter().map(|h| (h, children_height)).collect(); - let mut range_top = stored_range.1; + let mut range_end = stored_range.1; while let Some((hash, number)) = stack.pop() { let mut blocks_at_height = overlay.load_blocks_at_height(&number)?; blocks_at_height.retain(|h| h != &hash); // Check if we need to update the range top - if blocks_at_height.is_empty() && number <= range_top { - range_top = number.saturating_sub(1); + if blocks_at_height.is_empty() && number < range_end { + range_end = number; } overlay.write_blocks_at_height(number, blocks_at_height); @@ -389,9 +389,9 @@ pub fn revert_to( } // Check if our modifications to the dag has reduced the range top - if range_top != stored_range.1 { - if stored_range.0 <= range_top { - stored_range.1 = range_top; + if range_end != stored_range.1 { + if stored_range.0 < range_end { + stored_range.1 = range_end; overlay.write_stored_block_range(stored_range); } else { overlay.delete_stored_block_range(); From 6022986cc7c0b77952f6d5bfde50229185b49c08 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Sat, 7 May 2022 10:38:48 +0200 Subject: [PATCH 08/11] Apply code review suggestions --- node/core/approval-voting/src/backend.rs | 2 -- node/service/src/lib.rs | 15 +-------------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/node/core/approval-voting/src/backend.rs b/node/core/approval-voting/src/backend.rs index d9d335f752c8..843cb01001a9 100644 --- a/node/core/approval-voting/src/backend.rs +++ b/node/core/approval-voting/src/backend.rs @@ -145,8 +145,6 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { self.inner.load_candidate_entry(candidate_hash) } - // The assumption is that stored block range is only None on initialization. - // Therefore, there is no need to delete_stored_block_range. pub fn write_stored_block_range(&mut self, range: StoredBlockRange) { self.stored_block_range = Some(Some(range)); } diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index 3067d0e79a13..94f26f04cf48 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -1464,24 +1464,11 @@ fn revert_approval_voting(db: Arc, hash: Hash) -> sp_blockchain::R slot_duration_millis: Default::default(), }; - struct DummyOracle; - impl consensus_common::SyncOracle for DummyOracle { - fn is_major_syncing(&mut self) -> bool { - false - } - fn is_offline(&mut self) -> bool { - false - } - } - - // Construct an approval voting subsystem with a dummy sync oracle and keystore. - // This dummy components are not be used by the revert and are only necessary - // for construction. let approval_voting = approval_voting_subsystem::ApprovalVotingSubsystem::with_config( config, db, Arc::new(sc_keystore::LocalKeystore::in_memory()), - Box::new(DummyOracle), + Box::new(consensus_common::NoNetwork), approval_voting_subsystem::Metrics::default(), ); From 9ddcef248af75e21f8473649551d01727f22c92a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 10 May 2022 15:51:45 +0200 Subject: [PATCH 09/11] Use tristate enum for block range in backend overlay --- node/core/approval-voting/src/backend.rs | 36 +++++++++++++++--------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/node/core/approval-voting/src/backend.rs b/node/core/approval-voting/src/backend.rs index 843cb01001a9..405d3e4e7c5d 100644 --- a/node/core/approval-voting/src/backend.rs +++ b/node/core/approval-voting/src/backend.rs @@ -64,6 +64,14 @@ pub trait Backend { I: IntoIterator; } +// Status of block range in the `OverlayedBackend`. +#[derive(PartialEq)] +enum BlockRangeStatus { + NotModified, + Deleted, + Inserted(StoredBlockRange), +} + /// An in-memory overlay over the backend. /// /// This maintains read-only access to the underlying backend, but can be @@ -72,7 +80,7 @@ pub trait Backend { pub struct OverlayedBackend<'a, B: 'a> { inner: &'a B, // `Some(None)` means deleted. Missing (`None`) means query inner. - stored_block_range: Option>, + stored_block_range: BlockRangeStatus, // `None` means 'deleted', missing means query inner. blocks_at_height: HashMap>>, // `None` means 'deleted', missing means query inner. @@ -85,7 +93,7 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { pub fn new(backend: &'a B) -> Self { OverlayedBackend { inner: backend, - stored_block_range: None, + stored_block_range: BlockRangeStatus::NotModified, blocks_at_height: HashMap::new(), block_entries: HashMap::new(), candidate_entries: HashMap::new(), @@ -96,7 +104,7 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { self.block_entries.is_empty() && self.candidate_entries.is_empty() && self.blocks_at_height.is_empty() && - self.stored_block_range.is_none() + self.stored_block_range == BlockRangeStatus::NotModified } pub fn load_all_blocks(&self) -> SubsystemResult> { @@ -111,11 +119,11 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { } pub fn load_stored_blocks(&self) -> SubsystemResult> { - if let Some(val) = self.stored_block_range.clone() { - return Ok(val) + match self.stored_block_range { + BlockRangeStatus::Inserted(ref value) => Ok(Some(value.clone())), + BlockRangeStatus::Deleted => Ok(None), + BlockRangeStatus::NotModified => self.inner.load_stored_blocks(), } - - self.inner.load_stored_blocks() } pub fn load_blocks_at_height(&self, height: &BlockNumber) -> SubsystemResult> { @@ -146,11 +154,11 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { } pub fn write_stored_block_range(&mut self, range: StoredBlockRange) { - self.stored_block_range = Some(Some(range)); + self.stored_block_range = BlockRangeStatus::Inserted(range); } pub fn delete_stored_block_range(&mut self) { - self.stored_block_range = Some(None); + self.stored_block_range = BlockRangeStatus::Deleted; } pub fn write_blocks_at_height(&mut self, height: BlockNumber, blocks: Vec) { @@ -195,12 +203,14 @@ impl<'a, B: 'a + Backend> OverlayedBackend<'a, B> { None => BackendWriteOp::DeleteCandidateEntry(h), }); - let stored_block_range_ops = self.stored_block_range.into_iter().map(|v| match v { - Some(v) => BackendWriteOp::WriteStoredBlockRange(v), - None => BackendWriteOp::DeleteStoredBlockRange, - }); + let stored_block_range_ops = match self.stored_block_range { + BlockRangeStatus::Inserted(val) => Some(BackendWriteOp::WriteStoredBlockRange(val)), + BlockRangeStatus::Deleted => Some(BackendWriteOp::DeleteStoredBlockRange), + BlockRangeStatus::NotModified => None, + }; stored_block_range_ops + .into_iter() .chain(blocks_at_height_ops) .chain(block_entry_ops) .chain(candidate_entry_ops) From 7952b2d441afa11731a3b77079fe7eda5131519e Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 10 May 2022 15:55:17 +0200 Subject: [PATCH 10/11] Add clarification comment --- node/service/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index 94f26f04cf48..51826c51c493 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -1493,6 +1493,8 @@ impl ExecuteWithClient for RevertConsensus { Api: polkadot_client::RuntimeApiCollection, Client: AbstractClient + 'static, { + // Revert consensus-related components. + // The operations are not correlated, thus call order is not relevant. babe::revert(client.clone(), self.backend, self.blocks)?; grandpa::revert(client, self.blocks)?; Ok(()) From ca1e065d2cf0ca062a332ed2d8df26461e281910 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 10 May 2022 18:40:16 +0200 Subject: [PATCH 11/11] Add comments to private struct --- node/core/approval-voting/src/backend.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/node/core/approval-voting/src/backend.rs b/node/core/approval-voting/src/backend.rs index 405d3e4e7c5d..9cb2cb59ec78 100644 --- a/node/core/approval-voting/src/backend.rs +++ b/node/core/approval-voting/src/backend.rs @@ -67,8 +67,11 @@ pub trait Backend { // Status of block range in the `OverlayedBackend`. #[derive(PartialEq)] enum BlockRangeStatus { + // Value has not been modified. NotModified, + // Value has been deleted Deleted, + // Value has been updated. Inserted(StoredBlockRange), }