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

More secure Signed implementation #2963

Merged
merged 27 commits into from
May 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
fba901e
Remove signature verification in backing.
eskimor Apr 28, 2021
a72822f
Remove unused check_payload function.
eskimor Apr 28, 2021
759ba54
Introduced unchecked signed variants.
eskimor Apr 28, 2021
0573178
Fix inclusion to use unchecked variant.
eskimor Apr 28, 2021
91fade4
More unchecked variants.
eskimor Apr 28, 2021
26f09e2
Use unchecked variants in protocols.
eskimor Apr 28, 2021
56f71b2
Start fixing statement-distribution.
eskimor Apr 28, 2021
e615e8a
Fixup statement distribution.
eskimor Apr 28, 2021
6739a36
Fix inclusion.
eskimor Apr 28, 2021
0c8ee1d
Fix warning.
eskimor Apr 28, 2021
ef67d58
Fix backing properly.
eskimor Apr 28, 2021
999e161
Fix bitfield distribution.
eskimor Apr 28, 2021
8e69761
Make crypto store optional for `RuntimeInfo`.
eskimor Apr 28, 2021
1fe106e
Factor out utility functions.
eskimor Apr 28, 2021
ad91cfe
get_group_rotation_info
eskimor Apr 29, 2021
114dea1
WIP: Collator cleanup + check signatures.
eskimor Apr 29, 2021
95c5e07
Convenience signature checking functions.
eskimor Apr 29, 2021
2268809
Check signature on collator-side.
eskimor Apr 29, 2021
3ec469f
Fix warnings.
eskimor Apr 29, 2021
1e9a8e7
Fix collator side tests.
eskimor Apr 30, 2021
ce9c9d7
Get rid of warnings.
eskimor Apr 30, 2021
07579d0
Better Signed/UncheckedSigned implementation.
eskimor Apr 30, 2021
1332ef1
Get rid of dead code.
eskimor Apr 30, 2021
fe03aba
Move Signed in its own module.
eskimor Apr 30, 2021
68c6690
into_checked -> try_into_checked
eskimor Apr 30, 2021
52aa3c3
Merge branch 'master' into rk-secure-signed
eskimor May 3, 2021
ab3fc3d
Fix merge.
eskimor May 3, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

