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

Conversation

@andresilva
Copy link
Contributor

@andresilva andresilva commented Oct 2, 2019

Currently as we observe BABE epoch changes we track them in a fork tree which keeps growing indefinitely as the chain grows. Since we have finality, after a given block is finalized we know that the chain will not revert past that point and therefore we can prune the tree out of epochs that could not be live at the finalized block (i.e. epochs from other forks, and epochs on the canonical chain for which epoch.end_slot() <= finalized_slot). The implementation is rather simple since we just use find_node_where to find the deepest ancestor of the finalized block that passes the given predicate, and if found we re-root the tree to it.

I built polkadot with this patch using the database of one of the validators which was filled with ancient forks. The tree was pruned correctly and import time went back to normal afterwards (getting the current epoch from the tree taking ~3ms rather than a second). #3665 should also make this significantly faster.

Fixes #3651. @rphmeier suggested the following:

IMO we should not prune as eagerly as we possibly can, but instead bake in an allowance of a few epochs just in case we'd like to keep around data from those epochs.

I suggest that we implement this by subtracting a fixed delay to the finalized slot we pass in the predicate when searching the tree, i.e. epoch.end_slot() <= (finalized_slot - N_EPOCHS_TO_KEEP * SLOTS_PER_EPOCH). My initial implementation was keeping the path from root to the current node when traversing the tree, which was then used to traverse back arbitrarily when the correct node was found. This made the implementation much more complicated and I'd also like to avoid introducing parent pointers in the tree for the same reason.

TODO:

  • Test babe pruning on block import

@andresilva andresilva requested a review from Demi-Marie as a code owner October 2, 2019 15:25
@andresilva
Copy link
Contributor Author

@rphmeier I didn't include any changes to traverse the tree according to max depth (sorting children), I think if we have fast lca and a reasonably small tree by pruning that shouldn't be necessary. I can add it in a separate PR if you think it's worth it.

@andresilva andresilva added A0-please_review Pull request needs code review. M4-core labels Oct 2, 2019
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

This mostly LGTM but I am not at all familiar with tree pruning.

Copy link
Contributor

@marcio-diaz marcio-diaz 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, maybe add the test in your TODO or something closer.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

logic looks good to me.

@rphmeier rphmeier added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Oct 4, 2019
@rphmeier rphmeier added this to the 2.0-k milestone Oct 4, 2019
@andresilva andresilva force-pushed the andre/babe-epoch-pruning branch from f06e872 to 421e766 Compare October 4, 2019 16:13
@andresilva
Copy link
Contributor Author

Missing test added.

@andresilva andresilva added A0-please_review Pull request needs code review. and removed A6-mustntgrumble labels Oct 4, 2019
@gavofyork gavofyork merged commit b4cd248 into master Oct 4, 2019
@gavofyork gavofyork deleted the andre/babe-epoch-pruning branch October 4, 2019 17:51

// we found the deepest ancestor of the finalized block, so we prune
// out any children that don't include the finalized block.
root.children.retain(|node| {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is only 1 child that can fulfill this precondition, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, do you think this should be more explicit?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is just going to be a little more inefficient

Copy link
Contributor

Choose a reason for hiding this comment

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

let children = mem::replace(&mut root.children, Vec::new());
root.children = children.into_iter().filter(|node| ...).take(1).collect()

@gnunicorn gnunicorn modified the milestones: 2.1, Polkadot Mar 4, 2020
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BABE: prune ancient epochs from the fork-tree and prune on epoch input

7 participants