Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change forks pruning algorithm. #3962

Merged
merged 23 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9decf7b
Change forks pruning algorithm.
shamil-gadelshin Jan 19, 2024
64094f2
Update substrate/client/api/src/leaves.rs
shamil-gadelshin Apr 4, 2024
62ab0aa
Update substrate/primitives/blockchain/src/backend.rs
shamil-gadelshin Apr 4, 2024
962b5d3
Update fork calculation algorithm.
shamil-gadelshin Apr 5, 2024
d325870
Remove obsolete tests.
shamil-gadelshin Apr 5, 2024
7e05f0d
Add pr_3962.prdoc
shamil-gadelshin Apr 5, 2024
4aeeeb6
Update expand_fork() function and fix test.
shamil-gadelshin Apr 9, 2024
27fcc5f
Update `follow_report_multiple_pruned_block` test.
shamil-gadelshin Apr 9, 2024
57816e9
Update `sc-service-test` tests.
shamil-gadelshin Apr 9, 2024
6fbc756
Update substrate/primitives/blockchain/src/backend.rs
shamil-gadelshin Apr 17, 2024
af6afe4
Update substrate/primitives/blockchain/src/backend.rs
shamil-gadelshin Apr 17, 2024
75ad220
Update substrate/primitives/blockchain/src/backend.rs
shamil-gadelshin Apr 17, 2024
027739f
Apply review suggestions.
shamil-gadelshin Apr 17, 2024
cdd7aaf
Merge branch 'master' into change-fork-calculation
bkchr May 7, 2024
99b87a3
Merge branch 'master' into change-fork-calculation
shamil-gadelshin May 8, 2024
0ea2dfb
Update follow_unique_pruned_blocks test.
shamil-gadelshin May 8, 2024
98e47bd
Update the expand_forks() comment.
shamil-gadelshin May 8, 2024
f76be38
Update expand_forks().
shamil-gadelshin May 10, 2024
e56eed6
Update expand_forks() dependencies.
shamil-gadelshin May 10, 2024
c875bd2
Update flaky test.
shamil-gadelshin May 10, 2024
bc5290c
Merge branch 'master' into change-fork-calculation
bkchr May 14, 2024
7e769f6
Fix doc-comment.
shamil-gadelshin May 15, 2024
95c1d7d
Merge branch 'master' into change-fork-calculation
bkchr May 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions substrate/client/api/src/in_mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,20 +419,6 @@ impl<Block: BlockT> blockchain::Backend<Block> for Blockchain<Block> {
Ok(self.storage.read().leaves.hashes())
}

fn displaced_leaves_after_finalizing(
&self,
block_number: NumberFor<Block>,
) -> sp_blockchain::Result<Vec<Block::Hash>> {
Ok(self
.storage
.read()
.leaves
.displaced_by_finalize_height(block_number)
.leaves()
.cloned()
.collect::<Vec<_>>())
}

fn children(&self, _parent_hash: Block::Hash) -> sp_blockchain::Result<Vec<Block::Hash>> {
unimplemented!()
}
Expand Down
51 changes: 17 additions & 34 deletions substrate/client/api/src/leaves.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub struct FinalizationOutcome<H, N> {
removed: BTreeMap<Reverse<N>, Vec<H>>,
}