50 changes: 3 additions & 47 deletions node/core/backing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ const fn group_quorum(n_validators: usize) -> usize {

#[derive(Default)]
struct TableContext {
signing_context: SigningContext,
validator: Option<Validator>,
groups: HashMap<ParaId, Vec<ValidatorIndex>>,
validators: Vec<ValidatorId>,
Expand Down Expand Up @@ -870,7 +869,6 @@ impl CandidateBackingJob {
.with_candidate(statement.payload().candidate_hash())
.with_relay_parent(_relay_parent);

self.check_statement_signature(&statement)?;
match self.maybe_validate_and_import(&root_span, sender, statement).await {
Err(Error::ValidationFailed(_)) => return Ok(()),
Err(e) => return Err(e),
Expand Down Expand Up @@ -1028,22 +1026,6 @@ impl CandidateBackingJob {
Some(signed)
}

#[tracing::instrument(level = "trace", skip(self), fields(subsystem = LOG_TARGET))]
fn check_statement_signature(&self, statement: &SignedFullStatement) -> Result<(), Error> {
let idx = statement.validator_index().0 as usize;

if self.table_context.validators.len() > idx {
statement.check_signature(
&self.table_context.signing_context,
&self.table_context.validators[idx],
).map_err(|_| Error::InvalidSignature)?;
} else {
return Err(Error::InvalidSignature);
}

Ok(())
}

/// Insert or get the unbacked-span for the given candidate hash.
fn insert_or_get_unbacked_span(
&mut self,
Expand Down Expand Up @@ -1204,7 +1186,6 @@ impl util::JobTrait for CandidateBackingJob {
let table_context = TableContext {
groups,
validators,
signing_context,
validator,
};

Expand Down Expand Up @@ -1658,14 +1639,9 @@ mod tests {
AllMessages::StatementDistribution(
StatementDistributionMessage::Share(
parent_hash,
signed_statement,
_signed_statement,
)
) if parent_hash == test_state.relay_parent => {
signed_statement.check_signature(
&test_state.signing_context,
&test_state.validator_public[0],
).unwrap();
}
) if parent_hash == test_state.relay_parent => {}
);

assert_matches!(
Expand Down Expand Up @@ -1708,11 +1684,6 @@ mod tests {
}.build();

let candidate_a_hash = candidate_a.hash();
let public0 = CryptoStore::sr25519_generate_new(
&*test_state.keystore,
ValidatorId::ID,
Some(&test_state.validators[0].to_seed()),
).await.expect("Insert key into keystore");
let public1 = CryptoStore::sr25519_generate_new(
&*test_state.keystore,
ValidatorId::ID,
Expand Down Expand Up @@ -1795,10 +1766,9 @@ mod tests {
assert_matches!(
virtual_overseer.recv().await,
AllMessages::StatementDistribution(
StatementDistributionMessage::Share(hash, stmt)
StatementDistributionMessage::Share(hash, _stmt)
) => {
assert_eq!(test_state.relay_parent, hash);
stmt.check_signature(&test_state.signing_context, &public0.into()).expect("Is signed correctly");
}
);

Expand Down Expand Up @@ -2092,11 +2062,6 @@ mod tests {
signed_statement,
)
) if relay_parent == test_state.relay_parent => {
signed_statement.check_signature(
&test_state.signing_context,
&test_state.validator_public[0],
).unwrap();

assert_eq!(*signed_statement.payload(), Statement::Valid(candidate_a_hash));
}
);
Expand Down Expand Up @@ -2257,11 +2222,6 @@ mod tests {
signed_statement,
)
) if parent_hash == test_state.relay_parent => {
signed_statement.check_signature(
&test_state.signing_context,
&test_state.validator_public[0],
).unwrap();

assert_eq!(*signed_statement.payload(), Statement::Seconded(candidate_b));
}
);
Expand Down Expand Up @@ -2593,10 +2553,7 @@ mod tests {
use sp_core::Encode;
use std::convert::TryFrom;

let relay_parent = [1; 32].into();
let para_id = ParaId::from(10);
let session_index = 5;
let signing_context = SigningContext { parent_hash: relay_parent, session_index };
let validators = vec![
Sr25519Keyring::Alice,
Sr25519Keyring::Bob,
Expand All @@ -2614,7 +2571,6 @@ mod tests {
};

let table_context = TableContext {
signing_context,
validator: None,
groups: validator_groups,
validators: validator_public.clone(),
Expand Down
13 changes: 9 additions & 4 deletions node/core/candidate-selection/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,12 @@ impl CandidateSelectionJob {
.with_relay_parent(_relay_parent);
self.handle_invalid(sender, candidate_receipt).await;
}
Some(CandidateSelectionMessage::Seconded(_relay_parent, statement)) => {
Some(CandidateSelectionMessage::Seconded(relay_parent, statement)) => {
let _span = span.child("handle-seconded")
.with_stage(jaeger::Stage::CandidateSelection)
.with_candidate(statement.payload().candidate_hash())
.with_relay_parent(_relay_parent);
self.handle_seconded(sender, statement).await;
.with_relay_parent(relay_parent);
self.handle_seconded(sender, relay_parent, statement).await;
}
None => break,
}
Expand Down Expand Up @@ -345,6 +345,7 @@ impl CandidateSelectionJob {
async fn handle_seconded(
&mut self,
sender: &mut impl SubsystemSender,
relay_parent: Hash,
statement: SignedFullStatement,
) {
let received_from = match &self.seconded_candidate {
Expand All @@ -368,7 +369,11 @@ impl CandidateSelectionJob {
.await;

sender.send_message(
CollatorProtocolMessage::NotifyCollationSeconded(received_from.clone(), statement).into()
CollatorProtocolMessage::NotifyCollationSeconded(
received_from.clone(),
relay_parent,
statement
).into()
).await;
}
}
Expand Down
2 changes: 1 addition & 1 deletion node/core/parachains-inherent/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl ParachainsInherentDataProvider {

let inherent_data = match res {
Ok(pd) => ParachainsInherentData {
bitfields: pd.bitfields,
bitfields: pd.bitfields.into_iter().map(Into::into).collect(),
backed_candidates: pd.backed_candidates,
disputes: pd.disputes,
parent_header,
Expand Down
2 changes: 1 addition & 1 deletion node/network/availability-distribution/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl AvailabilityDistributionSubsystem {

/// Create a new instance of the availability distribution.
pub fn new(keystore: SyncCryptoStorePtr, metrics: Metrics) -> Self {
let runtime = RuntimeInfo::new(keystore.clone());
let runtime = RuntimeInfo::new(Some(keystore.clone()));
Self { keystore, runtime, metrics }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ mod tests {
let (mut context, mut virtual_overseer) =
test_helpers::make_subsystem_context::<AvailabilityDistributionMessage, TaskExecutor>(pool.clone());
let keystore = make_ferdie_keystore();
let mut runtime = polkadot_node_subsystem_util::runtime::RuntimeInfo::new(keystore);
let mut runtime = polkadot_node_subsystem_util::runtime::RuntimeInfo::new(Some(keystore));

let (tx, rx) = oneshot::channel();
let testee = async {
Expand Down
32 changes: 5 additions & 27 deletions node/network/availability-distribution/src/requester/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ use futures::{

use sp_keystore::SyncCryptoStorePtr;

use polkadot_node_subsystem_util::request_availability_cores;
use polkadot_primitives::v1::{CandidateHash, CoreState, Hash, OccupiedCore};
use polkadot_node_subsystem_util::runtime::get_occupied_cores;
use polkadot_primitives::v1::{CandidateHash, Hash, OccupiedCore};
use polkadot_subsystem::{
messages::AllMessages, ActiveLeavesUpdate, SubsystemContext, ActivatedLeaf,
};

use super::{error::recv_runtime, session_cache::SessionCache, LOG_TARGET, Metrics};
use crate::error::Error;
use super::{session_cache::SessionCache, LOG_TARGET, Metrics};


/// A task fetching a particular chunk.
mod fetch_task;
Expand Down Expand Up @@ -125,7 +125,7 @@ impl Requester {
Context: SubsystemContext,
{
for ActivatedLeaf { hash: leaf, .. } in new_heads {
let cores = query_occupied_cores(ctx, leaf).await?;
let cores = get_occupied_cores(ctx, leaf).await?;
tracing::trace!(
target: LOG_TARGET,
occupied_cores = ?cores,
Expand Down Expand Up @@ -226,25 +226,3 @@ impl Stream for Requester {
}
}

/// Query all hashes and descriptors of candidates pending availability at a particular block.
#[tracing::instrument(level = "trace", skip(ctx), fields(subsystem = LOG_TARGET))]
async fn query_occupied_cores<Context>(
ctx: &mut Context,
relay_parent: Hash,
) -> Result<Vec<OccupiedCore>, Error>
where
Context: SubsystemContext,
{
let cores = recv_runtime(request_availability_cores(relay_parent, ctx.sender()).await).await?;

Ok(cores
.into_iter()
.filter_map(|core_state| {
if let CoreState::Occupied(occupied) = core_state {
Some(occupied)
} else {
None
}
})
.collect())
}
1 change: 0 additions & 1 deletion node/network/bitfield-distribution/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ edition = "2018"
[dependencies]
futures = "0.3.12"
tracing = "0.1.25"
parity-scale-codec = { version = "2.0.0", default-features = false, features = ["derive"] }
polkadot-primitives = { path = "../../../primitives" }
polkadot-subsystem = { package = "polkadot-node-subsystem", path = "../../subsystem" }
polkadot-node-subsystem-util = { path = "../../subsystem-util" }
Expand Down
Loading