Revert "remove provisioner checks (#4254)" (#4375)

* Revert "remove provisioner checks (#4254)"

This reverts commit 71e76705.

* Remove TODO in implementer's guide

Cargo.lock

@@ -567,9 +567,9 @@ dependencies = [
 
 [[package]]
 name = "bitvec"
-version = "0.20.4"
+version = "0.20.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7774144344a4faa177370406a7ff5f1da24303817368584c6206c8303eb07848"
+checksum = "f5011ffc90248764d7005b0e10c7294f5aa1bd87d9dd7248f4ad475b347c294d"
 dependencies = [
  "funty",
  "radium 0.6.2",
@@ -729,7 +729,7 @@ dependencies = [
 name = "bp-messages"
 version = "0.1.0"
 dependencies = [
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "bp-runtime",
  "frame-support",
  "frame-system",
@@ -3179,7 +3179,7 @@ name = "kusama-runtime"
 version = "0.9.13"
 dependencies = [
  "beefy-primitives",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "frame-benchmarking",
  "frame-election-provider-support",
  "frame-executive",
@@ -4878,7 +4878,7 @@ dependencies = [
 name = "pallet-bridge-messages"
 version = "0.1.0"
 dependencies = [
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "bp-message-dispatch",
  "bp-messages",
  "bp-rialto",
@@ -5559,7 +5559,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "373b1a4c1338d9cd3d1fa53b3a11bdab5ab6bd80a20f7f7becd76953ae2be909"
 dependencies = [
  "arrayvec 0.7.2",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "byte-slice-cast",
  "impl-trait-for-tuples",
  "parity-scale-codec-derive",
@@ -5896,7 +5896,7 @@ name = "polkadot-availability-bitfield-distribution"
 version = "0.9.13"
 dependencies = [
  "assert_matches",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "env_logger 0.9.0",
  "futures 0.3.17",
  "log",
@@ -6170,7 +6170,7 @@ name = "polkadot-node-core-approval-voting"
 version = "0.9.13"
 dependencies = [
  "assert_matches",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "derive_more",
  "futures 0.3.17",
  "futures-timer 3.0.2",
@@ -6206,7 +6206,7 @@ name = "polkadot-node-core-av-store"
 version = "0.9.13"
 dependencies = [
  "assert_matches",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "env_logger 0.9.0",
  "futures 0.3.17",
  "futures-timer 3.0.2",
@@ -6233,7 +6233,7 @@ name = "polkadot-node-core-backing"
 version = "0.9.13"
 dependencies = [
  "assert_matches",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "futures 0.3.17",
  "polkadot-erasure-coding",
  "polkadot-node-primitives",
@@ -6333,7 +6333,7 @@ name = "polkadot-node-core-dispute-coordinator"
 version = "0.9.13"
 dependencies = [
  "assert_matches",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "derive_more",
  "futures 0.3.17",
  "kvdb",
@@ -6372,13 +6372,15 @@ dependencies = [
 name = "polkadot-node-core-provisioner"
 version = "0.9.13"
 dependencies = [
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "futures 0.3.17",
  "futures-timer 3.0.2",
  "polkadot-node-subsystem",
  "polkadot-node-subsystem-test-helpers",
  "polkadot-node-subsystem-util",
  "polkadot-primitives",
+ "sp-application-crypto",
+ "sp-keystore",
  "thiserror",
  "tracing",
 ]
@@ -6651,7 +6653,7 @@ dependencies = [
 name = "polkadot-primitives"
 version = "0.9.13"
 dependencies = [
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "frame-system",
  "hex-literal",
  "parity-scale-codec",
@@ -6711,7 +6713,7 @@ name = "polkadot-runtime"
 version = "0.9.13"
 dependencies = [
  "beefy-primitives",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "frame-benchmarking",
  "frame-election-provider-support",
  "frame-executive",
@@ -6798,7 +6800,7 @@ name = "polkadot-runtime-common"
 version = "0.9.13"
 dependencies = [
  "beefy-primitives",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "frame-benchmarking",
  "frame-election-provider-support",
  "frame-support",
@@ -6848,7 +6850,7 @@ name = "polkadot-runtime-parachains"
 version = "0.9.13"
 dependencies = [
  "bitflags",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "derive_more",
  "frame-benchmarking",
  "frame-support",
@@ -7138,7 +7140,7 @@ name = "polkadot-test-runtime"
 version = "0.9.13"
 dependencies = [
  "beefy-primitives",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "frame-election-provider-support",
  "frame-executive",
  "frame-support",
@@ -9049,7 +9051,7 @@ version = "1.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "5c55b744399c25532d63a0d2789b109df8d46fc93752d46b0782991a931a782f"
 dependencies = [
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "cfg-if 1.0.0",
  "derive_more",
  "parity-scale-codec",
@@ -11594,7 +11596,7 @@ name = "westend-runtime"
 version = "0.9.13"
 dependencies = [
  "beefy-primitives",
- "bitvec 0.20.4",
+ "bitvec 0.20.1",
  "frame-benchmarking",
  "frame-election-provider-support",
  "frame-executive",

node/core/provisioner/Cargo.toml

@@ -5,6 +5,7 @@ authors = ["Parity Technologies <admin@parity.io>"]
 edition = "2018"
 
 [dependencies]
+bitvec = { version = "0.20.1", default-features = false, features = ["alloc"] }
 futures = "0.3.17"
 tracing = "0.1.29"
 thiserror = "1.0.30"
@@ -14,5 +15,6 @@ polkadot-node-subsystem-util = { path = "../../subsystem-util" }
 futures-timer = "3.0.2"
 
 [dev-dependencies]
+sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" }
-bitvec = { version = "0.20.1", default-features = false, features = [] }

node/core/provisioner/src/lib.rs

@@ -14,11 +14,12 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-//! The provisioner is responsible for assembling a set of items, from which the
-//! runtime will pick a subset and create a relay chain block.
+//! The provisioner is responsible for assembling a relay chain block
+//! from a set of available parachain candidates of its choice.
 
 #![deny(missing_docs, unused_crate_dependencies)]
 
+use bitvec::vec::BitVec;
 use futures::{
 	channel::{mpsc, oneshot},
 	prelude::*,
@@ -28,18 +29,21 @@ use polkadot_node_subsystem::{
 	errors::{ChainApiError, RuntimeApiError},
 	jaeger,
 	messages::{
-		CandidateBackingMessage, DisputeCoordinatorMessage, ProvisionableData,
+		CandidateBackingMessage, ChainApiMessage, DisputeCoordinatorMessage, ProvisionableData,
 		ProvisionerInherentData, ProvisionerMessage,
 	},
 	PerLeafSpan, SubsystemSender,
 };
-use polkadot_node_subsystem_util::{self as util, JobSender, JobSubsystem, JobTrait};
+use polkadot_node_subsystem_util::{
+	self as util, request_availability_cores, request_persisted_validation_data, JobSender,
+	JobSubsystem, JobTrait,
+};
 use polkadot_primitives::v1::{
-	BackedCandidate, CandidateHash, CandidateReceipt, DisputeStatement, DisputeStatementSet, Hash,
-	Id as ParaId, MultiDisputeStatementSet, SignedAvailabilityBitfield,
-	SignedAvailabilityBitfields,
+	BackedCandidate, BlockNumber, CandidateReceipt, CoreState, DisputeStatement,
+	DisputeStatementSet, Hash, MultiDisputeStatementSet, OccupiedCoreAssumption,
+	SignedAvailabilityBitfield, ValidatorIndex,
 };
-use std::{collections::HashSet, pin::Pin, sync::Arc};
+use std::{collections::BTreeMap, pin::Pin, sync::Arc};
 use thiserror::Error;
 
 mod metrics;
@@ -104,17 +108,40 @@ pub enum Error {
 	#[error(transparent)]
 	Util(#[from] util::Error),
 
+	#[error("failed to get availability cores")]
+	CanceledAvailabilityCores(#[source] oneshot::Canceled),
+
+	#[error("failed to get persisted validation data")]
+	CanceledPersistedValidationData(#[source] oneshot::Canceled),
+
+	#[error("failed to get block number")]
+	CanceledBlockNumber(#[source] oneshot::Canceled),
+
 	#[error("failed to get backed candidates")]
 	CanceledBackedCandidates(#[source] oneshot::Canceled),
 
+	#[error("failed to get votes on dispute")]
+	CanceledCandidateVotes(#[source] oneshot::Canceled),
+
 	#[error(transparent)]
 	ChainApi(#[from] ChainApiError),
 
 	#[error(transparent)]
 	Runtime(#[from] RuntimeApiError),
 
+	#[error("failed to send message to ChainAPI")]
+	ChainApiMessageSend(#[source] mpsc::SendError),
+
+	#[error("failed to send message to CandidateBacking to get backed candidates")]
+	GetBackedCandidatesSend(#[source] mpsc::SendError),
+
 	#[error("failed to send return message with Inherents")]
 	InherentDataReturnChannel,
+
+	#[error(
+		"backed candidate does not correspond to selected candidate; check logic in provisioner"
+	)]
+	BackedCandidateOrderingProblem,
 }
 
 impl JobTrait for ProvisioningJob {
@@ -168,10 +195,11 @@ impl ProvisioningJob {
 		sender: &mut impl SubsystemSender,
 		span: PerLeafSpan,
 	) -> Result<(), Error> {
+		use ProvisionerMessage::{ProvisionableData, RequestInherentData};
 		loop {
 			futures::select! {
 				msg = self.receiver.next() => match msg {
-					Some(ProvisionerMessage::RequestInherentData(_, return_sender)) => {
+					Some(RequestInherentData(_, return_sender)) => {
 						let _span = span.child("req-inherent-data");
 						let _timer = self.metrics.time_request_inherent_data();
 
@@ -181,7 +209,7 @@ impl ProvisioningJob {
 							self.awaiting_inherent.push(return_sender);
 						}
 					}
-					Some(ProvisionerMessage::ProvisionableData(_, data)) => {
+					Some(ProvisionableData(_, data)) => {
 						let span = span.child("provisionable-data");
 						let _timer = self.metrics.time_provisionable_data();
 
@@ -209,8 +237,8 @@ impl ProvisioningJob {
 	) {
 		if let Err(err) = send_inherent_data(
 			self.relay_parent,
-			self.signed_bitfields.clone(),
-			self.backed_candidates.clone(),
+			&self.signed_bitfields,
+			&self.backed_candidates,
 			return_senders,
 			sender,
 			&self.metrics,
@@ -243,26 +271,46 @@ impl ProvisioningJob {
 	}
 }
 
-/// The provisioner is the subsystem best suited on the node side,
-/// yet it lacks sufficient information to do weight based inherents limiting.
-/// This does the minimalistic checks and forwards a most likely
-/// too large set of bitfields, candidates, and dispute votes to
-/// the runtime. The `fn create_inherent` in the runtime is responsible
-/// to use a subset of these.
+type CoreAvailability = BitVec<bitvec::order::Lsb0, u8>;
+
+/// The provisioner is the subsystem best suited to choosing which specific
+/// backed candidates and availability bitfields should be assembled into the
+/// block. To engage this functionality, a
+/// `ProvisionerMessage::RequestInherentData` is sent; the response is a set of
+/// non-conflicting candidates and the appropriate bitfields. Non-conflicting
+/// means that there are never two distinct parachain candidates included for
+/// the same parachain and that new parachain candidates cannot be included
+/// until the previous one either gets declared available or expired.
+///
+/// The main complication here is going to be around handling
+/// occupied-core-assumptions. We might have candidates that are only
+/// includable when some bitfields are included. And we might have candidates
+/// that are not includable when certain bitfields are included.
+///
+/// When we're choosing bitfields to include, the rule should be simple:
+/// maximize availability. So basically, include all bitfields. And then
+/// choose a coherent set of candidates along with that.
 async fn send_inherent_data(
 	relay_parent: Hash,
-	bitfields: SignedAvailabilityBitfields,
-	candidate_receipts: Vec<CandidateReceipt>,
+	bitfields: &[SignedAvailabilityBitfield],
+	candidates: &[CandidateReceipt],
 	return_senders: Vec<oneshot::Sender<ProvisionerInherentData>>,
 	from_job: &mut impl SubsystemSender,
 	metrics: &Metrics,
 ) -> Result<(), Error> {
-	let backed_candidates =
-		collect_backed_candidates(candidate_receipts, relay_parent, from_job).await?;
+	let availability_cores = request_availability_cores(relay_parent, from_job)
+		.await
+		.await
+		.map_err(|err| Error::CanceledAvailabilityCores(err))??;
 
-	let disputes = collect_disputes(from_job, metrics).await?;
+	let disputes = select_disputes(from_job, metrics).await?;
+	let bitfields = select_availability_bitfields(&availability_cores, bitfields);
+	let candidates =
+		select_candidates(&availability_cores, &bitfields, candidates, relay_parent, from_job)
+			.await?;
 
-	let inherent_data = ProvisionerInherentData { bitfields, backed_candidates, disputes };
+	let inherent_data =
+		ProvisionerInherentData { bitfields, backed_candidates: candidates, disputes };
 
 	for return_sender in return_senders {
 		return_sender
@@ -273,33 +321,120 @@ async fn send_inherent_data(
 	Ok(())
 }
 
-/// Collect backed candidates with a matching `relay_parent`.
-async fn collect_backed_candidates(
-	candidate_receipts: Vec<CandidateReceipt>,
+/// In general, we want to pick all the bitfields. However, we have the following constraints:
+///
+/// - not more than one per validator
+/// - each 1 bit must correspond to an occupied core
+///
+/// If we have too many, an arbitrary selection policy is fine. For purposes of maximizing availability,
+/// we pick the one with the greatest number of 1 bits.
+///
+/// Note: This does not enforce any sorting precondition on the output; the ordering there will be unrelated
+/// to the sorting of the input.
+fn select_availability_bitfields(
+	cores: &[CoreState],
+	bitfields: &[SignedAvailabilityBitfield],
+) -> Vec<SignedAvailabilityBitfield> {
+	let mut selected: BTreeMap<ValidatorIndex, SignedAvailabilityBitfield> = BTreeMap::new();
+
+	'a: for bitfield in bitfields.iter().cloned() {
+		if bitfield.payload().0.len() != cores.len() {
+			continue
+		}
+
+		let is_better = selected
+			.get(&bitfield.validator_index())
+			.map_or(true, |b| b.payload().0.count_ones() < bitfield.payload().0.count_ones());
+
+		if !is_better {
+			continue
+		}
+
+		for (idx, _) in cores.iter().enumerate().filter(|v| !v.1.is_occupied()) {
+			// Bit is set for an unoccupied core - invalid
+			if *bitfield.payload().0.get(idx).as_deref().unwrap_or(&false) {
+				continue 'a
+			}
+		}
+
+		let _ = selected.insert(bitfield.validator_index(), bitfield);
+	}
+
+	selected.into_iter().map(|(_, b)| b).collect()
+}
+
+/// Determine which cores are free, and then to the degree possible, pick a candidate appropriate to each free core.
+async fn select_candidates(
+	availability_cores: &[CoreState],
+	bitfields: &[SignedAvailabilityBitfield],
+	candidates: &[CandidateReceipt],
 	relay_parent: Hash,
 	sender: &mut impl SubsystemSender,
 ) -> Result<Vec<BackedCandidate>, Error> {
-	let max_one_candidate_per_para = HashSet::<ParaId>::with_capacity(candidate_receipts.len());
-	let selected_candidates = candidate_receipts
-		.into_iter()
-		.filter(|candidate_receipt| {
-			// assure the follow up query `GetBackedCandidate` succeeds
-			candidate_receipt.descriptor().relay_parent == relay_parent
-		})
-		.scan(max_one_candidate_per_para, |unique, candidate_receipt| {
-			let para_id = candidate_receipt.descriptor().para_id;
-			if unique.insert(para_id) {
-				Some(candidate_receipt.hash())
-			} else {
-				tracing::debug!(
-					target: LOG_TARGET,
-					?para_id,
-					"Duplicate candidate detected for para, only submitting one",
-				);
-				None
-			}
-		})
-		.collect::<Vec<CandidateHash>>();
+	let block_number = get_block_number_under_construction(relay_parent, sender).await?;
+
+	let mut selected_candidates =
+		Vec::with_capacity(candidates.len().min(availability_cores.len()));
+
+	for (core_idx, core) in availability_cores.iter().enumerate() {
+		let (scheduled_core, assumption) = match core {
+			CoreState::Scheduled(scheduled_core) => (scheduled_core, OccupiedCoreAssumption::Free),
+			CoreState::Occupied(occupied_core) => {
+				if bitfields_indicate_availability(core_idx, bitfields, &occupied_core.availability)
+				{
+					if let Some(ref scheduled_core) = occupied_core.next_up_on_available {
+						(scheduled_core, OccupiedCoreAssumption::Included)
+					} else {
+						continue
+					}
+				} else {
+					if occupied_core.time_out_at != block_number {
+						continue
+					}
+					if let Some(ref scheduled_core) = occupied_core.next_up_on_time_out {
+						(scheduled_core, OccupiedCoreAssumption::TimedOut)
+					} else {
+						continue
+					}
+				}
+			},
+			CoreState::Free => continue,
+		};
+
+		let validation_data = match request_persisted_validation_data(
+			relay_parent,
+			scheduled_core.para_id,
+			assumption,
+			sender,
+		)
+		.await
+		.await
+		.map_err(|err| Error::CanceledPersistedValidationData(err))??
+		{
+			Some(v) => v,
+			None => continue,
+		};
+
+		let computed_validation_data_hash = validation_data.hash();
+
+		// we arbitrarily pick the first of the backed candidates which match the appropriate selection criteria
+		if let Some(candidate) = candidates.iter().find(|backed_candidate| {
+			let descriptor = &backed_candidate.descriptor;
+			descriptor.para_id == scheduled_core.para_id &&
+				descriptor.persisted_validation_data_hash == computed_validation_data_hash
+