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

Dispute vote filtering for block authors #3498

Merged
merged 10 commits into from
Jul 21, 2021
Merged

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Jul 19, 2021

Closes #3472

When the node-side is authoring a block, it passes all votes from all disputes from recent sessions to an instance of the runtime as InherentData. The ProvideInherent implementation for the ParasInherent here invokes the filtering function, which is intended to detect duplicates and spam and so on.

This pull request actually provides the implementation of that filtering logic, to be used to create ParasInherent inherents that actually contain dispute votes so the chain can witness disputes.

@rphmeier rphmeier 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. labels Jul 19, 2021
Co-authored-by: Andronik Ordian <write@reusable.software>
Comment on lines +533 to +540
indices.sort();
indices.dedup();

// reverse order ensures correctness
for index in indices.into_iter().rev() {
// swap_remove guarantees linear complexity.
statement_set.statements.swap_remove(index);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Very elegant 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

credit to @ordian

Comment on lines 764 to 767
let valid = match statement {
DisputeStatement::Valid(_) => true,
DisputeStatement::Invalid(_) => false,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: adding a fn is_...(&self) -> bool would reduce the clutter a bit.

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

very much appreciate the effort put into documenting the rationale of innocent looking code 👍

@rphmeier
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jul 21, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Jul 21, 2021

Merge aborted: Checks failed for a4762ea

@ordian ordian added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 21, 2021
@ordian
Copy link
Member

ordian commented Jul 21, 2021

(feel free to edit the audit label)

bot merge

@ordian ordian merged commit e9fee36 into master Jul 21, 2021
@ordian ordian deleted the rh-filter-runtime-disputes branch July 21, 2021 19:48
ordian added a commit that referenced this pull request Jul 23, 2021
* master:
  Reduce staking miner reward (companion `substrate/pull/9395`) (#3465)
  Parachains shared.rs to Frame V2 (#3425)
  Parachains hrmp.rs to Frame V2 (#3475)
  Migrate slots pallet to pallet attribute macro. (#3218)
  Improve test in bridge (#3507)
  parachain dmp.rs to Frame V2 (#3426)
  Parachains inclusion.rs to Frame V2 (#3440)
  Dispute coordinator - Recover disputes on startup (#3481)
  Use correct syntax for owning all files in a folder (#3510)
  Add wococo-local chain spec (#3509)
  Dispute vote filtering for block authors (#3498)
  Bump indexmap from 1.6.1 to 1.7.0 (#3497)
  Companion for substrate #9315 (#3477)
@jakoblell jakoblell added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Jan 19, 2022
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. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disputes Runtime: Filter redundant and ancient votes from inherent MultiDisputeStatementSet
5 participants