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

Revert chain if at least f+1 validators voted against a candidate #7151

Merged
merged 7 commits into from
May 17, 2023
Merged

Revert chain if at least f+1 validators voted against a candidate #7151

merged 7 commits into from
May 17, 2023

Conversation

tdimitrov
Copy link
Contributor

@tdimitrov tdimitrov commented Apr 29, 2023 •
edited
Loading

Once we see byzantine threshold + 1 invalid votes for a dispute we can revert the chain to avoid building new blocks on top of it. Check #6950 for details

Addresses #6950

@tdimitrov tdimitrov added A0-please_review Pull request needs code review. 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. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes. labels Apr 29, 2023
@tdimitrov tdimitrov force-pushed the tsv-disp-coord-byzantine-revert branch from 8490501 to 344bbcb Compare April 29, 2023 17:56
@mrcnski
Copy link
Contributor

mrcnski commented Apr 30, 2023

I'm not too familiar with the code, but can you please explain why https://github.com/paritytech/polkadot/blob/32b9138042/runtime/parachains/src/disputes.rs#L573-L583 and https://github.com/paritytech/polkadot/blob/32b9138042/node/core/dispute-coordinator/src/import.rs#L198-L208 don't need to change? Other than that, I think it looks good.

@tdimitrov
Copy link
Contributor Author

This is a very good question. The way I understand the issue is that we don't want to change the disputes protocol altogether. We are just doing an implementation optimization.

From the implementor's gude:

Disputes conclude after ⅔ supermajority is reached in either direction.

For this reason we don't touch FOR_SUPERMAJORITY and AGAINST_SUPERMAJORITY. We still want the dispute to conclude after 2/3rds of the votes in one direction, not just f+1.

About CONFIRMED - a confirmed dispute is one dispute for which we have got f+1 votes. This means that at least one honest node sees this dispute as legit and it's not completely made up. Afaik this is an 'implementation' concept - we use it for spam protection. Note that it is just f+1 votes - they can't be f+1 invalid votes or f+1 valid votes. (Actually in this case there won't be a dispute because of the two opposing votes requirement).

So based on the above I think we shouldn't change the way disputes are handled altogether and hence - we shouldn't change the code you have mentioned.

But, if we have got f+1 INVALID votes for a dispute we can be pretty confident that this dispute will conclude as INVALID with very high probability. So at this point we better revert this for so that no additional candidates are built on top of it. This will be a wasted effort.

eskimor
eskimor reviewed May 3, 2023
View reviewed changes
Copy link
Member

@eskimor eskimor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I actually envisioned this to happen in the runtime - f+f invalid votes -> revert log -> done. Anyhow, node side is probably even better. As long as we do it both ways, it would probably make sense to keep them in sync - the very least to avoid confusion.

node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Outdated
Self::get_own_invalid_votes(self.old_state());

old_invalid_votes as usize <= byzantine_threshold &&
imported_invalid_votes as usize > byzantine_threshold
Copy link
Member

Choose a reason for hiding this comment

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

Why would we only check for newly imported invalid votes?

Copy link
Contributor Author

@tdimitrov tdimitrov May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I want has_fresh_byzantine_threshold_against() to return true only once - when byzantine_threshold is reached. So I check if the invalid votes before import were below (or equal) the byzantine_threshold and at the same time they are above it after the import.
But you probably are asking something else?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I understand that you want to trigger the message once the threshold is passed and not on every import, but you are comparing newly imported votes to the byzantine threshold - so any previously known votes are not taken into account. This would only trigger if we imported f+1 invalid votes all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, self.imported_invalid_votes() returns the votes from the last import, not all votes. This is totally messed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now, but I have to add more tests. The current ones didn't catch that error.

@ordian
Copy link
Member

ordian commented May 5, 2023

As long as we do it both ways, it would probably make sense to keep them in sync - the very least to avoid confusion.

Agree, we should also have this logic in the runtime.

@tdimitrov tdimitrov mentioned this pull request May 15, 2023
Merged
@tdimitrov
Copy link
Contributor Author

As long as we do it both ways, it would probably make sense to keep them in sync - the very least to avoid confusion.

Agree, we should also have this logic in the runtime.

Done: #7225

@tdimitrov
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 2b728eb into master May 17, 2023
3 checks passed
@paritytech-processbot paritytech-processbot bot deleted the tsv-disp-coord-byzantine-revert branch May 17, 2023 18:29
ordian added a commit that referenced this pull request May 23, 2023
@ordian
Merge branch 'master' into ao-past-session-slashing-runtime
f45594d 
* master: (60 commits)
  Ensure all `StorageVersion`s on Rococo/Westend are correct and migration hooks pass (#7251)
  Try-runtime proper return types (#7146)
  Have OCW mined election once a week on Westend (#7248)
  Bump enumn from 0.1.5 to 0.1.8 (#7226)
  Companion to #14183: FRAME: Allow message ID to be mutated in `ProcessMessage` (#7262)
  Remove TODO comment (#7260)
  Fix build (#7261)
  Update syn (#7258)
  Use Message Queue pallet for UMP dispatch (#6271)
  Freeze chain if there are byzantine threshold + 1 invalid votes against a local candidate (#7225)
  Revert chain if at least f+1 validators voted against a candidate (#7151)
  Ensure all `StorageVersion`s on Polkadot/Kusama are correct (#7199)
  Forgotten pub reexport for `GlobalConsensusParachainConvertsFor` (#7238)
  PVF: Vote invalid on panics in execution thread (after a retry) (#7155)
  PVF: Remove `rayon` and some uses of `tokio` (#7153)
  [xcm] Foreign global consensus parachain LocationToAccountId converter (#7016)
  Update docs (#7230)
  Bump parity-db to 0.4.8 (#7231)
  Merge branch 'master' of https://github.com/paritytech/polkadot (#7224)
  Relax the watermark rule in the runtime (#7188)
  ...
ordian added a commit that referenced this pull request May 23, 2023
@ordian
Merge branch 'ao-past-session-slashing-runtime' into ao-past-session-…
4bd06fb 
…slashing-client

* ao-past-session-slashing-runtime: (61 commits)
  Ensure all `StorageVersion`s on Rococo/Westend are correct and migration hooks pass (#7251)
  Try-runtime proper return types (#7146)
  Have OCW mined election once a week on Westend (#7248)
  Bump enumn from 0.1.5 to 0.1.8 (#7226)
  Companion to #14183: FRAME: Allow message ID to be mutated in `ProcessMessage` (#7262)
  Remove TODO comment (#7260)
  Fix build (#7261)
  Update syn (#7258)
  Use Message Queue pallet for UMP dispatch (#6271)
  Freeze chain if there are byzantine threshold + 1 invalid votes against a local candidate (#7225)
  Revert chain if at least f+1 validators voted against a candidate (#7151)
  Ensure all `StorageVersion`s on Polkadot/Kusama are correct (#7199)
  Forgotten pub reexport for `GlobalConsensusParachainConvertsFor` (#7238)
  PVF: Vote invalid on panics in execution thread (after a retry) (#7155)
  PVF: Remove `rayon` and some uses of `tokio` (#7153)
  [xcm] Foreign global consensus parachain LocationToAccountId converter (#7016)
  Update docs (#7230)
  Bump parity-db to 0.4.8 (#7231)
  Merge branch 'master' of https://github.com/paritytech/polkadot (#7224)
  Relax the watermark rule in the runtime (#7188)
  ...
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@kiltbot kiltbot mentioned this pull request Jun 21, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eskimor eskimor eskimor approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. 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. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tdimitrov @mrcnski @ordian @eskimor