From 855d6da0cfa3f67b80208a3068e5630bce27ae63 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 14 May 2022 18:15:34 +0300 Subject: [PATCH 01/35] Add `DisputeState` to `DisputeCoordinatorMessage::RecentDisputes` The new signature of the message is: ``` RecentDisputes(oneshot::Sender>), ``` As part of the change also add `DispiteStatus` to `polkadot_node_primitives`. --- node/core/dispute-coordinator/src/db/v1.rs | 2 +- .../dispute-coordinator/src/initialized.rs | 10 +- node/core/dispute-coordinator/src/status.rs | 100 +--------------- node/primitives/src/disputes/mod.rs | 2 + node/primitives/src/disputes/status.rs | 112 ++++++++++++++++++ node/primitives/src/lib.rs | 4 +- node/subsystem-types/src/messages.rs | 6 +- 7 files changed, 128 insertions(+), 108 deletions(-) create mode 100644 node/primitives/src/disputes/status.rs diff --git a/node/core/dispute-coordinator/src/db/v1.rs b/node/core/dispute-coordinator/src/db/v1.rs index 4d33949db644..2c643d341de2 100644 --- a/node/core/dispute-coordinator/src/db/v1.rs +++ b/node/core/dispute-coordinator/src/db/v1.rs @@ -16,6 +16,7 @@ //! `V1` database for the dispute coordinator. +use polkadot_node_primitives::DisputeStatus; use polkadot_node_subsystem::{SubsystemError, SubsystemResult}; use polkadot_node_subsystem_util::database::{DBTransaction, Database}; use polkadot_primitives::v2::{ @@ -31,7 +32,6 @@ use crate::{ backend::{Backend, BackendWriteOp, OverlayedBackend}, error::{FatalError, FatalResult}, metrics::Metrics, - status::DisputeStatus, DISPUTE_WINDOW, LOG_TARGET, }; diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index e37459dc5142..c1b34ce134cc 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -26,8 +26,8 @@ use futures::{ use sc_keystore::LocalKeystore; use polkadot_node_primitives::{ - CandidateVotes, DisputeMessage, DisputeMessageCheckError, SignedDisputeStatement, - DISPUTE_WINDOW, + CandidateVotes, DisputeMessage, DisputeMessageCheckError, DisputeStatus, + SignedDisputeStatement, Timestamp, DISPUTE_WINDOW, }; use polkadot_node_subsystem::{ messages::{ @@ -49,7 +49,7 @@ use crate::{ error::{log_error, Error, FatalError, FatalResult, JfyiError, JfyiResult, Result}, import::{CandidateEnvironment, CandidateVoteState}, metrics::Metrics, - status::{get_active_with_status, Clock, DisputeStatus, Timestamp}, + status::{get_active_with_status, Clock}, DisputeCoordinatorSubsystem, LOG_TARGET, }; @@ -599,7 +599,9 @@ impl Initialized { }; gum::trace!(target: LOG_TARGET, "Loaded recent disputes from db"); - let _ = tx.send(recent_disputes.keys().cloned().collect()); + let _ = tx.send( + recent_disputes.into_iter().map(|(k, v)| (k.0, k.1, v)).collect::>(), + ); }, DisputeCoordinatorMessage::ActiveDisputes(tx) => { // Return error if session information is missing. diff --git a/node/core/dispute-coordinator/src/status.rs b/node/core/dispute-coordinator/src/status.rs index d2ad551bd9ad..8a0e2de524d1 100644 --- a/node/core/dispute-coordinator/src/status.rs +++ b/node/core/dispute-coordinator/src/status.rs @@ -14,10 +14,9 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::time::{SystemTime, UNIX_EPOCH}; - -use parity_scale_codec::{Decode, Encode}; +use polkadot_node_primitives::{DisputeStatus, Timestamp}; use polkadot_primitives::v2::{CandidateHash, SessionIndex}; +use std::time::{SystemTime, UNIX_EPOCH}; use crate::LOG_TARGET; @@ -27,101 +26,6 @@ use crate::LOG_TARGET; /// disputes. pub const ACTIVE_DURATION_SECS: Timestamp = 180; -/// Timestamp based on the 1 Jan 1970 UNIX base, which is persistent across node restarts and OS reboots. -pub type Timestamp = u64; - -/// The status of dispute. This is a state machine which can be altered by the -/// helper methods. -#[derive(Debug, Clone, Copy, Encode, Decode, PartialEq)] -pub enum DisputeStatus { - /// The dispute is active and unconcluded. - #[codec(index = 0)] - Active, - /// The dispute has been concluded in favor of the candidate - /// since the given timestamp. - #[codec(index = 1)] - ConcludedFor(Timestamp), - /// The dispute has been concluded against the candidate - /// since the given timestamp. - /// - /// This takes precedence over `ConcludedFor` in the case that - /// both are true, which is impossible unless a large amount of - /// validators are participating on both sides. - #[codec(index = 2)] - ConcludedAgainst(Timestamp), - /// Dispute has been confirmed (more than `byzantine_threshold` have already participated/ or - /// we have seen the candidate included already/participated successfully ourselves). - #[codec(index = 3)] - Confirmed, -} - -impl DisputeStatus { - /// Initialize the status to the active state. - pub fn active() -> DisputeStatus { - DisputeStatus::Active - } - - /// Move status to confirmed status, if not yet concluded/confirmed already. - pub fn confirm(self) -> DisputeStatus { - match self { - DisputeStatus::Active => DisputeStatus::Confirmed, - DisputeStatus::Confirmed => DisputeStatus::Confirmed, - DisputeStatus::ConcludedFor(_) | DisputeStatus::ConcludedAgainst(_) => self, - } - } - - /// Check whether the dispute is not a spam dispute. - pub fn is_confirmed_concluded(&self) -> bool { - match self { - &DisputeStatus::Confirmed | - &DisputeStatus::ConcludedFor(_) | - DisputeStatus::ConcludedAgainst(_) => true, - &DisputeStatus::Active => false, - } - } - - /// Transition the status to a new status after observing the dispute has concluded for the candidate. - /// This may be a no-op if the status was already concluded. - pub fn concluded_for(self, now: Timestamp) -> DisputeStatus { - match self { - DisputeStatus::Active | DisputeStatus::Confirmed => DisputeStatus::ConcludedFor(now), - DisputeStatus::ConcludedFor(at) => DisputeStatus::ConcludedFor(std::cmp::min(at, now)), - against => against, - } - } - - /// Transition the status to a new status after observing the dispute has concluded against the candidate. - /// This may be a no-op if the status was already concluded. - pub fn concluded_against(self, now: Timestamp) -> DisputeStatus { - match self { - DisputeStatus::Active | DisputeStatus::Confirmed => - DisputeStatus::ConcludedAgainst(now), - DisputeStatus::ConcludedFor(at) => - DisputeStatus::ConcludedAgainst(std::cmp::min(at, now)), - DisputeStatus::ConcludedAgainst(at) => - DisputeStatus::ConcludedAgainst(std::cmp::min(at, now)), - } - } - - /// Whether the disputed candidate is possibly invalid. - pub fn is_possibly_invalid(&self) -> bool { - match self { - DisputeStatus::Active | - DisputeStatus::Confirmed | - DisputeStatus::ConcludedAgainst(_) => true, - DisputeStatus::ConcludedFor(_) => false, - } - } - - /// Yields the timestamp this dispute concluded at, if any. - pub fn concluded_at(&self) -> Option { - match self { - DisputeStatus::Active | DisputeStatus::Confirmed => None, - DisputeStatus::ConcludedFor(at) | DisputeStatus::ConcludedAgainst(at) => Some(*at), - } - } -} - /// Get active disputes as iterator, preserving its `DisputeStatus`. pub fn get_active_with_status( recent_disputes: impl Iterator, diff --git a/node/primitives/src/disputes/mod.rs b/node/primitives/src/disputes/mod.rs index ec7bb6abc3b7..051ec0233258 100644 --- a/node/primitives/src/disputes/mod.rs +++ b/node/primitives/src/disputes/mod.rs @@ -30,6 +30,8 @@ use polkadot_primitives::v2::{ /// `DisputeMessage` and related types. mod message; pub use message::{DisputeMessage, Error as DisputeMessageCheckError, UncheckedDisputeMessage}; +mod status; +pub use status::{DisputeStatus, Timestamp}; /// A checked dispute statement from an associated validator. #[derive(Debug, Clone)] diff --git a/node/primitives/src/disputes/status.rs b/node/primitives/src/disputes/status.rs new file mode 100644 index 000000000000..14ebcf0855d9 --- /dev/null +++ b/node/primitives/src/disputes/status.rs @@ -0,0 +1,112 @@ +// Copyright 2017-2022 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +use parity_scale_codec::{Decode, Encode}; + +/// Timestamp based on the 1 Jan 1970 UNIX base, which is persistent across node restarts and OS reboots. +pub type Timestamp = u64; + +/// The status of dispute. This is a state machine which can be altered by the +/// helper methods. +#[derive(Debug, Clone, Copy, Encode, Decode, PartialEq)] +pub enum DisputeStatus { + /// The dispute is active and unconcluded. + #[codec(index = 0)] + Active, + /// The dispute has been concluded in favor of the candidate + /// since the given timestamp. + #[codec(index = 1)] + ConcludedFor(Timestamp), + /// The dispute has been concluded against the candidate + /// since the given timestamp. + /// + /// This takes precedence over `ConcludedFor` in the case that + /// both are true, which is impossible unless a large amount of + /// validators are participating on both sides. + #[codec(index = 2)] + ConcludedAgainst(Timestamp), + /// Dispute has been confirmed (more than `byzantine_threshold` have already participated/ or + /// we have seen the candidate included already/participated successfully ourselves). + #[codec(index = 3)] + Confirmed, +} + +impl DisputeStatus { + /// Initialize the status to the active state. + pub fn active() -> DisputeStatus { + DisputeStatus::Active + } + + /// Move status to confirmed status, if not yet concluded/confirmed already. + pub fn confirm(self) -> DisputeStatus { + match self { + DisputeStatus::Active => DisputeStatus::Confirmed, + DisputeStatus::Confirmed => DisputeStatus::Confirmed, + DisputeStatus::ConcludedFor(_) | DisputeStatus::ConcludedAgainst(_) => self, + } + } + + /// Check whether the dispute is not a spam dispute. + pub fn is_confirmed_concluded(&self) -> bool { + match self { + &DisputeStatus::Confirmed | + &DisputeStatus::ConcludedFor(_) | + DisputeStatus::ConcludedAgainst(_) => true, + &DisputeStatus::Active => false, + } + } + + /// Transition the status to a new status after observing the dispute has concluded for the candidate. + /// This may be a no-op if the status was already concluded. + pub fn concluded_for(self, now: Timestamp) -> DisputeStatus { + match self { + DisputeStatus::Active | DisputeStatus::Confirmed => DisputeStatus::ConcludedFor(now), + DisputeStatus::ConcludedFor(at) => DisputeStatus::ConcludedFor(std::cmp::min(at, now)), + against => against, + } + } + + /// Transition the status to a new status after observing the dispute has concluded against the candidate. + /// This may be a no-op if the status was already concluded. + pub fn concluded_against(self, now: Timestamp) -> DisputeStatus { + match self { + DisputeStatus::Active | DisputeStatus::Confirmed => + DisputeStatus::ConcludedAgainst(now), + DisputeStatus::ConcludedFor(at) => + DisputeStatus::ConcludedAgainst(std::cmp::min(at, now)), + DisputeStatus::ConcludedAgainst(at) => + DisputeStatus::ConcludedAgainst(std::cmp::min(at, now)), + } + } + + /// Whether the disputed candidate is possibly invalid. + pub fn is_possibly_invalid(&self) -> bool { + match self { + DisputeStatus::Active | + DisputeStatus::Confirmed | + DisputeStatus::ConcludedAgainst(_) => true, + DisputeStatus::ConcludedFor(_) => false, + } + } + + /// Yields the timestamp this dispute concluded at, if any. + pub fn concluded_at(&self) -> Option { + match self { + DisputeStatus::Active | DisputeStatus::Confirmed => None, + DisputeStatus::ConcludedFor(at) | DisputeStatus::ConcludedAgainst(at) => Some(*at), + } + } +} diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 882b75a0e81f..17bc5129a664 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -46,8 +46,8 @@ pub mod approval; /// Disputes related types. pub mod disputes; pub use disputes::{ - CandidateVotes, DisputeMessage, DisputeMessageCheckError, InvalidDisputeVote, - SignedDisputeStatement, UncheckedDisputeMessage, ValidDisputeVote, + CandidateVotes, DisputeMessage, DisputeMessageCheckError, DisputeStatus, InvalidDisputeVote, + SignedDisputeStatement, Timestamp, UncheckedDisputeMessage, ValidDisputeVote, }; // For a 16-ary Merkle Prefix Trie, we can expect at most 16 32-byte hashes per node diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 10a5201cc524..c896454594b9 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -35,8 +35,8 @@ use polkadot_node_network_protocol::{ use polkadot_node_primitives::{ approval::{BlockApprovalMeta, IndirectAssignmentCert, IndirectSignedApprovalVote}, AvailableData, BabeEpoch, BlockWeight, CandidateVotes, CollationGenerationConfig, - CollationSecondedSignal, DisputeMessage, ErasureChunk, PoV, SignedDisputeStatement, - SignedFullStatement, ValidationResult, + CollationSecondedSignal, DisputeMessage, DisputeStatus, ErasureChunk, PoV, + SignedDisputeStatement, SignedFullStatement, ValidationResult, }; use polkadot_primitives::v2::{ AuthorityDiscoveryId, BackedCandidate, BlockNumber, CandidateEvent, CandidateHash, @@ -271,7 +271,7 @@ pub enum DisputeCoordinatorMessage { /// Fetch a list of all recent disputes the co-ordinator is aware of. /// These are disputes which have occurred any time in recent sessions, /// and which may have already concluded. - RecentDisputes(oneshot::Sender>), + RecentDisputes(oneshot::Sender>), /// Fetch a list of all active disputes that the coordinator is aware of. /// These disputes are either not yet concluded or recently concluded. ActiveDisputes(oneshot::Sender>), From 396187e9ae95379c057d07d163a414ea105ff88a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 11 May 2022 14:39:53 +0300 Subject: [PATCH 02/35] Move dummy_signature() in primitives/test-helpers --- node/network/approval-distribution/Cargo.toml | 1 + node/network/approval-distribution/src/tests.rs | 5 +---- primitives/test-helpers/Cargo.toml | 1 + primitives/test-helpers/src/lib.rs | 4 ++++ 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/node/network/approval-distribution/Cargo.toml b/node/network/approval-distribution/Cargo.toml index 68fae906c21e..e5c2865e4afe 100644 --- a/node/network/approval-distribution/Cargo.toml +++ b/node/network/approval-distribution/Cargo.toml @@ -21,6 +21,7 @@ sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", polkadot-node-subsystem-util = { path = "../../subsystem-util" } polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" } +polkadot-primitives-test-helpers = { path = "../../../primitives/test-helpers" } assert_matches = "1.4.0" schnorrkel = { version = "0.9.1", default-features = false } diff --git a/node/network/approval-distribution/src/tests.rs b/node/network/approval-distribution/src/tests.rs index b3d44bfe8c1e..90827f7d9e44 100644 --- a/node/network/approval-distribution/src/tests.rs +++ b/node/network/approval-distribution/src/tests.rs @@ -23,6 +23,7 @@ use polkadot_node_primitives::approval::{ }; use polkadot_node_subsystem::messages::{network_bridge_event, AllMessages, ApprovalCheckError}; use polkadot_node_subsystem_test_helpers as test_helpers; +use polkadot_primitives_test_helpers::dummy_signature; use polkadot_node_subsystem_util::TimeoutExt as _; use polkadot_primitives::v2::{AuthorityDiscoveryId, BlakeTwo256, HashT}; use rand::SeedableRng; @@ -32,10 +33,6 @@ use std::time::Duration; type VirtualOverseer = test_helpers::TestSubsystemContextHandle; -fn dummy_signature() -> polkadot_primitives::v2::ValidatorSignature { - sp_core::crypto::UncheckedFrom::unchecked_from([1u8; 64]) -} - fn test_harness>( mut state: State, test_fn: impl FnOnce(VirtualOverseer) -> T, diff --git a/primitives/test-helpers/Cargo.toml b/primitives/test-helpers/Cargo.toml index dd5e2ded6ae0..944e32333fd9 100644 --- a/primitives/test-helpers/Cargo.toml +++ b/primitives/test-helpers/Cargo.toml @@ -8,5 +8,6 @@ edition = "2021" sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-application-crypto = { package = "sp-application-crypto", git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master" } +sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", features = ["std"] } polkadot-primitives = { path = "../" } rand = "0.8.5" diff --git a/primitives/test-helpers/src/lib.rs b/primitives/test-helpers/src/lib.rs index 02ba009b13cc..8873d69cdb2f 100644 --- a/primitives/test-helpers/src/lib.rs +++ b/primitives/test-helpers/src/lib.rs @@ -255,3 +255,7 @@ impl rand::RngCore for AlwaysZeroRng { Ok(()) } } + +pub fn dummy_signature() -> polkadot_primitives::v2::ValidatorSignature { + sp_core::crypto::UncheckedFrom::unchecked_from([1u8; 64]) +} From 23013f48b05441d9e4db60aefa6d1decb4880a36 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 25 May 2022 14:07:16 +0300 Subject: [PATCH 03/35] Enable staging runtime api on Rococo --- runtime/rococo/Cargo.toml | 2 +- runtime/rococo/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/rococo/Cargo.toml b/runtime/rococo/Cargo.toml index 0ca78c98d2af..cc42af8255db 100644 --- a/runtime/rococo/Cargo.toml +++ b/runtime/rococo/Cargo.toml @@ -64,7 +64,7 @@ rococo-runtime-constants = { package = "rococo-runtime-constants", path = "./con runtime-common = { package = "polkadot-runtime-common", path = "../common", default-features = false } primitives = { package = "polkadot-primitives", path = "../../primitives", default-features = false } polkadot-parachain = { path = "../../parachain", default-features = false } -runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parachains", default-features = false } +runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parachains", default-features = false, features = ["vstaging"] } xcm = { package = "xcm", path = "../../xcm", default-features = false } xcm-executor = { package = "xcm-executor", path = "../../xcm/xcm-executor", default-features = false } diff --git a/runtime/rococo/src/lib.rs b/runtime/rococo/src/lib.rs index 14f67b162774..a4cace51c42b 100644 --- a/runtime/rococo/src/lib.rs +++ b/runtime/rococo/src/lib.rs @@ -1100,7 +1100,7 @@ sp_api::impl_runtime_apis! { } fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { - unimplemented!() + runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::() } } From 140c1e838fe22f4e2148703fea115318f452a871 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 16 May 2022 09:56:44 +0300 Subject: [PATCH 04/35] Implementation * Move disputes to separate module * Vote prioritisation * Duplicates handling * Double vote handling * Unit tests * Logs and metrics * Code review feedback * Fix ACTIVE/INACTIVE separation and update partition names * Add `fn dispute_is_inactive` to node primitives and refactor `fn get_active_with_status()` logic * Keep the 'old' logic if the staging api is not enabled * Fix some comments in tests * Add warning message if there are any inactive_unknown_onchain disputes * Add file headers and remove `use super::*;` usage outside tests * Adding doc comments * Fix test methods names --- Cargo.lock | 2 + node/core/dispute-coordinator/src/status.rs | 15 +- node/core/dispute-coordinator/src/tests.rs | 3 +- node/core/provisioner/Cargo.toml | 2 +- node/core/provisioner/src/disputes/mod.rs | 53 ++ .../src/disputes/with_staging_api/mod.rs | 439 +++++++++++ .../src/disputes/with_staging_api/tests.rs | 679 ++++++++++++++++++ .../src/disputes/without_staging_api/mod.rs | 192 +++++ node/core/provisioner/src/lib.rs | 295 +------- node/core/provisioner/src/metrics.rs | 51 ++ node/core/provisioner/src/onchain_disputes.rs | 77 -- node/core/provisioner/src/tests.rs | 402 +---------- .../approval-distribution/src/tests.rs | 2 +- node/primitives/src/disputes/mod.rs | 2 +- node/primitives/src/disputes/status.rs | 15 + node/primitives/src/lib.rs | 5 +- 16 files changed, 1455 insertions(+), 779 deletions(-) create mode 100644 node/core/provisioner/src/disputes/mod.rs create mode 100644 node/core/provisioner/src/disputes/with_staging_api/mod.rs create mode 100644 node/core/provisioner/src/disputes/with_staging_api/tests.rs create mode 100644 node/core/provisioner/src/disputes/without_staging_api/mod.rs delete mode 100644 node/core/provisioner/src/onchain_disputes.rs diff --git a/Cargo.lock b/Cargo.lock index bab0155beab3..a06119d698a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6014,6 +6014,7 @@ dependencies = [ "polkadot-node-subsystem-test-helpers", "polkadot-node-subsystem-util", "polkadot-primitives", + "polkadot-primitives-test-helpers", "rand 0.8.5", "rand_chacha 0.3.1", "rand_core 0.5.1", @@ -6916,6 +6917,7 @@ dependencies = [ "polkadot-primitives", "rand 0.8.5", "sp-application-crypto", + "sp-core", "sp-keyring", "sp-runtime", ] diff --git a/node/core/dispute-coordinator/src/status.rs b/node/core/dispute-coordinator/src/status.rs index 8a0e2de524d1..6332c3653274 100644 --- a/node/core/dispute-coordinator/src/status.rs +++ b/node/core/dispute-coordinator/src/status.rs @@ -14,29 +14,18 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use polkadot_node_primitives::{DisputeStatus, Timestamp}; +use polkadot_node_primitives::{dispute_is_inactive, DisputeStatus, Timestamp}; use polkadot_primitives::v2::{CandidateHash, SessionIndex}; use std::time::{SystemTime, UNIX_EPOCH}; use crate::LOG_TARGET; -/// The choice here is fairly arbitrary. But any dispute that concluded more than a few minutes ago -/// is not worth considering anymore. Changing this value has little to no bearing on consensus, -/// and really only affects the work that the node might do on startup during periods of many -/// disputes. -pub const ACTIVE_DURATION_SECS: Timestamp = 180; - /// Get active disputes as iterator, preserving its `DisputeStatus`. pub fn get_active_with_status( recent_disputes: impl Iterator, now: Timestamp, ) -> impl Iterator { - recent_disputes.filter_map(move |(disputed, status)| { - status - .concluded_at() - .filter(|at| *at + ACTIVE_DURATION_SECS < now) - .map_or(Some((disputed, status)), |_| None) - }) + recent_disputes.filter(move |(_, status)| !dispute_is_inactive(status, &now)) } pub trait Clock: Send + Sync { diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index 39fdc3a037e5..ff85319599ce 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -49,6 +49,7 @@ use sp_keyring::Sr25519Keyring; use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; use ::test_helpers::{dummy_candidate_receipt_bad_sig, dummy_digest, dummy_hash}; +use polkadot_node_primitives::{Timestamp, ACTIVE_DURATION_SECS}; use polkadot_node_subsystem::{ jaeger, messages::{AllMessages, BlockDescription, RuntimeApiMessage, RuntimeApiRequest}, @@ -66,7 +67,7 @@ use crate::{ backend::Backend, metrics::Metrics, participation::{participation_full_happy_path, participation_missing_availability}, - status::{Clock, Timestamp, ACTIVE_DURATION_SECS}, + status::Clock, Config, DisputeCoordinatorSubsystem, }; diff --git a/node/core/provisioner/Cargo.toml b/node/core/provisioner/Cargo.toml index 2f5c1f9aa5dd..7a06a615154e 100644 --- a/node/core/provisioner/Cargo.toml +++ b/node/core/provisioner/Cargo.toml @@ -13,8 +13,8 @@ polkadot-primitives = { path = "../../../primitives" } polkadot-node-primitives = { path = "../../primitives" } polkadot-node-subsystem = { path = "../../subsystem" } polkadot-node-subsystem-util = { path = "../../subsystem-util" } -futures-timer = "3.0.2" rand = "0.8.5" +futures-timer = "3.0.2" fatality = "0.0.6" [dev-dependencies] diff --git a/node/core/provisioner/src/disputes/mod.rs b/node/core/provisioner/src/disputes/mod.rs new file mode 100644 index 000000000000..f36f35671857 --- /dev/null +++ b/node/core/provisioner/src/disputes/mod.rs @@ -0,0 +1,53 @@ +// Copyright 2017-2022 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +//! The disputes module is responsible for selecting dispute votes to be sent with the inherent data. It contains two +//! different implementations, extracted in two separate modules - `with_staging_api` and `without_staging_api`. The +//! active one is controlled with a feature flag (`staging-client`). The entrypoint to these implementations is the +//! `select_disputes` function. Refer to the documentation of the modules for more details about each implementation. + +use crate::LOG_TARGET; +use futures::channel::oneshot; +use polkadot_node_primitives::CandidateVotes; +use polkadot_node_subsystem::{messages::DisputeCoordinatorMessage, overseer}; +use polkadot_primitives::v2::{CandidateHash, SessionIndex}; + +/// Request the relevant dispute statements for a set of disputes identified by `CandidateHash` and the `SessionIndex`. +async fn request_votes( + sender: &mut impl overseer::ProvisionerSenderTrait, + disputes_to_query: Vec<(SessionIndex, CandidateHash)>, +) -> Vec<(SessionIndex, CandidateHash, CandidateVotes)> { + let (tx, rx) = oneshot::channel(); + // Bounded by block production - `ProvisionerMessage::RequestInherentData`. + sender.send_unbounded_message(DisputeCoordinatorMessage::QueryCandidateVotes( + disputes_to_query, + tx, + )); + + match rx.await { + Ok(v) => v, + Err(oneshot::Canceled) => { + gum::warn!(target: LOG_TARGET, "Unable to query candidate votes"); + Vec::new() + }, + } +} + +#[cfg(feature = "staging-client")] +pub(crate) mod with_staging_api; + +#[cfg(not(feature = "staging-client"))] +pub(crate) mod without_staging_api; diff --git a/node/core/provisioner/src/disputes/with_staging_api/mod.rs b/node/core/provisioner/src/disputes/with_staging_api/mod.rs new file mode 100644 index 000000000000..3e9bcb342b82 --- /dev/null +++ b/node/core/provisioner/src/disputes/with_staging_api/mod.rs @@ -0,0 +1,439 @@ +// Copyright 2017-2022 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +//! This module uses different approach for selecting dispute votes. It queries the Runtime +//! about the votes already known onchain and tries to select only relevant votes. Refer to +//! the documentation of `select_disputes` for more details about the actual implementation. + +use crate::{ + error::{Error, GetOnchainDisputesError}, + metrics, LOG_TARGET, +}; +use futures::channel::oneshot; +use polkadot_node_primitives::{dispute_is_inactive, CandidateVotes, DisputeStatus, Timestamp}; +use polkadot_node_subsystem::{ + errors::RuntimeApiError, + messages::{DisputeCoordinatorMessage, RuntimeApiMessage, RuntimeApiRequest}, + overseer, ActivatedLeaf, +}; +use polkadot_primitives::v2::{ + supermajority_threshold, CandidateHash, DisputeState, DisputeStatement, DisputeStatementSet, + Hash, InvalidDisputeStatementKind, MultiDisputeStatementSet, SessionIndex, + ValidDisputeStatementKind, ValidatorIndex, +}; +use rand as _; +use std::{ + collections::{BTreeMap, HashMap}, + time::{SystemTime, UNIX_EPOCH}, +}; + +#[cfg(test)] +mod tests; + +/// The maximum number of disputes Provisioner will include in the inherent data. +/// Serves as a protection not to flood the Runtime with excessive data. +pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200 * 1_000; +// The magic numbers are: `estimated validators count` * `estimated disputes per validator` + +/// Implements the `select_disputes` function which selects dispute votes which should +/// be sent to the Runtime. +/// +/// # How the prioritization works +/// +/// Generally speaking disputes can be described as: +/// * Active vs Inactive +/// * Known vs Unknown onchain +/// * Offchain vs Onchain +/// * Concluded onchain vs Unconcluded onchain +/// +/// Provisioner fetches all disputes from `dispute-coordinator` and separates them in multiple partitions. +/// Please refer to `struct PartitionedDisputes` for details about the actual partitions. +/// Each partition has got a priority implicitly assigned to it and the disputes are selected based on thus +/// priority (e.g. disputes in partition 1, then if there is space - disputes from partition 2 and so on). +/// +/// # Votes selection +/// +/// Besides the prioritization described above the votes in each partition are filtered too. Provisioner +/// fetches all onchain votes and filters them out from all partitions. As a result the Runtime receives +/// only fresh votes (votes it didn't know about). +/// +/// # How the onchain votes are fetched +/// +/// The logic outlined above relies on `RuntimeApiRequest::StagingDisputes` message from the Runtime staging API. +/// If the staging API is not enabled - the same logic is executed with empty onchain votes set. Effectively this +/// means that all disputes are partitioned in groups 2 or 4 and all votes are sent to the Runtime. +pub async fn select_disputes( + sender: &mut Sender, + metrics: &metrics::Metrics, + leaf: &ActivatedLeaf, +) -> Result +where + Sender: overseer::ProvisionerSenderTrait, +{ + gum::trace!(target: LOG_TARGET, ?leaf, "Selecting disputes for inherent data"); + + // Fetch the onchain disputes. We'll do a prioritization based on them. + let onchain = match get_onchain_disputes(sender, leaf.hash.clone()).await { + Ok(r) => r, + Err(GetOnchainDisputesError::NotSupported(runtime_api_err, relay_parent)) => { + gum::debug!( + target: LOG_TARGET, + ?runtime_api_err, + ?relay_parent, + "Can't fetch onchain disputes, because runtime version is not recent enough. Will continue with empty onchain disputes set.", + ); + HashMap::new() + }, + Err(GetOnchainDisputesError::Channel) => { + // This error usually means the node is shutting down. Log just in case. + gum::debug!( + target: LOG_TARGET, + "Channel error occurred while fetching onchain disputes. Will continue with empty onchain disputes set.", + ); + HashMap::new() + }, + Err(GetOnchainDisputesError::Execution(runtime_api_err, parent_hash)) => { + gum::warn!( + target: LOG_TARGET, + ?runtime_api_err, + ?parent_hash, + "Unexpected execution error occurred while fetching onchain votes. Will continue with empty onchain disputes set.", + ); + HashMap::new() + }, + }; + + let recent_disputes = request_disputes(sender).await; + gum::trace!( + target: LOG_TARGET, + ?leaf, + "Got {} recent disputes and {} onchain disputes. Runtime supports staging API: {}", + recent_disputes.len(), + onchain.len(), + if cfg!(feature = "staging-client") { true } else { false } + ); + + let partitioned = partition_recent_disputes(recent_disputes, &onchain); + metrics.on_partition_recent_disputes(&partitioned); + + if partitioned.inactive_unknown_onchain.len() > 0 { + gum::warn!( + target: LOG_TARGET, + ?leaf, + "Got {} inactive unknown onchain disputes. This should not happen!", + partitioned.inactive_unknown_onchain.len() + ); + } + let result = vote_selection(sender, partitioned, &onchain).await; + + process_selected_disputes(metrics, result) +} + +/// Selects dispute votes from `PartitionedDispites` which should be sent to the runtime. Votes which +/// are already onchain are filtered out. Result should be sorted by `(SessionIndex, CandidateHash)` +/// which is enforced by the `BTreeMap`. This is a requirement from the runtime. +async fn vote_selection( + sender: &mut Sender, + partitioned: PartitionedDisputes, + onchain: &HashMap<(SessionIndex, CandidateHash), DisputeState>, +) -> BTreeMap<(SessionIndex, CandidateHash), CandidateVotes> +where + Sender: overseer::ProvisionerSenderTrait, +{ + const BATCH_SIZE: usize = 1_100; + + // fetch in batches until there are enough votes + let mut disputes = partitioned.into_iter().collect::>(); + let mut total_votes_len = 0; + let mut result = BTreeMap::new(); + while !disputes.is_empty() { + let batch_size = std::cmp::min(BATCH_SIZE, disputes.len()); + let batch = Vec::from_iter(disputes.drain(0..batch_size)); + + // Filter votes which are already onchain + let votes = super::request_votes(sender, batch) + .await + .into_iter() + .map(|(session_index, candidate_hash, mut votes)| { + let onchain_state = + if let Some(onchain_state) = onchain.get(&(session_index, candidate_hash)) { + onchain_state + } else { + // onchain knows nothing about this dispute - add all votes + return (session_index, candidate_hash, votes) + }; + + votes.valid.retain(|validator_idx, (statement_kind, _)| { + is_vote_worth_to_keep(validator_idx, statement_kind, &onchain_state) + }); + votes.invalid.retain(|validator_idx, (statement_kind, _)| { + is_vote_worth_to_keep(validator_idx, statement_kind, &onchain_state) + }); + (session_index, candidate_hash, votes) + }) + .collect::>(); + + // Check if votes are within the limit + for (session_index, candidate_hash, selected_votes) in votes { + let votes_len = selected_votes.valid.len() + selected_votes.invalid.len(); + if votes_len + total_votes_len > MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME { + // we are done - no more votes can be added + return result + } + result.insert((session_index, candidate_hash), selected_votes); + total_votes_len += votes_len + } + } + + result +} + +/// Contains disputes by partitions. Check the field comments for further details. +#[derive(Default)] +pub(crate) struct PartitionedDisputes { + /// Concluded and inactive disputes which are completely unknown for the Runtime. + /// Hopefully this should never happen. + /// Will be sent to the Runtime with FIRST priority. + pub inactive_unknown_onchain: Vec<(SessionIndex, CandidateHash)>, + /// Active disputes completely unknown onchain. + /// Will be sent to the Runtime with SECOND priority. + pub active_unknown_onchain: Vec<(SessionIndex, CandidateHash)>, + /// Active disputes unconcluded onchain. + /// Will be sent to the Runtime with THIRD priority. + pub active_unconcluded_onchain: Vec<(SessionIndex, CandidateHash)>, + /// Active disputes concluded onchain. New votes are not that important for + /// this partition. + /// Will be sent to the Runtime with FOURTH priority. + pub active_concluded_onchain: Vec<(SessionIndex, CandidateHash)>, + /// Inactive disputes which are known onchain. These are not + /// interesting and won't be sent to the Runtime. + pub inactive_known_onchain: Vec<(SessionIndex, CandidateHash)>, +} + +impl PartitionedDisputes { + fn new() -> PartitionedDisputes { + Default::default() + } + + fn into_iter(self) -> impl Iterator { + self.inactive_unknown_onchain + .into_iter() + .chain(self.active_unknown_onchain.into_iter()) + .chain(self.active_unconcluded_onchain.into_iter()) + .chain(self.active_concluded_onchain.into_iter()) + // inactive_known_onchain is dropped on purpose + } +} + +fn secs_since_epoch() -> Timestamp { + match SystemTime::now().duration_since(UNIX_EPOCH) { + Ok(d) => d.as_secs(), + Err(e) => { + gum::warn!( + target: LOG_TARGET, + err = ?e, + "Error getting system time." + ); + 0 + }, + } +} + +fn partition_recent_disputes( + recent: Vec<(SessionIndex, CandidateHash, DisputeStatus)>, + onchain: &HashMap<(SessionIndex, CandidateHash), DisputeState>, +) -> PartitionedDisputes { + let mut partitioned = PartitionedDisputes::new(); + + // Drop any duplicates + let unique_recent = recent + .into_iter() + .map(|(session_index, candidate_hash, dispute_state)| { + ((session_index, candidate_hash), dispute_state) + }) + .collect::>(); + + // Split ACTIVE from CONCLUDED disputes + let (active, inactive): ( + Vec<(SessionIndex, CandidateHash, DisputeStatus)>, + Vec<(SessionIndex, CandidateHash, DisputeStatus)>, + ) = unique_recent + .into_iter() + .map(|((session_index, candidate_hash), dispute_state)| { + (session_index, candidate_hash, dispute_state) + }) + .partition(|(_, _, status)| !dispute_is_inactive(status, &secs_since_epoch())); + + // Split ACTIVE in three groups... + for (session_index, candidate_hash, _) in active { + match onchain.get(&(session_index, candidate_hash)) { + Some(d) => { + // Check if there are enough onchain votes for or against to conclude the dispute + let supermajority = supermajority_threshold(d.validators_for.len()); + if d.validators_for.count_ones() >= supermajority || + d.validators_against.count_ones() >= supermajority + { + partitioned.active_concluded_onchain.push((session_index, candidate_hash)); + } else { + partitioned.active_unconcluded_onchain.push((session_index, candidate_hash)); + } + }, + None => partitioned.active_unknown_onchain.push((session_index, candidate_hash)), + }; + } + + // ... and INACTIVE in two more + for (session_index, candidate_hash, _) in inactive { + match onchain.get(&(session_index, candidate_hash)) { + Some(_) => partitioned.inactive_known_onchain.push((session_index, candidate_hash)), + None => partitioned.inactive_unknown_onchain.push((session_index, candidate_hash)), + } + } + + partitioned +} + +// Helper trait to obtain the value of vote for `InvalidDisputeStatementKind` and `ValidDisputeStatementKind`. +// The alternative was to pass a bool to `fn is_vote_worth_to_keep` explicitly but it's pointless as the value is already 'encoded' in the type. +trait VoteType { + fn vote_value() -> bool; +} + +impl VoteType for InvalidDisputeStatementKind { + fn vote_value() -> bool { + false + } +} + +impl VoteType for ValidDisputeStatementKind { + fn vote_value() -> bool { + true + } +} + +/// Determines if a vote is worth to be kept, based on the onchain disputes +fn is_vote_worth_to_keep( + validator_index: &ValidatorIndex, + _: &T, + onchain_state: &DisputeState, +) -> bool { + let offchain_vote = T::vote_value(); + let in_validators_for = onchain_state + .validators_for + .get(validator_index.0 as usize) + .as_deref() + .copied() + .unwrap_or(false); + let in_validators_against = onchain_state + .validators_against + .get(validator_index.0 as usize) + .as_deref() + .copied() + .unwrap_or(false); + + if in_validators_for && in_validators_against { + // The validator has double voted and runtime knows about this. Ignore this vote. + return false + } + + if offchain_vote && in_validators_against || !offchain_vote && in_validators_for { + // offchain vote differs from the onchain vote + // we need this vote to punish the offending validator + return true + } + + // The vote is valid. Return true if it is not seen onchain. + !in_validators_for && !in_validators_against +} + +/// Request disputes identified by `CandidateHash` and the `SessionIndex`. +async fn request_disputes( + sender: &mut impl overseer::ProvisionerSenderTrait, +) -> Vec<(SessionIndex, CandidateHash, DisputeStatus)> { + let (tx, rx) = oneshot::channel(); + let msg = DisputeCoordinatorMessage::RecentDisputes(tx); + + // Bounded by block production - `ProvisionerMessage::RequestInherentData`. + sender.send_unbounded_message(msg); + + let recent_disputes = rx.await.unwrap_or_else(|err| { + gum::warn!(target: LOG_TARGET, err=?err, "Unable to gather recent disputes"); + Vec::new() + }); + recent_disputes +} + +// This function produces the return value for `pub fn select_disputes()` +fn process_selected_disputes( + metrics: &metrics::Metrics, + dispute_candidate_votes: BTreeMap<(SessionIndex, CandidateHash), CandidateVotes>, +) -> Result { + // Transform all `CandidateVotes` into `MultiDisputeStatementSet`. + Ok(dispute_candidate_votes + .into_iter() + .map(|((session_index, candidate_hash), votes)| { + let valid_statements = votes + .valid + .into_iter() + .map(|(i, (s, sig))| (DisputeStatement::Valid(s), i, sig)); + + let invalid_statements = votes + .invalid + .into_iter() + .map(|(i, (s, sig))| (DisputeStatement::Invalid(s), i, sig)); + + metrics.inc_valid_statements_by(valid_statements.len()); + metrics.inc_invalid_statements_by(invalid_statements.len()); + metrics.inc_dispute_statement_sets_by(1); + + DisputeStatementSet { + candidate_hash, + session: session_index, + statements: valid_statements.chain(invalid_statements).collect(), + } + }) + .collect()) +} + +/// Gets the on-chain disputes at a given block number and returns them as a `HashMap` so that searching in them is cheap. +pub async fn get_onchain_disputes( + sender: &mut Sender, + relay_parent: Hash, +) -> Result, GetOnchainDisputesError> +where + Sender: overseer::ProvisionerSenderTrait, +{ + gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching on-chain disputes"); + let (tx, rx) = oneshot::channel(); + sender + .send_message(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::StagingDisputes(tx), + )) + .await; + + rx.await + .map_err(|_| GetOnchainDisputesError::Channel) + .and_then(|res| { + res.map_err(|e| match e { + RuntimeApiError::Execution { .. } => + GetOnchainDisputesError::Execution(e, relay_parent), + RuntimeApiError::NotSupported { .. } => + GetOnchainDisputesError::NotSupported(e, relay_parent), + }) + }) + .map(|v| v.into_iter().map(|e| ((e.0, e.1), e.2)).collect()) +} diff --git a/node/core/provisioner/src/disputes/with_staging_api/tests.rs b/node/core/provisioner/src/disputes/with_staging_api/tests.rs new file mode 100644 index 000000000000..bfd896a6534a --- /dev/null +++ b/node/core/provisioner/src/disputes/with_staging_api/tests.rs @@ -0,0 +1,679 @@ +// Copyright 2017-2022 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +use super::super::{ + super::{tests::common::test_harness, *}, + with_staging_api::*, +}; +use bitvec::prelude::*; +use futures::channel::mpsc; +use polkadot_node_primitives::{CandidateVotes, DisputeStatus}; +use polkadot_node_subsystem::messages::{ + AllMessages, DisputeCoordinatorMessage, RuntimeApiMessage, RuntimeApiRequest, +}; +use polkadot_node_subsystem_test_helpers::TestSubsystemSender; +use polkadot_primitives::v2::{ + CandidateHash, DisputeState, InvalidDisputeStatementKind, SessionIndex, + ValidDisputeStatementKind, ValidatorSignature, +}; +use std::sync::Arc; +use test_helpers; + +// +// Unit tests for various functions +// +#[test] +fn should_keep_vote_behaves() { + let onchain_state = DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 0, 1, 0, 1], + validators_against: bitvec![u8, Lsb0; 0, 1, 0, 0, 1], + start: 1, + concluded_at: None, + }; + + let local_valid_known = (ValidatorIndex(0), ValidDisputeStatementKind::Explicit); + let local_valid_unknown = (ValidatorIndex(3), ValidDisputeStatementKind::Explicit); + + let local_invalid_known = (ValidatorIndex(1), InvalidDisputeStatementKind::Explicit); + let local_invalid_unknown = (ValidatorIndex(3), InvalidDisputeStatementKind::Explicit); + + assert_eq!( + is_vote_worth_to_keep(&local_valid_known.0, &local_valid_known.1, &onchain_state), + false + ); + assert_eq!( + is_vote_worth_to_keep(&local_valid_unknown.0, &local_valid_unknown.1, &onchain_state), + true + ); + assert_eq!( + is_vote_worth_to_keep(&local_invalid_known.0, &local_invalid_known.1, &onchain_state), + false + ); + assert_eq!( + is_vote_worth_to_keep(&local_invalid_unknown.0, &local_invalid_unknown.1, &onchain_state), + true + ); + + //double voting - onchain knows + let local_double_vote_onchain_knows = + (ValidatorIndex(4), InvalidDisputeStatementKind::Explicit); + assert_eq!( + is_vote_worth_to_keep( + &local_double_vote_onchain_knows.0, + &local_double_vote_onchain_knows.1, + &onchain_state + ), + false + ); + + //double voting - onchain doesn't know + let local_double_vote_onchain_doesnt_knows = + (ValidatorIndex(0), InvalidDisputeStatementKind::Explicit); + assert_eq!( + is_vote_worth_to_keep( + &local_double_vote_onchain_doesnt_knows.0, + &local_double_vote_onchain_doesnt_knows.1, + &onchain_state + ), + true + ); + + // empty onchain state + let empty_onchain_state = DisputeState { + validators_for: BitVec::new(), + validators_against: BitVec::new(), + start: 1, + concluded_at: None, + }; + assert_eq!( + is_vote_worth_to_keep( + &local_double_vote_onchain_doesnt_knows.0, + &local_double_vote_onchain_doesnt_knows.1, + &empty_onchain_state + ), + true + ); +} + +#[test] +fn partitioning_happy_case() { + let mut input = Vec::<(SessionIndex, CandidateHash, DisputeStatus)>::new(); + let mut onchain = HashMap::<(u32, CandidateHash), DisputeState>::new(); + + // Create one dispute for each partition + + let unconcluded_onchain = (0, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(unconcluded_onchain.clone()); + onchain.insert( + (unconcluded_onchain.0, unconcluded_onchain.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 0, 0, 0, 0, 0, 0], + validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: None, + }, + ); + + let unknown_onchain = (1, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(unknown_onchain.clone()); + + let concluded_onchain = (2, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(concluded_onchain.clone()); + onchain.insert( + (concluded_onchain.0, concluded_onchain.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 0], + validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: None, + }, + ); + + let concluded_known_onchain = + (3, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); + input.push(concluded_known_onchain.clone()); + onchain.insert( + (concluded_known_onchain.0, concluded_known_onchain.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 1], + validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: None, + }, + ); + + let concluded_unknown_onchain = + (4, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); + input.push(concluded_unknown_onchain.clone()); + + let result = partition_recent_disputes(input, &onchain); + + assert_eq!(result.active_unconcluded_onchain.len(), 1); + assert_eq!( + result.active_unconcluded_onchain.get(0).unwrap(), + &(unconcluded_onchain.0, unconcluded_onchain.1) + ); + + assert_eq!(result.active_unknown_onchain.len(), 1); + assert_eq!( + result.active_unknown_onchain.get(0).unwrap(), + &(unknown_onchain.0, unknown_onchain.1) + ); + + assert_eq!(result.active_concluded_onchain.len(), 1); + assert_eq!( + result.active_concluded_onchain.get(0).unwrap(), + &(concluded_onchain.0, concluded_onchain.1) + ); + + assert_eq!(result.inactive_known_onchain.len(), 1); + assert_eq!( + result.inactive_known_onchain.get(0).unwrap(), + &(concluded_known_onchain.0, concluded_known_onchain.1) + ); + + assert_eq!(result.inactive_unknown_onchain.len(), 1); + assert_eq!( + result.inactive_unknown_onchain.get(0).unwrap(), + &(concluded_unknown_onchain.0, concluded_unknown_onchain.1) + ); +} + +// This test verifies the double voting behavior. Currently we don't care if a supermajority is achieved with or +// without the 'help' of a double vote (a validator voting for and against at the same time). This makes the test +// a bit pointless but anyway I'm leaving it here to make this decision explicit and have the test code ready in +// case this behavior needs to be further tested in the future. +// Link to the PR with the discussions: https://github.com/paritytech/polkadot/pull/5567 +#[test] +fn partitioning_doubled_onchain_vote() { + let mut input = Vec::<(SessionIndex, CandidateHash, DisputeStatus)>::new(); + let mut onchain = HashMap::<(u32, CandidateHash), DisputeState>::new(); + + // Dispute A relies on a 'double onchain vote' to conclude. Validator with index 0 has voted both `for` and `against`. + // Despite that this dispute should be considered 'can conclude onchain'. + let dispute_a = (3, CandidateHash(Hash::random()), DisputeStatus::Active); + // Dispute B has supermajority + 1 votes, so the doubled onchain vote doesn't affect it. It should be considered + // as 'can conclude onchain'. + let dispute_b = (4, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(dispute_a.clone()); + input.push(dispute_b.clone()); + onchain.insert( + (dispute_a.0, dispute_a.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 0, 0], + validators_against: bitvec![u8, Lsb0; 1, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: None, + }, + ); + onchain.insert( + (dispute_b.0, dispute_b.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 0], + validators_against: bitvec![u8, Lsb0; 1, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: None, + }, + ); + + let result = partition_recent_disputes(input, &onchain); + + assert_eq!(result.active_unconcluded_onchain.len(), 0); + assert_eq!(result.active_concluded_onchain.len(), 2); +} + +#[test] +fn partitioning_duplicated_dispute() { + let mut input = Vec::<(SessionIndex, CandidateHash, DisputeStatus)>::new(); + let mut onchain = HashMap::<(u32, CandidateHash), DisputeState>::new(); + + let some_dispute = (3, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); + input.push(some_dispute.clone()); + input.push(some_dispute.clone()); + onchain.insert( + (some_dispute.0, some_dispute.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 1], + validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: None, + }, + ); + + let result = partition_recent_disputes(input, &onchain); + + assert_eq!(result.inactive_known_onchain.len(), 1); + assert_eq!(result.inactive_known_onchain.get(0).unwrap(), &(some_dispute.0, some_dispute.1)); +} + +// +// end-to-end tests for select_disputes() +// + +async fn mock_overseer( + mut receiver: mpsc::UnboundedReceiver, + disputes_db: &mut TestDisputes, + vote_queries_count: &mut usize, +) { + while let Some(from_job) = receiver.next().await { + match from_job { + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _, + RuntimeApiRequest::StagingDisputes(sender), + )) => { + let _ = sender.send(Ok(disputes_db + .onchain_disputes + .clone() + .into_iter() + .map(|(k, v)| (k.0, k.1, v)) + .collect::>())); + }, + AllMessages::RuntimeApi(_) => panic!("Unexpected RuntimeApi request"), + AllMessages::DisputeCoordinator(DisputeCoordinatorMessage::RecentDisputes(sender)) => { + let _ = sender.send(disputes_db.local_disputes.clone()); + }, + AllMessages::DisputeCoordinator(DisputeCoordinatorMessage::QueryCandidateVotes( + disputes, + sender, + )) => { + *vote_queries_count += 1; + let mut res = Vec::new(); + for d in disputes.iter() { + let v = disputes_db.votes_db.get(d).unwrap().clone(); + res.push((d.0, d.1, v)); + } + + let _ = sender.send(res); + }, + _ => panic!("Unexpected message: {:?}", from_job), + } + } +} + +fn leaf() -> ActivatedLeaf { + ActivatedLeaf { + hash: Hash::repeat_byte(0xAA), + number: 0xAA, + status: LeafStatus::Fresh, + span: Arc::new(jaeger::Span::Disabled), + } +} + +struct TestDisputes { + pub local_disputes: Vec<(SessionIndex, CandidateHash, DisputeStatus)>, + pub votes_db: HashMap<(SessionIndex, CandidateHash), CandidateVotes>, + pub onchain_disputes: HashMap<(u32, CandidateHash), DisputeState>, + validators_count: usize, +} + +impl TestDisputes { + pub fn new(validators_count: usize) -> TestDisputes { + TestDisputes { + local_disputes: Vec::<(SessionIndex, CandidateHash, DisputeStatus)>::new(), + votes_db: HashMap::<(SessionIndex, CandidateHash), CandidateVotes>::new(), + onchain_disputes: HashMap::<(u32, CandidateHash), DisputeState>::new(), + validators_count, + } + } + + // Offchain disputes are on node side + fn add_offchain_dispute( + &mut self, + dispute: (SessionIndex, CandidateHash, DisputeStatus), + local_votes_count: usize, + dummy_receipt: CandidateReceipt, + ) { + self.local_disputes.push(dispute.clone()); + self.votes_db.insert( + (dispute.0, dispute.1), + CandidateVotes { + candidate_receipt: dummy_receipt, + valid: TestDisputes::generate_local_votes( + ValidDisputeStatementKind::Explicit, + 0, + local_votes_count, + ), + invalid: BTreeMap::new(), + }, + ); + } + + fn add_onchain_dispute( + &mut self, + dispute: (SessionIndex, CandidateHash, DisputeStatus), + onchain_votes_count: usize, + ) { + let concluded_at = match dispute.2 { + DisputeStatus::Active | DisputeStatus::Confirmed => None, + DisputeStatus::ConcludedAgainst(_) | DisputeStatus::ConcludedFor(_) => Some(1), + }; + self.onchain_disputes.insert( + (dispute.0, dispute.1.clone()), + DisputeState { + validators_for: TestDisputes::generate_bitvec( + self.validators_count, + 0, + onchain_votes_count, + ), + validators_against: bitvec![u8, Lsb0; 0; self.validators_count], + start: 1, + concluded_at, + }, + ); + } + + pub fn add_unconfirmed_disputes_concluded_onchain( + &mut self, + dispute_count: usize, + ) -> (u32, usize) { + let local_votes_count = self.validators_count / 100 * 90; + let onchain_votes_count = self.validators_count / 100 * 80; + let session_idx = 0; + let lf = leaf(); + let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); + for _ in 0..dispute_count { + let d = (session_idx, CandidateHash(Hash::random()), DisputeStatus::Active); + self.add_offchain_dispute(d.clone(), local_votes_count, dummy_receipt.clone()); + self.add_onchain_dispute(d, onchain_votes_count); + } + + (session_idx, (local_votes_count - onchain_votes_count) * dispute_count) + } + + pub fn add_unconfirmed_disputes_unconcluded_onchain( + &mut self, + dispute_count: usize, + ) -> (u32, usize) { + let local_votes_count = self.validators_count / 100 * 90; + let onchain_votes_count = self.validators_count / 100 * 40; + let session_idx = 1; + let lf = leaf(); + let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); + for _ in 0..dispute_count { + let d = (session_idx, CandidateHash(Hash::random()), DisputeStatus::Active); + self.add_offchain_dispute(d.clone(), local_votes_count, dummy_receipt.clone()); + self.add_onchain_dispute(d, onchain_votes_count); + } + + (session_idx, (local_votes_count - onchain_votes_count) * dispute_count) + } + + pub fn add_unconfirmed_disputes_unknown_onchain( + &mut self, + dispute_count: usize, + ) -> (u32, usize) { + let local_votes_count = self.validators_count / 100 * 70; + let session_idx = 2; + let lf = leaf(); + let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); + for _ in 0..dispute_count { + let d = (session_idx, CandidateHash(Hash::random()), DisputeStatus::Active); + self.add_offchain_dispute(d.clone(), local_votes_count, dummy_receipt.clone()); + } + (session_idx, local_votes_count * dispute_count) + } + + pub fn add_concluded_disputes_known_onchain(&mut self, dispute_count: usize) -> (u32, usize) { + let local_votes_count = self.validators_count / 100 * 80; + let onchain_votes_count = self.validators_count / 100 * 75; + let session_idx = 3; + let lf = leaf(); + let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); + for _ in 0..dispute_count { + let d = (session_idx, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); + self.add_offchain_dispute(d.clone(), local_votes_count, dummy_receipt.clone()); + self.add_onchain_dispute(d, onchain_votes_count); + } + (session_idx, (local_votes_count - onchain_votes_count) * dispute_count) + } + + pub fn add_concluded_disputes_unknown_onchain(&mut self, dispute_count: usize) -> (u32, usize) { + let local_votes_count = self.validators_count / 100 * 80; + let session_idx = 4; + let lf = leaf(); + let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); + for _ in 0..dispute_count { + let d = (session_idx, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); + self.add_offchain_dispute(d.clone(), local_votes_count, dummy_receipt.clone()); + } + (session_idx, local_votes_count * dispute_count) + } + + fn generate_local_votes( + statement_kind: T, + start_idx: usize, + count: usize, + ) -> BTreeMap { + assert!(start_idx < count); + (start_idx..count) + .map(|idx| { + ( + ValidatorIndex(idx as u32), + (statement_kind.clone(), test_helpers::dummy_signature()), + ) + }) + .collect::>() + } + + fn generate_bitvec( + validator_count: usize, + start_idx: usize, + count: usize, + ) -> BitVec { + assert!(start_idx < count); + assert!(start_idx + count < validator_count); + let mut res = bitvec![u8, Lsb0; 0; validator_count]; + for idx in start_idx..count { + res.set(idx, true); + } + + res + } +} + +#[test] +fn normal_flow() { + const VALIDATOR_COUNT: usize = 100; + const DISPUTES_PER_BATCH: usize = 10; + const ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: usize = 1; + + let mut input = TestDisputes::new(VALIDATOR_COUNT); + + // active, concluded onchain + let (third_idx, third_votes) = + input.add_unconfirmed_disputes_concluded_onchain(DISPUTES_PER_BATCH); + + // active unconcluded onchain + let (first_idx, first_votes) = + input.add_unconfirmed_disputes_unconcluded_onchain(DISPUTES_PER_BATCH); + + //concluded disputes unknown onchain + let (fifth_idx, fifth_votes) = input.add_concluded_disputes_unknown_onchain(DISPUTES_PER_BATCH); + + // concluded disputes known onchain + let (fourth_idx, _) = input.add_concluded_disputes_known_onchain(DISPUTES_PER_BATCH); + + // active disputes unknown onchain + let (second_idx, second_votes) = + input.add_unconfirmed_disputes_unknown_onchain(DISPUTES_PER_BATCH); + + let metrics = metrics::Metrics::new_dummy(); + let mut vote_queries: usize = 0; + test_harness( + |r| mock_overseer(r, &mut input, &mut vote_queries), + |mut tx: TestSubsystemSender| async move { + let lf = leaf(); + let result = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); + + assert!(!result.is_empty()); + + if cfg!(feature = "staging-client") { + assert_eq!(result.len(), 4 * DISPUTES_PER_BATCH); + + // Naive checks that the result is partitioned correctly + let (first_batch, rest): (Vec, Vec) = + result.into_iter().partition(|d| d.session == first_idx); + assert_eq!(first_batch.len(), DISPUTES_PER_BATCH); + + let (second_batch, rest): (Vec, Vec) = + rest.into_iter().partition(|d| d.session == second_idx); + assert_eq!(second_batch.len(), DISPUTES_PER_BATCH); + + let (third_batch, rest): (Vec, Vec) = + rest.into_iter().partition(|d| d.session == third_idx); + assert_eq!(third_batch.len(), DISPUTES_PER_BATCH); + + let (fifth_batch, rest): (Vec, Vec) = + rest.into_iter().partition(|d| d.session == fifth_idx); + assert_eq!(fifth_batch.len(), DISPUTES_PER_BATCH); + + // Ensure there are no more disputes - fourth_batch should be dropped + assert_eq!(rest.len(), 0); + + assert_eq!( + first_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + first_votes + ); + assert_eq!( + second_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + second_votes + ); + assert_eq!( + third_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + third_votes + ); + assert_eq!( + fifth_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + fifth_votes + ); + } else { + assert_eq!(result.len(), 5 * DISPUTES_PER_BATCH); + for i in 0..3 * DISPUTES_PER_BATCH { + assert_ne!(result.get(i).unwrap().session, fourth_idx); + assert_ne!(result.get(i).unwrap().session, fifth_idx); + } + } + }, + ); + assert!(vote_queries <= ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT); +} + +#[test] +fn many_batches() { + const VALIDATOR_COUNT: usize = 100; + const DISPUTES_PER_PARTITION: usize = 1000; + // Around 4_000 disputes are generated. `BATCH_SIZE` is 1_100. + const ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: usize = 4; + + let mut input = TestDisputes::new(VALIDATOR_COUNT); + + // active which can conclude onchain + input.add_unconfirmed_disputes_concluded_onchain(DISPUTES_PER_PARTITION); + + // active which can't conclude onchain + input.add_unconfirmed_disputes_unconcluded_onchain(DISPUTES_PER_PARTITION); + + //concluded disputes unknown onchain + input.add_concluded_disputes_unknown_onchain(DISPUTES_PER_PARTITION); + + // concluded disputes known onchain + input.add_concluded_disputes_known_onchain(DISPUTES_PER_PARTITION); + + // active disputes unknown onchain + input.add_unconfirmed_disputes_unknown_onchain(DISPUTES_PER_PARTITION); + + let metrics = metrics::Metrics::new_dummy(); + let mut vote_queries: usize = 0; + test_harness( + |r| mock_overseer(r, &mut input, &mut vote_queries), + |mut tx: TestSubsystemSender| async move { + let lf = leaf(); + let result = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); + + assert!(!result.is_empty()); + + let vote_count = result.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v); + + assert!( + MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME - VALIDATOR_COUNT <= vote_count && + vote_count <= MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME, + "vote_count: {}", + vote_count + ); + }, + ); + + assert!( + vote_queries <= ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT, + "vote_queries: {} ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: {}", + vote_queries, + ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT + ); +} + +#[test] +fn votes_above_limit() { + const VALIDATOR_COUNT: usize = 100; + const DISPUTES_PER_PARTITION: usize = 5_000; + const ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: usize = 4; + + let mut input = TestDisputes::new(VALIDATOR_COUNT); + + // active which can conclude onchain + let (_, second_votes) = + input.add_unconfirmed_disputes_concluded_onchain(DISPUTES_PER_PARTITION); + + // active which can't conclude onchain + let (_, first_votes) = + input.add_unconfirmed_disputes_unconcluded_onchain(DISPUTES_PER_PARTITION); + + //concluded disputes unknown onchain + let (_, third_votes) = input.add_concluded_disputes_unknown_onchain(DISPUTES_PER_PARTITION); + + assert!( + first_votes + second_votes + third_votes > 3 * MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME, + "Total relevant votes generated: {}", + first_votes + second_votes + third_votes + ); + + let metrics = metrics::Metrics::new_dummy(); + let mut vote_queries: usize = 0; + test_harness( + |r| mock_overseer(r, &mut input, &mut vote_queries), + |mut tx: TestSubsystemSender| async move { + let lf = leaf(); + let result = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); + + assert!(!result.is_empty()); + + let vote_count = result.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v); + + assert!( + MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME - VALIDATOR_COUNT <= vote_count && + vote_count <= MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME, + "vote_count: {}", + vote_count + ); + }, + ); + + assert!( + vote_queries <= ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT, + "vote_queries: {} ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: {}", + vote_queries, + ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT + ); +} diff --git a/node/core/provisioner/src/disputes/without_staging_api/mod.rs b/node/core/provisioner/src/disputes/without_staging_api/mod.rs new file mode 100644 index 000000000000..3bf5cac340d2 --- /dev/null +++ b/node/core/provisioner/src/disputes/without_staging_api/mod.rs @@ -0,0 +1,192 @@ +// Copyright 2017-2022 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +//! This module selects all RECENT disputes, fetches the votes for them from dispute-coordinator and +//! returns them as MultiDisputeStatementSet. If the RECENT disputes are more than +//! `MAX_DISPUTES_FORWARDED_TO_RUNTIME` constant - the ACTIVE disputes plus a random selection of +//! RECENT disputes (up to `MAX_DISPUTES_FORWARDED_TO_RUNTIME`) are returned instead. +//! If the ACTIVE disputes are also above `MAX_DISPUTES_FORWARDED_TO_RUNTIME` limit - a random selection +//! of them is generated. + +use crate::{error::Error, metrics, LOG_TARGET}; +use futures::channel::oneshot; +use polkadot_node_subsystem::{messages::DisputeCoordinatorMessage, overseer}; +use polkadot_primitives::v2::{ + CandidateHash, DisputeStatement, DisputeStatementSet, MultiDisputeStatementSet, SessionIndex, +}; +use std::collections::HashSet; + +#[derive(Debug)] +enum RequestType { + /// Query recent disputes, could be an excessive amount. + Recent, + /// Query the currently active and very recently concluded disputes. + Active, +} + +/// Request open disputes identified by `CandidateHash` and the `SessionIndex`. +async fn request_disputes( + sender: &mut impl overseer::ProvisionerSenderTrait, + active_or_recent: RequestType, +) -> Vec<(SessionIndex, CandidateHash)> { + let disputes = match active_or_recent { + RequestType::Recent => { + let (tx, rx) = oneshot::channel(); + let msg = DisputeCoordinatorMessage::RecentDisputes(tx); + sender.send_unbounded_message(msg); + let recent_disputes = match rx.await { + Ok(r) => r, + Err(oneshot::Canceled) => { + gum::warn!( + target: LOG_TARGET, + "Unable to gather {:?} disputes", + active_or_recent + ); + Vec::new() + }, + }; + recent_disputes + .into_iter() + .map(|(sesion_idx, candodate_hash, _)| (sesion_idx, candodate_hash)) + .collect::>() + }, + RequestType::Active => { + let (tx, rx) = oneshot::channel(); + let msg = DisputeCoordinatorMessage::ActiveDisputes(tx); + sender.send_unbounded_message(msg); + let active_disputes = match rx.await { + Ok(r) => r, + Err(oneshot::Canceled) => { + gum::warn!( + target: LOG_TARGET, + "Unable to gather {:?} disputes", + active_or_recent + ); + Vec::new() + }, + }; + active_disputes + }, + }; + + disputes +} + +/// Extend `acc` by `n` random, picks of not-yet-present in `acc` items of `recent` without repetition and additions of recent. +fn extend_by_random_subset_without_repetition( + acc: &mut Vec<(SessionIndex, CandidateHash)>, + extension: Vec<(SessionIndex, CandidateHash)>, + n: usize, +) { + use rand::Rng; + + let lut = acc.iter().cloned().collect::>(); + + let mut unique_new = + extension.into_iter().filter(|recent| !lut.contains(recent)).collect::>(); + + // we can simply add all + if unique_new.len() <= n { + acc.extend(unique_new) + } else { + acc.reserve(n); + let mut rng = rand::thread_rng(); + for _ in 0..n { + let idx = rng.gen_range(0..unique_new.len()); + acc.push(unique_new.swap_remove(idx)); + } + } + // assure sorting stays candid according to session index + acc.sort_unstable_by(|a, b| a.0.cmp(&b.0)); +} + +pub async fn select_disputes( + sender: &mut Sender, + metrics: &metrics::Metrics, +) -> Result +where + Sender: overseer::ProvisionerSenderTrait, +{ + /// The maximum number of disputes Provisioner will include in the inherent data. + /// Serves as a protection not to flood the Runtime with excessive data. + const MAX_DISPUTES_FORWARDED_TO_RUNTIME: usize = 1_000; + + // We use `RecentDisputes` instead of `ActiveDisputes` because redundancy is fine. + // It's heavier than `ActiveDisputes` but ensures that everything from the dispute + // window gets on-chain, unlike `ActiveDisputes`. + // In case of an overload condition, we limit ourselves to active disputes, and fill up to the + // upper bound of disputes to pass to wasm `fn create_inherent_data`. + // If the active ones are already exceeding the bounds, randomly select a subset. + let recent = request_disputes(sender, RequestType::Recent).await; + let disputes = if recent.len() > MAX_DISPUTES_FORWARDED_TO_RUNTIME { + gum::warn!( + target: LOG_TARGET, + "Recent disputes are excessive ({} > {}), reduce to active ones, and selected", + recent.len(), + MAX_DISPUTES_FORWARDED_TO_RUNTIME + ); + let mut active = request_disputes(sender, RequestType::Active).await; + let n_active = active.len(); + let active = if active.len() > MAX_DISPUTES_FORWARDED_TO_RUNTIME { + let mut picked = Vec::with_capacity(MAX_DISPUTES_FORWARDED_TO_RUNTIME); + extend_by_random_subset_without_repetition( + &mut picked, + active, + MAX_DISPUTES_FORWARDED_TO_RUNTIME, + ); + picked + } else { + extend_by_random_subset_without_repetition( + &mut active, + recent, + MAX_DISPUTES_FORWARDED_TO_RUNTIME.saturating_sub(n_active), + ); + active + }; + active + } else { + recent + }; + + // Load all votes for all disputes from the coordinator. + let dispute_candidate_votes = super::request_votes(sender, disputes).await; + + // Transform all `CandidateVotes` into `MultiDisputeStatementSet`. + Ok(dispute_candidate_votes + .into_iter() + .map(|(session_index, candidate_hash, votes)| { + let valid_statements = votes + .valid + .into_iter() + .map(|(i, (s, sig))| (DisputeStatement::Valid(s), i, sig)); + + let invalid_statements = votes + .invalid + .into_iter() + .map(|(i, (s, sig))| (DisputeStatement::Invalid(s), i, sig)); + + metrics.inc_valid_statements_by(valid_statements.len()); + metrics.inc_invalid_statements_by(invalid_statements.len()); + metrics.inc_dispute_statement_sets_by(1); + + DisputeStatementSet { + candidate_hash, + session: session_index, + statements: valid_statements.chain(invalid_statements).collect(), + } + }) + .collect()) +} diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index 0f3099c7df33..9c1e3605e1b6 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -25,12 +25,11 @@ use futures::{ }; use futures_timer::Delay; -use polkadot_node_primitives::CandidateVotes; use polkadot_node_subsystem::{ jaeger, messages::{ - CandidateBackingMessage, ChainApiMessage, DisputeCoordinatorMessage, ProvisionableData, - ProvisionerInherentData, ProvisionerMessage, + CandidateBackingMessage, ChainApiMessage, ProvisionableData, ProvisionerInherentData, + ProvisionerMessage, }, overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, LeafStatus, OverseerSignal, PerLeafSpan, SpawnedSubsystem, SubsystemError, @@ -39,15 +38,14 @@ use polkadot_node_subsystem_util::{ request_availability_cores, request_persisted_validation_data, TimeoutExt, }; use polkadot_primitives::v2::{ - BackedCandidate, BlockNumber, CandidateHash, CandidateReceipt, CoreState, DisputeState, - DisputeStatement, DisputeStatementSet, Hash, MultiDisputeStatementSet, OccupiedCoreAssumption, - SessionIndex, SignedAvailabilityBitfield, ValidatorIndex, + BackedCandidate, BlockNumber, CandidateReceipt, CoreState, Hash, OccupiedCoreAssumption, + SignedAvailabilityBitfield, ValidatorIndex, }; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap}; +mod disputes; mod error; mod metrics; -mod onchain_disputes; pub use self::metrics::*; use error::{Error, FatalResult}; @@ -361,7 +359,13 @@ async fn send_inherent_data( relay_parent = ?leaf.hash, "Selecting disputes" ); - let disputes = select_disputes(from_job, metrics, leaf).await?; + + #[cfg(feature = "staging-client")] + let disputes = disputes::with_staging_api::select_disputes(from_job, metrics, leaf).await?; + + #[cfg(not(feature = "staging-client"))] + let disputes = disputes::without_staging_api::select_disputes(from_job, metrics).await?; + gum::trace!( target: LOG_TARGET, relay_parent = ?leaf.hash, @@ -676,276 +680,3 @@ fn bitfields_indicate_availability( 3 * availability.count_ones() >= 2 * availability.len() } - -#[derive(Debug)] -enum RequestType { - /// Query recent disputes, could be an excessive amount. - Recent, - /// Query the currently active and very recently concluded disputes. - Active, -} - -/// Request open disputes identified by `CandidateHash` and the `SessionIndex`. -async fn request_disputes( - sender: &mut impl overseer::ProvisionerSenderTrait, - active_or_recent: RequestType, -) -> Vec<(SessionIndex, CandidateHash)> { - let (tx, rx) = oneshot::channel(); - let msg = match active_or_recent { - RequestType::Recent => DisputeCoordinatorMessage::RecentDisputes(tx), - RequestType::Active => DisputeCoordinatorMessage::ActiveDisputes(tx), - }; - // Bounded by block production - `ProvisionerMessage::RequestInherentData`. - sender.send_unbounded_message(msg); - - let recent_disputes = match rx.await { - Ok(r) => r, - Err(oneshot::Canceled) => { - gum::warn!(target: LOG_TARGET, "Unable to gather {:?} disputes", active_or_recent); - Vec::new() - }, - }; - recent_disputes -} - -/// Request the relevant dispute statements for a set of disputes identified by `CandidateHash` and the `SessionIndex`. -async fn request_votes( - sender: &mut impl overseer::ProvisionerSenderTrait, - disputes_to_query: Vec<(SessionIndex, CandidateHash)>, -) -> Vec<(SessionIndex, CandidateHash, CandidateVotes)> { - // No need to send dummy request, if nothing to request: - if disputes_to_query.is_empty() { - gum::trace!(target: LOG_TARGET, "No disputes, nothing to request - returning empty `Vec`."); - - return Vec::new() - } - let (tx, rx) = oneshot::channel(); - // Bounded by block production - `ProvisionerMessage::RequestInherentData`. - sender.send_unbounded_message(DisputeCoordinatorMessage::QueryCandidateVotes( - disputes_to_query, - tx, - )); - - match rx.await { - Ok(v) => v, - Err(oneshot::Canceled) => { - gum::warn!(target: LOG_TARGET, "Unable to query candidate votes"); - Vec::new() - }, - } -} - -/// Extend `acc` by `n` random, picks of not-yet-present in `acc` items of `recent` without repetition and additions of recent. -fn extend_by_random_subset_without_repetition( - acc: &mut Vec<(SessionIndex, CandidateHash)>, - extension: Vec<(SessionIndex, CandidateHash)>, - n: usize, -) { - use rand::Rng; - - let lut = acc.iter().cloned().collect::>(); - - let mut unique_new = - extension.into_iter().filter(|recent| !lut.contains(recent)).collect::>(); - - // we can simply add all - if unique_new.len() <= n { - acc.extend(unique_new) - } else { - acc.reserve(n); - let mut rng = rand::thread_rng(); - for _ in 0..n { - let idx = rng.gen_range(0..unique_new.len()); - acc.push(unique_new.swap_remove(idx)); - } - } - // assure sorting stays candid according to session index - acc.sort_unstable_by(|a, b| a.0.cmp(&b.0)); -} - -/// The maximum number of disputes Provisioner will include in the inherent data. -/// Serves as a protection not to flood the Runtime with excessive data. -const MAX_DISPUTES_FORWARDED_TO_RUNTIME: usize = 1_000; - -async fn select_disputes( - sender: &mut impl overseer::ProvisionerSenderTrait, - metrics: &metrics::Metrics, - _leaf: &ActivatedLeaf, -) -> Result { - // Helper lambda - // Gets the active disputes as input and partitions it in seen and unseen disputes by the Runtime - // Returns as much unseen disputes as possible and optionally some seen disputes up to `MAX_DISPUTES_FORWARDED_TO_RUNTIME` limit. - let generate_unseen_active_subset = - |active: Vec<(SessionIndex, CandidateHash)>, - onchain: HashMap<(SessionIndex, CandidateHash), DisputeState>| - -> Vec<(SessionIndex, CandidateHash)> { - let (seen_onchain, mut unseen_onchain): ( - Vec<(SessionIndex, CandidateHash)>, - Vec<(SessionIndex, CandidateHash)>, - ) = active.into_iter().partition(|d| onchain.contains_key(d)); - - if unseen_onchain.len() > MAX_DISPUTES_FORWARDED_TO_RUNTIME { - // Even unseen on-chain don't fit within the limit. Add as many as possible. - let mut unseen_subset = Vec::with_capacity(MAX_DISPUTES_FORWARDED_TO_RUNTIME); - extend_by_random_subset_without_repetition( - &mut unseen_subset, - unseen_onchain, - MAX_DISPUTES_FORWARDED_TO_RUNTIME, - ); - unseen_subset - } else { - // Add all unseen onchain disputes and as much of the seen ones as there is space. - let n_unseen_onchain = unseen_onchain.len(); - extend_by_random_subset_without_repetition( - &mut unseen_onchain, - seen_onchain, - MAX_DISPUTES_FORWARDED_TO_RUNTIME.saturating_sub(n_unseen_onchain), - ); - unseen_onchain - } - }; - - // Helper lambda - // Extends the active disputes with recent ones up to `MAX_DISPUTES_FORWARDED_TO_RUNTIME` limit. Unseen recent disputes are prioritised. - let generate_active_and_unseen_recent_subset = - |recent: Vec<(SessionIndex, CandidateHash)>, - mut active: Vec<(SessionIndex, CandidateHash)>, - onchain: HashMap<(SessionIndex, CandidateHash), DisputeState>| - -> Vec<(SessionIndex, CandidateHash)> { - let mut n_active = active.len(); - // All active disputes can be sent. Fill the rest of the space with recent ones. - // We assume there is not enough space for all recent disputes. So we prioritise the unseen ones. - let (seen_onchain, unseen_onchain): ( - Vec<(SessionIndex, CandidateHash)>, - Vec<(SessionIndex, CandidateHash)>, - ) = recent.into_iter().partition(|d| onchain.contains_key(d)); - - extend_by_random_subset_without_repetition( - &mut active, - unseen_onchain, - MAX_DISPUTES_FORWARDED_TO_RUNTIME.saturating_sub(n_active), - ); - n_active = active.len(); - - if n_active < MAX_DISPUTES_FORWARDED_TO_RUNTIME { - // Looks like we can add some of the seen disputes too - extend_by_random_subset_without_repetition( - &mut active, - seen_onchain, - MAX_DISPUTES_FORWARDED_TO_RUNTIME.saturating_sub(n_active), - ); - } - active - }; - - gum::trace!( - target: LOG_TARGET, - relay_parent = ?_leaf.hash, - "Request recent disputes" - ); - - // We use `RecentDisputes` instead of `ActiveDisputes` because redundancy is fine. - // It's heavier than `ActiveDisputes` but ensures that everything from the dispute - // window gets on-chain, unlike `ActiveDisputes`. - // In case of an overload condition, we limit ourselves to active disputes, and fill up to the - // upper bound of disputes to pass to wasm `fn create_inherent_data`. - // If the active ones are already exceeding the bounds, randomly select a subset. - let recent = request_disputes(sender, RequestType::Recent).await; - - gum::trace!( - target: LOG_TARGET, - relay_paent = ?_leaf.hash, - "Received recent disputes" - ); - - gum::trace!( - target: LOG_TARGET, - relay_paent = ?_leaf.hash, - "Request on chain disputes" - ); - - // On chain disputes are fetched from the runtime. We want to prioritise the inclusion of unknown - // disputes in the inherent data. The call relies on staging Runtime API. If the staging API is not - // enabled in the binary an empty set is generated which doesn't affect the rest of the logic. - let onchain = match onchain_disputes::get_onchain_disputes(sender, _leaf.hash.clone()).await { - Ok(r) => r, - Err(e) => { - gum::debug!( - target: LOG_TARGET, - ?e, - "Can't fetch onchain disputes. Will continue with empty onchain disputes set.", - ); - HashMap::new() - }, - }; - - gum::trace!( - target: LOG_TARGET, - relay_paent = ?_leaf.hash, - "Received on chain disputes" - ); - - gum::trace!( - target: LOG_TARGET, - relay_paent = ?_leaf.hash, - "Filtering disputes" - ); - - let disputes = if recent.len() > MAX_DISPUTES_FORWARDED_TO_RUNTIME { - gum::warn!( - target: LOG_TARGET, - "Recent disputes are excessive ({} > {}), reduce to active ones, and selected", - recent.len(), - MAX_DISPUTES_FORWARDED_TO_RUNTIME - ); - let active = request_disputes(sender, RequestType::Active).await; - if active.len() > MAX_DISPUTES_FORWARDED_TO_RUNTIME { - generate_unseen_active_subset(active, onchain) - } else { - generate_active_and_unseen_recent_subset(recent, active, onchain) - } - } else { - recent - }; - - gum::trace!( - target: LOG_TARGET, - relay_paent = ?_leaf.hash, - "Calling `request_votes`" - ); - - // Load all votes for all disputes from the coordinator. - let dispute_candidate_votes = request_votes(sender, disputes).await; - - gum::trace!( - target: LOG_TARGET, - relay_paent = ?_leaf.hash, - "Finished `request_votes`" - ); - - // Transform all `CandidateVotes` into `MultiDisputeStatementSet`. - Ok(dispute_candidate_votes - .into_iter() - .map(|(session_index, candidate_hash, votes)| { - let valid_statements = votes - .valid - .into_iter() - .map(|(i, (s, sig))| (DisputeStatement::Valid(s), i, sig)); - - let invalid_statements = votes - .invalid - .into_iter() - .map(|(i, (s, sig))| (DisputeStatement::Invalid(s), i, sig)); - - metrics.inc_valid_statements_by(valid_statements.len()); - metrics.inc_invalid_statements_by(invalid_statements.len()); - metrics.inc_dispute_statement_sets_by(1); - - DisputeStatementSet { - candidate_hash, - session: session_index, - statements: valid_statements.chain(invalid_statements).collect(), - } - }) - .collect()) -} diff --git a/node/core/provisioner/src/metrics.rs b/node/core/provisioner/src/metrics.rs index 829293dfed53..e3991f88d664 100644 --- a/node/core/provisioner/src/metrics.rs +++ b/node/core/provisioner/src/metrics.rs @@ -14,6 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +#[cfg(feature = "staging-client")] +use crate::disputes::with_staging_api::PartitionedDisputes; use polkadot_node_subsystem_util::metrics::{self, prometheus}; #[derive(Clone)] @@ -28,6 +30,10 @@ struct MetricsInner { /// 4 hours on Polkadot. The metrics are updated only when the node authors a block, so values vary across nodes. inherent_data_dispute_statement_sets: prometheus::Counter, inherent_data_dispute_statements: prometheus::CounterVec, + + #[cfg(feature = "staging-client")] + // The disputes received from `disputes-coordinator` by partition + partitioned_disputes: prometheus::CounterVec, } /// Provisioner metrics. @@ -95,6 +101,40 @@ impl Metrics { .inc_by(disputes.try_into().unwrap_or(0)); } } + + #[cfg(feature = "staging-client")] + pub(crate) fn on_partition_recent_disputes(&self, disputes: &PartitionedDisputes) { + if let Some(metrics) = &self.0 { + let PartitionedDisputes { + active_unconcluded_onchain: cant_conclude_onchain, + active_unknown_onchain: unknown_onchain, + active_concluded_onchain: can_conclude_onchain, + inactive_known_onchain: concluded_known_onchain, + inactive_unknown_onchain: concluded_unknown_onchain, + } = disputes; + + metrics + .partitioned_disputes + .with_label_values(&["cant_conclude_onchain"]) + .inc_by(cant_conclude_onchain.len().try_into().unwrap_or(0)); + metrics + .partitioned_disputes + .with_label_values(&["unknown_onchain"]) + .inc_by(unknown_onchain.len().try_into().unwrap_or(0)); + metrics + .partitioned_disputes + .with_label_values(&["can_conclude_onchain"]) + .inc_by(can_conclude_onchain.len().try_into().unwrap_or(0)); + metrics + .partitioned_disputes + .with_label_values(&["concluded_known_onchain"]) + .inc_by(concluded_known_onchain.len().try_into().unwrap_or(0)); + metrics + .partitioned_disputes + .with_label_values(&["unknown_onchain"]) + .inc_by(concluded_unknown_onchain.len().try_into().unwrap_or(0)); + } + } } impl metrics::Metrics for Metrics { @@ -150,6 +190,17 @@ impl metrics::Metrics for Metrics { )?, registry, )?, + #[cfg(feature = "staging-client")] + partitioned_disputes: prometheus::register( + prometheus::CounterVec::new( + prometheus::Opts::new( + "polkadot_parachain_provisioner_partitioned_disputes", + "some fancy description", + ), + &["partition"], + )?, + ®istry, + )?, }; Ok(Metrics(Some(metrics))) } diff --git a/node/core/provisioner/src/onchain_disputes.rs b/node/core/provisioner/src/onchain_disputes.rs deleted file mode 100644 index 6810f512173f..000000000000 --- a/node/core/provisioner/src/onchain_disputes.rs +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2017-2022 Parity Technologies (UK) Ltd. -// This file is part of Polkadot. - -// Polkadot is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Polkadot is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Polkadot. If not, see . - -use crate::error::GetOnchainDisputesError; -use polkadot_node_subsystem::overseer; -use polkadot_primitives::v2::{CandidateHash, DisputeState, Hash, SessionIndex}; -use std::collections::HashMap; - -pub async fn get_onchain_disputes( - _sender: &mut Sender, - _relay_parent: Hash, -) -> Result, GetOnchainDisputesError> -where - Sender: overseer::ProvisionerSenderTrait, -{ - let _onchain = Result::< - HashMap<(SessionIndex, CandidateHash), DisputeState>, - GetOnchainDisputesError, - >::Ok(HashMap::new()); - #[cfg(feature = "staging-client")] - let _onchain = self::staging_impl::get_onchain_disputes(_sender, _relay_parent).await; - - _onchain -} - -// Merge this module with the outer (current one) when promoting to stable -#[cfg(feature = "staging-client")] -mod staging_impl { - use super::*; // remove this when promoting to stable - use crate::LOG_TARGET; - use futures::channel::oneshot; - use polkadot_node_subsystem::{ - errors::RuntimeApiError, - messages::{RuntimeApiMessage, RuntimeApiRequest}, - SubsystemSender, - }; - - /// Gets the on-chain disputes at a given block number and returns them as a `HashSet` so that searching in them is cheap. - pub async fn get_onchain_disputes( - sender: &mut impl SubsystemSender, - relay_parent: Hash, - ) -> Result, GetOnchainDisputesError> { - gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching on-chain disputes"); - let (tx, rx) = oneshot::channel(); - sender - .send_message( - RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::StagingDisputes(tx)) - .into(), - ) - .await; - - rx.await - .map_err(|_| GetOnchainDisputesError::Channel) - .and_then(|res| { - res.map_err(|e| match e { - RuntimeApiError::Execution { .. } => - GetOnchainDisputesError::Execution(e, relay_parent), - RuntimeApiError::NotSupported { .. } => - GetOnchainDisputesError::NotSupported(e, relay_parent), - }) - }) - .map(|v| v.into_iter().map(|e| ((e.0, e.1), e.2)).collect()) - } -} diff --git a/node/core/provisioner/src/tests.rs b/node/core/provisioner/src/tests.rs index d0ca425210ed..08eba8eabe80 100644 --- a/node/core/provisioner/src/tests.rs +++ b/node/core/provisioner/src/tests.rs @@ -195,7 +195,7 @@ mod select_availability_bitfields { } } -mod common { +pub(crate) mod common { use super::super::*; use futures::channel::mpsc; use polkadot_node_subsystem::messages::AllMessages; @@ -497,403 +497,3 @@ mod select_candidates { ) } } - -mod select_disputes { - use super::{super::*, common::test_harness}; - use futures::channel::mpsc; - use polkadot_node_subsystem::{ - messages::{AllMessages, DisputeCoordinatorMessage, RuntimeApiMessage, RuntimeApiRequest}, - RuntimeApiError, - }; - use polkadot_node_subsystem_test_helpers::TestSubsystemSender; - use polkadot_primitives::v2::DisputeState; - use std::sync::Arc; - use test_helpers; - - // Global Test Data - fn recent_disputes(len: usize) -> Vec<(SessionIndex, CandidateHash)> { - let mut res = Vec::with_capacity(len); - for _ in 0..len { - res.push((0, CandidateHash(Hash::random()))); - } - - res - } - - // same as recent_disputes() but with SessionIndex set to 1 - fn active_disputes(len: usize) -> Vec<(SessionIndex, CandidateHash)> { - let mut res = Vec::with_capacity(len); - for _ in 0..len { - res.push((1, CandidateHash(Hash::random()))); - } - - res - } - - fn leaf() -> ActivatedLeaf { - ActivatedLeaf { - hash: Hash::repeat_byte(0xAA), - number: 0xAA, - status: LeafStatus::Fresh, - span: Arc::new(jaeger::Span::Disabled), - } - } - - async fn mock_overseer( - leaf: ActivatedLeaf, - mut receiver: mpsc::UnboundedReceiver, - onchain_disputes: Result, RuntimeApiError>, - recent_disputes: Vec<(SessionIndex, CandidateHash)>, - active_disputes: Vec<(SessionIndex, CandidateHash)>, - ) { - while let Some(from_job) = receiver.next().await { - match from_job { - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _, - RuntimeApiRequest::StagingDisputes(sender), - )) => { - let _ = sender.send(onchain_disputes.clone()); - }, - AllMessages::RuntimeApi(_) => panic!("Unexpected RuntimeApi request"), - AllMessages::DisputeCoordinator(DisputeCoordinatorMessage::RecentDisputes( - sender, - )) => { - let _ = sender.send(recent_disputes.clone()); - }, - AllMessages::DisputeCoordinator(DisputeCoordinatorMessage::ActiveDisputes( - sender, - )) => { - let _ = sender.send(active_disputes.clone()); - }, - AllMessages::DisputeCoordinator( - DisputeCoordinatorMessage::QueryCandidateVotes(disputes, sender), - ) => { - let mut res = Vec::new(); - let v = CandidateVotes { - candidate_receipt: test_helpers::dummy_candidate_receipt(leaf.hash.clone()), - valid: BTreeMap::new(), - invalid: BTreeMap::new(), - }; - for r in disputes.iter() { - res.push((r.0, r.1, v.clone())); - } - - let _ = sender.send(res); - }, - _ => panic!("Unexpected message: {:?}", from_job), - } - } - } - - #[test] - fn recent_disputes_are_withing_onchain_limit() { - const RECENT_DISPUTES_SIZE: usize = 10; - let metrics = metrics::Metrics::new_dummy(); - let onchain_disputes = Ok(Vec::new()); - let active_disputes = Vec::new(); - let recent_disputes = recent_disputes(RECENT_DISPUTES_SIZE); - - let recent_disputes_overseer = recent_disputes.clone(); - test_harness( - |r| { - mock_overseer( - leaf(), - r, - onchain_disputes, - recent_disputes_overseer, - active_disputes, - ) - }, - |mut tx: TestSubsystemSender| async move { - let lf = leaf(); - let disputes = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); - - assert!(!disputes.is_empty()); - - let result = disputes.iter().zip(recent_disputes.iter()); - // We should get all recent disputes. - for (d, r) in result { - assert_eq!(d.session, r.0); - assert_eq!(d.candidate_hash, r.1); - } - }, - ) - } - - #[test] - fn recent_disputes_are_too_much_but_active_are_within_limit() { - const RECENT_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 10; - const ACTIVE_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME; - let metrics = metrics::Metrics::new_dummy(); - let onchain_disputes = Ok(Vec::new()); - let recent_disputes = recent_disputes(RECENT_DISPUTES_SIZE); - let active_disputes = active_disputes(ACTIVE_DISPUTES_SIZE); - - let active_disputes_overseer = active_disputes.clone(); - test_harness( - |r| { - mock_overseer( - leaf(), - r, - onchain_disputes, - recent_disputes, - active_disputes_overseer, - ) - }, - |mut tx: TestSubsystemSender| async move { - let lf = leaf(); - let disputes = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); - - assert!(!disputes.is_empty()); - - let result = disputes.iter().zip(active_disputes.iter()); - // We should get all active disputes. - for (d, r) in result { - assert_eq!(d.session, r.0); - assert_eq!(d.candidate_hash, r.1); - } - }, - ) - } - - #[test] - fn recent_disputes_are_too_much_but_active_are_less_than_the_limit() { - // In this case all active disputes + a random set of recent disputes should be returned - const RECENT_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 10; - const ACTIVE_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME - 10; - let metrics = metrics::Metrics::new_dummy(); - let onchain_disputes = Ok(Vec::new()); - let recent_disputes = recent_disputes(RECENT_DISPUTES_SIZE); - let active_disputes = active_disputes(ACTIVE_DISPUTES_SIZE); - - let active_disputes_overseer = active_disputes.clone(); - test_harness( - |r| { - mock_overseer( - leaf(), - r, - onchain_disputes, - recent_disputes, - active_disputes_overseer, - ) - }, - |mut tx: TestSubsystemSender| async move { - let lf = leaf(); - let disputes = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); - - assert!(!disputes.is_empty()); - - // Recent disputes are generated with `SessionIndex` = 0 - let (res_recent, res_active): (Vec, Vec) = - disputes.into_iter().partition(|d| d.session == 0); - - // It should be good enough the count the number of active disputes and not compare them one by one. Checking the exact values is already covered by the previous tests. - assert_eq!(res_active.len(), active_disputes.len()); // We have got all active disputes - assert_eq!(res_active.len() + res_recent.len(), MAX_DISPUTES_FORWARDED_TO_RUNTIME); - // And some recent ones. - }, - ) - } - - //These tests rely on staging Runtime functions so they are separated and compiled conditionally. - #[cfg(feature = "staging-client")] - mod staging_tests { - use super::*; - - fn dummy_dispute_state() -> DisputeState { - DisputeState { - validators_for: BitVec::new(), - validators_against: BitVec::new(), - start: 0, - concluded_at: None, - } - } - - #[test] - fn recent_disputes_are_too_much_active_fits_test_recent_prioritisation() { - // In this case recent disputes are above `MAX_DISPUTES_FORWARDED_TO_RUNTIME` limit and the active ones are below it. - // The expected behaviour is to send all active disputes and extend the set with recent ones. During the extension the disputes unknown for the Runtime are added with priority. - const RECENT_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 10; - const ACTIVE_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME - 10; - const ONCHAIN_DISPUTE_SIZE: usize = RECENT_DISPUTES_SIZE - 9; - let metrics = metrics::Metrics::new_dummy(); - let recent_disputes = recent_disputes(RECENT_DISPUTES_SIZE); - let active_disputes = active_disputes(ACTIVE_DISPUTES_SIZE); - let onchain_disputes: Result< - Vec<(SessionIndex, CandidateHash, DisputeState)>, - RuntimeApiError, - > = Ok(Vec::from(&recent_disputes[0..ONCHAIN_DISPUTE_SIZE]) - .iter() - .map(|(session_index, candidate_hash)| { - (*session_index, candidate_hash.clone(), dummy_dispute_state()) - }) - .collect()); - let active_disputes_overseer = active_disputes.clone(); - let recent_disputes_overseer = recent_disputes.clone(); - test_harness( - |r| { - mock_overseer( - leaf(), - r, - onchain_disputes, - recent_disputes_overseer, - active_disputes_overseer, - ) - }, - |mut tx: TestSubsystemSender| async move { - let lf = leaf(); - let disputes = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); - - assert!(!disputes.is_empty()); - - // Recent disputes are generated with `SessionIndex` = 0 - let (res_recent, res_active): ( - Vec, - Vec, - ) = disputes.into_iter().partition(|d| d.session == 0); - - // It should be good enough the count the number of the disputes and not compare them one by one as this was already covered in other tests. - assert_eq!(res_active.len(), active_disputes.len()); // We've got all active disputes. - assert_eq!( - res_recent.len(), - MAX_DISPUTES_FORWARDED_TO_RUNTIME - active_disputes.len() - ); // And some recent ones. - - // Check if the recent disputes were unknown for the Runtime. - let expected_recent_disputes = - Vec::from(&recent_disputes[ONCHAIN_DISPUTE_SIZE..]); - let res_recent_set: HashSet<(SessionIndex, CandidateHash)> = HashSet::from_iter( - res_recent.iter().map(|d| (d.session, d.candidate_hash)), - ); - - // Explicitly check that all unseen disputes are sent to the Runtime. - for d in &expected_recent_disputes { - assert_eq!(res_recent_set.contains(d), true); - } - }, - ) - } - - #[test] - fn active_disputes_are_too_much_test_active_prioritisation() { - // In this case the active disputes are above the `MAX_DISPUTES_FORWARDED_TO_RUNTIME` limit so the unseen ones should be prioritised. - const RECENT_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 10; - const ACTIVE_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 10; - const ONCHAIN_DISPUTE_SIZE: usize = ACTIVE_DISPUTES_SIZE - 9; - - let metrics = metrics::Metrics::new_dummy(); - let recent_disputes = recent_disputes(RECENT_DISPUTES_SIZE); - let active_disputes = active_disputes(ACTIVE_DISPUTES_SIZE); - let onchain_disputes: Result< - Vec<(SessionIndex, CandidateHash, DisputeState)>, - RuntimeApiError, - > = Ok(Vec::from(&active_disputes[0..ONCHAIN_DISPUTE_SIZE]) - .iter() - .map(|(session_index, candidate_hash)| { - (*session_index, candidate_hash.clone(), dummy_dispute_state()) - }) - .collect()); - let active_disputes_overseer = active_disputes.clone(); - let recent_disputes_overseer = recent_disputes.clone(); - test_harness( - |r| { - mock_overseer( - leaf(), - r, - onchain_disputes, - recent_disputes_overseer, - active_disputes_overseer, - ) - }, - |mut tx: TestSubsystemSender| async move { - let lf = leaf(); - let disputes = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); - - assert!(!disputes.is_empty()); - - // Recent disputes are generated with `SessionIndex` = 0 - let (res_recent, res_active): ( - Vec, - Vec, - ) = disputes.into_iter().partition(|d| d.session == 0); - - // It should be good enough the count the number of the disputes and not compare them one by one - assert_eq!(res_recent.len(), 0); // We expect no recent disputes - assert_eq!(res_active.len(), MAX_DISPUTES_FORWARDED_TO_RUNTIME); - - let expected_active_disputes = - Vec::from(&active_disputes[ONCHAIN_DISPUTE_SIZE..]); - let res_active_set: HashSet<(SessionIndex, CandidateHash)> = HashSet::from_iter( - res_active.iter().map(|d| (d.session, d.candidate_hash)), - ); - - // Explicitly check that the unseen disputes are delivered to the Runtime. - for d in &expected_active_disputes { - assert_eq!(res_active_set.contains(d), true); - } - }, - ) - } - - #[test] - fn active_disputes_are_too_much_and_are_all_unseen() { - // In this case there are a lot of active disputes unseen by the Runtime. The focus of the test is to verify that in such cases known disputes are NOT sent to the Runtime. - const RECENT_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 10; - const ACTIVE_DISPUTES_SIZE: usize = MAX_DISPUTES_FORWARDED_TO_RUNTIME + 5; - const ONCHAIN_DISPUTE_SIZE: usize = 5; - - let metrics = metrics::Metrics::new_dummy(); - let recent_disputes = recent_disputes(RECENT_DISPUTES_SIZE); - let active_disputes = active_disputes(ACTIVE_DISPUTES_SIZE); - let onchain_disputes: Result< - Vec<(SessionIndex, CandidateHash, DisputeState)>, - RuntimeApiError, - > = Ok(Vec::from(&active_disputes[0..ONCHAIN_DISPUTE_SIZE]) - .iter() - .map(|(session_index, candidate_hash)| { - (*session_index, candidate_hash.clone(), dummy_dispute_state()) - }) - .collect()); - let active_disputes_overseer = active_disputes.clone(); - let recent_disputes_overseer = recent_disputes.clone(); - test_harness( - |r| { - mock_overseer( - leaf(), - r, - onchain_disputes, - recent_disputes_overseer, - active_disputes_overseer, - ) - }, - |mut tx: TestSubsystemSender| async move { - let lf = leaf(); - let disputes = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); - assert!(!disputes.is_empty()); - - // Recent disputes are generated with `SessionIndex` = 0 - let (res_recent, res_active): ( - Vec, - Vec, - ) = disputes.into_iter().partition(|d| d.session == 0); - - // It should be good enough the count the number of the disputes and not compare them one by one - assert_eq!(res_recent.len(), 0); - assert_eq!(res_active.len(), MAX_DISPUTES_FORWARDED_TO_RUNTIME); - - // For sure we don't want to see any of this disputes in the result - let unexpected_active_disputes = - Vec::from(&active_disputes[0..ONCHAIN_DISPUTE_SIZE]); - let res_active_set: HashSet<(SessionIndex, CandidateHash)> = HashSet::from_iter( - res_active.iter().map(|d| (d.session, d.candidate_hash)), - ); - - // Verify that the result DOESN'T contain known disputes (because there is an excessive number of unknown onces). - for d in &unexpected_active_disputes { - assert_eq!(res_active_set.contains(d), false); - } - }, - ) - } - } -} diff --git a/node/network/approval-distribution/src/tests.rs b/node/network/approval-distribution/src/tests.rs index 90827f7d9e44..a96a89bb58eb 100644 --- a/node/network/approval-distribution/src/tests.rs +++ b/node/network/approval-distribution/src/tests.rs @@ -23,9 +23,9 @@ use polkadot_node_primitives::approval::{ }; use polkadot_node_subsystem::messages::{network_bridge_event, AllMessages, ApprovalCheckError}; use polkadot_node_subsystem_test_helpers as test_helpers; -use polkadot_primitives_test_helpers::dummy_signature; use polkadot_node_subsystem_util::TimeoutExt as _; use polkadot_primitives::v2::{AuthorityDiscoveryId, BlakeTwo256, HashT}; +use polkadot_primitives_test_helpers::dummy_signature; use rand::SeedableRng; use sp_authority_discovery::AuthorityPair as AuthorityDiscoveryPair; use sp_core::crypto::Pair as PairT; diff --git a/node/primitives/src/disputes/mod.rs b/node/primitives/src/disputes/mod.rs index 051ec0233258..ee047c7bcc22 100644 --- a/node/primitives/src/disputes/mod.rs +++ b/node/primitives/src/disputes/mod.rs @@ -31,7 +31,7 @@ use polkadot_primitives::v2::{ mod message; pub use message::{DisputeMessage, Error as DisputeMessageCheckError, UncheckedDisputeMessage}; mod status; -pub use status::{DisputeStatus, Timestamp}; +pub use status::{dispute_is_inactive, DisputeStatus, Timestamp, ACTIVE_DURATION_SECS}; /// A checked dispute statement from an associated validator. #[derive(Debug, Clone)] diff --git a/node/primitives/src/disputes/status.rs b/node/primitives/src/disputes/status.rs index 14ebcf0855d9..52d003ab7c7f 100644 --- a/node/primitives/src/disputes/status.rs +++ b/node/primitives/src/disputes/status.rs @@ -110,3 +110,18 @@ impl DisputeStatus { } } } + +/// The choice here is fairly arbitrary. But any dispute that concluded more than a few minutes ago +/// is not worth considering anymore. Changing this value has little to no bearing on consensus, +/// and really only affects the work that the node might do on startup during periods of many +/// disputes. +pub const ACTIVE_DURATION_SECS: Timestamp = 180; + +/// Checks if dispute is inactive. Returns true if EITHER of the following statements is valid: +/// - The dispute has concluded OR +/// - The dispute has been active for duration more than ACTIVE_DURATION_SECS +pub fn dispute_is_inactive(status: &DisputeStatus, now: &Timestamp) -> bool { + let at = status.concluded_at(); + + at.is_some() && at.unwrap() + ACTIVE_DURATION_SECS < *now +} diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 17bc5129a664..895ed2732024 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -46,8 +46,9 @@ pub mod approval; /// Disputes related types. pub mod disputes; pub use disputes::{ - CandidateVotes, DisputeMessage, DisputeMessageCheckError, DisputeStatus, InvalidDisputeVote, - SignedDisputeStatement, Timestamp, UncheckedDisputeMessage, ValidDisputeVote, + dispute_is_inactive, CandidateVotes, DisputeMessage, DisputeMessageCheckError, DisputeStatus, + InvalidDisputeVote, SignedDisputeStatement, Timestamp, UncheckedDisputeMessage, + ValidDisputeVote, ACTIVE_DURATION_SECS, }; // For a 16-ary Merkle Prefix Trie, we can expect at most 16 32-byte hashes per node From 040d1fbe93de41b1d96e8b89763ec8bdce3d82f0 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 24 Aug 2022 11:50:50 +0300 Subject: [PATCH 05/35] Fix staging api usage --- node/core/provisioner/src/disputes/mod.rs | 2 - .../src/disputes/with_staging_api/tests.rs | 88 +++++++++---------- node/core/provisioner/src/lib.rs | 69 +++++++++++++-- node/core/provisioner/src/metrics.rs | 4 - 4 files changed, 102 insertions(+), 61 deletions(-) diff --git a/node/core/provisioner/src/disputes/mod.rs b/node/core/provisioner/src/disputes/mod.rs index f36f35671857..f89d2b974809 100644 --- a/node/core/provisioner/src/disputes/mod.rs +++ b/node/core/provisioner/src/disputes/mod.rs @@ -46,8 +46,6 @@ async fn request_votes( } } -#[cfg(feature = "staging-client")] pub(crate) mod with_staging_api; -#[cfg(not(feature = "staging-client"))] pub(crate) mod without_staging_api; diff --git a/node/core/provisioner/src/disputes/with_staging_api/tests.rs b/node/core/provisioner/src/disputes/with_staging_api/tests.rs index bfd896a6534a..6eb74cb044d4 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/tests.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/tests.rs @@ -503,8 +503,8 @@ fn normal_flow() { //concluded disputes unknown onchain let (fifth_idx, fifth_votes) = input.add_concluded_disputes_unknown_onchain(DISPUTES_PER_BATCH); - // concluded disputes known onchain - let (fourth_idx, _) = input.add_concluded_disputes_known_onchain(DISPUTES_PER_BATCH); + // concluded disputes known onchain - these should be ignored + let (_, _) = input.add_concluded_disputes_known_onchain(DISPUTES_PER_BATCH); // active disputes unknown onchain let (second_idx, second_votes) = @@ -520,52 +520,44 @@ fn normal_flow() { assert!(!result.is_empty()); - if cfg!(feature = "staging-client") { - assert_eq!(result.len(), 4 * DISPUTES_PER_BATCH); - - // Naive checks that the result is partitioned correctly - let (first_batch, rest): (Vec, Vec) = - result.into_iter().partition(|d| d.session == first_idx); - assert_eq!(first_batch.len(), DISPUTES_PER_BATCH); - - let (second_batch, rest): (Vec, Vec) = - rest.into_iter().partition(|d| d.session == second_idx); - assert_eq!(second_batch.len(), DISPUTES_PER_BATCH); - - let (third_batch, rest): (Vec, Vec) = - rest.into_iter().partition(|d| d.session == third_idx); - assert_eq!(third_batch.len(), DISPUTES_PER_BATCH); - - let (fifth_batch, rest): (Vec, Vec) = - rest.into_iter().partition(|d| d.session == fifth_idx); - assert_eq!(fifth_batch.len(), DISPUTES_PER_BATCH); - - // Ensure there are no more disputes - fourth_batch should be dropped - assert_eq!(rest.len(), 0); - - assert_eq!( - first_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), - first_votes - ); - assert_eq!( - second_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), - second_votes - ); - assert_eq!( - third_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), - third_votes - ); - assert_eq!( - fifth_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), - fifth_votes - ); - } else { - assert_eq!(result.len(), 5 * DISPUTES_PER_BATCH); - for i in 0..3 * DISPUTES_PER_BATCH { - assert_ne!(result.get(i).unwrap().session, fourth_idx); - assert_ne!(result.get(i).unwrap().session, fifth_idx); - } - } + assert_eq!(result.len(), 4 * DISPUTES_PER_BATCH); + + // Naive checks that the result is partitioned correctly + let (first_batch, rest): (Vec, Vec) = + result.into_iter().partition(|d| d.session == first_idx); + assert_eq!(first_batch.len(), DISPUTES_PER_BATCH); + + let (second_batch, rest): (Vec, Vec) = + rest.into_iter().partition(|d| d.session == second_idx); + assert_eq!(second_batch.len(), DISPUTES_PER_BATCH); + + let (third_batch, rest): (Vec, Vec) = + rest.into_iter().partition(|d| d.session == third_idx); + assert_eq!(third_batch.len(), DISPUTES_PER_BATCH); + + let (fifth_batch, rest): (Vec, Vec) = + rest.into_iter().partition(|d| d.session == fifth_idx); + assert_eq!(fifth_batch.len(), DISPUTES_PER_BATCH); + + // Ensure there are no more disputes - fourth_batch should be dropped + assert_eq!(rest.len(), 0); + + assert_eq!( + first_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + first_votes + ); + assert_eq!( + second_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + second_votes + ); + assert_eq!( + third_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + third_votes + ); + assert_eq!( + fifth_batch.iter().map(|d| d.statements.len()).fold(0, |acc, v| acc + v), + fifth_votes + ); }, ); assert!(vote_queries <= ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT); diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index 9c1e3605e1b6..5b80cda09741 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -29,10 +29,10 @@ use polkadot_node_subsystem::{ jaeger, messages::{ CandidateBackingMessage, ChainApiMessage, ProvisionableData, ProvisionerInherentData, - ProvisionerMessage, + ProvisionerMessage, RuntimeApiMessage, RuntimeApiRequest, }, overseer, ActivatedLeaf, ActiveLeavesUpdate, FromOrchestra, LeafStatus, OverseerSignal, - PerLeafSpan, SpawnedSubsystem, SubsystemError, + PerLeafSpan, RuntimeApiError, SpawnedSubsystem, SubsystemError, }; use polkadot_node_subsystem_util::{ request_availability_cores, request_persisted_validation_data, TimeoutExt, @@ -60,6 +60,8 @@ const SEND_INHERENT_DATA_TIMEOUT: std::time::Duration = core::time::Duration::fr const LOG_TARGET: &str = "parachain::provisioner"; +const STAGING_RUNTIME_VERSION_REQUIREMENT: u32 = 3; + /// The provisioner subsystem. pub struct ProvisionerSubsystem { metrics: Metrics, @@ -360,11 +362,13 @@ async fn send_inherent_data( "Selecting disputes" ); - #[cfg(feature = "staging-client")] - let disputes = disputes::with_staging_api::select_disputes(from_job, metrics, leaf).await?; - - #[cfg(not(feature = "staging-client"))] - let disputes = disputes::without_staging_api::select_disputes(from_job, metrics).await?; + let disputes = + match has_staging_runtime(from_job, leaf.hash.clone(), STAGING_RUNTIME_VERSION_REQUIREMENT) + .await + { + true => disputes::with_staging_api::select_disputes(from_job, metrics, leaf).await?, + false => disputes::without_staging_api::select_disputes(from_job, metrics).await?, + }; gum::trace!( target: LOG_TARGET, @@ -680,3 +684,54 @@ fn bitfields_indicate_availability( 3 * availability.count_ones() >= 2 * availability.len() } + +async fn has_staging_runtime( + sender: &mut impl overseer::ProvisionerSenderTrait, + relay_parent: Hash, + required_runtime_version: u32, +) -> bool { + gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching runtime version"); + + let (tx, rx) = oneshot::channel(); + sender + .send_message(RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::Version(tx))) + .await; + + match rx.await { + Result::Ok(Ok(runtime_version)) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + ?runtime_version, + ?required_runtime_version, + "Fetched runtime version" + ); + runtime_version >= required_runtime_version + }, + Result::Ok(Err(RuntimeApiError::Execution { source: error, .. })) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + ?error, + "Execution error while fetching runtime version" + ); + false + }, + Result::Ok(Err(RuntimeApiError::NotSupported { .. })) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + "NotSupported error while fetching runtime version" + ); + false + }, + Result::Err(_) => { + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + "Cancelled error while fetching runtime version" + ); + false + }, + } +} diff --git a/node/core/provisioner/src/metrics.rs b/node/core/provisioner/src/metrics.rs index e3991f88d664..23c3a84de228 100644 --- a/node/core/provisioner/src/metrics.rs +++ b/node/core/provisioner/src/metrics.rs @@ -14,7 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -#[cfg(feature = "staging-client")] use crate::disputes::with_staging_api::PartitionedDisputes; use polkadot_node_subsystem_util::metrics::{self, prometheus}; @@ -31,7 +30,6 @@ struct MetricsInner { inherent_data_dispute_statement_sets: prometheus::Counter, inherent_data_dispute_statements: prometheus::CounterVec, - #[cfg(feature = "staging-client")] // The disputes received from `disputes-coordinator` by partition partitioned_disputes: prometheus::CounterVec, } @@ -102,7 +100,6 @@ impl Metrics { } } - #[cfg(feature = "staging-client")] pub(crate) fn on_partition_recent_disputes(&self, disputes: &PartitionedDisputes) { if let Some(metrics) = &self.0 { let PartitionedDisputes { @@ -190,7 +187,6 @@ impl metrics::Metrics for Metrics { )?, registry, )?, - #[cfg(feature = "staging-client")] partitioned_disputes: prometheus::register( prometheus::CounterVec::new( prometheus::Opts::new( From b8f65667da168be07d47fa6bca89b0a1d98cfe51 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 24 Aug 2022 15:39:48 +0300 Subject: [PATCH 06/35] Fix `get_disputes` runtime function implementation --- node/core/runtime-api/src/lib.rs | 2 +- node/core/runtime-api/src/tests.rs | 4 ---- node/subsystem-types/src/runtime_client.rs | 6 +++--- primitives/src/runtime_api.rs | 4 ++-- runtime/kusama/src/lib.rs | 13 ++++--------- runtime/polkadot/src/lib.rs | 13 ++++--------- runtime/rococo/src/lib.rs | 3 ++- runtime/test-runtime/src/lib.rs | 4 ---- runtime/westend/src/lib.rs | 3 ++- 9 files changed, 18 insertions(+), 34 deletions(-) diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index a815b76a8d7c..3ee143d87918 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -500,6 +500,6 @@ where Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), Request::StagingDisputes(sender) => - query!(StagingDisputes, staging_get_disputes(), ver = 2, sender), + query!(StagingDisputes, get_disputes(), ver = 2, sender), } } diff --git a/node/core/runtime-api/src/tests.rs b/node/core/runtime-api/src/tests.rs index b1a1bba73769..563749319d3d 100644 --- a/node/core/runtime-api/src/tests.rs +++ b/node/core/runtime-api/src/tests.rs @@ -196,10 +196,6 @@ sp_api::mock_impl_runtime_apis! { ) -> Option { self.validation_code_hash.get(¶).map(|c| c.clone()) } - - fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { - unimplemented!() - } } impl BabeApi for MockRuntimeApi { diff --git a/node/subsystem-types/src/runtime_client.rs b/node/subsystem-types/src/runtime_client.rs index 2aa9e2bffb82..b7e458fdd6d1 100644 --- a/node/subsystem-types/src/runtime_client.rs +++ b/node/subsystem-types/src/runtime_client.rs @@ -186,7 +186,7 @@ pub trait RuntimeApiSubsystemClient { /// Returns all onchain disputes. /// This is a staging method! Do not use on production runtimes! - async fn staging_get_disputes( + async fn get_disputes( &self, at: Hash, ) -> Result)>, ApiError>; @@ -375,10 +375,10 @@ where self.runtime_api().session_info_before_version_2(&BlockId::Hash(at), index) } - async fn staging_get_disputes( + async fn get_disputes( &self, at: Hash, ) -> Result)>, ApiError> { - self.runtime_api().staging_get_disputes(&BlockId::Hash(at)) + self.runtime_api().get_disputes(&BlockId::Hash(at)) } } diff --git a/primitives/src/runtime_api.rs b/primitives/src/runtime_api.rs index 84d2cf0ec4ca..2c95b2b0eb26 100644 --- a/primitives/src/runtime_api.rs +++ b/primitives/src/runtime_api.rs @@ -153,7 +153,7 @@ sp_api::decl_runtime_apis! { /***** STAGING *****/ /// Returns all onchain disputes. - /// This is a staging method! Do not use on production runtimes! - fn staging_get_disputes() -> Vec<(v2::SessionIndex, v2::CandidateHash, v2::DisputeState)>; + #[api_version(3)] + fn get_disputes() -> Vec<(v2::SessionIndex, v2::CandidateHash, v2::DisputeState)>; } } diff --git a/runtime/kusama/src/lib.rs b/runtime/kusama/src/lib.rs index 250dcfa6868a..330040b343f1 100644 --- a/runtime/kusama/src/lib.rs +++ b/runtime/kusama/src/lib.rs @@ -23,11 +23,10 @@ use pallet_transaction_payment::CurrencyAdapter; use parity_scale_codec::{Decode, Encode, MaxEncodedLen}; use primitives::v2::{ - AccountId, AccountIndex, Balance, BlockNumber, CandidateEvent, CandidateHash, - CommittedCandidateReceipt, CoreState, DisputeState, GroupRotationInfo, Hash, Id as ParaId, - InboundDownwardMessage, InboundHrmpMessage, Moment, Nonce, OccupiedCoreAssumption, - PersistedValidationData, ScrapedOnChainVotes, SessionInfo, Signature, ValidationCode, - ValidationCodeHash, ValidatorId, ValidatorIndex, + AccountId, AccountIndex, Balance, BlockNumber, CandidateEvent, CommittedCandidateReceipt, + CoreState, GroupRotationInfo, Hash, Id as ParaId, InboundDownwardMessage, InboundHrmpMessage, + Moment, Nonce, OccupiedCoreAssumption, PersistedValidationData, ScrapedOnChainVotes, + SessionInfo, Signature, ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, }; use runtime_common::{ auctions, claims, crowdloan, impl_runtime_weights, impls::DealWithFees, paras_registrar, @@ -1670,10 +1669,6 @@ sp_api::impl_runtime_apis! { { parachains_runtime_api_impl::validation_code_hash::(para_id, assumption) } - - fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { - unimplemented!() - } } impl beefy_primitives::BeefyApi for Runtime { diff --git a/runtime/polkadot/src/lib.rs b/runtime/polkadot/src/lib.rs index d429ea849915..c0fc2ced9f23 100644 --- a/runtime/polkadot/src/lib.rs +++ b/runtime/polkadot/src/lib.rs @@ -54,11 +54,10 @@ use pallet_session::historical as session_historical; use pallet_transaction_payment::{FeeDetails, RuntimeDispatchInfo}; use parity_scale_codec::{Decode, Encode, MaxEncodedLen}; use primitives::v2::{ - AccountId, AccountIndex, Balance, BlockNumber, CandidateEvent, CandidateHash, - CommittedCandidateReceipt, CoreState, DisputeState, GroupRotationInfo, Hash, Id as ParaId, - InboundDownwardMessage, InboundHrmpMessage, Moment, Nonce, OccupiedCoreAssumption, - PersistedValidationData, ScrapedOnChainVotes, SessionInfo, Signature, ValidationCode, - ValidationCodeHash, ValidatorId, ValidatorIndex, + AccountId, AccountIndex, Balance, BlockNumber, CandidateEvent, CommittedCandidateReceipt, + CoreState, GroupRotationInfo, Hash, Id as ParaId, InboundDownwardMessage, InboundHrmpMessage, + Moment, Nonce, OccupiedCoreAssumption, PersistedValidationData, ScrapedOnChainVotes, + SessionInfo, Signature, ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, }; use sp_core::OpaqueMetadata; use sp_mmr_primitives as mmr; @@ -1799,10 +1798,6 @@ sp_api::impl_runtime_apis! { { parachains_runtime_api_impl::validation_code_hash::(para_id, assumption) } - - fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { - unimplemented!() - } } impl beefy_primitives::BeefyApi for Runtime { diff --git a/runtime/rococo/src/lib.rs b/runtime/rococo/src/lib.rs index a4cace51c42b..48e9552af023 100644 --- a/runtime/rococo/src/lib.rs +++ b/runtime/rococo/src/lib.rs @@ -1004,6 +1004,7 @@ sp_api::impl_runtime_apis! { } } + #[api_version(3)] impl primitives::runtime_api::ParachainHost for Runtime { fn validators() -> Vec { runtime_api_impl::validators::() @@ -1099,7 +1100,7 @@ sp_api::impl_runtime_apis! { runtime_api_impl::validation_code_hash::(para_id, assumption) } - fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { + fn get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::() } } diff --git a/runtime/test-runtime/src/lib.rs b/runtime/test-runtime/src/lib.rs index d18a2c9bb95c..d5bdbf1e0b07 100644 --- a/runtime/test-runtime/src/lib.rs +++ b/runtime/test-runtime/src/lib.rs @@ -905,10 +905,6 @@ sp_api::impl_runtime_apis! { { runtime_impl::validation_code_hash::(para_id, assumption) } - - fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { - polkadot_runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::() - } } impl beefy_primitives::BeefyApi for Runtime { diff --git a/runtime/westend/src/lib.rs b/runtime/westend/src/lib.rs index 9bdde2ea5f38..db06daa5fa96 100644 --- a/runtime/westend/src/lib.rs +++ b/runtime/westend/src/lib.rs @@ -1276,6 +1276,7 @@ sp_api::impl_runtime_apis! { } } + #[api_version(3)] impl primitives::runtime_api::ParachainHost for Runtime { fn validators() -> Vec { parachains_runtime_api_impl::validators::() @@ -1374,7 +1375,7 @@ sp_api::impl_runtime_apis! { parachains_runtime_api_impl::validation_code_hash::(para_id, assumption) } - fn staging_get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { + fn get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::() } } From 1be798383da114cefdf1c51661e17ebf6acf8a28 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 24 Aug 2022 17:11:01 +0300 Subject: [PATCH 07/35] Fix compilation error --- node/core/runtime-api/src/tests.rs | 10 +++++----- runtime/test-runtime/src/lib.rs | 11 +++++------ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/node/core/runtime-api/src/tests.rs b/node/core/runtime-api/src/tests.rs index 563749319d3d..9c14561b1f48 100644 --- a/node/core/runtime-api/src/tests.rs +++ b/node/core/runtime-api/src/tests.rs @@ -23,11 +23,11 @@ use polkadot_node_subsystem_test_helpers::make_subsystem_context; use polkadot_primitives::{ runtime_api::ParachainHost, v2::{ - AuthorityDiscoveryId, Block, BlockNumber, CandidateEvent, CandidateHash, - CommittedCandidateReceipt, CoreState, DisputeState, GroupRotationInfo, Id as ParaId, - InboundDownwardMessage, InboundHrmpMessage, OccupiedCoreAssumption, - PersistedValidationData, PvfCheckStatement, ScrapedOnChainVotes, SessionIndex, SessionInfo, - ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, + AuthorityDiscoveryId, Block, CandidateEvent, CommittedCandidateReceipt, CoreState, + GroupRotationInfo, Id as ParaId, InboundDownwardMessage, InboundHrmpMessage, + OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement, ScrapedOnChainVotes, + SessionIndex, SessionInfo, ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, + ValidatorSignature, }, }; use sp_api::ProvideRuntimeApi; diff --git a/runtime/test-runtime/src/lib.rs b/runtime/test-runtime/src/lib.rs index d5bdbf1e0b07..f2b84b5f0616 100644 --- a/runtime/test-runtime/src/lib.rs +++ b/runtime/test-runtime/src/lib.rs @@ -45,12 +45,11 @@ use pallet_session::historical as session_historical; use pallet_transaction_payment::{FeeDetails, RuntimeDispatchInfo}; use polkadot_runtime_parachains::reward_points::RewardValidatorsWithEraPoints; use primitives::v2::{ - AccountId, AccountIndex, Balance, BlockNumber, CandidateEvent, CandidateHash, - CommittedCandidateReceipt, CoreState, DisputeState, GroupRotationInfo, Hash as HashT, - Id as ParaId, InboundDownwardMessage, InboundHrmpMessage, Moment, Nonce, - OccupiedCoreAssumption, PersistedValidationData, ScrapedOnChainVotes, - SessionInfo as SessionInfoData, Signature, ValidationCode, ValidationCodeHash, ValidatorId, - ValidatorIndex, + AccountId, AccountIndex, Balance, BlockNumber, CandidateEvent, CommittedCandidateReceipt, + CoreState, GroupRotationInfo, Hash as HashT, Id as ParaId, InboundDownwardMessage, + InboundHrmpMessage, Moment, Nonce, OccupiedCoreAssumption, PersistedValidationData, + ScrapedOnChainVotes, SessionInfo as SessionInfoData, Signature, ValidationCode, + ValidationCodeHash, ValidatorId, ValidatorIndex, }; use runtime_common::{ claims, impl_runtime_weights, paras_sudo_wrapper, BlockHashCount, BlockLength, From 60c1ddba04ae17104dfdf17f362f5ae67e455de6 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 14:33:55 +0300 Subject: [PATCH 08/35] Fix arithmetic operations in tests --- .../src/disputes/with_staging_api/tests.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/node/core/provisioner/src/disputes/with_staging_api/tests.rs b/node/core/provisioner/src/disputes/with_staging_api/tests.rs index 6eb74cb044d4..a67aab69eb02 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/tests.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/tests.rs @@ -379,8 +379,8 @@ impl TestDisputes { &mut self, dispute_count: usize, ) -> (u32, usize) { - let local_votes_count = self.validators_count / 100 * 90; - let onchain_votes_count = self.validators_count / 100 * 80; + let local_votes_count = self.validators_count * 90 / 100; + let onchain_votes_count = self.validators_count * 80 / 100; let session_idx = 0; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); @@ -397,8 +397,8 @@ impl TestDisputes { &mut self, dispute_count: usize, ) -> (u32, usize) { - let local_votes_count = self.validators_count / 100 * 90; - let onchain_votes_count = self.validators_count / 100 * 40; + let local_votes_count = self.validators_count * 90 / 100; + let onchain_votes_count = self.validators_count * 40 / 100; let session_idx = 1; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); @@ -415,7 +415,7 @@ impl TestDisputes { &mut self, dispute_count: usize, ) -> (u32, usize) { - let local_votes_count = self.validators_count / 100 * 70; + let local_votes_count = self.validators_count * 70 / 100; let session_idx = 2; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); @@ -427,8 +427,8 @@ impl TestDisputes { } pub fn add_concluded_disputes_known_onchain(&mut self, dispute_count: usize) -> (u32, usize) { - let local_votes_count = self.validators_count / 100 * 80; - let onchain_votes_count = self.validators_count / 100 * 75; + let local_votes_count = self.validators_count * 80 / 100; + let onchain_votes_count = self.validators_count * 75 / 100; let session_idx = 3; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); @@ -441,7 +441,7 @@ impl TestDisputes { } pub fn add_concluded_disputes_unknown_onchain(&mut self, dispute_count: usize) -> (u32, usize) { - let local_votes_count = self.validators_count / 100 * 80; + let local_votes_count = self.validators_count * 80 / 100; let session_idx = 4; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); From e1654e7702bc83b065bb21da8eaf1a2c99cef67e Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 14:36:43 +0300 Subject: [PATCH 09/35] Use smaller test data --- .../src/disputes/with_staging_api/mod.rs | 6 +++++ .../src/disputes/with_staging_api/tests.rs | 23 +++++++++++-------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/node/core/provisioner/src/disputes/with_staging_api/mod.rs b/node/core/provisioner/src/disputes/with_staging_api/mod.rs index 3e9bcb342b82..6630fe942131 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/mod.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/mod.rs @@ -45,7 +45,10 @@ mod tests; /// The maximum number of disputes Provisioner will include in the inherent data. /// Serves as a protection not to flood the Runtime with excessive data. +#[cfg(not(test))] pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200 * 1_000; +#[cfg(test)] +pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200; // The magic numbers are: `estimated validators count` * `estimated disputes per validator` /// Implements the `select_disputes` function which selects dispute votes which should @@ -153,7 +156,10 @@ async fn vote_selection( where Sender: overseer::ProvisionerSenderTrait, { + #[cfg(not(test))] const BATCH_SIZE: usize = 1_100; + #[cfg(test)] + const BATCH_SIZE: usize = 11; // fetch in batches until there are enough votes let mut disputes = partitioned.into_iter().collect::>(); diff --git a/node/core/provisioner/src/disputes/with_staging_api/tests.rs b/node/core/provisioner/src/disputes/with_staging_api/tests.rs index a67aab69eb02..c8c30d9dbd9e 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/tests.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/tests.rs @@ -415,7 +415,7 @@ impl TestDisputes { &mut self, dispute_count: usize, ) -> (u32, usize) { - let local_votes_count = self.validators_count * 70 / 100; + let local_votes_count = self.validators_count * 90 / 100; let session_idx = 2; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); @@ -427,7 +427,7 @@ impl TestDisputes { } pub fn add_concluded_disputes_known_onchain(&mut self, dispute_count: usize) -> (u32, usize) { - let local_votes_count = self.validators_count * 80 / 100; + let local_votes_count = self.validators_count * 90 / 100; let onchain_votes_count = self.validators_count * 75 / 100; let session_idx = 3; let lf = leaf(); @@ -441,7 +441,7 @@ impl TestDisputes { } pub fn add_concluded_disputes_unknown_onchain(&mut self, dispute_count: usize) -> (u32, usize) { - let local_votes_count = self.validators_count * 80 / 100; + let local_votes_count = self.validators_count * 90 / 100; let session_idx = 4; let lf = leaf(); let dummy_receipt = test_helpers::dummy_candidate_receipt(lf.hash.clone()); @@ -486,8 +486,8 @@ impl TestDisputes { #[test] fn normal_flow() { - const VALIDATOR_COUNT: usize = 100; - const DISPUTES_PER_BATCH: usize = 10; + const VALIDATOR_COUNT: usize = 10; + const DISPUTES_PER_BATCH: usize = 2; const ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: usize = 1; let mut input = TestDisputes::new(VALIDATOR_COUNT); @@ -565,9 +565,12 @@ fn normal_flow() { #[test] fn many_batches() { - const VALIDATOR_COUNT: usize = 100; - const DISPUTES_PER_PARTITION: usize = 1000; - // Around 4_000 disputes are generated. `BATCH_SIZE` is 1_100. + const VALIDATOR_COUNT: usize = 10; + const DISPUTES_PER_PARTITION: usize = 10; + + // 10 disputes per partition * 4 partitions = 40 disputes + // BATCH_SIZE = 11 + // => There should be no more than 40 / 11 queries ( ~4 ) const ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: usize = 4; let mut input = TestDisputes::new(VALIDATOR_COUNT); @@ -618,8 +621,8 @@ fn many_batches() { #[test] fn votes_above_limit() { - const VALIDATOR_COUNT: usize = 100; - const DISPUTES_PER_PARTITION: usize = 5_000; + const VALIDATOR_COUNT: usize = 10; + const DISPUTES_PER_PARTITION: usize = 50; const ACCEPTABLE_RUNTIME_VOTES_QUERIES_COUNT: usize = 4; let mut input = TestDisputes::new(VALIDATOR_COUNT); From e3179cbb736a1411ba5a779d1a47f01d3a23eb47 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 15:33:19 +0300 Subject: [PATCH 10/35] Rename `RuntimeApiRequest::StagingDisputes` to `RuntimeApiRequest::Disputes` --- .../provisioner/src/disputes/with_staging_api/mod.rs | 7 ++++--- .../provisioner/src/disputes/with_staging_api/tests.rs | 2 +- node/core/provisioner/src/error.rs | 2 +- node/core/runtime-api/src/cache.rs | 2 +- node/core/runtime-api/src/lib.rs | 10 +++++----- node/subsystem-types/src/messages.rs | 4 ++-- 6 files changed, 14 insertions(+), 13 deletions(-) diff --git a/node/core/provisioner/src/disputes/with_staging_api/mod.rs b/node/core/provisioner/src/disputes/with_staging_api/mod.rs index 6630fe942131..4f0adeb74d99 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/mod.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/mod.rs @@ -75,7 +75,7 @@ pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200; /// /// # How the onchain votes are fetched /// -/// The logic outlined above relies on `RuntimeApiRequest::StagingDisputes` message from the Runtime staging API. +/// The logic outlined above relies on `RuntimeApiRequest::Disputes` message from the Runtime staging API. /// If the staging API is not enabled - the same logic is executed with empty onchain votes set. Effectively this /// means that all disputes are partitioned in groups 2 or 4 and all votes are sent to the Runtime. pub async fn select_disputes( @@ -92,7 +92,8 @@ where let onchain = match get_onchain_disputes(sender, leaf.hash.clone()).await { Ok(r) => r, Err(GetOnchainDisputesError::NotSupported(runtime_api_err, relay_parent)) => { - gum::debug!( + // Runtime version is checked before calling this method, so the error below should never happen! + gum::error!( target: LOG_TARGET, ?runtime_api_err, ?relay_parent, @@ -427,7 +428,7 @@ where sender .send_message(RuntimeApiMessage::Request( relay_parent, - RuntimeApiRequest::StagingDisputes(tx), + RuntimeApiRequest::Disputes(tx), )) .await; diff --git a/node/core/provisioner/src/disputes/with_staging_api/tests.rs b/node/core/provisioner/src/disputes/with_staging_api/tests.rs index c8c30d9dbd9e..c03a2612acd2 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/tests.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/tests.rs @@ -272,7 +272,7 @@ async fn mock_overseer( match from_job { AllMessages::RuntimeApi(RuntimeApiMessage::Request( _, - RuntimeApiRequest::StagingDisputes(sender), + RuntimeApiRequest::Disputes(sender), )) => { let _ = sender.send(Ok(disputes_db .onchain_disputes diff --git a/node/core/provisioner/src/error.rs b/node/core/provisioner/src/error.rs index 05e437854eac..9411a580b6ee 100644 --- a/node/core/provisioner/src/error.rs +++ b/node/core/provisioner/src/error.rs @@ -89,7 +89,7 @@ pub enum GetOnchainDisputesError { Execution(#[source] RuntimeApiError, Hash), #[error( - "runtime doesn't support RuntimeApiRequest::Disputes/RuntimeApiRequest::StagingDisputes for parent {1}" + "runtime doesn't support RuntimeApiRequest::Disputes for parent {1}" )] NotSupported(#[source] RuntimeApiError, Hash), } diff --git a/node/core/runtime-api/src/cache.rs b/node/core/runtime-api/src/cache.rs index 6f5fdc5d4657..0fe9b74dc86d 100644 --- a/node/core/runtime-api/src/cache.rs +++ b/node/core/runtime-api/src/cache.rs @@ -463,5 +463,5 @@ pub(crate) enum RequestResult { SubmitPvfCheckStatement(Hash, PvfCheckStatement, ValidatorSignature, ()), ValidationCodeHash(Hash, ParaId, OccupiedCoreAssumption, Option), Version(Hash, u32), - StagingDisputes(Hash, Vec<(SessionIndex, CandidateHash, DisputeState)>), + Disputes(Hash, Vec<(SessionIndex, CandidateHash, DisputeState)>), } diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index 3ee143d87918..297cc8db9c65 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -153,7 +153,7 @@ where .cache_validation_code_hash((relay_parent, para_id, assumption), hash), Version(relay_parent, version) => self.requests_cache.cache_version(relay_parent, version), - StagingDisputes(relay_parent, disputes) => + Disputes(relay_parent, disputes) => self.requests_cache.cache_disputes(relay_parent, disputes), } } @@ -256,8 +256,8 @@ where Request::ValidationCodeHash(para, assumption, sender) => query!(validation_code_hash(para, assumption), sender) .map(|sender| Request::ValidationCodeHash(para, assumption, sender)), - Request::StagingDisputes(sender) => - query!(disputes(), sender).map(|sender| Request::StagingDisputes(sender)), + Request::Disputes(sender) => + query!(disputes(), sender).map(|sender| Request::Disputes(sender)), } } @@ -499,7 +499,7 @@ where }, Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), - Request::StagingDisputes(sender) => - query!(StagingDisputes, get_disputes(), ver = 2, sender), + Request::Disputes(sender) => + query!(Disputes, get_disputes(), ver = 2, sender), } } diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index c896454594b9..458110939936 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -699,8 +699,8 @@ pub enum RuntimeApiRequest { OccupiedCoreAssumption, RuntimeApiSender>, ), - /// Returns all on-chain disputes at given block number. - StagingDisputes( + /// Returns all on-chain disputes at given block number. Available in v3. + Disputes( RuntimeApiSender)>>, ), } From 7e3bab7051d1843f0dfa0215c1131b82d2ba59b9 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 16:25:37 +0300 Subject: [PATCH 11/35] Remove `staging-client` feature flag --- Cargo.toml | 1 - cli/Cargo.toml | 1 - node/core/provisioner/Cargo.toml | 3 --- node/core/provisioner/src/disputes/with_staging_api/mod.rs | 3 +-- node/service/Cargo.toml | 2 -- 5 files changed, 1 insertion(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index cf238625e2d6..0e662030faa8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -199,7 +199,6 @@ try-runtime = [ "polkadot-cli/try-runtime" ] fast-runtime = [ "polkadot-cli/fast-runtime" ] runtime-metrics = [ "polkadot-cli/runtime-metrics" ] pyroscope = ["polkadot-cli/pyroscope"] -staging-client = ["polkadot-cli/staging-client"] # Configuration for building a .deb package - for use with `cargo-deb` [package.metadata.deb] diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 1a161db7916d..0f009e2afb81 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -74,4 +74,3 @@ rococo-native = ["service/rococo-native"] malus = ["full-node", "service/malus"] runtime-metrics = ["service/runtime-metrics", "polkadot-node-metrics/runtime-metrics"] -staging-client = ["service/staging-client"] diff --git a/node/core/provisioner/Cargo.toml b/node/core/provisioner/Cargo.toml index 7a06a615154e..47e519087723 100644 --- a/node/core/provisioner/Cargo.toml +++ b/node/core/provisioner/Cargo.toml @@ -22,6 +22,3 @@ sp-application-crypto = { git = "https://github.com/paritytech/substrate", branc sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" } polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" } test-helpers = { package = "polkadot-primitives-test-helpers", path = "../../../primitives/test-helpers" } - -[features] -staging-client = [] diff --git a/node/core/provisioner/src/disputes/with_staging_api/mod.rs b/node/core/provisioner/src/disputes/with_staging_api/mod.rs index 4f0adeb74d99..7b75d1e2facd 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/mod.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/mod.rs @@ -124,10 +124,9 @@ where gum::trace!( target: LOG_TARGET, ?leaf, - "Got {} recent disputes and {} onchain disputes. Runtime supports staging API: {}", + "Got {} recent disputes and {} onchain disputes.", recent_disputes.len(), onchain.len(), - if cfg!(feature = "staging-client") { true } else { false } ); let partitioned = partition_recent_disputes(recent_disputes, &onchain); diff --git a/node/service/Cargo.toml b/node/service/Cargo.toml index a3873d9cf2da..43c246675762 100644 --- a/node/service/Cargo.toml +++ b/node/service/Cargo.toml @@ -200,5 +200,3 @@ runtime-metrics = [ "polkadot-runtime/runtime-metrics", "polkadot-runtime-parachains/runtime-metrics" ] - -staging-client = ["polkadot-node-core-provisioner/staging-client"] From 24ae8cea1f8473b219b00e793a6652a3406e4c94 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 16:31:36 +0300 Subject: [PATCH 12/35] fmt --- node/core/provisioner/src/disputes/with_staging_api/mod.rs | 5 +---- node/core/provisioner/src/error.rs | 4 +--- node/core/runtime-api/src/lib.rs | 3 +-- node/subsystem-types/src/messages.rs | 4 +--- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/node/core/provisioner/src/disputes/with_staging_api/mod.rs b/node/core/provisioner/src/disputes/with_staging_api/mod.rs index 7b75d1e2facd..6886b3a47792 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/mod.rs +++ b/node/core/provisioner/src/disputes/with_staging_api/mod.rs @@ -425,10 +425,7 @@ where gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching on-chain disputes"); let (tx, rx) = oneshot::channel(); sender - .send_message(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::Disputes(tx), - )) + .send_message(RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::Disputes(tx))) .await; rx.await diff --git a/node/core/provisioner/src/error.rs b/node/core/provisioner/src/error.rs index 9411a580b6ee..9fb958c4f339 100644 --- a/node/core/provisioner/src/error.rs +++ b/node/core/provisioner/src/error.rs @@ -88,9 +88,7 @@ pub enum GetOnchainDisputesError { #[error("runtime execution error occurred while fetching onchain disputes for parent {1}")] Execution(#[source] RuntimeApiError, Hash), - #[error( - "runtime doesn't support RuntimeApiRequest::Disputes for parent {1}" - )] + #[error("runtime doesn't support RuntimeApiRequest::Disputes for parent {1}")] NotSupported(#[source] RuntimeApiError, Hash), } diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index 297cc8db9c65..cbae6b266422 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -499,7 +499,6 @@ where }, Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), - Request::Disputes(sender) => - query!(Disputes, get_disputes(), ver = 2, sender), + Request::Disputes(sender) => query!(Disputes, get_disputes(), ver = 2, sender), } } diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 458110939936..73063ed27c9e 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -700,9 +700,7 @@ pub enum RuntimeApiRequest { RuntimeApiSender>, ), /// Returns all on-chain disputes at given block number. Available in v3. - Disputes( - RuntimeApiSender)>>, - ), + Disputes(RuntimeApiSender)>>), } /// A message to the Runtime API subsystem. From 00bd01ba12456cd554ac9c62b0a3a47e5a6fe53c Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 16:46:09 +0300 Subject: [PATCH 13/35] Remove `vstaging` feature flag --- runtime/parachains/Cargo.toml | 1 - runtime/parachains/src/runtime_api_impl/mod.rs | 1 - runtime/rococo/Cargo.toml | 2 +- runtime/test-runtime/Cargo.toml | 2 +- runtime/westend/Cargo.toml | 2 +- 5 files changed, 3 insertions(+), 5 deletions(-) diff --git a/runtime/parachains/Cargo.toml b/runtime/parachains/Cargo.toml index b913adcf1639..becbf26e68e0 100644 --- a/runtime/parachains/Cargo.toml +++ b/runtime/parachains/Cargo.toml @@ -109,4 +109,3 @@ try-runtime = [ "pallet-vesting/try-runtime", ] runtime-metrics = ["sp-tracing/with-tracing", "polkadot-runtime-metrics/runtime-metrics"] -vstaging = [] diff --git a/runtime/parachains/src/runtime_api_impl/mod.rs b/runtime/parachains/src/runtime_api_impl/mod.rs index 603b6c4cb385..e11728f032ee 100644 --- a/runtime/parachains/src/runtime_api_impl/mod.rs +++ b/runtime/parachains/src/runtime_api_impl/mod.rs @@ -21,5 +21,4 @@ //! to a v2 would be done. pub mod v2; -#[cfg(feature = "vstaging")] pub mod vstaging; diff --git a/runtime/rococo/Cargo.toml b/runtime/rococo/Cargo.toml index cc42af8255db..0ca78c98d2af 100644 --- a/runtime/rococo/Cargo.toml +++ b/runtime/rococo/Cargo.toml @@ -64,7 +64,7 @@ rococo-runtime-constants = { package = "rococo-runtime-constants", path = "./con runtime-common = { package = "polkadot-runtime-common", path = "../common", default-features = false } primitives = { package = "polkadot-primitives", path = "../../primitives", default-features = false } polkadot-parachain = { path = "../../parachain", default-features = false } -runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parachains", default-features = false, features = ["vstaging"] } +runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parachains", default-features = false } xcm = { package = "xcm", path = "../../xcm", default-features = false } xcm-executor = { package = "xcm-executor", path = "../../xcm/xcm-executor", default-features = false } diff --git a/runtime/test-runtime/Cargo.toml b/runtime/test-runtime/Cargo.toml index 2313be141443..3779390b2365 100644 --- a/runtime/test-runtime/Cargo.toml +++ b/runtime/test-runtime/Cargo.toml @@ -58,7 +58,7 @@ runtime-common = { package = "polkadot-runtime-common", path = "../common", defa primitives = { package = "polkadot-primitives", path = "../../primitives", default-features = false } pallet-xcm = { path = "../../xcm/pallet-xcm", default-features = false } polkadot-parachain = { path = "../../parachain", default-features = false } -polkadot-runtime-parachains = { path = "../parachains", default-features = false, features = ["vstaging"]} +polkadot-runtime-parachains = { path = "../parachains", default-features = false } xcm-builder = { path = "../../xcm/xcm-builder", default-features = false } xcm-executor = { path = "../../xcm/xcm-executor", default-features = false } xcm = { path = "../../xcm", default-features = false } diff --git a/runtime/westend/Cargo.toml b/runtime/westend/Cargo.toml index 0e0a6dbce3f7..3509cfaeda1f 100644 --- a/runtime/westend/Cargo.toml +++ b/runtime/westend/Cargo.toml @@ -87,7 +87,7 @@ hex-literal = { version = "0.3.4", optional = true } runtime-common = { package = "polkadot-runtime-common", path = "../common", default-features = false } primitives = { package = "polkadot-primitives", path = "../../primitives", default-features = false } polkadot-parachain = { path = "../../parachain", default-features = false } -runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parachains", default-features = false, features = ["vstaging"] } +runtime-parachains = { package = "polkadot-runtime-parachains", path = "../parachains", default-features = false } xcm = { package = "xcm", path = "../../xcm", default-features = false } xcm-executor = { package = "xcm-executor", path = "../../xcm/xcm-executor", default-features = false } From c771aa618220f2988ce012e9caeaf2984f39903b Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 16:57:00 +0300 Subject: [PATCH 14/35] Some comments regarding the staging api --- primitives/src/lib.rs | 6 +++--- primitives/src/vstaging/mod.rs | 2 +- runtime/parachains/src/runtime_api_impl/mod.rs | 11 ++++++++--- runtime/parachains/src/runtime_api_impl/vstaging.rs | 2 +- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/primitives/src/lib.rs b/primitives/src/lib.rs index 121f7cb40d23..168b5795b040 100644 --- a/primitives/src/lib.rs +++ b/primitives/src/lib.rs @@ -22,9 +22,9 @@ // `v2` is currently the latest stable version of the runtime API. pub mod v2; -// The 'staging' version is special - while other versions are set in stone, -// the staging version is malleable. Once it's released, it gets the next -// version number. +// The 'staging' version is special - it contains primitives which are +// still in development. Once they are considered stable, they will be +// moved to a new versioned module. pub mod vstaging; // `runtime_api` contains the actual API implementation. It contains stable and diff --git a/primitives/src/vstaging/mod.rs b/primitives/src/vstaging/mod.rs index 2f29ffbe60b7..64671bd48a60 100644 --- a/primitives/src/vstaging/mod.rs +++ b/primitives/src/vstaging/mod.rs @@ -16,4 +16,4 @@ //! Staging Primitives. -// Put any primitives used by staging API functions here +// Put any primitives used by staging APIs functions here diff --git a/runtime/parachains/src/runtime_api_impl/mod.rs b/runtime/parachains/src/runtime_api_impl/mod.rs index e11728f032ee..deee7c7827bb 100644 --- a/runtime/parachains/src/runtime_api_impl/mod.rs +++ b/runtime/parachains/src/runtime_api_impl/mod.rs @@ -17,8 +17,13 @@ //! Runtime API implementations for Parachains. //! //! These are exposed as different modules using different sets of primitives. -//! At the moment there is only a v2 module and it is not completely clear how migration -//! to a v2 would be done. - +//! At the moment there is a v2 module for the current stable api and +//! vstaging module for all staging methods. +//! When new version of the stable api is released it will be based on v2 and +//! will contain methods from vstaging. +//! The promotion consists of the following steps: +//! 1. Bump the version of the stable module (e.g. v2 becomes v3) +//! 2. Move methods from vstaging to v3. The new stable version can include some +//! or all methods from vstaging. pub mod v2; pub mod vstaging; diff --git a/runtime/parachains/src/runtime_api_impl/vstaging.rs b/runtime/parachains/src/runtime_api_impl/vstaging.rs index 8715cdc53121..7ae235c8133a 100644 --- a/runtime/parachains/src/runtime_api_impl/vstaging.rs +++ b/runtime/parachains/src/runtime_api_impl/vstaging.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -// Put implementations of functions from staging API here. +//! Put implementations of functions from staging APIs here. use crate::disputes; use primitives::v2::{CandidateHash, DisputeState, SessionIndex}; From 543b4ef2ccd8a5474d0193c6f48a8a4c171280f2 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 17:13:00 +0300 Subject: [PATCH 15/35] Rename dispute selection modules in provisioner with_staging_api -> prioritized_selection without_staging_api -> random_selection --- node/core/provisioner/src/disputes/mod.rs | 12 ++++++----- .../mod.rs | 0 .../tests.rs | 2 +- .../mod.rs | 0 node/core/provisioner/src/lib.rs | 21 +++++++++++-------- node/core/provisioner/src/metrics.rs | 2 +- 6 files changed, 21 insertions(+), 16 deletions(-) rename node/core/provisioner/src/disputes/{with_staging_api => prioritized_selection}/mod.rs (100%) rename node/core/provisioner/src/disputes/{with_staging_api => prioritized_selection}/tests.rs (99%) rename node/core/provisioner/src/disputes/{without_staging_api => random_selection}/mod.rs (100%) diff --git a/node/core/provisioner/src/disputes/mod.rs b/node/core/provisioner/src/disputes/mod.rs index f89d2b974809..404e800702b1 100644 --- a/node/core/provisioner/src/disputes/mod.rs +++ b/node/core/provisioner/src/disputes/mod.rs @@ -15,9 +15,11 @@ // along with Polkadot. If not, see . //! The disputes module is responsible for selecting dispute votes to be sent with the inherent data. It contains two -//! different implementations, extracted in two separate modules - `with_staging_api` and `without_staging_api`. The -//! active one is controlled with a feature flag (`staging-client`). The entrypoint to these implementations is the -//! `select_disputes` function. Refer to the documentation of the modules for more details about each implementation. +//! different implementations, extracted in two separate modules - `random_selection` and `prioritized_selection`. Which +//! implementation will be executed depends on the version of the runtime. Runtime v2 supports `random_selection`. Runtime +//! v3 and above - `prioritized_selection`. The entrypoint to these implementations is the `select_disputes` function. +//! prioritized_selection` is considered superior and will be the default one in the future. Refer to the documentation of +//! the modules for more details about each implementation. use crate::LOG_TARGET; use futures::channel::oneshot; @@ -46,6 +48,6 @@ async fn request_votes( } } -pub(crate) mod with_staging_api; +pub(crate) mod prioritized_selection; -pub(crate) mod without_staging_api; +pub(crate) mod random_selection; diff --git a/node/core/provisioner/src/disputes/with_staging_api/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs similarity index 100% rename from node/core/provisioner/src/disputes/with_staging_api/mod.rs rename to node/core/provisioner/src/disputes/prioritized_selection/mod.rs diff --git a/node/core/provisioner/src/disputes/with_staging_api/tests.rs b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs similarity index 99% rename from node/core/provisioner/src/disputes/with_staging_api/tests.rs rename to node/core/provisioner/src/disputes/prioritized_selection/tests.rs index c03a2612acd2..ee566e362b7a 100644 --- a/node/core/provisioner/src/disputes/with_staging_api/tests.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs @@ -16,7 +16,7 @@ use super::super::{ super::{tests::common::test_harness, *}, - with_staging_api::*, + prioritized_selection::*, }; use bitvec::prelude::*; use futures::channel::mpsc; diff --git a/node/core/provisioner/src/disputes/without_staging_api/mod.rs b/node/core/provisioner/src/disputes/random_selection/mod.rs similarity index 100% rename from node/core/provisioner/src/disputes/without_staging_api/mod.rs rename to node/core/provisioner/src/disputes/random_selection/mod.rs diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index 5b80cda09741..33f76dc95da9 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -60,7 +60,7 @@ const SEND_INHERENT_DATA_TIMEOUT: std::time::Duration = core::time::Duration::fr const LOG_TARGET: &str = "parachain::provisioner"; -const STAGING_RUNTIME_VERSION_REQUIREMENT: u32 = 3; +const PRIORITIZED_SELECTION_RUNTIME_VERSION_REQUIREMENT: u32 = 3; /// The provisioner subsystem. pub struct ProvisionerSubsystem { @@ -362,13 +362,16 @@ async fn send_inherent_data( "Selecting disputes" ); - let disputes = - match has_staging_runtime(from_job, leaf.hash.clone(), STAGING_RUNTIME_VERSION_REQUIREMENT) - .await - { - true => disputes::with_staging_api::select_disputes(from_job, metrics, leaf).await?, - false => disputes::without_staging_api::select_disputes(from_job, metrics).await?, - }; + let disputes = match has_required_runtime( + from_job, + leaf.hash.clone(), + PRIORITIZED_SELECTION_RUNTIME_VERSION_REQUIREMENT, + ) + .await + { + true => disputes::prioritized_selection::select_disputes(from_job, metrics, leaf).await?, + false => disputes::random_selection::select_disputes(from_job, metrics).await?, + }; gum::trace!( target: LOG_TARGET, @@ -685,7 +688,7 @@ fn bitfields_indicate_availability( 3 * availability.count_ones() >= 2 * availability.len() } -async fn has_staging_runtime( +async fn has_required_runtime( sender: &mut impl overseer::ProvisionerSenderTrait, relay_parent: Hash, required_runtime_version: u32, diff --git a/node/core/provisioner/src/metrics.rs b/node/core/provisioner/src/metrics.rs index 23c3a84de228..4498f66fc7e0 100644 --- a/node/core/provisioner/src/metrics.rs +++ b/node/core/provisioner/src/metrics.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::disputes::with_staging_api::PartitionedDisputes; +use crate::disputes::prioritized_selection::PartitionedDisputes; use polkadot_node_subsystem_util::metrics::{self, prometheus}; #[derive(Clone)] From 98ee76f7d098f754613433358bdc3d1be8f1ec50 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 17:45:34 +0300 Subject: [PATCH 16/35] Comments for staging api --- primitives/src/runtime_api.rs | 102 ++++++++++++++++++++++++++++------ 1 file changed, 84 insertions(+), 18 deletions(-) diff --git a/primitives/src/runtime_api.rs b/primitives/src/runtime_api.rs index 2c95b2b0eb26..5acb9f041c10 100644 --- a/primitives/src/runtime_api.rs +++ b/primitives/src/runtime_api.rs @@ -18,31 +18,97 @@ //! of the Runtime API exposed from the Runtime to the Host. //! //! The functions in trait ParachainHost` can be part of the stable API -//! (which is versioned) or they can be staging (aka unstable functions). +//! (which is versioned) or they can be staging (aka unstable/testing +//! functions). //! -//! All stable API functions should use primitives from the latest version. -//! In the time of writing of this document - this is v2. So for example: -//! ```ignore -//! fn validators() -> Vec; -//! ``` -//! indicates a function from the stable v2 API. +//! The separation outlined above is achieved with the versioned api feature +//! of `decl_runtime_apis!` and `impl_runtime_apis!`. Before moving on let's +//! see a quick example about how api versioning works. //! -//! On the other hand a staging function's name should be prefixed with -//! `staging_` like this: -//! ```ignore -//! fn staging_get_disputes() -> Vec<(vstaging::SessionIndex, vstaging::CandidateHash, vstaging::DisputeState)>; +//! # Runtime api versioning crash course +//! +//! The versioning is achieved with the `api_version` attribute. It can be +//! placed on: +//! * trait declaration - represents the base version of the api. +//! * method declaration (inside a trait declaration) - represents a versioned +//! method, which is not available in the base version. +//! * trait implementation - represents which version of the api is being +//! implemented. +//! +//! Let's see a quick example: +//! +//! ```rust(ignore) +//! sp_api::decl_runtime_apis! { +//! #[api_version(2)] +//! pub trait MyApi { +//! fn fn1(); +//! fn fn2(); +//! #[api_version(3)] +//! fn fn3(); +//! #[api_version(4)] +//! fn fn4(); +//! } +//! } +//! +//! struct Runtime {} +//! +//! sp_api::impl_runtime_apis! { +//! #[api_version(3)] +//! impl self::MyApi for Runtime { +//! fn fn1() {} +//! fn fn2() {} +//! fn fn3() {} +//! } +//! } //! ``` +//! A new api named `MyApi` is declared with `decl_runtime_apis!`. The trait declaration +//! has got an `api_version` attribute which represents its base version - 2 in this case. +//! +//! The api has got three methods - `fn1`, `fn2`, `fn3` and `fn4`. `fn3` and `fn4` has got +//! an `api_version` attribute which makes them versioned methods. These methods do not exist +//! in the base version of the api. Behind the scenes the declaration above creates three +//! runtime apis: +//! * MyApiV2 with `fn1` and `fn2` +//! * MyApiV3 with `fn1`, `fn2` and `fn3`. +//! * MyApiV4 with `fn1`, `fn2`, `fn3` and `fn4`. //! -//! How a staging function becomes stable? +//! Please note that v4 contains all methods from v3, v3 all methods from v2 and so on. //! -//! Once a staging function is ready to be versioned the `renamed` macro -//! should be used to rename it and version it. For the example above: +//! Back to our example. At the end runtime api is implemented for `struct Runtime` with +//! `impl_runtime_apis` macro. `api_version` attribute is attached to the impl block which +//! means that a version different from the base one is being implemented - in our case this +//! is v3. +//! +//! This version of the api contains three methods so the `impl` block has got definitions +//! for them. Note that `fn4` is not implemented as it is not part of this version of the api. +//! `impl_runtime_apis` generates a default implementation for it calling `unimplemented!()`. +//! +//! Hopefully this should be all you need to know in order to use versioned methods in the node. +//! For more details about how the api versioning works refer to `spi_api` +//! documentation [here](https://docs.substrate.io/rustdocs/latest/sp_api/macro.decl_runtime_apis.html). +//! +//! # How versioned methods are used for `ParachainHost` +//! +//! Let's introduce two types of `ParachainHost` api implementation: +//! * stable - used on stable production networks like Polkadot and Kusama. There is only one +//! stable api at a single point in time. +//! * staging - used on test networks like Westend or Rococo. Depending on the development needs +//! there can be zero, one or multiple staging apis. +//! +//! The stable version of `ParachainHost` is indicated by the base version of the api. Any staging +//! method must use `api_version` attribute so that it is assigned to a specific version of a +//! staging api. This way in a single declaration one can see what's the stable version of +//! `ParachainHost` and what staging versions/functions are available. +//! +//! All stable api functions should use primitives from the latest version. +//! In the time of writing of this document - this is v2. So for example: //! ```ignore -//! #[renamed("staging_get_session_disputes", 3)] -//! fn get_session_disputes() -> Vec<(v3::SessionIndex, v3::CandidateHash, v3::DisputeState)>; +//! fn validators() -> Vec; //! ``` -//! For more details about how the API versioning works refer to `spi_api` -//! documentation [here](https://docs.substrate.io/rustdocs/latest/sp_api/macro.decl_runtime_apis.html). +//! indicates a function from the stable v2 API. +//! +//! All staging api functions should use primitives from vstaging. They should be clearly separated +//! from the stable primitives. use crate::v2; use parity_scale_codec::{Decode, Encode}; From 1884262e29536200eec4be0da7e4467ce4386091 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 17:58:13 +0300 Subject: [PATCH 17/35] Comments --- .../core/provisioner/src/disputes/prioritized_selection/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 6886b3a47792..3a96a2c66034 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -46,10 +46,9 @@ mod tests; /// The maximum number of disputes Provisioner will include in the inherent data. /// Serves as a protection not to flood the Runtime with excessive data. #[cfg(not(test))] -pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200 * 1_000; +pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200_000; #[cfg(test)] pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200; -// The magic numbers are: `estimated validators count` * `estimated disputes per validator` /// Implements the `select_disputes` function which selects dispute votes which should /// be sent to the Runtime. From e6a684340dea8a8422f85e4b29fdfb7f47106f97 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 Aug 2022 21:23:22 +0300 Subject: [PATCH 18/35] Additional logging --- .../provisioner/src/disputes/prioritized_selection/mod.rs | 6 +++++- node/core/provisioner/src/disputes/random_selection/mod.rs | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 3a96a2c66034..da7f051fa19b 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -85,7 +85,11 @@ pub async fn select_disputes( where Sender: overseer::ProvisionerSenderTrait, { - gum::trace!(target: LOG_TARGET, ?leaf, "Selecting disputes for inherent data"); + gum::trace!( + target: LOG_TARGET, + ?leaf, + "Selecting disputes for inherent data using prioritized selection" + ); // Fetch the onchain disputes. We'll do a prioritization based on them. let onchain = match get_onchain_disputes(sender, leaf.hash.clone()).await { diff --git a/node/core/provisioner/src/disputes/random_selection/mod.rs b/node/core/provisioner/src/disputes/random_selection/mod.rs index 3bf5cac340d2..96a00801cbac 100644 --- a/node/core/provisioner/src/disputes/random_selection/mod.rs +++ b/node/core/provisioner/src/disputes/random_selection/mod.rs @@ -120,6 +120,8 @@ pub async fn select_disputes( where Sender: overseer::ProvisionerSenderTrait, { + gum::trace!(target: LOG_TARGET, "Selecting disputes for inherent data using random selection"); + /// The maximum number of disputes Provisioner will include in the inherent data. /// Serves as a protection not to flood the Runtime with excessive data. const MAX_DISPUTES_FORWARDED_TO_RUNTIME: usize = 1_000; From 5f8b4622ddc751e39e88e0282b13b1502bdf995c Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 29 Aug 2022 22:30:36 +0300 Subject: [PATCH 19/35] Code review feedback process_selected_disputes -> into_multi_dispute_statement_set typo In trait VoteType: vote_value -> is_valid --- .../src/disputes/prioritized_selection/mod.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index da7f051fa19b..82eef0416e6f 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -145,10 +145,10 @@ where } let result = vote_selection(sender, partitioned, &onchain).await; - process_selected_disputes(metrics, result) + into_multi_dispute_statement_set(metrics, result) } -/// Selects dispute votes from `PartitionedDispites` which should be sent to the runtime. Votes which +/// Selects dispute votes from `PartitionedDisputes` which should be sent to the runtime. Votes which /// are already onchain are filtered out. Result should be sorted by `(SessionIndex, CandidateHash)` /// which is enforced by the `BTreeMap`. This is a requirement from the runtime. async fn vote_selection( @@ -318,17 +318,17 @@ fn partition_recent_disputes( // Helper trait to obtain the value of vote for `InvalidDisputeStatementKind` and `ValidDisputeStatementKind`. // The alternative was to pass a bool to `fn is_vote_worth_to_keep` explicitly but it's pointless as the value is already 'encoded' in the type. trait VoteType { - fn vote_value() -> bool; + fn is_valid() -> bool; } impl VoteType for InvalidDisputeStatementKind { - fn vote_value() -> bool { + fn is_valid() -> bool { false } } impl VoteType for ValidDisputeStatementKind { - fn vote_value() -> bool { + fn is_valid() -> bool { true } } @@ -339,7 +339,7 @@ fn is_vote_worth_to_keep( _: &T, onchain_state: &DisputeState, ) -> bool { - let offchain_vote = T::vote_value(); + let offchain_vote = T::is_valid(); let in_validators_for = onchain_state .validators_for .get(validator_index.0 as usize) @@ -386,7 +386,7 @@ async fn request_disputes( } // This function produces the return value for `pub fn select_disputes()` -fn process_selected_disputes( +fn into_multi_dispute_statement_set( metrics: &metrics::Metrics, dispute_candidate_votes: BTreeMap<(SessionIndex, CandidateHash), CandidateVotes>, ) -> Result { From 95b0096cfd65d99b2d267aaf096bb0614fdd1777 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 30 Aug 2022 10:12:56 +0300 Subject: [PATCH 20/35] Code review feedback --- node/core/runtime-api/src/lib.rs | 2 +- node/primitives/src/disputes/status.rs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index cbae6b266422..b9bde6cc07ae 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -499,6 +499,6 @@ where }, Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), - Request::Disputes(sender) => query!(Disputes, get_disputes(), ver = 2, sender), + Request::Disputes(sender) => query!(Disputes, get_disputes(), ver = 3, sender), } } diff --git a/node/primitives/src/disputes/status.rs b/node/primitives/src/disputes/status.rs index 52d003ab7c7f..44aed9b78e20 100644 --- a/node/primitives/src/disputes/status.rs +++ b/node/primitives/src/disputes/status.rs @@ -117,9 +117,7 @@ impl DisputeStatus { /// disputes. pub const ACTIVE_DURATION_SECS: Timestamp = 180; -/// Checks if dispute is inactive. Returns true if EITHER of the following statements is valid: -/// - The dispute has concluded OR -/// - The dispute has been active for duration more than ACTIVE_DURATION_SECS +/// Returns true if the dispute has concluded for longer than ACTIVE_DURATION_SECS pub fn dispute_is_inactive(status: &DisputeStatus, now: &Timestamp) -> bool { let at = status.concluded_at(); From 0f9cf6aca33e65008b3f3928e762481603a69a07 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 30 Aug 2022 10:19:38 +0300 Subject: [PATCH 21/35] Fix metrics --- node/core/provisioner/src/metrics.rs | 30 ++++++++++++++-------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/node/core/provisioner/src/metrics.rs b/node/core/provisioner/src/metrics.rs index 4498f66fc7e0..7385f774cf42 100644 --- a/node/core/provisioner/src/metrics.rs +++ b/node/core/provisioner/src/metrics.rs @@ -103,33 +103,33 @@ impl Metrics { pub(crate) fn on_partition_recent_disputes(&self, disputes: &PartitionedDisputes) { if let Some(metrics) = &self.0 { let PartitionedDisputes { - active_unconcluded_onchain: cant_conclude_onchain, - active_unknown_onchain: unknown_onchain, - active_concluded_onchain: can_conclude_onchain, - inactive_known_onchain: concluded_known_onchain, - inactive_unknown_onchain: concluded_unknown_onchain, + active_unconcluded_onchain, + active_unknown_onchain, + active_concluded_onchain, + inactive_known_onchain, + inactive_unknown_onchain, } = disputes; metrics .partitioned_disputes - .with_label_values(&["cant_conclude_onchain"]) - .inc_by(cant_conclude_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["active_unconcluded_onchain"]) + .inc_by(active_unconcluded_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["unknown_onchain"]) - .inc_by(unknown_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["active_unknown_onchain"]) + .inc_by(active_unknown_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["can_conclude_onchain"]) - .inc_by(can_conclude_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["active_concluded_onchain"]) + .inc_by(active_concluded_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["concluded_known_onchain"]) - .inc_by(concluded_known_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["inactive_known_onchain"]) + .inc_by(inactive_known_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["unknown_onchain"]) - .inc_by(concluded_unknown_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["inactive_unknown_onchain"]) + .inc_by(inactive_unknown_onchain.len().try_into().unwrap_or(0)); } } } From 308a3637cfdbb7e87225f99564ca76ad09c91af1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 30 Aug 2022 10:26:39 +0300 Subject: [PATCH 22/35] get_disputes -> disputes --- node/core/runtime-api/src/lib.rs | 2 +- node/subsystem-types/src/runtime_client.rs | 6 +++--- primitives/src/runtime_api.rs | 2 +- runtime/rococo/src/lib.rs | 2 +- runtime/westend/src/lib.rs | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index b9bde6cc07ae..91340f11df91 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -499,6 +499,6 @@ where }, Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), - Request::Disputes(sender) => query!(Disputes, get_disputes(), ver = 3, sender), + Request::Disputes(sender) => query!(Disputes, disputes(), ver = 3, sender), } } diff --git a/node/subsystem-types/src/runtime_client.rs b/node/subsystem-types/src/runtime_client.rs index b7e458fdd6d1..259c94fd4e51 100644 --- a/node/subsystem-types/src/runtime_client.rs +++ b/node/subsystem-types/src/runtime_client.rs @@ -186,7 +186,7 @@ pub trait RuntimeApiSubsystemClient { /// Returns all onchain disputes. /// This is a staging method! Do not use on production runtimes! - async fn get_disputes( + async fn disputes( &self, at: Hash, ) -> Result)>, ApiError>; @@ -375,10 +375,10 @@ where self.runtime_api().session_info_before_version_2(&BlockId::Hash(at), index) } - async fn get_disputes( + async fn disputes( &self, at: Hash, ) -> Result)>, ApiError> { - self.runtime_api().get_disputes(&BlockId::Hash(at)) + self.runtime_api().disputes(&BlockId::Hash(at)) } } diff --git a/primitives/src/runtime_api.rs b/primitives/src/runtime_api.rs index 5acb9f041c10..d0d0b7220bb9 100644 --- a/primitives/src/runtime_api.rs +++ b/primitives/src/runtime_api.rs @@ -220,6 +220,6 @@ sp_api::decl_runtime_apis! { /// Returns all onchain disputes. #[api_version(3)] - fn get_disputes() -> Vec<(v2::SessionIndex, v2::CandidateHash, v2::DisputeState)>; + fn disputes() -> Vec<(v2::SessionIndex, v2::CandidateHash, v2::DisputeState)>; } } diff --git a/runtime/rococo/src/lib.rs b/runtime/rococo/src/lib.rs index 48e9552af023..64ad3646e6e1 100644 --- a/runtime/rococo/src/lib.rs +++ b/runtime/rococo/src/lib.rs @@ -1100,7 +1100,7 @@ sp_api::impl_runtime_apis! { runtime_api_impl::validation_code_hash::(para_id, assumption) } - fn get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { + fn disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::() } } diff --git a/runtime/westend/src/lib.rs b/runtime/westend/src/lib.rs index db06daa5fa96..6d08049910e3 100644 --- a/runtime/westend/src/lib.rs +++ b/runtime/westend/src/lib.rs @@ -1375,7 +1375,7 @@ sp_api::impl_runtime_apis! { parachains_runtime_api_impl::validation_code_hash::(para_id, assumption) } - fn get_disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { + fn disputes() -> Vec<(SessionIndex, CandidateHash, DisputeState)> { runtime_parachains::runtime_api_impl::vstaging::get_session_disputes::() } } From a969eba8c309d640d25151c65354b3b07e1e74f6 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 30 Aug 2022 10:38:04 +0300 Subject: [PATCH 23/35] Get time only once during partitioning --- .../provisioner/src/disputes/prioritized_selection/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 82eef0416e6f..1effdec0d930 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -275,7 +275,8 @@ fn partition_recent_disputes( }) .collect::>(); - // Split ACTIVE from CONCLUDED disputes + // Split recent disputes in ACTIVE and INACTIVE + let time_now = &secs_since_epoch(); let (active, inactive): ( Vec<(SessionIndex, CandidateHash, DisputeStatus)>, Vec<(SessionIndex, CandidateHash, DisputeStatus)>, @@ -284,7 +285,7 @@ fn partition_recent_disputes( .map(|((session_index, candidate_hash), dispute_state)| { (session_index, candidate_hash, dispute_state) }) - .partition(|(_, _, status)| !dispute_is_inactive(status, &secs_since_epoch())); + .partition(|(_, _, status)| !dispute_is_inactive(status, time_now)); // Split ACTIVE in three groups... for (session_index, candidate_hash, _) in active { From 57e1b8889a6969ceba09201a102befb502efda49 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 30 Aug 2022 16:23:23 +0300 Subject: [PATCH 24/35] Fix partitioning --- .../src/disputes/prioritized_selection/mod.rs | 59 ++++++---- .../disputes/prioritized_selection/tests.rs | 104 ++++++++++++------ node/core/provisioner/src/metrics.rs | 27 +++-- 3 files changed, 121 insertions(+), 69 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 1effdec0d930..5316da5cb9db 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -74,9 +74,9 @@ pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200; /// /// # How the onchain votes are fetched /// -/// The logic outlined above relies on `RuntimeApiRequest::Disputes` message from the Runtime staging API. -/// If the staging API is not enabled - the same logic is executed with empty onchain votes set. Effectively this -/// means that all disputes are partitioned in groups 2 or 4 and all votes are sent to the Runtime. +/// The logic outlined above relies on `RuntimeApiRequest::Disputes` message from the Runtime. The user +/// check the Runtime version before calling `select_disputes`. If the function is used with old runtime +/// an error is logged and the logic will continue with empty onchain votes HashMap. pub async fn select_disputes( sender: &mut Sender, metrics: &metrics::Metrics, @@ -217,19 +217,26 @@ pub(crate) struct PartitionedDisputes { /// Hopefully this should never happen. /// Will be sent to the Runtime with FIRST priority. pub inactive_unknown_onchain: Vec<(SessionIndex, CandidateHash)>, - /// Active disputes completely unknown onchain. + /// Disputes which are INACTIVE locally but they are unconcluded for the Runtime. + /// A dispute can have enough local vote to conclude and at the same time the + /// Runtime knows nothing about them at treats it as unconcluded. This discrepancy + /// should be treated with high priority. /// Will be sent to the Runtime with SECOND priority. + pub inactive_unconcluded_onchain: Vec<(SessionIndex, CandidateHash)>, + /// Active disputes completely unknown onchain. + /// Will be sent to the Runtime with THIRD priority. pub active_unknown_onchain: Vec<(SessionIndex, CandidateHash)>, /// Active disputes unconcluded onchain. - /// Will be sent to the Runtime with THIRD priority. + /// Will be sent to the Runtime with FOURTH priority. pub active_unconcluded_onchain: Vec<(SessionIndex, CandidateHash)>, /// Active disputes concluded onchain. New votes are not that important for /// this partition. - /// Will be sent to the Runtime with FOURTH priority. + /// Will be sent to the Runtime with FIFTH priority. pub active_concluded_onchain: Vec<(SessionIndex, CandidateHash)>, - /// Inactive disputes which are known onchain. These are not - /// interesting and won't be sent to the Runtime. - pub inactive_known_onchain: Vec<(SessionIndex, CandidateHash)>, + /// Inactive disputes which has concluded onchain. These are not interesting and + /// won't be sent to the Runtime. + /// Will be DROPPED + pub inactive_concluded_onchain: Vec<(SessionIndex, CandidateHash)>, } impl PartitionedDisputes { @@ -240,10 +247,11 @@ impl PartitionedDisputes { fn into_iter(self) -> impl Iterator { self.inactive_unknown_onchain .into_iter() + .chain(self.inactive_unconcluded_onchain.into_iter()) .chain(self.active_unknown_onchain.into_iter()) .chain(self.active_unconcluded_onchain.into_iter()) .chain(self.active_concluded_onchain.into_iter()) - // inactive_known_onchain is dropped on purpose + // inactive_concluded_onchain is dropped on purpose } } @@ -261,6 +269,14 @@ fn secs_since_epoch() -> Timestamp { } } +fn concluded_onchain(onchain_state: &DisputeState) -> bool { + // Check if there are enough onchain votes for or against to conclude the dispute + let supermajority = supermajority_threshold(onchain_state.validators_for.len()); + + onchain_state.validators_for.count_ones() >= supermajority || + onchain_state.validators_against.count_ones() >= supermajority +} + fn partition_recent_disputes( recent: Vec<(SessionIndex, CandidateHash, DisputeStatus)>, onchain: &HashMap<(SessionIndex, CandidateHash), DisputeState>, @@ -290,25 +306,24 @@ fn partition_recent_disputes( // Split ACTIVE in three groups... for (session_index, candidate_hash, _) in active { match onchain.get(&(session_index, candidate_hash)) { - Some(d) => { - // Check if there are enough onchain votes for or against to conclude the dispute - let supermajority = supermajority_threshold(d.validators_for.len()); - if d.validators_for.count_ones() >= supermajority || - d.validators_against.count_ones() >= supermajority - { - partitioned.active_concluded_onchain.push((session_index, candidate_hash)); - } else { - partitioned.active_unconcluded_onchain.push((session_index, candidate_hash)); - } + Some(d) => match concluded_onchain(d) { + true => partitioned.active_concluded_onchain.push((session_index, candidate_hash)), + false => + partitioned.active_unconcluded_onchain.push((session_index, candidate_hash)), }, None => partitioned.active_unknown_onchain.push((session_index, candidate_hash)), }; } - // ... and INACTIVE in two more + // ... and INACTIVE in three more for (session_index, candidate_hash, _) in inactive { match onchain.get(&(session_index, candidate_hash)) { - Some(_) => partitioned.inactive_known_onchain.push((session_index, candidate_hash)), + Some(onchain_state) => match concluded_onchain(onchain_state) { + true => + partitioned.inactive_concluded_onchain.push((session_index, candidate_hash)), + false => + partitioned.inactive_unconcluded_onchain.push((session_index, candidate_hash)), + }, None => partitioned.inactive_unknown_onchain.push((session_index, candidate_hash)), } } diff --git a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs index ee566e362b7a..6b7f99a1df07 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs @@ -20,7 +20,7 @@ use super::super::{ }; use bitvec::prelude::*; use futures::channel::mpsc; -use polkadot_node_primitives::{CandidateVotes, DisputeStatus}; +use polkadot_node_primitives::{CandidateVotes, DisputeStatus, ACTIVE_DURATION_SECS}; use polkadot_node_subsystem::messages::{ AllMessages, DisputeCoordinatorMessage, RuntimeApiMessage, RuntimeApiRequest, }; @@ -112,13 +112,24 @@ fn should_keep_vote_behaves() { fn partitioning_happy_case() { let mut input = Vec::<(SessionIndex, CandidateHash, DisputeStatus)>::new(); let mut onchain = HashMap::<(u32, CandidateHash), DisputeState>::new(); + let time_now = secs_since_epoch(); // Create one dispute for each partition + let inactive_unknown_onchain = ( + 0, + CandidateHash(Hash::random()), + DisputeStatus::ConcludedFor(time_now - ACTIVE_DURATION_SECS * 2), + ); + input.push(inactive_unknown_onchain.clone()); - let unconcluded_onchain = (0, CandidateHash(Hash::random()), DisputeStatus::Active); - input.push(unconcluded_onchain.clone()); + let inactive_unconcluded_onchain = ( + 1, + CandidateHash(Hash::random()), + DisputeStatus::ConcludedFor(time_now - ACTIVE_DURATION_SECS * 2), + ); + input.push(inactive_unconcluded_onchain.clone()); onchain.insert( - (unconcluded_onchain.0, unconcluded_onchain.1.clone()), + (inactive_unconcluded_onchain.0, inactive_unconcluded_onchain.1.clone()), DisputeState { validators_for: bitvec![u8, Lsb0; 1, 1, 1, 0, 0, 0, 0, 0, 0], validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], @@ -127,68 +138,86 @@ fn partitioning_happy_case() { }, ); - let unknown_onchain = (1, CandidateHash(Hash::random()), DisputeStatus::Active); - input.push(unknown_onchain.clone()); + let active_unknown_onchain = (2, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(active_unknown_onchain.clone()); - let concluded_onchain = (2, CandidateHash(Hash::random()), DisputeStatus::Active); - input.push(concluded_onchain.clone()); + let active_unconcluded_onchain = (3, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(active_unconcluded_onchain.clone()); onchain.insert( - (concluded_onchain.0, concluded_onchain.1.clone()), + (active_unconcluded_onchain.0, active_unconcluded_onchain.1.clone()), DisputeState { - validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 0], + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 0, 0, 0, 0, 0, 0], validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], start: 1, concluded_at: None, }, ); - let concluded_known_onchain = - (3, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); - input.push(concluded_known_onchain.clone()); + let active_concluded_onchain = (4, CandidateHash(Hash::random()), DisputeStatus::Active); + input.push(active_concluded_onchain.clone()); onchain.insert( - (concluded_known_onchain.0, concluded_known_onchain.1.clone()), + (active_concluded_onchain.0, active_concluded_onchain.1.clone()), DisputeState { - validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 1], + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 0], validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], start: 1, - concluded_at: None, + concluded_at: Some(3), }, ); - let concluded_unknown_onchain = - (4, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); - input.push(concluded_unknown_onchain.clone()); + let inactive_concluded_onchain = ( + 5, + CandidateHash(Hash::random()), + DisputeStatus::ConcludedFor(time_now - ACTIVE_DURATION_SECS * 2), + ); + input.push(inactive_concluded_onchain.clone()); + onchain.insert( + (inactive_concluded_onchain.0, inactive_concluded_onchain.1.clone()), + DisputeState { + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 0, 0], + validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], + start: 1, + concluded_at: Some(3), + }, + ); let result = partition_recent_disputes(input, &onchain); - assert_eq!(result.active_unconcluded_onchain.len(), 1); + // Check results + assert_eq!(result.inactive_unknown_onchain.len(), 1); assert_eq!( - result.active_unconcluded_onchain.get(0).unwrap(), - &(unconcluded_onchain.0, unconcluded_onchain.1) + result.inactive_unknown_onchain.get(0).unwrap(), + &(inactive_unknown_onchain.0, inactive_unknown_onchain.1) + ); + + assert_eq!(result.inactive_unconcluded_onchain.len(), 1); + assert_eq!( + result.inactive_unconcluded_onchain.get(0).unwrap(), + &(inactive_unconcluded_onchain.0, inactive_unconcluded_onchain.1) ); assert_eq!(result.active_unknown_onchain.len(), 1); assert_eq!( result.active_unknown_onchain.get(0).unwrap(), - &(unknown_onchain.0, unknown_onchain.1) + &(active_unknown_onchain.0, active_unknown_onchain.1) ); - assert_eq!(result.active_concluded_onchain.len(), 1); + assert_eq!(result.active_unconcluded_onchain.len(), 1); assert_eq!( - result.active_concluded_onchain.get(0).unwrap(), - &(concluded_onchain.0, concluded_onchain.1) + result.active_unconcluded_onchain.get(0).unwrap(), + &(active_unconcluded_onchain.0, active_unconcluded_onchain.1) ); - assert_eq!(result.inactive_known_onchain.len(), 1); + assert_eq!(result.active_concluded_onchain.len(), 1); assert_eq!( - result.inactive_known_onchain.get(0).unwrap(), - &(concluded_known_onchain.0, concluded_known_onchain.1) + result.active_concluded_onchain.get(0).unwrap(), + &(active_concluded_onchain.0, active_concluded_onchain.1) ); - assert_eq!(result.inactive_unknown_onchain.len(), 1); + assert_eq!(result.inactive_concluded_onchain.len(), 1); assert_eq!( - result.inactive_unknown_onchain.get(0).unwrap(), - &(concluded_unknown_onchain.0, concluded_unknown_onchain.1) + result.inactive_concluded_onchain.get(0).unwrap(), + &(inactive_concluded_onchain.0, inactive_concluded_onchain.1) ); } @@ -240,13 +269,13 @@ fn partitioning_duplicated_dispute() { let mut input = Vec::<(SessionIndex, CandidateHash, DisputeStatus)>::new(); let mut onchain = HashMap::<(u32, CandidateHash), DisputeState>::new(); - let some_dispute = (3, CandidateHash(Hash::random()), DisputeStatus::ConcludedFor(0)); + let some_dispute = (3, CandidateHash(Hash::random()), DisputeStatus::Active); input.push(some_dispute.clone()); input.push(some_dispute.clone()); onchain.insert( (some_dispute.0, some_dispute.1.clone()), DisputeState { - validators_for: bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 1, 1, 1, 1], + validators_for: bitvec![u8, Lsb0; 1, 1, 1, 0, 0, 0, 0, 0, 0], validators_against: bitvec![u8, Lsb0; 0, 0, 0, 0, 0, 0, 0, 0, 0], start: 1, concluded_at: None, @@ -255,8 +284,11 @@ fn partitioning_duplicated_dispute() { let result = partition_recent_disputes(input, &onchain); - assert_eq!(result.inactive_known_onchain.len(), 1); - assert_eq!(result.inactive_known_onchain.get(0).unwrap(), &(some_dispute.0, some_dispute.1)); + assert_eq!(result.active_unconcluded_onchain.len(), 1); + assert_eq!( + result.active_unconcluded_onchain.get(0).unwrap(), + &(some_dispute.0, some_dispute.1) + ); } // diff --git a/node/core/provisioner/src/metrics.rs b/node/core/provisioner/src/metrics.rs index 7385f774cf42..aaa57fcc9da5 100644 --- a/node/core/provisioner/src/metrics.rs +++ b/node/core/provisioner/src/metrics.rs @@ -103,33 +103,38 @@ impl Metrics { pub(crate) fn on_partition_recent_disputes(&self, disputes: &PartitionedDisputes) { if let Some(metrics) = &self.0 { let PartitionedDisputes { - active_unconcluded_onchain, + inactive_unknown_onchain, + inactive_unconcluded_onchain: inactive_unconcluded_known_onchain, active_unknown_onchain, + active_unconcluded_onchain, active_concluded_onchain, - inactive_known_onchain, - inactive_unknown_onchain, + inactive_concluded_onchain: inactive_concluded_known_onchain, } = disputes; metrics .partitioned_disputes - .with_label_values(&["active_unconcluded_onchain"]) - .inc_by(active_unconcluded_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["inactive_unknown_onchain"]) + .inc_by(inactive_unknown_onchain.len().try_into().unwrap_or(0)); + metrics + .partitioned_disputes + .with_label_values(&["inactive_unconcluded_known_onchain"]) + .inc_by(inactive_unconcluded_known_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes .with_label_values(&["active_unknown_onchain"]) .inc_by(active_unknown_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["active_concluded_onchain"]) - .inc_by(active_concluded_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["active_unconcluded_onchain"]) + .inc_by(active_unconcluded_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["inactive_known_onchain"]) - .inc_by(inactive_known_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["active_concluded_onchain"]) + .inc_by(active_concluded_onchain.len().try_into().unwrap_or(0)); metrics .partitioned_disputes - .with_label_values(&["inactive_unknown_onchain"]) - .inc_by(inactive_unknown_onchain.len().try_into().unwrap_or(0)); + .with_label_values(&["inactive_concluded_known_onchain"]) + .inc_by(inactive_concluded_known_onchain.len().try_into().unwrap_or(0)); } } } From db34125a105f68c8544a459201277e89cfc9c2d1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 31 Aug 2022 10:02:39 +0300 Subject: [PATCH 25/35] Comments --- runtime/parachains/src/runtime_api_impl/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/runtime_api_impl/mod.rs b/runtime/parachains/src/runtime_api_impl/mod.rs index deee7c7827bb..c045b4747868 100644 --- a/runtime/parachains/src/runtime_api_impl/mod.rs +++ b/runtime/parachains/src/runtime_api_impl/mod.rs @@ -23,7 +23,8 @@ //! will contain methods from vstaging. //! The promotion consists of the following steps: //! 1. Bump the version of the stable module (e.g. v2 becomes v3) -//! 2. Move methods from vstaging to v3. The new stable version can include some -//! or all methods from vstaging. +//! 2. Move methods from vstaging to v3. The new stable version should include +//! all methods from vstaging tagged with the new version number (e.g. all +//! v3 methods). pub mod v2; pub mod vstaging; From d2af967d4a00af4e2a2d185ee0da82e949bb2de1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 31 Aug 2022 10:43:46 +0300 Subject: [PATCH 26/35] Reduce the number of hardcoded api versions --- node/core/provisioner/src/lib.rs | 3 ++- node/core/runtime-api/src/lib.rs | 8 +++++--- node/subsystem-types/src/messages.rs | 7 +++++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index 33f76dc95da9..4c14d9977f75 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -60,7 +60,8 @@ const SEND_INHERENT_DATA_TIMEOUT: std::time::Duration = core::time::Duration::fr const LOG_TARGET: &str = "parachain::provisioner"; -const PRIORITIZED_SELECTION_RUNTIME_VERSION_REQUIREMENT: u32 = 3; +const PRIORITIZED_SELECTION_RUNTIME_VERSION_REQUIREMENT: u32 = + RuntimeApiRequest::DISPUTES_RUNTIME_REQ; /// The provisioner subsystem. pub struct ProvisionerSubsystem { diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index 91340f11df91..4dad23a9c0be 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -351,8 +351,9 @@ where let _timer = metrics.time_make_runtime_api_request(); macro_rules! query { - ($req_variant:ident, $api_name:ident ($($param:expr),*), ver = $version:literal, $sender:expr) => {{ + ($req_variant:ident, $api_name:ident ($($param:expr),*), ver = $version:expr, $sender:expr) => {{ let sender = $sender; + let version: u32 = $version; // enforce type for the version expression let runtime_version = client.api_version_parachain_host(relay_parent).await .unwrap_or_else(|e| { gum::warn!( @@ -370,7 +371,7 @@ where 0 }); - let res = if runtime_version >= $version { + let res = if runtime_version >= version { client.$api_name(relay_parent $(, $param.clone() )*).await .map_err(|e| RuntimeApiError::Execution { runtime_api_name: stringify!($api_name), @@ -499,6 +500,7 @@ where }, Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), - Request::Disputes(sender) => query!(Disputes, disputes(), ver = 3, sender), + Request::Disputes(sender) => + query!(Disputes, disputes(), ver = Request::DISPUTES_RUNTIME_REQ, sender), } } diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 73063ed27c9e..aa88162e8505 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -703,6 +703,13 @@ pub enum RuntimeApiRequest { Disputes(RuntimeApiSender)>>), } +impl RuntimeApiRequest { + /// Runtime version requirements for each message + + /// `Disputes` + pub const DISPUTES_RUNTIME_REQ: u32 = 3; +} + /// A message to the Runtime API subsystem. #[derive(Debug)] pub enum RuntimeApiMessage { From 1366d905f0ad75d775e194e66e30bae4fd67221f Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 31 Aug 2022 17:15:55 +0300 Subject: [PATCH 27/35] Code review feedback --- .../src/disputes/prioritized_selection/mod.rs | 12 ++++++------ node/core/provisioner/src/lib.rs | 2 +- node/core/runtime-api/src/lib.rs | 2 +- node/subsystem-types/src/messages.rs | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 5316da5cb9db..e43be6fa9f1d 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -318,12 +318,12 @@ fn partition_recent_disputes( // ... and INACTIVE in three more for (session_index, candidate_hash, _) in inactive { match onchain.get(&(session_index, candidate_hash)) { - Some(onchain_state) => match concluded_onchain(onchain_state) { - true => - partitioned.inactive_concluded_onchain.push((session_index, candidate_hash)), - false => - partitioned.inactive_unconcluded_onchain.push((session_index, candidate_hash)), - }, + Some(onchain_state) => + if concluded_onchain(onchain_state) { + partitioned.inactive_concluded_onchain.push((session_index, candidate_hash)); + } else { + partitioned.inactive_unconcluded_onchain.push((session_index, candidate_hash)); + }, None => partitioned.inactive_unknown_onchain.push((session_index, candidate_hash)), } } diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index 4c14d9977f75..fc6fa3e18ddd 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -61,7 +61,7 @@ const SEND_INHERENT_DATA_TIMEOUT: std::time::Duration = core::time::Duration::fr const LOG_TARGET: &str = "parachain::provisioner"; const PRIORITIZED_SELECTION_RUNTIME_VERSION_REQUIREMENT: u32 = - RuntimeApiRequest::DISPUTES_RUNTIME_REQ; + RuntimeApiRequest::DISPUTES_RUNTIME_REQUIREMENT; /// The provisioner subsystem. pub struct ProvisionerSubsystem { diff --git a/node/core/runtime-api/src/lib.rs b/node/core/runtime-api/src/lib.rs index 4dad23a9c0be..36355b5759e6 100644 --- a/node/core/runtime-api/src/lib.rs +++ b/node/core/runtime-api/src/lib.rs @@ -501,6 +501,6 @@ where Request::ValidationCodeHash(para, assumption, sender) => query!(ValidationCodeHash, validation_code_hash(para, assumption), ver = 2, sender), Request::Disputes(sender) => - query!(Disputes, disputes(), ver = Request::DISPUTES_RUNTIME_REQ, sender), + query!(Disputes, disputes(), ver = Request::DISPUTES_RUNTIME_REQUIREMENT, sender), } } diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index aa88162e8505..c37f773b3839 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -707,7 +707,7 @@ impl RuntimeApiRequest { /// Runtime version requirements for each message /// `Disputes` - pub const DISPUTES_RUNTIME_REQ: u32 = 3; + pub const DISPUTES_RUNTIME_REQUIREMENT: u32 = 3; } /// A message to the Runtime API subsystem. From aeef11657c308b236b1028f490c6cfb324ae9cc1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 1 Sep 2022 10:39:06 +0300 Subject: [PATCH 28/35] Unused import --- node/core/provisioner/src/disputes/prioritized_selection/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index e43be6fa9f1d..056fd61b7227 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -34,7 +34,6 @@ use polkadot_primitives::v2::{ Hash, InvalidDisputeStatementKind, MultiDisputeStatementSet, SessionIndex, ValidDisputeStatementKind, ValidatorIndex, }; -use rand as _; use std::{ collections::{BTreeMap, HashMap}, time::{SystemTime, UNIX_EPOCH}, From fd24899ced5b64dbe7ea2061609a2aa2210a294b Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 5 Sep 2022 10:14:41 +0300 Subject: [PATCH 29/35] Comments --- node/core/provisioner/src/metrics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/provisioner/src/metrics.rs b/node/core/provisioner/src/metrics.rs index aaa57fcc9da5..b8032a32f89d 100644 --- a/node/core/provisioner/src/metrics.rs +++ b/node/core/provisioner/src/metrics.rs @@ -30,7 +30,7 @@ struct MetricsInner { inherent_data_dispute_statement_sets: prometheus::Counter, inherent_data_dispute_statements: prometheus::CounterVec, - // The disputes received from `disputes-coordinator` by partition + /// The disputes received from `disputes-coordinator` by partition partitioned_disputes: prometheus::CounterVec, } From 49a7b24cd76f1d038b3476d21b82bc797c7910e0 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 14 Sep 2022 13:44:03 +0300 Subject: [PATCH 30/35] More precise log messages --- node/core/provisioner/src/lib.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index fc6fa3e18ddd..0754e2c4c87e 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -689,12 +689,14 @@ fn bitfields_indicate_availability( 3 * availability.count_ones() >= 2 * availability.len() } +// If we have to be absolutely precise here, this method gets the version of the `ParachainHost` api. +// For brevity we'll just call it 'runtime version'. async fn has_required_runtime( sender: &mut impl overseer::ProvisionerSenderTrait, relay_parent: Hash, required_runtime_version: u32, ) -> bool { - gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching runtime version"); + gum::trace!(target: LOG_TARGET, ?relay_parent, "Fetching ParachainHost runtime api version"); let (tx, rx) = oneshot::channel(); sender @@ -708,7 +710,7 @@ async fn has_required_runtime( ?relay_parent, ?runtime_version, ?required_runtime_version, - "Fetched runtime version" + "Fetched ParachainHost runtime api version" ); runtime_version >= required_runtime_version }, @@ -717,7 +719,7 @@ async fn has_required_runtime( target: LOG_TARGET, ?relay_parent, ?error, - "Execution error while fetching runtime version" + "Execution error while fetching ParachainHost runtime api version" ); false }, @@ -725,7 +727,7 @@ async fn has_required_runtime( gum::trace!( target: LOG_TARGET, ?relay_parent, - "NotSupported error while fetching runtime version" + "NotSupported error while fetching ParachainHost runtime api version" ); false }, @@ -733,7 +735,7 @@ async fn has_required_runtime( gum::trace!( target: LOG_TARGET, ?relay_parent, - "Cancelled error while fetching runtime version" + "Cancelled error while fetching ParachainHost runtime api version" ); false }, From 3b018cfd37f2a961c20c6a033d81d6f98d83ee8e Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Sep 2022 12:52:33 +0300 Subject: [PATCH 31/35] Code review feedback --- .../provisioner/src/disputes/prioritized_selection/mod.rs | 4 ++-- node/core/provisioner/src/disputes/random_selection/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 056fd61b7227..98d8985539da 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -62,7 +62,7 @@ pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200; /// /// Provisioner fetches all disputes from `dispute-coordinator` and separates them in multiple partitions. /// Please refer to `struct PartitionedDisputes` for details about the actual partitions. -/// Each partition has got a priority implicitly assigned to it and the disputes are selected based on thus +/// Each partition has got a priority implicitly assigned to it and the disputes are selected based on this /// priority (e.g. disputes in partition 1, then if there is space - disputes from partition 2 and so on). /// /// # Votes selection @@ -99,7 +99,7 @@ where target: LOG_TARGET, ?runtime_api_err, ?relay_parent, - "Can't fetch onchain disputes, because runtime version is not recent enough. Will continue with empty onchain disputes set.", + "Can't fetch onchain disputes, because ParachainHost runtime api version is old. Will continue with empty onchain disputes set.", ); HashMap::new() }, diff --git a/node/core/provisioner/src/disputes/random_selection/mod.rs b/node/core/provisioner/src/disputes/random_selection/mod.rs index 96a00801cbac..9275358e1276 100644 --- a/node/core/provisioner/src/disputes/random_selection/mod.rs +++ b/node/core/provisioner/src/disputes/random_selection/mod.rs @@ -52,7 +52,7 @@ async fn request_disputes( Err(oneshot::Canceled) => { gum::warn!( target: LOG_TARGET, - "Unable to gather {:?} disputes", + "Channel closed: unable to gather {:?} disputes", active_or_recent ); Vec::new() From 58a0f0173edb295de7ab5ece7b5746b74e96e70a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Sep 2022 13:04:31 +0300 Subject: [PATCH 32/35] Code review feedback --- .../src/disputes/prioritized_selection/mod.rs | 13 +++++-------- .../src/disputes/prioritized_selection/tests.rs | 6 +++--- .../src/disputes/random_selection/mod.rs | 8 ++++---- node/core/provisioner/src/lib.rs | 4 ++-- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 98d8985539da..7d650560d762 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -18,10 +18,7 @@ //! about the votes already known onchain and tries to select only relevant votes. Refer to //! the documentation of `select_disputes` for more details about the actual implementation. -use crate::{ - error::{Error, GetOnchainDisputesError}, - metrics, LOG_TARGET, -}; +use crate::{error::GetOnchainDisputesError, metrics, LOG_TARGET}; use futures::channel::oneshot; use polkadot_node_primitives::{dispute_is_inactive, CandidateVotes, DisputeStatus, Timestamp}; use polkadot_node_subsystem::{ @@ -80,7 +77,7 @@ pub async fn select_disputes( sender: &mut Sender, metrics: &metrics::Metrics, leaf: &ActivatedLeaf, -) -> Result +) -> MultiDisputeStatementSet where Sender: overseer::ProvisionerSenderTrait, { @@ -404,9 +401,9 @@ async fn request_disputes( fn into_multi_dispute_statement_set( metrics: &metrics::Metrics, dispute_candidate_votes: BTreeMap<(SessionIndex, CandidateHash), CandidateVotes>, -) -> Result { +) -> MultiDisputeStatementSet { // Transform all `CandidateVotes` into `MultiDisputeStatementSet`. - Ok(dispute_candidate_votes + dispute_candidate_votes .into_iter() .map(|((session_index, candidate_hash), votes)| { let valid_statements = votes @@ -429,7 +426,7 @@ fn into_multi_dispute_statement_set( statements: valid_statements.chain(invalid_statements).collect(), } }) - .collect()) + .collect() } /// Gets the on-chain disputes at a given block number and returns them as a `HashMap` so that searching in them is cheap. diff --git a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs index 6b7f99a1df07..a84525e43ffb 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs @@ -548,7 +548,7 @@ fn normal_flow() { |r| mock_overseer(r, &mut input, &mut vote_queries), |mut tx: TestSubsystemSender| async move { let lf = leaf(); - let result = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); + let result = select_disputes(&mut tx, &metrics, &lf).await; assert!(!result.is_empty()); @@ -628,7 +628,7 @@ fn many_batches() { |r| mock_overseer(r, &mut input, &mut vote_queries), |mut tx: TestSubsystemSender| async move { let lf = leaf(); - let result = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); + let result = select_disputes(&mut tx, &metrics, &lf).await; assert!(!result.is_empty()); @@ -682,7 +682,7 @@ fn votes_above_limit() { |r| mock_overseer(r, &mut input, &mut vote_queries), |mut tx: TestSubsystemSender| async move { let lf = leaf(); - let result = select_disputes(&mut tx, &metrics, &lf).await.unwrap(); + let result = select_disputes(&mut tx, &metrics, &lf).await; assert!(!result.is_empty()); diff --git a/node/core/provisioner/src/disputes/random_selection/mod.rs b/node/core/provisioner/src/disputes/random_selection/mod.rs index 9275358e1276..843b4c2fe94d 100644 --- a/node/core/provisioner/src/disputes/random_selection/mod.rs +++ b/node/core/provisioner/src/disputes/random_selection/mod.rs @@ -21,7 +21,7 @@ //! If the ACTIVE disputes are also above `MAX_DISPUTES_FORWARDED_TO_RUNTIME` limit - a random selection //! of them is generated. -use crate::{error::Error, metrics, LOG_TARGET}; +use crate::{metrics, LOG_TARGET}; use futures::channel::oneshot; use polkadot_node_subsystem::{messages::DisputeCoordinatorMessage, overseer}; use polkadot_primitives::v2::{ @@ -116,7 +116,7 @@ fn extend_by_random_subset_without_repetition( pub async fn select_disputes( sender: &mut Sender, metrics: &metrics::Metrics, -) -> Result +) -> MultiDisputeStatementSet where Sender: overseer::ProvisionerSenderTrait, { @@ -167,7 +167,7 @@ where let dispute_candidate_votes = super::request_votes(sender, disputes).await; // Transform all `CandidateVotes` into `MultiDisputeStatementSet`. - Ok(dispute_candidate_votes + dispute_candidate_votes .into_iter() .map(|(session_index, candidate_hash, votes)| { let valid_statements = votes @@ -190,5 +190,5 @@ where statements: valid_statements.chain(invalid_statements).collect(), } }) - .collect()) + .collect() } diff --git a/node/core/provisioner/src/lib.rs b/node/core/provisioner/src/lib.rs index 0754e2c4c87e..301aec32c15b 100644 --- a/node/core/provisioner/src/lib.rs +++ b/node/core/provisioner/src/lib.rs @@ -370,8 +370,8 @@ async fn send_inherent_data( ) .await { - true => disputes::prioritized_selection::select_disputes(from_job, metrics, leaf).await?, - false => disputes::random_selection::select_disputes(from_job, metrics).await?, + true => disputes::prioritized_selection::select_disputes(from_job, metrics, leaf).await, + false => disputes::random_selection::select_disputes(from_job, metrics).await, }; gum::trace!( From babd20db82b668982e21d7e26e30bce08f66965f Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Sep 2022 14:40:33 +0300 Subject: [PATCH 33/35] Code review feedback - remove `trait VoteType` --- .../src/disputes/prioritized_selection/mod.rs | 42 ++++++++----------- .../disputes/prioritized_selection/tests.rs | 30 +++++++++---- 2 files changed, 40 insertions(+), 32 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 7d650560d762..177e65f7b2eb 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -28,8 +28,7 @@ use polkadot_node_subsystem::{ }; use polkadot_primitives::v2::{ supermajority_threshold, CandidateHash, DisputeState, DisputeStatement, DisputeStatementSet, - Hash, InvalidDisputeStatementKind, MultiDisputeStatementSet, SessionIndex, - ValidDisputeStatementKind, ValidatorIndex, + Hash, MultiDisputeStatementSet, SessionIndex, ValidatorIndex, }; use std::{ collections::{BTreeMap, HashMap}, @@ -182,10 +181,18 @@ where }; votes.valid.retain(|validator_idx, (statement_kind, _)| { - is_vote_worth_to_keep(validator_idx, statement_kind, &onchain_state) + is_vote_worth_to_keep( + validator_idx, + DisputeStatement::Valid(*statement_kind), + &onchain_state, + ) }); votes.invalid.retain(|validator_idx, (statement_kind, _)| { - is_vote_worth_to_keep(validator_idx, statement_kind, &onchain_state) + is_vote_worth_to_keep( + validator_idx, + DisputeStatement::Invalid(*statement_kind), + &onchain_state, + ) }); (session_index, candidate_hash, votes) }) @@ -327,31 +334,16 @@ fn partition_recent_disputes( partitioned } -// Helper trait to obtain the value of vote for `InvalidDisputeStatementKind` and `ValidDisputeStatementKind`. -// The alternative was to pass a bool to `fn is_vote_worth_to_keep` explicitly but it's pointless as the value is already 'encoded' in the type. -trait VoteType { - fn is_valid() -> bool; -} - -impl VoteType for InvalidDisputeStatementKind { - fn is_valid() -> bool { - false - } -} - -impl VoteType for ValidDisputeStatementKind { - fn is_valid() -> bool { - true - } -} - /// Determines if a vote is worth to be kept, based on the onchain disputes -fn is_vote_worth_to_keep( +fn is_vote_worth_to_keep( validator_index: &ValidatorIndex, - _: &T, + dispute_statement: DisputeStatement, onchain_state: &DisputeState, ) -> bool { - let offchain_vote = T::is_valid(); + let offchain_vote = match dispute_statement { + DisputeStatement::Valid(_) => true, + DisputeStatement::Invalid(_) => false, + }; let in_validators_for = onchain_state .validators_for .get(validator_index.0 as usize) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs index a84525e43ffb..f76107dc65d4 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs @@ -51,19 +51,35 @@ fn should_keep_vote_behaves() { let local_invalid_unknown = (ValidatorIndex(3), InvalidDisputeStatementKind::Explicit); assert_eq!( - is_vote_worth_to_keep(&local_valid_known.0, &local_valid_known.1, &onchain_state), + is_vote_worth_to_keep( + &local_valid_known.0, + DisputeStatement::Valid(local_valid_known.1), + &onchain_state + ), false ); assert_eq!( - is_vote_worth_to_keep(&local_valid_unknown.0, &local_valid_unknown.1, &onchain_state), + is_vote_worth_to_keep( + &local_valid_unknown.0, + DisputeStatement::Valid(local_valid_unknown.1), + &onchain_state + ), true ); assert_eq!( - is_vote_worth_to_keep(&local_invalid_known.0, &local_invalid_known.1, &onchain_state), + is_vote_worth_to_keep( + &local_invalid_known.0, + DisputeStatement::Invalid(local_invalid_known.1), + &onchain_state + ), false ); assert_eq!( - is_vote_worth_to_keep(&local_invalid_unknown.0, &local_invalid_unknown.1, &onchain_state), + is_vote_worth_to_keep( + &local_invalid_unknown.0, + DisputeStatement::Invalid(local_invalid_unknown.1), + &onchain_state + ), true ); @@ -73,7 +89,7 @@ fn should_keep_vote_behaves() { assert_eq!( is_vote_worth_to_keep( &local_double_vote_onchain_knows.0, - &local_double_vote_onchain_knows.1, + DisputeStatement::Invalid(local_double_vote_onchain_knows.1), &onchain_state ), false @@ -85,7 +101,7 @@ fn should_keep_vote_behaves() { assert_eq!( is_vote_worth_to_keep( &local_double_vote_onchain_doesnt_knows.0, - &local_double_vote_onchain_doesnt_knows.1, + DisputeStatement::Invalid(local_double_vote_onchain_doesnt_knows.1), &onchain_state ), true @@ -101,7 +117,7 @@ fn should_keep_vote_behaves() { assert_eq!( is_vote_worth_to_keep( &local_double_vote_onchain_doesnt_knows.0, - &local_double_vote_onchain_doesnt_knows.1, + DisputeStatement::Invalid(local_double_vote_onchain_doesnt_knows.1), &empty_onchain_state ), true From a47722676c099840cc3c7a98be0eabc60f0ded89 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Sep 2022 15:20:24 +0300 Subject: [PATCH 34/35] Code review feedback --- .../src/disputes/prioritized_selection/mod.rs | 29 ++++++++++++++----- .../src/disputes/random_selection/mod.rs | 8 ++--- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index 177e65f7b2eb..e262346c93bb 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -45,6 +45,24 @@ pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200_000; #[cfg(test)] pub const MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME: usize = 200; +/// Controls how much dispute votes to be fetched from the runtime per iteration in `fn vote_selection`. +/// The purpose is to fetch the votes in batches until `MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME` is +/// reached. This value should definitely be less than `MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME`. +/// +/// The ratio `MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME` / `VOTES_SELECTION_BATCH_SIZE` gives an +/// approximation about how many runtime requests will be issued to fetch votes from the runtime in +/// a single `select_disputes` call. Ideally we don't want to make more than 2-3 calls. In practice +/// it's hard to predict this number because we can't guess how many new votes (for the runtime) a +/// batch will contain. +/// +/// The value below is reached by: `MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME` / 2 + 10% +/// The 10% makes approximately means '10% new votes'. Tweak this if provisioner makes excessive +/// number of runtime calls. +#[cfg(not(test))] +const VOTES_SELECTION_BATCH_SIZE: usize = 1_100; +#[cfg(test)] +const VOTES_SELECTION_BATCH_SIZE: usize = 11; // Just a small value for tests. Doesn't follow the rules above + /// Implements the `select_disputes` function which selects dispute votes which should /// be sent to the Runtime. /// @@ -140,7 +158,7 @@ where } let result = vote_selection(sender, partitioned, &onchain).await; - into_multi_dispute_statement_set(metrics, result) + make_multi_dispute_statement_set(metrics, result) } /// Selects dispute votes from `PartitionedDisputes` which should be sent to the runtime. Votes which @@ -154,17 +172,12 @@ async fn vote_selection( where Sender: overseer::ProvisionerSenderTrait, { - #[cfg(not(test))] - const BATCH_SIZE: usize = 1_100; - #[cfg(test)] - const BATCH_SIZE: usize = 11; - // fetch in batches until there are enough votes let mut disputes = partitioned.into_iter().collect::>(); let mut total_votes_len = 0; let mut result = BTreeMap::new(); while !disputes.is_empty() { - let batch_size = std::cmp::min(BATCH_SIZE, disputes.len()); + let batch_size = std::cmp::min(VOTES_SELECTION_BATCH_SIZE, disputes.len()); let batch = Vec::from_iter(disputes.drain(0..batch_size)); // Filter votes which are already onchain @@ -390,7 +403,7 @@ async fn request_disputes( } // This function produces the return value for `pub fn select_disputes()` -fn into_multi_dispute_statement_set( +fn make_multi_dispute_statement_set( metrics: &metrics::Metrics, dispute_candidate_votes: BTreeMap<(SessionIndex, CandidateHash), CandidateVotes>, ) -> MultiDisputeStatementSet { diff --git a/node/core/provisioner/src/disputes/random_selection/mod.rs b/node/core/provisioner/src/disputes/random_selection/mod.rs index 843b4c2fe94d..7af025700bae 100644 --- a/node/core/provisioner/src/disputes/random_selection/mod.rs +++ b/node/core/provisioner/src/disputes/random_selection/mod.rs @@ -29,6 +29,10 @@ use polkadot_primitives::v2::{ }; use std::collections::HashSet; +/// The maximum number of disputes Provisioner will include in the inherent data. +/// Serves as a protection not to flood the Runtime with excessive data. +const MAX_DISPUTES_FORWARDED_TO_RUNTIME: usize = 1_000; + #[derive(Debug)] enum RequestType { /// Query recent disputes, could be an excessive amount. @@ -122,10 +126,6 @@ where { gum::trace!(target: LOG_TARGET, "Selecting disputes for inherent data using random selection"); - /// The maximum number of disputes Provisioner will include in the inherent data. - /// Serves as a protection not to flood the Runtime with excessive data. - const MAX_DISPUTES_FORWARDED_TO_RUNTIME: usize = 1_000; - // We use `RecentDisputes` instead of `ActiveDisputes` because redundancy is fine. // It's heavier than `ActiveDisputes` but ensures that everything from the dispute // window gets on-chain, unlike `ActiveDisputes`. From 9eeedbb8fd8e47d69bae347d64cd5b5cdc82d871 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 16 Sep 2022 15:34:08 +0300 Subject: [PATCH 35/35] Trace log for DisputeCoordinatorMessage::QueryCandidateVotes counter in vote_selection --- .../provisioner/src/disputes/prioritized_selection/mod.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs index e262346c93bb..6582f0a612ff 100644 --- a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs +++ b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs @@ -176,11 +176,13 @@ where let mut disputes = partitioned.into_iter().collect::>(); let mut total_votes_len = 0; let mut result = BTreeMap::new(); + let mut request_votes_counter = 0; while !disputes.is_empty() { let batch_size = std::cmp::min(VOTES_SELECTION_BATCH_SIZE, disputes.len()); let batch = Vec::from_iter(disputes.drain(0..batch_size)); // Filter votes which are already onchain + request_votes_counter += 1; let votes = super::request_votes(sender, batch) .await .into_iter() @@ -223,6 +225,12 @@ where } } + gum::trace!( + target: LOG_TARGET, + ?request_votes_counter, + "vote_selection DisputeCoordinatorMessage::QueryCandidateVotes counter", + ); + result }