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

Conversation

shamil-gadelshin
Copy link
Contributor

This PR changes the fork calculation and pruning algorithm to enable future block header pruning. It's required because the previous algorithm relied on the block header persistence. It follows the related discussion

The previous code contained this comment describing the situation:

	/// 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> {

The previous algorithm relied on the existing block headers to prune forks later and to enable block header pruning we need to clear all obsolete forks right after the block finalization to not depend on the related block headers in the future.

- Prune all possible forks after block finalizing without height limit.
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/block-header-pruning/7198/1

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the logic is correct, it is much more costly doing this than doing the "lazy" pruning as before. If we would record the block at which a certain leaf forked off, we should probably be able to achieve the same without iterating always over all the headers from the leaf down to the finalized block?

substrate/client/api/src/leaves.rs Outdated Show resolved Hide resolved
substrate/primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
shamil-gadelshin and others added 2 commits April 4, 2024 15:00
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <git@kchr.de>
@shamil-gadelshin
Copy link
Contributor Author

it is much more costly doing this than doing the "lazy" pruning as before

  1. I assume that we traverse the block subtree which is relatively shallow with a small of leaves on each block finalization and I see the most expensive operation here is "read header from DB (disk)" in a loop. (Am I correct here?).
    a) However, this operation is cached and is likely to be accessible from the cache in the majority of header acquisitions.
    b) Both the previous and the current algorithms invoke this function chain to prune blocks: note_finalized()->prune_blocks()->prune_displaced_branches()->tree_route() and tree_route is invoked for each dispaced leaf and contains within it a similar loop with extracting header metadata (which invokes the same "read header" operation each time). Even if the new algorithm will read all the necessary headers from DB instead of the cache (likely on a restart) - the subsequent tree_route operations will be much faster. To be precise the archive pruning mode won't invoke the prune_block code but we consider the worst case anyway.

If we would record the block at which a certain leaf forked off, we should probably be able to achieve the same without iterating always over all the headers from the leaf down to the finalized block?

  1. I understand your suggestion as follows: on each block import operation calculate a branching block for the fork, save it to a dedicated structure, and use this information later instead of calculation.
    a) Because of the possible restarts we need to make it persistent, but it contradicts your previous guideline: Add APIs for block pruning manually #1570 (comment)
    I understood that comment as "in general, we should avoid adding persistent data if we can calculate the result on the fly (similar to tree_route operation)".
    b) Even if we add the persistent cached data for current leaves it seems we will update this data in a similar loop but in the opposite direction: on each new block we would check whether it's a new best and recalculate saved "fork-points" for all the leaves. However, this time we won't be able to cache disk "write" operations. Did I understand it correctly?

Summary:
The new proposed algorithm doesn't seem to worsen the operation cost much because of the mostly cached operations and already existing tree_route operation.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/block-header-pruning/7198/2

@bkchr
Copy link
Member

bkchr commented Apr 4, 2024

  1. a) However, this operation is cached and is likely to be accessible from the cache in the majority of header acquisitions.

Yes, I mean I would assume the same as well. I think the best is to ask @arkpar on why he hasn't done it this way from the beginning. There is maybe something we oversee, otherwise I'm also fine with the approach you are proposing here.

@arkpar
Copy link
Member

arkpar commented Apr 5, 2024

This was written by someone else, but I guess it was done for simplicity, as the quoted comment explains.

As for this PR, it looks good for me. I wonder if it can be optimized though. prune_displaced_branches calls sp_blockchain::tree_route for each displaced leaf anyway. Instead of traversing the header chain, it should be possible to just call tree_route once somewhere and then store or pass around the retracted path for each leaf.

@shamil-gadelshin
Copy link
Contributor Author

I implemented the suggestions from @arkpar The code seems simpler now.

`finalize_height` method doesn’t exist. It was used to determine the forks and that algorithm changed.
@shamil-gadelshin
Copy link
Contributor Author

The last commit removes the tests related to the removed method: finalize_height.
The new code is tested with other tests related to pruning blocks (displaced leaves).

@shamil-gadelshin
Copy link
Contributor Author

