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

finalization: Skip tree route calculation if no forks present #4721

Merged
merged 5 commits into from
Jun 11, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Jun 6, 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 Change forks pruning algorithm. #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

@skunert skunert added the T0-node This PR/Issue is related to the topic “node”. label Jun 6, 2024
@skunert skunert requested review from bkchr and a team June 6, 2024 15:29
@burdges
Copy link

burdges commented Jun 6, 2024

Around this, how do we hash headers?

It's often problematic if we do H(big_header) with parent in big_header. It's often faster if we do H(small_header, H(big_header)) with the parent hash in small_header.

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM!

Merged via the queue into master with commit 96ab686 Jun 11, 2024
159 checks passed
@skunert skunert deleted the skunert/collator-sync-fix branch June 11, 2024 13:25
EgorPopelyaev pushed a commit that referenced this pull request Jun 11, 2024
EgorPopelyaev pushed a commit that referenced this pull request Jun 11, 2024
ordian added a commit that referenced this pull request Jun 13, 2024
* master: (29 commits)
  Append overlay optimization. (#1223)
  finalization: Skip tree route calculation if no forks present (#4721)
  Remove unncessary call remove_from_peers_set (#4742)
  add pov-recovery unit tests and support for elastic scaling (#4733)
  approval-voting: Add no shows debug information (#4726)
  Revamp the Readme of the parachain template (#4713)
  Update README.md to move the PSVM link under a "Tooling" section under the "Releases" section (#4734)
  frame/proc-macro: Refactor code for better readability (#4712)
  Contracts:  update wasmi to 0.32 (#3679)
  Backport style changes from P<>K bridge to R<>W bridge (#4732)
  New reference doc for Custom RPC V2 (#4654)
  Frame Pallets: Clean a lot of test setups (#4642)
  Fix occupied core handling (#4691)
  statement-distribution: Fix false warning (#4727)
  Update the README to include a link to the Polkadot SDK Version Manager (#4718)
  Cleanup PVF artifact by cache limit and stale time (#4662)
  Update link to a latest polkadot release (#4711)
  [CI] Delete cargo-deny config (#4677)
  fix build on MacOS: bump secp256k1 and secp256k1-sys to patched versions (#4709)
  Unify dependency aliases (#4633)
  ...
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
@nazar-pc nazar-pc mentioned this pull request Jun 27, 2024
2 tasks
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
This pull request was closed.
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
Status: done
Development

Successfully merging this pull request may close these issues.

Cant sync a kusama-people node from scratch with 1.12, working with 1.11, regression?
6 participants