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

Better spam slots handling #4845

Merged
merged 15 commits into from
Feb 17, 2022
Merged

Better spam slots handling #4845

merged 15 commits into from
Feb 17, 2022

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Feb 3, 2022

and a few more tests.

We should actually only increment spam slots on voting invalid validators, as only those actually could raise a dispute. A valid vote is either backing or approval or an explicit Valid vote, but the later is only ever accepted if some other validator previously already voted invalid on that candidate - it is enough to count this vote as spam.

@eskimor eskimor 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 3, 2022
@eskimor eskimor added this to the v0.9.17 milestone Feb 3, 2022
rphmeier
rphmeier approved these changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

I didn't take a hard look at the tests but logic changes seem OK.

eskimor and others added 2 commits February 4, 2022 06:35
@eskimor @rphmeier
Typo
b4c5d89 
Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
@eskimor
Fix doc.
2d73fd1
@rphmeier
Copy link
Contributor

rphmeier commented Feb 6, 2022

Why do we consider 're-import' of votes at all? Re: #4854 we could just use the 'fresh' check and not double-count.

drahnr
drahnr approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@drahnr drahnr 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 verified two tests explicitly and the logic change is inline with the stated changeset.

@drahnr
Copy link
Contributor

drahnr commented Feb 7, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

ordian
ordian reviewed Feb 7, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@ordian
Copy link
Member

ordian commented Feb 7, 2022

bot merge cancel

@paritytech-processbot
Copy link

Merge cancelled.

@eskimor
Copy link
Member Author

eskimor commented Feb 14, 2022 •
edited
Loading

Why do we consider 're-import' of votes at all? Re: #4854 we could just use the 'fresh' check and not double-count.

We are not double counting, because we will only increment spam slots once per candidate & validator.

One side effect exists though. Assuming a validator A has exceeded its spam slots via other disputes, after it got his vote already successfully imported, then the import of a validator B will fail, if it references validator As vote as an opposing vote.

Consequences should be minor though:

  • Either validator A is really a spamming validator, in that case validator B will also just be a spamming validator (as honest validators can not participate), so we fail to import a vote from a spamming validator, which might lead to it getting punished less, but we will still punish validator A ... a lot, because it exceeded its spam slots.
  • Or validator A is honest, in that case disputes will eventually get confirmed, spam slots get cleared and the import of validator Bs vote will succeed eventually (it keeps re-sending on failure).

Either way, that should be improved as well (carefully) as changes to this code tend to open up security issues. I am about to create a separate issue for that.

@eskimor eskimor mentioned this pull request Feb 14, 2022
Closed
@eskimor eskimor merged commit 8f4ee9a into master Feb 17, 2022
@eskimor eskimor deleted the rk-fix-backing-vote-import branch February 17, 2022 14:18
ordian added a commit that referenced this pull request Feb 22, 2022
@ordian
Merge branch 'master' into ao-approval-distribution-fix
b611ab8 
* master: (27 commits)
  Bump `tokio` to 1.17.0 (#4965)
  bump transaction_version (#4956)
  Bump tracing-subscriber from 0.3.8 to 0.3.9 (#4954)
  staking miner: reuse ws conn for remote-ext (#4849)
  Revert "collator-protocol: short-term fixes for connectivity (#4640)" (#4914)
  Bump tracing from 0.1.30 to 0.1.31 (#4941)
  Better spam slots handling (#4845)
  Bump proc-macro-crate from 1.1.0 to 1.1.2 (#4936)
  corrected paras code validation event comments (#4932)
  Companion for refactor election score #10834 (#4927)
  Companion CI: Make sure to pass the update crates properly (#4928)
  update digest to v0.10.2 (#4907)
  Companion for #10832 (#4918)
  Companion for `Remove u32_trait` (#4920)
  Bump serde_json from 1.0.78 to 1.0.79 (#4916)
  Bump rand from 0.8.4 to 0.8.5 (#4917)
  Remove stale migrations post 9.16 release (#4848)
  Add proxy type for Kappa Sigma Mu (#4851)
  Baseline weights for `force_apply_min_commission` (#4896)
  Allow two Parachains to swap (#4772)
  ...
@coderobe coderobe mentioned this pull request Mar 17, 2022
Closed
13 tasks
@eskimor eskimor mentioned this pull request Nov 11, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drahnr drahnr drahnr approved these changes

@rphmeier rphmeier rphmeier approved these changes

@ordian ordian Awaiting requested review from ordian

@sandreim sandreim Awaiting requested review from sandreim

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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Milestone
v0.9.18
Development

Successfully merging this pull request may close these issues.
5 participants
@eskimor @rphmeier @drahnr @ordian @coderobe