impl<H, N: Ord> FinalizationOutcome<H, N> {
impl<H: Copy, N: Ord> FinalizationOutcome<H, N> {
/// Merge with another. This should only be used for displaced items that
/// are produced within one transaction of each other.
pub fn merge(&mut self, mut other: Self) {
Expand All @@ -63,6 +63,16 @@ impl<H, N: Ord> FinalizationOutcome<H, N> {
pub fn leaves(&self) -> impl Iterator<Item = &H> {
self.removed.values().flatten()
}

/// Constructor
pub fn new(new_displaced: impl Iterator<Item = (H, N)>) -> Self {
let mut removed = BTreeMap::<Reverse<N>, Vec<H>>::new();
for (hash, number) in new_displaced {
removed.entry(Reverse(number)).or_default().push(hash);
}

FinalizationOutcome { removed }
}
}

/// list of leaf hashes ordered by number (descending).
Expand Down Expand Up @@ -151,39 +161,12 @@ where
Some(RemoveOutcome { inserted, removed: LeafSetItem { hash, number } })
}

/// Note a block height finalized, displacing all leaves with number less than the finalized
/// block's.
///
/// Although it would be more technically correct to also prune out leaves at the
/// same number as the finalized block, but with different hashes, the current behavior
/// is simpler and our assumptions about how finalization works means that those leaves
/// will be pruned soon afterwards anyway.
pub fn finalize_height(&mut self, number: N) -> FinalizationOutcome<H, N> {
let boundary = if number == N::zero() {
return FinalizationOutcome { removed: BTreeMap::new() }
} else {
number - N::one()
};

let below_boundary = self.storage.split_off(&Reverse(boundary));
FinalizationOutcome { removed: below_boundary }
}

/// The same as [`Self::finalize_height`], but it only simulates the operation.
///
/// This means that no changes are done.
///
/// Returns the leaves that would be displaced by finalizing the given block.
pub fn displaced_by_finalize_height(&self, number: N) -> FinalizationOutcome<H, N> {
let boundary = if number == N::zero() {
return FinalizationOutcome { removed: BTreeMap::new() }
} else {
number - N::one()
};

let below_boundary = self.storage.range(&Reverse(boundary)..);
FinalizationOutcome {
removed: below_boundary.map(|(k, v)| (k.clone(), v.clone())).collect(),
/// Remove all leaves displaced by the last block finalization.
pub fn remove_displaced_leaves(&mut self, displaced_leaves: &FinalizationOutcome<H, N>) {
for (number, hashes) in &displaced_leaves.removed {
for hash in hashes.iter() {
self.remove_leaf(number, hash);
}
}
}

Expand Down
72 changes: 25 additions & 47 deletions substrate/client/db/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ use sc_client_api::{
use sc_state_db::{IsPruned, LastCanonicalized, StateDb};
use sp_arithmetic::traits::Saturating;
use sp_blockchain::{
Backend as _, CachedHeaderMetadata, Error as ClientError, HeaderBackend, HeaderMetadata,
HeaderMetadataCache, Result as ClientResult,
Backend as _, CachedHeaderMetadata, DisplacedLeavesAfterFinalization, Error as ClientError,
HeaderBackend, HeaderMetadata, HeaderMetadataCache, Result as ClientResult,
};
use sp_core::{
offchain::OffchainOverlayedChange,
Expand Down Expand Up @@ -747,19 +747,6 @@ impl<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
Ok(self.leaves.read().hashes())
}

fn displaced_leaves_after_finalizing(
&self,
block_number: NumberFor<Block>,
) -> ClientResult<Vec<Block::Hash>> {
Ok(self
.leaves
.read()
.displaced_by_finalize_height(block_number)
.leaves()
.cloned()
.collect::<Vec<_>>())
}

fn children(&self, parent_hash: Block::Hash) -> ClientResult<Vec<Block::Hash>> {
children::read_children(&*self.db, columns::META, meta_keys::CHILDREN_PREFIX, parent_hash)
}
Expand Down Expand Up @@ -1813,14 +1800,13 @@ impl<Block: BlockT> Backend<Block> {
apply_state_commit(transaction, commit);
}

let new_displaced = self.blockchain.leaves.write().finalize_height(f_num);
self.prune_blocks(
transaction,
f_num,
f_hash,
&new_displaced,
current_transaction_justifications,
)?;
let new_displaced = self.blockchain.displaced_leaves_after_finalizing(f_hash, f_num)?;
let finalization_outcome =
FinalizationOutcome::new(new_displaced.displaced_leaves.clone().into_iter());

self.blockchain.leaves.write().remove_displaced_leaves(&finalization_outcome);

self.prune_blocks(transaction, f_num, &new_displaced, current_transaction_justifications)?;

Ok(())
}
Expand All @@ -1829,8 +1815,7 @@ impl<Block: BlockT> Backend<Block> {
&self,
transaction: &mut Transaction<DbHash>,
finalized_number: NumberFor<Block>,
finalized_hash: Block::Hash,
displaced: &FinalizationOutcome<Block::Hash, NumberFor<Block>>,
displaced: &DisplacedLeavesAfterFinalization<Block>,
current_transaction_justifications: &mut HashMap<Block::Hash, Justification>,
) -> ClientResult<()> {
match self.blocks_pruning {
Expand Down Expand Up @@ -1858,10 +1843,10 @@ impl<Block: BlockT> Backend<Block> {

self.prune_block(transaction, BlockId::<Block>::number(number))?;
}
self.prune_displaced_branches(transaction, finalized_hash, displaced)?;
self.prune_displaced_branches(transaction, displaced)?;
},
BlocksPruning::KeepFinalized => {
self.prune_displaced_branches(transaction, finalized_hash, displaced)?;
self.prune_displaced_branches(transaction, displaced)?;
},
}
Ok(())
Expand All @@ -1870,21 +1855,13 @@ impl<Block: BlockT> Backend<Block> {
fn prune_displaced_branches(
&self,
transaction: &mut Transaction<DbHash>,
finalized: Block::Hash,
displaced: &FinalizationOutcome<Block::Hash, NumberFor<Block>>,
displaced: &DisplacedLeavesAfterFinalization<Block>,
) -> ClientResult<()> {
// Discard all blocks from displaced branches
for h in displaced.leaves() {
match sp_blockchain::tree_route(&self.blockchain, *h, finalized) {
Ok(tree_route) =>
for r in tree_route.retracted() {
self.blockchain.insert_persisted_body_if_pinned(r.hash)?;
self.prune_block(transaction, BlockId::<Block>::hash(r.hash))?;
},
Err(sp_blockchain::Error::UnknownBlock(_)) => {
// Sometimes routes can't be calculated. E.g. after warp sync.
},
Err(e) => Err(e)?,
for (_, tree_route) in displaced.tree_routes.iter() {
for r in tree_route.retracted() {
self.blockchain.insert_persisted_body_if_pinned(r.hash)?;
self.prune_block(transaction, BlockId::<Block>::hash(r.hash))?;
}
}
Ok(())
Expand Down Expand Up @@ -3190,6 +3167,9 @@ pub(crate) mod tests {

#[test]
fn test_leaves_pruned_on_finality() {
// / 1b - 2b - 3b
// 0 - 1a - 2a
// \ 1c
let backend: Backend<Block> = Backend::new_test(10, 10);
let block0 = insert_header(&backend, 0, Default::default(), None, Default::default());

Expand All @@ -3201,18 +3181,16 @@ pub(crate) mod tests {

let block2_a = insert_header(&backend, 2, block1_a, None, Default::default());
let block2_b = insert_header(&backend, 2, block1_b, None, Default::default());
let block2_c = insert_header(&backend, 2, block1_b, None, [1; 32].into());

assert_eq!(
backend.blockchain().leaves().unwrap(),
vec![block2_a, block2_b, block2_c, block1_c]
);
let block3_b = insert_header(&backend, 3, block2_b, None, [3; 32].into());

assert_eq!(backend.blockchain().leaves().unwrap(), vec![block3_b, block2_a, block1_c]);

backend.finalize_block(block1_a, None).unwrap();
backend.finalize_block(block2_a, None).unwrap();

// leaves at same height stay. Leaves at lower heights pruned.
assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a, block2_b, block2_c]);
// All leaves are pruned that are known to not belong to canonical branch
assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a]);
}

#[test]
Expand Down
7 changes: 5 additions & 2 deletions substrate/client/service/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -978,8 +978,11 @@ where

// The stale heads are the leaves that will be displaced after the
// block is finalized.
let stale_heads =
self.backend.blockchain().displaced_leaves_after_finalizing(block_number)?;
let stale_heads = self
.backend
.blockchain()
.displaced_leaves_after_finalizing(hash, block_number)?
.hashes();

let header = self
.backend
Expand Down
78 changes: 67 additions & 11 deletions substrate/primitives/blockchain/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@ use log::warn;
use parking_lot::RwLock;
use sp_runtime::{
generic::BlockId,
traits::{Block as BlockT, Header as HeaderT, NumberFor, Saturating},
traits::{Block as BlockT, Header as HeaderT, NumberFor, Saturating, Zero},
Justifications,
};
use std::collections::btree_set::BTreeSet;
use std::collections::{btree_map::BTreeMap, btree_set::BTreeSet};

use crate::header_metadata::HeaderMetadata;

use crate::error::{Error, Result};
use crate::{
error::{Error, Result},
tree_route, TreeRoute,
};

/// Blockchain database header backend. Does not perform any validation.
pub trait HeaderBackend<Block: BlockT>: Send + Sync {
Expand Down Expand Up @@ -172,14 +175,6 @@ pub trait Backend<Block: BlockT>:
/// Results must be ordered best (longest, highest) chain first.
fn leaves(&self) -> Result<Vec<Block::Hash>>;

/// Returns displaced leaves after the given block would be finalized.
///
/// The returned leaves do not contain the leaves from the same height as `block_number`.
fn displaced_leaves_after_finalizing(
&self,
block_number: NumberFor<Block>,
) -> Result<Vec<Block::Hash>>;

/// Return hashes of all blocks that are children of the block with `parent_hash`.
fn children(&self, parent_hash: Block::Hash) -> Result<Vec<Block::Hash>>;

Expand Down Expand Up @@ -255,6 +250,67 @@ pub trait Backend<Block: BlockT>:
}

fn block_indexed_body(&self, hash: Block::Hash) -> Result<Option<Vec<Vec<u8>>>>;

/// Returns all leaves that will be displaced after the block finalization.
fn displaced_leaves_after_finalizing(
&self,
finalized_block_hash: Block::Hash,
finalized_block_number: NumberFor<Block>,
) -> std::result::Result<DisplacedLeavesAfterFinalization<Block>, Error> {
let mut result = DisplacedLeavesAfterFinalization::new();
shamil-gadelshin marked this conversation as resolved.
Show resolved Hide resolved

if finalized_block_number == Zero::zero() {
return Ok(result)
}

// For each leaf determine whether it belongs to a non-canonical branch.
for leaf_hash in self.leaves()? {
let leaf_block_header = self.expect_header(leaf_hash)?;
let leaf_number = *leaf_block_header.number();

let leaf_tree_route = match tree_route(self, leaf_hash, finalized_block_hash) {
Ok(tree_route) => tree_route,
Err(Error::UnknownBlock(_)) => {
// Sometimes routes can't be calculated. E.g. after warp sync.
continue;
},
Err(e) => Err(e)?,
};

// Is it a stale fork?
let needs_pruning = leaf_tree_route.common_block().hash != finalized_block_hash;

if needs_pruning {
result.displaced_leaves.insert(leaf_hash, leaf_number);
result.tree_routes.insert(leaf_hash, leaf_tree_route);
}
}

Ok(result)
}
}

/// Calculation result of the displaced leaves after the block finalization.
#[derive(Clone, Debug)]
pub struct DisplacedLeavesAfterFinalization<Block: BlockT> {
/// A collection of hashes and block numbers for displaced leaves.
pub displaced_leaves: BTreeMap<Block::Hash, NumberFor<Block>>,

/// A collection of tree routes from the leaves to finalized block.
pub tree_routes: BTreeMap<Block::Hash, TreeRoute<Block>>,
}
shamil-gadelshin marked this conversation as resolved.
Show resolved Hide resolved

impl<Block: BlockT> DisplacedLeavesAfterFinalization<Block> {
/// Returns a collection of hashes for the displaced leaves.
pub fn hashes(&self) -> Vec<Block::Hash> {
self.displaced_leaves.keys().map(|h| *h).collect()
shamil-gadelshin marked this conversation as resolved.
Show resolved Hide resolved
}

/// Constructor. We need this explicit initialization to not introduce a requirement
/// `Block: Default`
pub fn new() -> Self {
Self { displaced_leaves: BTreeMap::new(), tree_routes: BTreeMap::new() }
}
shamil-gadelshin marked this conversation as resolved.
Show resolved Hide resolved
}

/// Blockchain info
Expand Down
2 changes: 1 addition & 1 deletion substrate/primitives/blockchain/src/header_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ pub fn lowest_common_ancestor<Block: BlockT, T: HeaderMetadata<Block> + ?Sized>(
}

/// Compute a tree-route between two blocks. See tree-route docs for more details.
pub fn tree_route<Block: BlockT, T: HeaderMetadata<Block>>(
pub fn tree_route<Block: BlockT, T: HeaderMetadata<Block> + ?Sized>(
backend: &T,
from: Block::Hash,
to: Block::Hash,
Expand Down
Loading