Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

statement-distribution: prep for re-enabling #4431

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

ordian
Copy link
Member

@ordian ordian commented May 10, 2024

In preparation for launching re-enabling (#2418), we need to adjust the disabling strategy of statement-distribution to use the relay parent's state instead of the latest state (union of active leaves). This will also ensure no raciness of getting the latest state vs accepting statements from disabling validators at the cost of being more lenient/potentially accepting more statements from disabled validators.

  • PRDoc

@ordian ordian added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label May 10, 2024
* master:
  improve MockValidationDataInherentDataProvider to support async backing (#4442)
  Bump `proc-macro-crate` to the latest version (#4409)
  [ci] Run check-runtime-migration in GHA (#4441)
  prospective-parachains rework (#4035)
  [ci] Add forklift to GHA ARC (#4372)
  `CheckWeight` SE: Check for extrinsic length + proof size combined (#4326)
  Add generate and verify logic for `AncestryProof` (#4430)
  Rococo AH: undeploy trie migration (#4414)
  Remove `substrate-frame-cli` (#4403)
  migrations: `take()`should consume read and write operation weight (#4302)
  `remote-externalities`: store block header in snapshot (#4349)
  xcm-emlator: Use `BlockNumberFor` instead of `parachains_common::BlockNumber=u32` (#4434)
  Remove `pallet::getter` usage from authority-discovery pallet (#4091)
  Remove pallet::getter usage from pallet-contracts-mock-network (#4417)
  Add docs to request_core_count (#4423)
Copy link
Contributor

@Overkillus Overkillus left a comment

Choose a reason for hiding this comment

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

Thanks for prepping it 👍 Makes everything easier to reason about, and once we discuss the fragmentation vector LGTM

validator_index = ?statement.unchecked_validator_index(),
"Ignoring a statement from disabled validator."
);
modify_reputation(reputation, ctx.sender(), peer, COST_DISABLED_VALIDATOR).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

In general we want it for sure. I if it poses network fragmentation risks on different node version interacting.

Thinking in context of only a fraction of the network updating. Those are node changes mostly so very likely it will go like that.

Ths change means that new nodes will be generally considering themselves disabled later - only when it gets to the relay parent rather then leaf. So consider validator A that was disabled in some leaf. Validator A updated to new node with relay parent disabling. He got disabled in one of the leafs already but A does not yet see it as it hasn't yet been in the relay parent. That means he might be sending statements and old nodes that consider him disabled will rep punish him. Potential fragmentation risk?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might not be enough rep damages to be a problem but just highlighting the possibility

polkadot/node/network/statement-distribution/src/v2/mod.rs Outdated Show resolved Hide resolved
ordian and others added 3 commits May 30, 2024 18:03
* master: (93 commits)
  Fix broken windows build (#4636)
  Beefy client generic on aduthority Id (#1816)
  pallet-staking: Put tests behind `cfg(debug_assertions)` (#4620)
  Broker new price adapter (#4521)
  Change `XcmDryRunApi::dry_run_extrinsic` to take a call instead (#4621)
  Update README.md (#4623)
  Publish `chain-spec-builder` (#4518)
  Add omni bencher & chain-spec-builder bins to release (#4557)
  Moves runtime macro out of experimental flag (#4249)
  Filter workspace dependencies in the templates (#4599)
  parachain-inherent: Make `para_id` more prominent (#4555)
  Add metric to measure the time it takes to gather enough assignments (#4587)
  Improve On_demand_assigner events (#4339)
  Conditional `required` checks (#4544)
  [CI] Deny adding git deps (#4572)
  [subsytem-bench] Remove redundant banchmark_name param (#4540)
  Add availability-recovery from systematic chunks (#1644)
  Remove workspace lints from templates (#4598)
  `sc-chain-spec`: deprecated code removed (#4410)
  [subsystem-benchmarks] Add statement-distribution benchmarks (#3863)
  ...
@ordian ordian enabled auto-merge June 5, 2024 13:24
@ordian ordian added this pull request to the merge queue Jun 5, 2024
Merged via the queue into master with commit 0d661ea Jun 5, 2024
156 of 157 checks passed
@ordian ordian deleted the ao-statement-distribution-reenabling-prep branch June 5, 2024 14:28
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
In preparation for launching re-enabling
(paritytech#2418), we need to
adjust the disabling strategy of statement-distribution to use the relay
parent's state instead of the latest state (union of active leaves).
This will also ensure no raciness of getting the latest state vs
accepting statements from disabling validators at the cost of being more
lenient/potentially accepting more statements from disabled validators.

- [x] PRDoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statement-distribution uses relay-parent state instead of union of leaves for purposes of disabled_validators
3 participants