From 48afbe352507888934280bfac61918e2c36dcbbd Mon Sep 17 00:00:00 2001
From: Andrei Sandu <54316454+sandreim@users.noreply.github.com>
Date: Thu, 25 Jul 2024 12:44:06 +0300
Subject: [PATCH] CandidateDescriptor: disable collator signature and collator
id usage (#4665)
Collator id and collator signature do not serve any useful purpose.
Removing the signature check from runtime but keeping the checks in the
node until the runtime is upgraded.
TODO:
- [x] PRDoc
- [x] Add node feature for core index commitment enablement
---------
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
---
.../src/fragment_chain/mod.rs | 2 -
.../src/inclusion_emulator/mod.rs | 26 ++-------
polkadot/primitives/src/v7/mod.rs | 6 +-
polkadot/runtime/parachains/src/builder.rs | 38 ++++++-------
.../runtime/parachains/src/inclusion/mod.rs | 8 ---
.../runtime/parachains/src/inclusion/tests.rs | 56 +++++++++++++++----
prdoc/pr_4665.prdoc | 21 +++++++
7 files changed, 96 insertions(+), 61 deletions(-)
create mode 100644 prdoc/pr_4665.prdoc
diff --git a/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs b/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs
index f87d4820ff9..b5fe70e7692 100644
--- a/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs
+++ b/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs
@@ -160,8 +160,6 @@ impl CandidateStorage {
state,
candidate: Arc::new(ProspectiveCandidate {
commitments: candidate.commitments,
- collator: candidate.descriptor.collator,
- collator_signature: candidate.descriptor.signature,
persisted_validation_data,
pov_hash: candidate.descriptor.pov_hash,
validation_code_hash: candidate.descriptor.validation_code_hash,
diff --git a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs
index b5aef325c8b..2272f048089 100644
--- a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs
+++ b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs
@@ -117,9 +117,8 @@
/// That means a few blocks of execution time lost, which is not a big deal for code upgrades
/// in practice at most once every few weeks.
use polkadot_primitives::{
- async_backing::Constraints as PrimitiveConstraints, BlockNumber, CandidateCommitments,
- CollatorId, CollatorSignature, Hash, HeadData, Id as ParaId, PersistedValidationData,
- UpgradeRestriction, ValidationCodeHash,
+ async_backing::Constraints as PrimitiveConstraints, BlockNumber, CandidateCommitments, Hash,
+ HeadData, Id as ParaId, PersistedValidationData, UpgradeRestriction, ValidationCodeHash,
};
use std::{collections::HashMap, sync::Arc};
@@ -521,18 +520,13 @@ impl ConstraintModifications {
/// The prospective candidate.
///
/// This comprises the key information that represent a candidate
-/// without pinning it to a particular session. For example, everything
-/// to do with the collator's signature and commitments are represented
-/// here. But the erasure-root is not. This means that prospective candidates
+/// without pinning it to a particular session. For example commitments are
+/// represented here. But the erasure-root is not. This means that prospective candidates
/// are not correlated to any session in particular.
#[derive(Debug, Clone, PartialEq)]
pub struct ProspectiveCandidate {
/// The commitments to the output of the execution.
pub commitments: CandidateCommitments,
- /// The collator that created the candidate.
- pub collator: CollatorId,
- /// The signature of the collator on the payload.
- pub collator_signature: CollatorSignature,
/// The persisted validation data used to create the candidate.
pub persisted_validation_data: PersistedValidationData,
/// The hash of the PoV.
@@ -800,10 +794,7 @@ fn validate_against_constraints(
#[cfg(test)]
mod tests {
use super::*;
- use polkadot_primitives::{
- CollatorPair, HorizontalMessages, OutboundHrmpMessage, ValidationCode,
- };
- use sp_application_crypto::Pair;
+ use polkadot_primitives::{HorizontalMessages, OutboundHrmpMessage, ValidationCode};
#[test]
fn stack_modifications() {
@@ -1162,11 +1153,6 @@ mod tests {
constraints: &Constraints,
relay_parent: &RelayChainBlockInfo,
) -> ProspectiveCandidate {
- let collator_pair = CollatorPair::generate().0;
- let collator = collator_pair.public();
-
- let sig = collator_pair.sign(b"blabla".as_slice());
-
ProspectiveCandidate {
commitments: CandidateCommitments {
upward_messages: Default::default(),
@@ -1176,8 +1162,6 @@ mod tests {
processed_downward_messages: 0,
hrmp_watermark: relay_parent.number,
},
- collator,
- collator_signature: sig,
persisted_validation_data: PersistedValidationData {
parent_head: constraints.required_parent.clone(),
relay_parent_number: relay_parent.number,
diff --git a/polkadot/primitives/src/v7/mod.rs b/polkadot/primitives/src/v7/mod.rs
index 06b70465208..a729d8cee4c 100644
--- a/polkadot/primitives/src/v7/mod.rs
+++ b/polkadot/primitives/src/v7/mod.rs
@@ -2040,10 +2040,14 @@ pub mod node_features {
/// Must not be enabled unless all validators and collators have stopped using `req_chunk`
/// protocol version 1. If it is enabled, validators can start systematic chunk recovery.
AvailabilityChunkMapping = 2,
+ /// Enables node side support of `CoreIndex` committed candidate receipts.
+ /// See [RFC-103](https://github.com/polkadot-fellows/RFCs/pull/103) for details.
+ /// Only enable if at least 2/3 of nodes support the feature.
+ CandidateReceiptV2 = 3,
/// First unassigned feature bit.
/// Every time a new feature flag is assigned it should take this value.
/// and this should be incremented.
- FirstUnassigned = 3,
+ FirstUnassigned = 4,
}
}
diff --git a/polkadot/runtime/parachains/src/builder.rs b/polkadot/runtime/parachains/src/builder.rs
index ec07cca2107..b2a67ee8dd2 100644
--- a/polkadot/runtime/parachains/src/builder.rs
+++ b/polkadot/runtime/parachains/src/builder.rs
@@ -30,21 +30,30 @@ use bitvec::{order::Lsb0 as BitOrderLsb0, vec::BitVec};
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use polkadot_primitives::{
- collator_signature_payload, node_features::FeatureIndex, AvailabilityBitfield, BackedCandidate,
- CandidateCommitments, CandidateDescriptor, CandidateHash, CollatorId, CollatorSignature,
- CommittedCandidateReceipt, CompactStatement, CoreIndex, DisputeStatement, DisputeStatementSet,
- GroupIndex, HeadData, Id as ParaId, IndexedVec, InherentData as ParachainsInherentData,
- InvalidDisputeStatementKind, PersistedValidationData, SessionIndex, SigningContext,
- UncheckedSigned, ValidDisputeStatementKind, ValidationCode, ValidatorId, ValidatorIndex,
- ValidityAttestation,
+ node_features::FeatureIndex, AvailabilityBitfield, BackedCandidate, CandidateCommitments,
+ CandidateDescriptor, CandidateHash, CollatorId, CollatorSignature, CommittedCandidateReceipt,
+ CompactStatement, CoreIndex, DisputeStatement, DisputeStatementSet, GroupIndex, HeadData,
+ Id as ParaId, IndexedVec, InherentData as ParachainsInherentData, InvalidDisputeStatementKind,
+ PersistedValidationData, SessionIndex, SigningContext, UncheckedSigned,
+ ValidDisputeStatementKind, ValidationCode, ValidatorId, ValidatorIndex, ValidityAttestation,
};
-use sp_core::{sr25519, H256};
+use sp_core::{sr25519, ByteArray, H256};
use sp_runtime::{
generic::Digest,
traits::{Header as HeaderT, One, TrailingZeroInput, Zero},
RuntimeAppPublic,
};
+/// Create a null collator id.
+pub fn dummy_collator() -> CollatorId {
+ CollatorId::from_slice(&vec![0u8; 32]).expect("32 bytes; qed")
+}
+
+/// Create a null collator signature.
+pub fn dummy_collator_signature() -> CollatorSignature {
+ CollatorSignature::from_slice(&vec![0u8; 64]).expect("64 bytes; qed")
+}
+
fn mock_validation_code() -> ValidationCode {
ValidationCode(vec![1, 2, 3])
}
@@ -584,7 +593,6 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
// This generates a pair and adds it to the keystore, returning just the
// public.
- let collator_public = CollatorId::generate_pair(None);
let header = Self::header(self.block_number);
let relay_parent = header.hash();
@@ -614,14 +622,6 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
let pov_hash = Default::default();
let validation_code_hash = mock_validation_code().hash();
- let payload = collator_signature_payload(
- &relay_parent,
- ¶_id,
- &persisted_validation_data_hash,
- &pov_hash,
- &validation_code_hash,
- );
- let signature = collator_public.sign(&payload).unwrap();
let mut past_code_meta =
paras::ParaPastCodeMeta::<BlockNumberFor<T>>::default();
@@ -634,11 +634,11 @@ impl<T: paras_inherent::Config> BenchBuilder<T> {
descriptor: CandidateDescriptor::<T::Hash> {
para_id,
relay_parent,
- collator: collator_public,
+ collator: dummy_collator(),
persisted_validation_data_hash,
pov_hash,
erasure_root: Default::default(),
- signature,
+ signature: dummy_collator_signature(),
para_head: head_data.hash(),
validation_code_hash,
},
diff --git a/polkadot/runtime/parachains/src/inclusion/mod.rs b/polkadot/runtime/parachains/src/inclusion/mod.rs
index 281dc5d0c5f..a5bb6e6cc60 100644
--- a/polkadot/runtime/parachains/src/inclusion/mod.rs
+++ b/polkadot/runtime/parachains/src/inclusion/mod.rs
@@ -338,8 +338,6 @@ pub mod pallet {
InsufficientBacking,
/// Invalid (bad signature, unknown validator, etc.) backing.
InvalidBacking,
- /// Collator did not sign PoV.
- NotCollatorSigned,
/// The validation data hash does not match expected.
ValidationDataHashMismatch,
/// The downward message queue is not processed correctly.
@@ -1222,7 +1220,6 @@ impl<T: Config> CandidateCheckContext<T> {
///
/// Assures:
/// * relay-parent in-bounds
- /// * collator signature check passes
/// * code hash of commitments matches current code hash
/// * para head in the descriptor and commitments match
///
@@ -1259,11 +1256,6 @@ impl<T: Config> CandidateCheckContext<T> {
);
}
- ensure!(
- backed_candidate_receipt.descriptor().check_collator_signature().is_ok(),
- Error::<T>::NotCollatorSigned,
- );
-
let validation_code_hash = paras::CurrentCodeHash::<T>::get(para_id)
// A candidate for a parachain without current validation code is not scheduled.
.ok_or_else(|| Error::<T>::UnscheduledCandidate)?;
diff --git a/polkadot/runtime/parachains/src/inclusion/tests.rs b/polkadot/runtime/parachains/src/inclusion/tests.rs
index 18def664f4b..3ead456cde5 100644
--- a/polkadot/runtime/parachains/src/inclusion/tests.rs
+++ b/polkadot/runtime/parachains/src/inclusion/tests.rs
@@ -1545,16 +1545,52 @@ fn candidate_checks() {
None,
);
- assert_noop!(
- ParaInclusion::process_candidates(
- &allowed_relay_parents,
- &vec![(thread_a_assignment.0, vec![(backed, thread_a_assignment.1)])]
- .into_iter()
- .collect(),
- &group_validators,
- false
- ),
- Error::<Test>::NotCollatorSigned
+ let ProcessedCandidates {
+ core_indices: occupied_cores,
+ candidate_receipt_with_backing_validator_indices,
+ } = ParaInclusion::process_candidates(
+ &allowed_relay_parents,
+ &vec![(thread_a_assignment.0, vec![(backed.clone(), thread_a_assignment.1)])]
+ .into_iter()
+ .collect(),
+ &group_validators,
+ false,
+ )
+ .expect("candidate is accepted with bad collator signature");
+
+ assert_eq!(occupied_cores, vec![(CoreIndex::from(2), thread_a)]);
+
+ let mut expected = std::collections::HashMap::<
+ CandidateHash,
+ (CandidateReceipt, Vec<(ValidatorIndex, ValidityAttestation)>),
+ >::new();
+ let backed_candidate = backed;
+ let candidate_receipt_with_backers = expected
+ .entry(backed_candidate.hash())
+ .or_insert_with(|| (backed_candidate.receipt(), Vec::new()));
+ let (validator_indices, _maybe_core_index) =
+ backed_candidate.validator_indices_and_core_index(true);
+ assert_eq!(backed_candidate.validity_votes().len(), validator_indices.count_ones());
+ candidate_receipt_with_backers.1.extend(
+ validator_indices
+ .iter()
+ .enumerate()
+ .filter(|(_, signed)| **signed)
+ .zip(backed_candidate.validity_votes().iter().cloned())
+ .filter_map(|((validator_index_within_group, _), attestation)| {
+ let grp_idx = GroupIndex(2);
+ group_validators(grp_idx).map(|validator_indices| {
+ (validator_indices[validator_index_within_group], attestation)
+ })
+ }),
+ );
+
+ assert_eq!(
+ expected,
+ candidate_receipt_with_backing_validator_indices
+ .into_iter()
+ .map(|c| (c.0.hash(), c))
+ .collect()
);
}
diff --git a/prdoc/pr_4665.prdoc b/prdoc/pr_4665.prdoc
new file mode 100644
index 00000000000..7a8ec7398e6
--- /dev/null
+++ b/prdoc/pr_4665.prdoc
@@ -0,0 +1,21 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "Remove runtime collator signature checks"
+
+doc:
+ - audience: [Runtime Dev, Node Dev]
+ description: |
+ Removes runtime collator signature checks, but these are still being done on the node. Remove collator
+ and signature from the `ProspectiveCandidate` definition in the inclusion emulator. Add
+ `CandidateReceiptV2` node feature bit.
+
+crates:
+- name: polkadot-primitives
+ bump: minor
+- name: polkadot-node-subsystem-util
+ bump: minor
+- name: polkadot-node-core-prospective-parachains
+ bump: patch
+- name: polkadot-runtime-parachains
+ bump: patch
--
GitLab