BABE's revert procedure #11022

davxy · 2022-03-13T13:38:59Z

Partially addresses #10962

This PR addresses the rollback of BABE's data structures on "revert" command.

In particular this involves the removal of some nodes from the EpochChanges tree.

This functionality is mostly provided via the ForkTree new filter method.

Given a ForkTree instance this method allows to remove nodes (and their subtrees) from the overall tree via a predicate closure.

The predicate drives the filtering behavior, in particular allows to:

P1. remove a node (and its subtree);
P2. early stop the overall operation;
P3. stop the operation for the node under processing, thus skipping its subtree from the operation.

When used by the BABE's revert procedure, given the hash of THE block we what to revert to, the passed predicate:

searches for the highest (wrt tree height) ForkTree's nodes such that THE block is an ancestor of the block that instanced such nodes (announced the epoch). These nodes (and their subtrees) are removed from the tree (leverages P1);
detects a "backtrack", i.e. detects if some nodes were already removed and the search started looking into some higher (wrt tree height) branches. At this point the search can stop since no other nodes can satisfy 1 (leverages P2);
avoid searching into the tree branches that are not containing the block we are looking for (leverages P3).

Known limitations

Assuming a strictly monotonic machine clock, revert across more than one epoch breaks BABE's algorithm.
- the next imported block will have a slot greater than last epoch end slot, thus it will immediately start a new epoch;
- this new epoch end-slot is already smaller than the imported block slot. Nevertheless the block is imported in this new epoch;
- following imported blocks will generate the infamous "Unexpected Epoch Changes" error (How to prevent and deal with unexpected epoch changes #4464). This is due to the fact that the block will come with the announcement of another new epoch BUT the parent_slot is greater than epoch descriptor start slot (see here and here)
Revert is not atomic wrt blockchain state revert procedure. Currently the two procedures are independent and the blockchain state revert may fail after we successfully reverted BABE structures.

polkadot companion: paritytech/polkadot#5109
cumulus companion: paritytech/cumulus#1089

sergejparity · 2022-03-14T12:43:47Z

Please consider to pull latest master into your branch to fix GHA Check labels failure. Location of CI scripts has changed in #11008

davxy · 2022-03-14T18:14:32Z

client/cli/src/commands/revert_cmd.rs

@@ -43,16 +43,27 @@ pub struct RevertCmd {
 	pub pruning_params: PruningParams,
 }

+/// Revert handler for auxiliary data (e.g. consensus).
+type AuxRevertHandler<B> = Box<dyn FnOnce(NumberFor<B>) -> error::Result<()>>;


The aux data revert can be made atomic by returning a Vec<(Vec<u8>, Option<Vec<u8>>)>.

This vector has the same purpose of BlockImportParams::auxiliary for block import, i.e. perform a bunch of operations on aux data atomically together with blockchain state operations.

The only drawback is that currently the blockchain state is reverted one block at a time (see here). If we want an "atomic" revert of "aux-data + blockchain-state" then we will have to call AuxRevertHandler multiple times as well (once for each reverted block)

I am surprised we do not support begin/commit/rollback transaction for batching multiple mutations across aux and main state storage already.

skunert · 2022-03-16T09:16:48Z

Looks Good to me!

Revert is not atomic wrt blockchain state revert procedure. Currently the two procedures are independent and the blockchain state revert may fail after we successfully reverted BABE structures.

Regarding this - If for any reason we fail after reverting the BABE structures and end up in an inconsistent node state, we should still be able to re-execute the revert right?

davxy · 2022-03-16T12:39:43Z

Regarding this - If for any reason we fail after reverting the BABE structures and end up in an inconsistent node state, we should still be able to re-execute the revert right?

YES, you can retry.

The BABE's "revert" procedure fails only if it cannot find the header of the block we are reverting to (and the headers are still there), so the procedure can be re-executed.

The second time the BABE's "revert" procedure is run, no node will be deleted and it will then re-try blockchain state revert.

As said, because blockchain-state and BABE revert is not atomic, the only caveat is if the first pass of BABE's "revert" deleted some epoch nodes then the chain state is inconsistent until the blockchain-state is reverted as well.

bin/node/cli/src/command.rs

wigy-opensource-developer · 2022-03-16T16:31:08Z

client/cli/src/commands/revert_cmd.rs

@@ -43,16 +43,27 @@ pub struct RevertCmd {
 	pub pruning_params: PruningParams,
 }

+/// Revert handler for auxiliary data (e.g. consensus).
+type AuxRevertHandler<B> = Box<dyn FnOnce(NumberFor<B>) -> error::Result<()>>;


I am surprised we do not support begin/commit/rollback transaction for batching multiple mutations across aux and main state storage already.

utils/fork-tree/src/lib.rs

client/consensus/babe/src/lib.rs

utils/fork-tree/src/lib.rs

client/consensus/epochs/src/lib.rs

client/consensus/babe/src/lib.rs

andresilva · 2022-03-22T17:30:18Z

LGTM, just left some minor comments regarding the filter interface. Thanks @davxy!

davxy · 2022-03-24T08:51:44Z

bot merge

cumulus: 4e95228 polkadot: 975e780ae0d988dc033f400ba822d14b326ee5b9 substrate: 89fcb3e4f62d221d4e161a437768e77d6265889e bump serde_json to 1.0.79 sync changes from paritytech/substrate#11022 Signed-off-by: acatangiu <[email protected]>

cumulus: b468d0c polkadot: 827792ca833396c82c726eda0bc2ad32ecddba73 substrate: 666f39b8a22108f57732215de006518738034ba2 bump serde_json to 1.0.79 sync changes from paritytech/substrate#11022 Signed-off-by: acatangiu <[email protected]>

cumulus: b468d0c polkadot: 827792ca833396c82c726eda0bc2ad32ecddba73 substrate: 666f39b8a22108f57732215de006518738034ba2 bump serde_json to 1.0.79 sync changes from paritytech/substrate#11022 fixed clippy warnings Signed-off-by: acatangiu <[email protected]>

* First rough draft for BABE revert * Proper babe revert test * Cleanup * Test trivial cleanup * Fix to make clippy happy * Check polkadot companion * Check cumulus companion * Remove babe's blocks weight on revert * Handle "empty" blockchain edge case * Run companions * Simplify the filter predicate * Saturating sub is not required * Run pipeline * Run pipeline again...

cumulus: b468d0c polkadot: 827792ca833396c82c726eda0bc2ad32ecddba73 substrate: 666f39b8a22108f57732215de006518738034ba2 bump serde_json to 1.0.79 sync changes from paritytech/substrate#11022 fixed clippy warnings Signed-off-by: acatangiu <[email protected]>

* First rough draft for BABE revert * Proper babe revert test * Cleanup * Test trivial cleanup * Fix to make clippy happy * Check polkadot companion * Check cumulus companion * Remove babe's blocks weight on revert * Handle "empty" blockchain edge case * Run companions * Simplify the filter predicate * Saturating sub is not required * Run pipeline * Run pipeline again...

First rough draft for BABE revert

a80baae

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 13, 2022

Proper babe revert test

a494b12

davxy added 2 commits March 14, 2022 15:26

Merge branch 'master' into davxy-revert-command-fix

e8c1640

Cleanup

3e95228

davxy added 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 Mar 14, 2022

davxy marked this pull request as ready for review March 14, 2022 15:03

davxy requested a review from andresilva as a code owner March 14, 2022 15:03

davxy requested a review from bkchr March 14, 2022 15:03

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 14, 2022

davxy requested a review from a team March 14, 2022 15:03

davxy added 2 commits March 14, 2022 16:25

Test trivial cleanup

cbbd61f

Fix to make clippy happy

301f55a

davxy commented Mar 14, 2022

View reviewed changes

davxy mentioned this pull request Mar 14, 2022

[DO NOT MERGE] Companion for revert command new API (#11022) paritytech/polkadot#5109

Closed

Check polkadot companion

2cf3e1c

davxy mentioned this pull request Mar 15, 2022

Companion for revert command new API (#11022) paritytech/cumulus#1089

Merged

Check cumulus companion

71ae24d

wigy-opensource-developer reviewed Mar 16, 2022

View reviewed changes

Remove babe's blocks weight on revert

c052a28

davxy commented Mar 16, 2022

View reviewed changes

client/consensus/babe/src/lib.rs Show resolved Hide resolved

Handle "empty" blockchain edge case

81f8f85

github-actions bot added the A7-needspolkadotpr label Mar 16, 2022

Run companions

a418f81

github-actions bot removed the A7-needspolkadotpr label Mar 16, 2022

andresilva approved these changes Mar 22, 2022

View reviewed changes

utils/fork-tree/src/lib.rs Outdated Show resolved Hide resolved

client/consensus/epochs/src/lib.rs Outdated Show resolved Hide resolved

client/consensus/babe/src/lib.rs Show resolved Hide resolved

Simplify the filter predicate

de1ede9

wigy-opensource-developer approved these changes Mar 23, 2022

View reviewed changes

Saturating sub is not required

41b192e

davxy added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 23, 2022

Run pipeline

042b0ea

github-actions bot added the A7-needspolkadotpr label Mar 23, 2022

Run pipeline again...

b9d1549

paritytech-processbot bot merged commit 56c28e8 into master Mar 24, 2022

paritytech-processbot bot deleted the davxy-revert-command-fix branch March 24, 2022 08:51

acatangiu mentioned this pull request Mar 28, 2022

Update Substrate/Polkadot/Cumulus references paritytech/parity-bridges-common#1364

Merged

3 tasks

davxy mentioned this pull request Apr 6, 2022

Revert persistent data related to parachains management #11178

Closed

davxy mentioned this pull request Nov 2, 2022

Member Request polkadot-fellows/seeding#56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BABE's revert procedure #11022

BABE's revert procedure #11022

davxy commented Mar 13, 2022 •

edited

Loading

sergejparity commented Mar 14, 2022

davxy Mar 14, 2022 •

edited

Loading

wigy-opensource-developer Mar 16, 2022

skunert commented Mar 16, 2022 •

edited

Loading

davxy commented Mar 16, 2022

wigy-opensource-developer Mar 16, 2022

andresilva commented Mar 22, 2022

davxy commented Mar 24, 2022

BABE's revert procedure #11022

BABE's revert procedure #11022

Conversation

davxy commented Mar 13, 2022 • edited Loading

Known limitations

sergejparity commented Mar 14, 2022

davxy Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

wigy-opensource-developer Mar 16, 2022

Choose a reason for hiding this comment

skunert commented Mar 16, 2022 • edited Loading

davxy commented Mar 16, 2022

wigy-opensource-developer Mar 16, 2022

Choose a reason for hiding this comment

andresilva commented Mar 22, 2022

davxy commented Mar 24, 2022

davxy commented Mar 13, 2022 •

edited

Loading

davxy Mar 14, 2022 •

edited

Loading

skunert commented Mar 16, 2022 •

edited

Loading