Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dispute-coordinator: disabling in participation #2637

Merged
merged 9 commits into from
Jan 9, 2024

Conversation

ordian
Copy link
Member

@ordian ordian commented Dec 6, 2023

Closes #2225.

  • tests
  • fix todos
  • fix duplicates
  • make the check part of potential_spam
  • fix a bug with votes insertion
  • guide changes
  • docs

@ordian ordian linked an issue Dec 6, 2023 that may be closed by this pull request
@tdimitrov
Copy link
Contributor

Looks very good!

Copy link
Contributor

@Overkillus Overkillus left a comment

Choose a reason for hiding this comment

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

Looking overall but dropped some questions/nits

polkadot/node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
polkadot/node/core/dispute-coordinator/src/initialized.rs Outdated Show resolved Hide resolved
polkadot/node/core/dispute-coordinator/src/initialized.rs Outdated Show resolved Hide resolved
.map_err(Error::UtilError)?;
let disabled_validators =
get_disabled_validators_with_fallback(ctx.sender(), parent).await.map_err(|e| {
Error::UtilError(TryFrom::try_from(e).expect("the conversion is infallible; qed"))
Copy link
Member Author

Choose a reason for hiding this comment

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

the error conversion is a bit ugly, but I'm open for suggestions how to improve it

@ordian ordian marked this pull request as ready for review December 13, 2023 20:20
@ordian ordian changed the base branch from tsv-disabling to master December 13, 2023 20:31
@ordian ordian requested a review from athei as a code owner December 13, 2023 20:31
@ordian ordian requested review from a team December 13, 2023 20:31
@ordian ordian requested a review from a team as a code owner December 13, 2023 20:31
@ordian ordian changed the base branch from master to tsv-disabling December 13, 2023 20:31
@ordian ordian removed request for a team and athei December 13, 2023 20:31
@ordian ordian added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Dec 14, 2023
Copy link
Contributor

@Overkillus Overkillus left a comment

Choose a reason for hiding this comment

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

Looking good. Guide excerpt straight to the point so I'd keep it for now. I might alter it slightly later on once we have the overall guide out but for now it's 👍

polkadot/node/core/dispute-coordinator/src/tests.rs Outdated Show resolved Hide resolved
polkadot/node/core/dispute-coordinator/src/tests.rs Outdated Show resolved Hide resolved
@@ -2542,6 +2583,353 @@ fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation
});
}

#[test]
fn participation_with_onchain_disabling() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You have 2 scenarios. First of all I'd consider splitting scenarios into separate tests.

Scenario 1: unconfirmed dispute with onchain disabled validator against -> expected that we import but not participate

✔️

Scenario 2: confirmed dispute with disabled validator

This one might need to be split into 2 cases. I think it mixes up 2 different behaviours that we need to verify independently.

Firstly confirmation should work even if ALL the votes are coming from disabled guys simply as a defence in depth against something going really badly within the system. So instead of importing votes from normal validators push into the confirmation only with disabled and ensure it works correctly.

Secondly, even if the dispute is unconfirmed once we receive an invalid vote from a non-disabled validator we should participate.

Then we have all important cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Firstly confirmation should work even if ALL the votes are coming from disabled guys simply as a defence in depth against something going really badly within the system.

Does this mean that if we have got a dispute storm, as the last incident on Kusama, disabling won't save us?

Copy link
Contributor

@Overkillus Overkillus Dec 15, 2023

Choose a reason for hiding this comment

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

Re: confirmation trumping disablement I've opened a new issue because there is some extra analysis needed there.

Nevertheless.... generally what happened in the recent kusama parachain stall would not be fully handled by the disabling strategy. We can deal with some dispute storms but not all dispute storms are alike. The recent one was caused by the faulty pruning logic which was present in all honest nodes. All honest nodes participated in something they shouldn't have. No disabling strategy can save us from that point so the best we could have done is freeze parachains and fix the critical error before the security is fully compromised.

If every node malfunctions no disabling will ever save us. The goal of the disabling strat is reducing the damages from a few (up to 1/3) faulty nodes but do it in such a way that even if those punished are honest the system does not collapse or reduce it's security significantly.

Copy link
Contributor

Choose a reason for hiding this comment

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

All honest nodes participated in something they shouldn't have.

Valid point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Take a look at 49c066f

Copy link
Contributor

@tdimitrov tdimitrov left a comment

Choose a reason for hiding this comment

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

Good job @ordian

Handles validator disabling in the backing subsystem. The PR introduces
two changes:
1. Don't import statements from disabled validators.
2. Don't validate and second if the local validator is disabled.

The purpose of this change is first to ignore statements from disabled
validators on the node side. This is an optimisation as these statements
are supposed to be cleared in the runtime too (still not implemented at
the moment). Additionally if the local node is a validator which is
disabled - don't process any new candidates.

---------

Co-authored-by: ordian <noreply@reusable.software>
Co-authored-by: ordian <write@reusable.software>
@ordian ordian changed the base branch from tsv-disabling to ao-backport-backing-disabling December 20, 2023 15:29
prdoc/pr_2637.prdoc Outdated Show resolved Hide resolved
Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>
tdimitrov added a commit that referenced this pull request Jan 8, 2024
…tem (#1259) (#2764)

#1259 was merged into a feature branch, but we've decided to merge
node-side changes for disabling straight into master.
This is a dependency of #1841 and #2637.

---------

Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>
Base automatically changed from ao-backport-backing-disabling to master January 8, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disablement: Dispute participation disabling
4 participants