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

disputes pallet: Filter unconfirmed disputes#6330

Closed
tdimitrov wants to merge 3 commits intomasterfrom
tsv-disputes-runtime
Closed

disputes pallet: Filter unconfirmed disputes#6330
tdimitrov wants to merge 3 commits intomasterfrom
tsv-disputes-runtime

Conversation

@tdimitrov
Copy link
Copy Markdown
Contributor

@tdimitrov tdimitrov commented Nov 22, 2022

On dispute import pallet disputes - don't include any disputes with less than required votes for dispute confirmation.

Part of #6331

@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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes. labels Nov 22, 2022
@tdimitrov
Copy link
Copy Markdown
Contributor Author

A tiny change - I want it merged before I start removing spam slots.


// Reject disputes containing less votes than needed for confirmation.
if summary.state.validators_for.count_ones() + summary.state.validators_against.count_ones() <
supermajority_threshold(summary.state.validators_for.len())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How does it handle duplicate votes? Shouldn't the definition of a confirmed dispute be count_ones of a union of bitvecs, not the sum of count_ones?

Copy link
Copy Markdown
Contributor Author

@tdimitrov tdimitrov Nov 22, 2022

Choose a reason for hiding this comment

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

Yes, I haven't thought about double voting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also why does it use supermajority threshold?

Copy link
Copy Markdown
Contributor Author

@tdimitrov tdimitrov Nov 22, 2022

Choose a reason for hiding this comment

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

Two bugs in one line - this is a good PR! :)

Should be byzantine_threshold(). +1 if used on the line above.

@ordian
Copy link
Copy Markdown

ordian commented Nov 22, 2022

Would be nice to add some tests.

@tdimitrov
Copy link
Copy Markdown
Contributor Author

Would be nice to add some tests.

This change breaks a lot of spam slots related tests. I think fixing them is worhless considering that we want to remove them.

I'll stack the PR for spam slot removal on top of this one and I'll write/modify tests for both of them in the second one.

@tdimitrov tdimitrov marked this pull request as draft November 23, 2022 12:21
@eskimor eskimor changed the title disputes pallet: Filter disputes with votes less than supermajority threshold disputes pallet: Filter unconfirmed disputes Nov 23, 2022
@tdimitrov tdimitrov force-pushed the tsv-disputes-runtime branch from 81b54a3 to ac5b9f7 Compare December 1, 2022 14:31
@paritytech-cicd-pr
Copy link
Copy Markdown

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2106858

@tdimitrov
Copy link
Copy Markdown
Contributor Author

The changes in this PR are 'transferred' to #6345 to avoid merge/rebase hell.

@tdimitrov tdimitrov closed this Dec 1, 2022
@tdimitrov tdimitrov deleted the tsv-disputes-runtime branch December 1, 2022 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants