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

Ensure BeforeBestBlockBy voting rule accounts for base #9920

Merged
merged 3 commits into from
Oct 3, 2021

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Oct 2, 2021

The changeset here is just to modify BeforeBestBlockBy to check whether voting N blocks behind is even possible, and if not, it votes on the base.

This is fixing a bug that manifested on the Rococo network after activating Polkadot's custom fork-choice rule, which can prioritize shorter chains over longer ones.

A diagram for illustration:

B <- B1 <- B2 (at this block, nodes vote to finalize B, but the chain also signals an intent to revert and ignore B1)
  \- B1' (built after B2, but is the best chain since B1 was reverted and took B2 down with it)

In this scenario, finality for B is racing with the production of B1'. If B is finalized before B1' is produced, nodes with B1' as their best chain will attempt to vote before block B and this is ignored by the voting rule machinery and leads to voting to finalize B1'. Then, if B1' is finalized before a block B2' is built on top of it, the cycle repeats, and can repeat indefinitely. This undermines the intention of the voting rule, hence the fix.

@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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 2, 2021
* master: (67 commits)
  Downstream `node-template` pull (#9915)
  Implement core::fmt::Debug for BoundedVec (#9914)
  Quickly skip invalid transactions during block authorship. (#9789)
  Add SS58 prefix for Automata (#9805)
  Clean up sc-peerset (#9806)
  Test each benchmark case in own #[test] (#9860)
  Add build with docker section to README (#9792)
  Simple Trait to Inspect Metadata (#9893)
  Pallet Assets: Create new asset classes from genesis config (#9742)
  doc: subkey usage (#9905)
  Silence alert about large-statement-fetcher (#9882)
  Fix democracy on-initialize weight (#9890)
  Fix basic authorship flaky test (#9906)
  contracts: Add event field names (#9896)
  subkey readme update on install (#9900)
  add feature wasmtime-jitdump (#9871)
  Return `target_hash` for finality_target instead of an Option (#9867)
  Update wasmtime to 0.29.0 (#9552)
  Less sleeps (#9848)
  remove unidiomatic (#9895)
  ...
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Good catch and thanks for the fix!

@ordian ordian merged commit 9be8fdd into master Oct 3, 2021
@ordian ordian deleted the rh-voting-rule-tweak branch October 3, 2021 13:15
ordian added a commit that referenced this pull request Oct 5, 2021
* master: (125 commits)
  Update multiple dependencies (#9936)
  Speed up timestamp generation when logging (#9933)
  First word should be Substrate not Polkadot (#9935)
  Improved file not found error message (#9931)
  don't read events in elections anymore. (#9898)
  Remove incorrect sanity check (#9924)
  Require crypto scheme for `insert-key` (#9909)
  chore: refresh of the substrate_builder image (#9808)
  Introduce block authorship soft deadline (#9663)
  Rework Transaction Priority calculation (#9834)
  Do not propagate host RUSTFLAGS when checking for WASM toolchain (#9926)
  Small quoting comment fix (#9927)
  add clippy to CI (#9694)
  Ensure BeforeBestBlockBy voting rule accounts for base (#9920)
  rm `.maintain` lock (#9919)
  Downstream `node-template` pull (#9915)
  Implement core::fmt::Debug for BoundedVec (#9914)
  Quickly skip invalid transactions during block authorship. (#9789)
  Add SS58 prefix for Automata (#9805)
  Clean up sc-peerset (#9806)
  ...
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants