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

dispute-coordinator: past session dispute slashing #6811

Merged
merged 76 commits into from
Jun 5, 2023

Conversation

ordian
Copy link
Member

@ordian ordian commented Mar 2, 2023

Closes #5947.

On top of #6667.

TODO:

  • Prettify
  • Fix tests
  • Zombienet test
  • Investigate WARN tokio-runtime-worker txpool::api: (offchain call) Error submitting a transaction to the pool: Transaction pool error: Invalid transaction validity: InvalidTransaction::ExhaustsResources
  • Investigate panic in debug assertion
  • Fix zombienet test (blocked on zndsl: support subtracting metrics zombienet#1044)

@ordian ordian added 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. T5-parachains_protocol This PR/Issue is related to Parachains features and protocol changes. T0-node This PR/Issue is related to the topic “node”. A3-inprogress labels Mar 2, 2023
@ordian ordian force-pushed the ao-past-session-slashing-client branch from 7689006 to 5223ef3 Compare March 2, 2023 17:41
ordian added 10 commits March 7, 2023 12:51
* master: (27 commits)
  bump `zombienet` version to v1.3.37 (#6773)
  Bump `blake2b_simd` to 1.0.1 (#6829)
  changelog: update template for new label behavior (E3/E4) (#6804)
  Companion for paritytech/substrate#12828 (#6380)
  Don't send `ActiveLeaves` from leaves in db on startup in Overseer (#6727)
  Polkadot XCM Body constants (#6788)
  Decrease expected peer count in zombinenet tests (#6826)
  Additional tracing in `provisioner`, `vote_selection` and `dispute-coordinator` (#6775)
  Change node-key for bootnodes (#6772)
  Change handle_import_statements to FatalResult (#6820)
  Introduce XCM matcher for writing barriers (#6756)
  Freeze note on `SessionInfo`. (#6818)
  Bump parity-db (#6816)
  Removing Outdated References to Misbehavior Arbitration Subsystem (#6814)
  Forgotten re-export for `MatchedConvertedConcreteId` (#6815)
  Companion for substrate#13509: bump API versions of {Beefy,Mmr}Api (#6809)
  Migrate to `Weight::from_parts` (#6794)
  [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error (#6739)
  Get rid of unnecessary cloning and work. (#6808)
  changelog: fix migration listing (#6806)
  ...
* master: (27 commits)
  bump `zombienet` version to v1.3.37 (#6773)
  Bump `blake2b_simd` to 1.0.1 (#6829)
  changelog: update template for new label behavior (E3/E4) (#6804)
  Companion for paritytech/substrate#12828 (#6380)
  Don't send `ActiveLeaves` from leaves in db on startup in Overseer (#6727)
  Polkadot XCM Body constants (#6788)
  Decrease expected peer count in zombinenet tests (#6826)
  Additional tracing in `provisioner`, `vote_selection` and `dispute-coordinator` (#6775)
  Change node-key for bootnodes (#6772)
  Change handle_import_statements to FatalResult (#6820)
  Introduce XCM matcher for writing barriers (#6756)
  Freeze note on `SessionInfo`. (#6818)
  Bump parity-db (#6816)
  Removing Outdated References to Misbehavior Arbitration Subsystem (#6814)
  Forgotten re-export for `MatchedConvertedConcreteId` (#6815)
  Companion for substrate#13509: bump API versions of {Beefy,Mmr}Api (#6809)
  Migrate to `Weight::from_parts` (#6794)
  [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error (#6739)
  Get rid of unnecessary cloning and work. (#6808)
  changelog: fix migration listing (#6806)
  ...
…slashing-client

* ao-past-session-slashing-runtime:
Copy link
Contributor

@tdimitrov tdimitrov left a comment

Choose a reason for hiding this comment

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

Great work @ordian 💯

* master:
  PVF: instantiate runtime from bytes (#7270)
* master:
  [companion] Fix request-response protocols backpressure mechanism (#7276)
* master:
  XCM: Tools for uniquely referencing messages (#7234)
  Companion: Substrate#13869 (#7119)
  Companion for Substrate#14214 (#7283)
  Fix flaky test and error reporting (#7282)
  impl guide: Update Collator Generation (#7250)
  Add staking-miner bin (#7273)
  metrics: tests: Fix flaky runtime_can_publish_metrics (#7279)
…slashing-client

* ao-past-session-slashing-runtime:
  XCM: Tools for uniquely referencing messages (#7234)
  Companion: Substrate#13869 (#7119)
  Companion for Substrate#14214 (#7283)
  Fix flaky test and error reporting (#7282)
  impl guide: Update Collator Generation (#7250)
  Add staking-miner bin (#7273)
  metrics: tests: Fix flaky runtime_can_publish_metrics (#7279)
  [companion] Fix request-response protocols backpressure mechanism (#7276)
  PVF: instantiate runtime from bytes (#7270)
* master:
  bump zombienet version (#7292)
  xcm-builder: remove clone for clippy (#7291)
…slashing-client

* ao-past-session-slashing-runtime:
  bump zombienet version (#7292)
  xcm-builder: remove clone for clippy (#7291)
Base automatically changed from ao-past-session-slashing-runtime to master May 26, 2023 09:35
* master:
  runtime: past session slashing runtime API (#6667)
  cli: enable BEEFY by default on test networks (#7293)
  pallets: implement `Default` for `GenesisConfig` in `no_std` (#7271)
  Update quote to 1.0.27 (#7280)
  PVF: Refactor workers into separate crates, remove host dependency (#7253)
@ordian
Copy link
Member Author

ordian commented May 26, 2023

@paritytech/ci @paritytech/release-engineering @chevdor an approval please 🙏
@eskimor a review please 🙏 🙏 🙏

"Processing unapplied validator slashes",
);

let inclusions = self.scraper.get_blocks_including_candidate(&candidate_hash);
Copy link
Member

Choose a reason for hiding this comment

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

We only keep that information for the unfinalized chain (and a bit more). Is that enough for this purpose?

Copy link
Member Author

@ordian ordian May 27, 2023

Choose a reason for hiding this comment

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

Good point. For the scraped included blocks this is fine. If we finalized a different block at the same height, that probably means the dispute has concluded before, so we should have called this already on some block producing node and DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION helps with this race condition.

There's a potential problem with runtime API calls to key_ownership_proof though since this block will be pruned once we reorg to and finalize the undisputed chain. But again, there's a time window between dispute concluding and that happening, so we should be able to slash with some high probability in that case.

Copy link
Member

@eskimor eskimor May 30, 2023

Choose a reason for hiding this comment

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

I am uneasy about this. The raciness for participation already bothers me, but here it is even worse. I am not blocking this PR on this, as racy past session slashing is better than no past session slashing, but let's discuss this further.

@paritytech-ci paritytech-ci requested a review from a team May 31, 2023 18:33
@eskimor eskimor merged commit cab7ae5 into master Jun 5, 2023
@eskimor eskimor deleted the ao-past-session-slashing-client branch June 5, 2023 16:21
@eskimor
Copy link
Member

eskimor commented Jun 5, 2023

@the-right-joyce worth mentioning this does not yet enable past session slashes on Kusama or Polkadot, only Westend and Rococo.

@ordian
Copy link
Member Author

ordian commented Jun 6, 2023

Yes, enabling it is easy though: we need to implement v5 of ParachainHost and do the runtime upgrade:

#[api_version(5)]

impl primitives::runtime_api::ParachainHost<Block, Hash, BlockNumber> for Runtime {

@eskimor
Copy link
Member

eskimor commented Jun 7, 2023

Working on making this available on Polkadot and Kusama. Versioning was a bit messed up in places, fixing. @ordian was this tested with a runtime not yet supporting the calls? Seems we just debug log any error anyway.

@ordian
Copy link
Member Author

ordian commented Jun 7, 2023

Ouch, I think I tested it before #6885 was merged, but yeah, the only trouble is we will get a different type of error in debug logs. These requirements need to be bumped to 5 to fix that indeed.

@eskimor
Copy link
Member

eskimor commented Jun 7, 2023

Ouch, I think I tested it before #6885 was merged, but yeah, the only trouble is we will get a different type of error in debug logs. These requirements need to be bumped to 5 to fix that indeed.

Already on it.

@eskimor
Copy link
Member

eskimor commented Jun 7, 2023

#7341

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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. T0-node This PR/Issue is related to the topic “node”. 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.

disputes: implement past session slashing
7 participants