The last commits fix tests by updating the expected block where stale heads appear. However, I also changed expand_forks to use tree_route function first. It seems to me that the function works incorrectly because it uses hash(block_number) function to work with the canonical chain to get a parent block by number. I don't think that hash(block_number) defines its behavior in the presence of forks (it just reads from DB). Please, correct me if I'm wrong. I didn't delete the original code and left it as a fallback option because it tries to return a partial route in case where some blocks are missing and tree_route doesn't do that.

@shamil-gadelshin
Copy link
Contributor Author

@arkpar Do you mind looking at the expand_fork() changes?

@bkchr
Copy link
Member

bkchr commented Apr 16, 2024

I don't think that hash(block_number) defines its behavior in the presence of forks (it just reads from DB). Please, correct me if I'm wrong.

When you call hash(block_number), it will return the hash of the canonical block at block_number. If the hash that is returned, is different to your parent you are still in a fork from the POV of the canonical chain. This implementation is correct and also faster than tree_route, because it doesn't go up the chain again.

@@ -479,35 +436,4 @@ mod tests {
assert!(set.contains(10, 1_2));
assert!(!set.contains(10, 1_3));
}

#[test]
fn finalization_consistent_with_disk() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finalize_height was removed from LeafSet. Please, let me know if you see how this test should be reimplemented differently.

Comment on lines 110 to 121
match tree_route(self, *fork_head, self.info().finalized_hash) {
Ok(tree_route) => {
for block in tree_route.retracted() {
expanded_forks.insert(block.hash);
}
continue
},
Err(_) => {
// Continue with fallback algorithm
},
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
match tree_route(self, *fork_head, self.info().finalized_hash) {
Ok(tree_route) => {
for block in tree_route.retracted() {
expanded_forks.insert(block.hash);
}
continue
},
Err(_) => {
// Continue with fallback algorithm
},
}

See my comment in the pr.

substrate/primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
substrate/primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
substrate/primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
substrate/primitives/blockchain/src/backend.rs Outdated Show resolved Hide resolved
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Apr 16, 2024
Co-authored-by: Bastian Köcher <git@kchr.de>
@github-actions github-actions bot requested review from arkpar and bkchr April 17, 2024 14:24
@shamil-gadelshin
Copy link
Contributor Author

Could you please remove one of the code paths?

Sure. I removed the old code and updated the function comment as well as its dependencies. I also updated one of the tests to counter its flaky behavior.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@bkchr bkchr enabled auto-merge May 14, 2024 20:22
@bkchr
Copy link
Member

bkchr commented May 14, 2024

@shamil-gadelshin the rustdoc jobs are still failing.

auto-merge was automatically disabled May 15, 2024 07:09

Head branch was pushed to by a user without write access

@github-actions github-actions bot requested a review from bkchr May 15, 2024 07:09
@bkchr bkchr enabled auto-merge May 15, 2024 07:57
@bkchr bkchr added this pull request to the merge queue May 15, 2024
Merged via the queue into paritytech:master with commit 9c69bb9 May 15, 2024
146 of 150 checks passed
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
This PR changes the fork calculation and pruning algorithm to enable
future block header pruning. It's required because the previous
algorithm relied on the block header persistence. It follows the
[related
discussion](paritytech#1570)

The previous code contained this comment describing the situation:
```
	/// 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> {
```

The previous algorithm relied on the existing block headers to prune
forks later and to enable block header pruning we need to clear all
obsolete forks right after the block finalization to not depend on the
related block headers in the future.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
github-merge-queue bot pushed a commit that referenced this pull request Jun 11, 2024
## Issue

Currently, syncing parachains from scratch can lead to a very long
finalization time once they reach the tip of the chain. The problem is
that we try to finalize everything from 0 to the tip, which can be
thousands or even millions of blocks.

We finalize sequentially and try to compute displaced branches during
finalization. So for every block on the way, we compute an expensive
tree route.

## Proposed Improvements

In this PR, I propose improvements that solve this situation:

- **Skip tree route calculation if `leaves().len() == 1`:** This should
be enough for 90% of cases where there is only one leaf after sync.
- **Optimize finalization for long distances:** It can happen that the
parachain has imported some leaf and then receives a relay chain
notification with the finalized block. In that case, the previous
optimization will not trigger. A second mechanism should ensure that we
do not need to compute the full tree route. If the finalization distance
is long, we check the lowest common ancestor of all the leaves. If it is
above the to-be-finalized block, we know that there are no displaced
leaves. This is fast because forks are short and close to the tip, so we
can leverage the header cache.

## Alternative Approach

- The problem was introduced in #3962. Reverting that PR is another
possible strategy.
- We could store for every fork where it begins, however sounds a bit
more involved to me.


fixes #4614
Ank4n pushed a commit that referenced this pull request Jun 14, 2024
## Issue

Currently, syncing parachains from scratch can lead to a very long
finalization time once they reach the tip of the chain. The problem is
that we try to finalize everything from 0 to the tip, which can be
thousands or even millions of blocks.

We finalize sequentially and try to compute displaced branches during
finalization. So for every block on the way, we compute an expensive
tree route.

## Proposed Improvements

In this PR, I propose improvements that solve this situation:

- **Skip tree route calculation if `leaves().len() == 1`:** This should
be enough for 90% of cases where there is only one leaf after sync.
- **Optimize finalization for long distances:** It can happen that the
parachain has imported some leaf and then receives a relay chain
notification with the finalized block. In that case, the previous
optimization will not trigger. A second mechanism should ensure that we
do not need to compute the full tree route. If the finalization distance
is long, we check the lowest common ancestor of all the leaves. If it is
above the to-be-finalized block, we know that there are no displaced
leaves. This is fast because forks are short and close to the tip, so we
can leverage the header cache.

## Alternative Approach

- The problem was introduced in #3962. Reverting that PR is another
possible strategy.
- We could store for every fork where it begins, however sounds a bit
more involved to me.


fixes #4614
liuchengxu pushed a commit to liuchengxu/polkadot-sdk that referenced this pull request Jun 19, 2024
This PR changes the fork calculation and pruning algorithm to enable
future block header pruning. It's required because the previous
algorithm relied on the block header persistence. It follows the
[related
discussion](paritytech#1570)

The previous code contained this comment describing the situation:
```
	/// 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> {
```

The previous algorithm relied on the existing block headers to prune
forks later and to enable block header pruning we need to clear all
obsolete forks right after the block finalization to not depend on the
related block headers in the future.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
@bkchr bkchr mentioned this pull request Jun 28, 2024
2 tasks
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
This PR changes the fork calculation and pruning algorithm to enable
future block header pruning. It's required because the previous
algorithm relied on the block header persistence. It follows the
[related
discussion](paritytech#1570)

The previous code contained this comment describing the situation:
```
	/// 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> {
```

The previous algorithm relied on the existing block headers to prune
forks later and to enable block header pruning we need to clear all
obsolete forks right after the block finalization to not depend on the
related block headers in the future.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…tech#4721)

## Issue

Currently, syncing parachains from scratch can lead to a very long
finalization time once they reach the tip of the chain. The problem is
that we try to finalize everything from 0 to the tip, which can be
thousands or even millions of blocks.

We finalize sequentially and try to compute displaced branches during
finalization. So for every block on the way, we compute an expensive
tree route.

## Proposed Improvements

In this PR, I propose improvements that solve this situation:

- **Skip tree route calculation if `leaves().len() == 1`:** This should
be enough for 90% of cases where there is only one leaf after sync.
- **Optimize finalization for long distances:** It can happen that the
parachain has imported some leaf and then receives a relay chain
notification with the finalized block. In that case, the previous
optimization will not trigger. A second mechanism should ensure that we
do not need to compute the full tree route. If the finalization distance
is long, we check the lowest common ancestor of all the leaves. If it is
above the to-be-finalized block, we know that there are no displaced
leaves. This is fast because forks are short and close to the tip, so we
can leverage the header cache.

## Alternative Approach

- The problem was introduced in paritytech#3962. Reverting that PR is another
possible strategy.
- We could store for every fork where it begins, however sounds a bit
more involved to me.


fixes paritytech#4614
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants