revert should work even for finalised blocks

[gavofyork]

Doesn't seem to want to revert the correct chain.

➜  substrate git:(a456f8f1) ✗ ~/Desktop/substrate.master --name "Not So Fast"
2019-01-09 14:51:03 Substrate Node
2019-01-09 14:51:04   version 0.9.1-5fc4ef6a-x86_64-macos
2019-01-09 14:51:04   by Parity Technologies, 2017, 2018
2019-01-09 14:51:04 Chain specification: Charred Cherry
2019-01-09 14:51:04 Node name: Not So Fast
2019-01-09 14:51:04 Roles: FULL
2019-01-09 14:51:04 Best block: #164712
2019-01-09 14:51:04 Local node address is: /ip4/0.0.0.0/tcp/30333/p2p/QmZQiepeArd1U1JvuW51jGN96tcLR2FoTREkFXsipsQVqn
2019-01-09 14:51:04 Idle (0 peers), best: #164712 (0x371a…4058)
^C
➜  substrate git:(a456f8f1) ✗ ~/Desktop/substrate.master revert --chain cherry 1
2019-01-09 14:51:07 Reverted 0 blocks. Best: #0 (0x7e22…f3c3)
➜  substrate git:(a456f8f1) ✗ ~/Desktop/substrate.master revert 1               
2019-01-09 14:51:12 Reverted 0 blocks. Best: #0 (0x7e22…f3c3)

[bkchr]

I looked into it, revert only reverts blocks that are not finalized, aka in the noncanonical store. We need to remove blocks from the database as well. @arkpar I think you should be most familiar with the database code?

[arkpar]

Deleting blocks is easy enough. Not sure if any finality records should be reverted as well.
@rphmeier @svyatonik ?

[svyatonik]

What I know:

1. there's also authorities cache, which should be cleared in this case (it is safe). But since it isn't used currently, we could just ignore it for now;
2. there are changes tries, which are possibly pruned for old finalized blocks. But since changes tries are not enabled by default (=> on CC afaiu), we could just ignore it. There's no correct+optimal solution for restoring pruned changes tries (i.e. when last_finalized_block decreases) - I hope that reverting finalized blocks is a temporary problem, isn't it? Otherwise we'll need to reconstruct pruned changes tries by re-executing all blocks starting from genesis;
3. the othere problem, iiuc, will be reverting aux storage, which is used by GRANDPA. It tracks GRANDPA authority set id, which in turn is used to check block' finalization justification. So simply deleting it is wrong (if set has been ever changed, it will be > 0 and after deletion it will be 0 => justification check will fail). And restoring it is non-trivial - like you need to scan all blocks from genesis to the best block && check it for GRANDPA authority set change events. I may be wrong here - hope @rphmeier will check this.

[rphmeier]

yeah, you would have to keep history of all set changes in the finality-grandpa aux-db, which is a lot, or scan absolutely everything on reversion.

And even if we can do this, it will lead to slashing once we have runtimes that know how to handle safety violations.

[marcio-diaz]

I would like to fix this issue but probably I'll need some mentoring.

[rphmeier]

@marcio-diaz : I can mentor. If we don't care about the DB state bloat for now we can just keep track of all historic signalled changes somewhere in GRANDPA and add some trait-based machinery around the revert chain operation.

[rphmeier]

This will add a lot of plumbing to the consensus/client layers, introducing a circular dependency which currently does not exist, so I am really wary of including something like this.

As an alternative, we could introduce GRANDPA finality limits (via a --finality-limit flag in Node, node-template, and Polkadot) and resync authorities with this flag when we need to revert finality.

There should also be a log about being unable to revert unfinalized blocks.

[andresilva]

The --finality-limit would only allow reverting finality of blocks within the latest authority set epoch right? Also it's possible that when resyncing the authorities we would get blocks from the network with justifications attached (which we would use to finalize the block). Or maybe I'm misunderstanding how that would work.

[jasl]

I'm trying this at latest Polkadot v0.9.22

./polkadot revert --chain kusama --base-path data/polkadot 325000
2022-05-21 08:58:25 Essential task `txpool-background` failed. Shutting down service.
2022-05-21 08:58:25 Essential task `transaction-pool-task-1` failed. Shutting down service.
2022-05-21 08:58:25 Essential task `transaction-pool-task-0` failed. Shutting down service.
2022-05-21 08:58:25 Essential task `basic-block-import-worker` failed. Shutting down service.
2022-05-21 08:58:25 There aren't any non-finalized blocks to revert.

it not revert finalized blocks, does the behavior changed?

UPDATE:

after run revert, the db seems corrupted

2022-05-21 22:18:34 [Relaychain] ⚙️  Syncing  0.0 bps, target=#12786636 (11 peers), best: #12778495 (0x2999…9b1f), finalized #12778495 (0x2999…9b1f), ⬇ 44.6kiB/s ⬆ 14.3kiB/s
2022-05-21 22:18:34 [Parachain] 💤 Idle (25 peers), best: #1651718 (0xa80c…b26c), finalized #1648352 (0x228e…848f), ⬇ 8.9kiB/s ⬆ 3.2kiB/s
2022-05-21 22:18:35 [Relaychain] 💔 Error importing block 0xd12a7b1fc2f28b0152797c143d6164ec15ace445047233b64616da9bd92bf181: consensus error: Import failed: Parent block of 0xd12a…f181 has no associated weight
2022-05-21 22:18:37 [Relaychain] 💔 Error importing block 0xd12a7b1fc2f28b0152797c143d6164ec15ace445047233b64616da9bd92bf181: consensus error: Import failed: Parent block of 0xd12a…f181 has no associated weight

It can no longer sync new block

[MrishoLukamba]

Okey am not an expert on consensus but a the meaning of finalization is irreversible

[davxy]

after run revert, the db seems corrupted

2022-05-21 22:18:34 [Relaychain] ⚙️  Syncing  0.0 bps, target=#12786636 (11 peers), best: #12778495 (0x2999…9b1f), finalized #12778495 (0x2999…9b1f), ⬇ 44.6kiB/s ⬆ 14.3kiB/s
2022-05-21 22:18:34 [Parachain] 💤 Idle (25 peers), best: #1651718 (0xa80c…b26c), finalized #1648352 (0x228e…848f), ⬇ 8.9kiB/s ⬆ 3.2kiB/s
2022-05-21 22:18:35 [Relaychain] 💔 Error importing block 0xd12a7b1fc2f28b0152797c143d6164ec15ace445047233b64616da9bd92bf181: consensus error: Import failed: Parent block of 0xd12a…f181 has no associated weight
2022-05-21 22:18:37 [Relaychain] 💔 Error importing block 0xd12a7b1fc2f28b0152797c143d6164ec15ace445047233b64616da9bd92bf181: consensus error: Import failed: Parent block of 0xd12a…f181 has no associated weight

It can no longer sync new block

Addressed by #11500