inherent disputes: remove per block initializer and disputes timeout event #6937

sandreim · 2023-03-22T13:59:41Z

TODO:

remove timeout related configuration
Final versi burnin

https://github.com/paritytech/polkadot/blob/master/runtime/parachains/src/disputes.rs#L918 is the culprit for the heavy weight blocks. During each block execution, but before paras inherent enter, this piece of code iterates all active/inactive disputes to mark them as timed out if that is the case. We keep disputes for 6 sessions, then we prune them. For 60k disputes as seen on Polkadot, this means 1,200000s (20usec * 60000) of weight not accounted for.

My fix is to remove the timeout event and the code relevant to it, and allow those disputes to be pruned as session advances. As discussed with @eskimor we don't really need the timeout event. However I don't have a good understanding if we would want to slash timed out disputes in the future, or how that should work. cc @ordian . If these are still needed I have already explored some options like:

separate concluded disputes storage
per block expire entries and multi-block processing if its really needed

Reducing the maximum allowed dispute weight to just 70% of the total block weight is to reduce heavy block import times during high load. I would personally go even lower to 50%, but with current value plus backing and bitfield processing weight should be 90% of a block. Initially I have wrongly assumed that without pushing enough disputes on chain it would slow finality, but this is wrong as finality happens offchain and we just need the votes for slashing.

I've tested this fix on Versi (without reducing the dispute weight) and metrics below confirm it works. By time index in the chart we start before the fix with disputes ongoing. Then disputes are stopped and you can see the stair pattern (10m sessions) as disputes are pruned on each new session. After the fix is applied, the block times have different distribution as we are no longer paying that cost of iterating every dispute and also get back to normal as soon as disputes are ended.

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

runtime/parachains/src/paras_inherent/mod.rs

ordian · 2023-03-22T14:13:13Z

runtime/parachains/src/disputes.rs

-			{
-				Self::deposit_event(Event::DisputeTimedOut(candidate_hash));
-
-				dispute.concluded_at = Some(now);


As discussed with @eskimor we don't really need the timeout event

Could you post the gist of the discussion here? This code not only removes timeout events, but the notion of timing out for disputes. I'm lacking the context here. cc @burdges

Yes:

Disputes that hit the runtime should actually never time out - they have been confirmed, so they should conclude.

We don't have a timeout on the node side. So timing them out in the runtime is kind of pointless and out of sync with what the node would assume about the dispute.

We drop disputes after 6 sessions anyway.

We can not and should not slash anybody on a timed out dispute - the people who voted are the good guys (most likely).

We drop disputes after 6 sessions anyway.

Yes, but maybe we should be reverting a block if the dispute hasn't concluded instead of finalizing it after some time?

Yes, what we could and probably should do: Revert immediately once f+1 validators voted against a candidate.

runtime/parachains/src/paras_inherent/mod.rs

eskimor · 2023-03-22T14:25:32Z

runtime/parachains/src/disputes.rs

-			{
-				Self::deposit_event(Event::DisputeTimedOut(candidate_hash));
-
-				dispute.concluded_at = Some(now);


Yes:

Disputes that hit the runtime should actually never time out - they have been confirmed, so they should conclude.

We don't have a timeout on the node side. So timing them out in the runtime is kind of pointless and out of sync with what the node would assume about the dispute.

We drop disputes after 6 sessions anyway.

We can not and should not slash anybody on a timed out dispute - the people who voted are the good guys (most likely).

runtime/parachains/src/paras_inherent/mod.rs

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

burdges · 2023-03-22T15:51:47Z

separate concluded disputes storage

We'd dump all pruned disputes here whenever this happens? Or just the ones where we lost money? (excepting some special config or whatever)

runtime/parachains/src/disputes.rs

sandreim · 2023-03-22T17:58:34Z

separate concluded disputes storage

We'd dump all pruned disputes here whenever this happens? Or just the ones where we lost money? (excepting some special config or whatever)

Most of the disputes are concluded and there is no point in iterating them for everyblock in each session until they are pruned. We only have a limited number of active disputes, because they conclude fast. Iterating them should be fast.

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

ordian

Need some other changes to the guide, at least: https://paritytech.github.io/polkadot/book/runtime/disputes.html

Other than that, looks good.

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

runtime/parachains/src/configuration/migration.rs

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

…/fix_disputes_initializer

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

ggwpez · 2023-03-28T15:19:54Z

runtime/parachains/src/configuration/migration.rs

@@ -148,15 +148,15 @@ pub mod v4 {
 		}

 		fn on_runtime_upgrade() -> Weight {
-			if StorageVersion::get::<Pallet<T>>() == 3 {


In line 146 above it is checking for version 3.
No idea why the CI is not catching that, with this recent change the pre- and post checks should run. cc @kianenigma

oops, looks like I missed changing this and indeed CI did not fail ....

* master: bump timestamp script to v0.2 (#6954) Subsystem channel tweaks (#6905) Companion for #13683 (#6944) inherent disputes: remove per block initializer and disputes timeout event (#6937)

* master: Companion: wasm-builder support stable Rust (#6967) Fix feature (#6966) configuration: backport async backing parameters from the feature branch (#6961) Histogram support in runtime metrics (#6935) Bump openssl from 0.10.38 to 0.10.48 (#6955) Proxy for Nomination Pools (#6846) Nomination Pools migration v5: RewardPool fix (#6957) bump timestamp script to v0.2 (#6954) Subsystem channel tweaks (#6905) Companion for #13683 (#6944) inherent disputes: remove per block initializer and disputes timeout event (#6937)

Polkadot-Forum · 2023-04-07T14:33:01Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-dispute-storm-the-postmortem/2550/1

# Goal The goal of this PR is upgrade Polkadot to v0.9.42 Polkadot Release Notes: https://github.com/paritytech/polkadot/releases/tag/v0.9.42 https://github.com/paritytech/polkadot/releases/tag/v0.9.41 https://github.com/paritytech/polkadot/releases/tag/v0.9.40 Polkadot Release Analysis: https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-41-v0-9-42/2828/1 Closes #1472 Closes #1270 Closes #1332 # Discussion - v0.9.40 was not working and there was evidence that changes in v0.9.42 were related, so we decided to jump ahead to v0.9.42  # Runtime Migrations Included - [x] paritytech/polkadot#6937 - [x] paritytech/substrate#13715 - [x] paritytech/substrate#13936 - [x] paritytech/polkadot#7114 - [x] Further all migrations from 9.38+ are included: paritytech/polkadot#7162) # Checklist - [x] Chain spec updated - [ ] Custom RPC OR Runtime API added/changed? Updated js/api-augment. - [ ] Design doc(s) updated - [ ] Tests added - [ ] Benchmarks added - [x] Weights updated # Tests Performed - [x] `make ci-local` -- Passing (includes lint, docs, unit-test and integration-test) - [x] `make start-native` -- Successfully attached debugger when creating MSA with Polkadot UI - [x] `make start-relay` -- Docker Containers successfully started, but too slow in emulation on Apple Silicon M2. - [x] `make upgrade-local` -- Successfully started local relay network and upgraded to a newer test runtime. - [x] `make upgrade-local` -- Successfully updated a node running the current version on branch `main` --------- Co-authored-by: Frequency CI [bot] <do-not-reply@users.noreply.github.com> Co-authored-by: Matthew Orris <--help> Co-authored-by: Robert La Ferla <robert@onemsg.co>

sandreim added 3 commits March 22, 2023 13:01

Limit disputes weight and remove initializer code

4e0f65b

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

const

de17b48

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

remove timeout test

882e7d3

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim mentioned this pull request Mar 22, 2023

Polkadot Incident 11.03.2023 #6862

Closed

ordian reviewed Mar 22, 2023

View reviewed changes

runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 22, 2023

View reviewed changes

runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 22, 2023

View reviewed changes

eskimor reviewed Mar 22, 2023

View reviewed changes

tdimitrov reviewed Mar 22, 2023

View reviewed changes

runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

review feedback #1

a5ba764

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

ordian reviewed Mar 22, 2023

View reviewed changes

runtime/parachains/src/disputes.rs Show resolved Hide resolved

Remove the new weight limiting for disputes

91efd2b

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim changed the title ~~inherent disputes: remove per block initializer and reduce total allowed dispute weight~~ inherent disputes: remove per block initializer Mar 23, 2023

sandreim added 3 commits March 23, 2023 11:16

cargo lock

4790096

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Remove dispute_conclusion_by_time_out_period

96a5804

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Enable migrations

4aa0170

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

paritytech-ci requested review from a team March 23, 2023 11:30

sandreim added the E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. label Mar 23, 2023

sandreim changed the title ~~inherent disputes: remove per block initializer~~ inherent disputes: remove per block initializer and disputes timeout event Mar 23, 2023

ordian approved these changes Mar 23, 2023

View reviewed changes

Update guide

0c52e6b

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

mrcnski approved these changes Mar 23, 2023

View reviewed changes

runtime/parachains/src/configuration/migration.rs Outdated Show resolved Hide resolved

eskimor approved these changes Mar 23, 2023

View reviewed changes

sandreim added 2 commits March 23, 2023 17:23

Fix comment

0b98d33

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

More guide fixes

2cc2574

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

tdimitrov approved these changes Mar 23, 2023

View reviewed changes

sandreim added 2 commits March 24, 2023 06:56

Merge branch 'master' of github.com:paritytech/polkadot into sandreim…

93d9b3a

…/fix_disputes_initializer

Also migrate pending configs

3abaf1f

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim added B1-note_worthy Changes should be noted in the release notes and removed B0-silent Changes should not be mentioned in any release notes labels Mar 24, 2023

fix build

eb00f89

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim added the T1-runtime This PR/Issue is related to the topic “runtime”. label Mar 24, 2023

fix test

589733c

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim enabled auto-merge (squash) March 24, 2023 11:41

sandreim merged commit 2375a62 into master Mar 24, 2023

sandreim deleted the sandreim/fix_disputes_initializer branch March 24, 2023 11:48

slumber mentioned this pull request Mar 27, 2023

configuration: backport async backing parameters from the feature branch #6961

Merged

ggwpez reviewed Mar 28, 2023

View reviewed changes

mattheworris mentioned this pull request Jun 12, 2023

Upgrade to Polkadot 0.9.42 frequency-chain/frequency#1590

Merged

16 tasks

github-actions bot mentioned this pull request Jun 15, 2023

Update substrate/polkadot/cumulus from v0.9.40 to v0.9.43 moonbeam-foundation/moonbeam#2354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inherent disputes: remove per block initializer and disputes timeout event #6937

inherent disputes: remove per block initializer and disputes timeout event #6937

sandreim commented Mar 22, 2023 •

edited

Loading

ordian Mar 22, 2023

eskimor Mar 22, 2023

ordian Mar 22, 2023

eskimor Mar 24, 2023

eskimor Mar 22, 2023

burdges commented Mar 22, 2023

sandreim commented Mar 22, 2023

ordian left a comment

ggwpez Mar 28, 2023

sandreim Mar 28, 2023 •

edited

Loading

Polkadot-Forum commented Apr 7, 2023

inherent disputes: remove per block initializer and disputes timeout event #6937

inherent disputes: remove per block initializer and disputes timeout event #6937

Conversation

sandreim commented Mar 22, 2023 • edited Loading

ordian Mar 22, 2023

Choose a reason for hiding this comment

eskimor Mar 22, 2023

Choose a reason for hiding this comment

ordian Mar 22, 2023

Choose a reason for hiding this comment

eskimor Mar 24, 2023

Choose a reason for hiding this comment

eskimor Mar 22, 2023

Choose a reason for hiding this comment

burdges commented Mar 22, 2023

sandreim commented Mar 22, 2023

ordian left a comment

Choose a reason for hiding this comment

ggwpez Mar 28, 2023

Choose a reason for hiding this comment

sandreim Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Polkadot-Forum commented Apr 7, 2023

sandreim commented Mar 22, 2023 •

edited

Loading

sandreim Mar 28, 2023 •

edited

Loading