Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

BABE: Fix aux data cleaning #11263

Merged
merged 1 commit into from
Apr 22, 2022
Merged

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Apr 21, 2022

With the latest optimizations of the FinalityNotification generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the heigh_limit. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than heigh_limit (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the finalized blocks in
the FinalityNotification.

Fixes one part of: paritytech/polkadot-sdk#59

With the latest optimizations of the `FinalityNotification` generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the `heigh_limit`. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than `heigh_limit` (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the `finalized` blocks in
the `FinalityNotification`.
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Apr 21, 2022
@bkchr bkchr requested a review from davxy April 21, 2022 20:17
@bkchr bkchr requested a review from andresilva as a code owner April 21, 2022 20:17
Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

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

Looks good to me

client/consensus/babe/src/lib.rs Show resolved Hide resolved
@bkchr bkchr merged commit 6625989 into master Apr 22, 2022
@bkchr bkchr deleted the bkchr-babe-aux-data-cleaning-fix-up branch April 22, 2022 14:58
chevdor pushed a commit that referenced this pull request Apr 25, 2022
With the latest optimizations of the `FinalityNotification` generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the `heigh_limit`. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than `heigh_limit` (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the `finalized` blocks in
the `FinalityNotification`.
bkchr added a commit that referenced this pull request Apr 25, 2022
With the latest optimizations of the `FinalityNotification` generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the `heigh_limit`. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than `heigh_limit` (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the `finalized` blocks in
the `FinalityNotification`.

Co-authored-by: Bastian Köcher <[email protected]>
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
With the latest optimizations of the `FinalityNotification` generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the `heigh_limit`. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than `heigh_limit` (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the `finalized` blocks in
the `FinalityNotification`.
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
With the latest optimizations of the `FinalityNotification` generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the `heigh_limit`. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than `heigh_limit` (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the `finalized` blocks in
the `FinalityNotification`.
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
With the latest optimizations of the `FinalityNotification` generation, the aux data pruning started
to print a warning. The problem here was that we printed a warning and stopped the adding of blocks
to prune when we hit the `heigh_limit`. This is now wrong, as we could for example have two 512 long
forks and then we start finalizing one of them. The second fork head would be part of the stale
heads at some point (in the current implementation when we finalize second fork head number + 1),
but then we would actually need to go back into the past than `heigh_limit` (which was actually
last_finalized - 1). We now go back until we reach the canonical chain.

Also fixed some wrong comment that was added by be about the content of the `finalized` blocks in
the `FinalityNotification`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants