Skip to content

Commit

Permalink
Handling of disabled validators in backing subsystem - initial work
Browse files Browse the repository at this point in the history
  • Loading branch information
tdimitrov committed Aug 29, 2023
1 parent 1d1f852 commit b5c6f07
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 19 deletions.
44 changes: 29 additions & 15 deletions polkadot/node/core/backing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ struct TableContext {
validator: Option<Validator>,
groups: HashMap<ParaId, Vec<ValidatorIndex>>,
validators: Vec<ValidatorId>,
disabled_validators: Vec<ValidatorIndex>,
}

impl TableContextTrait for TableContext {
Expand Down Expand Up @@ -983,37 +984,46 @@ async fn construct_per_relay_parent_state<Context>(

let parent = relay_parent;

let (validators, groups, session_index, cores) = futures::try_join!(
let (validators, groups, session_index, cores, disabled_validators) = futures::try_join!(
request_validators(parent, ctx.sender()).await,
request_validator_groups(parent, ctx.sender()).await,
request_session_index_for_child(parent, ctx.sender()).await,
request_from_runtime(parent, ctx.sender(), |tx| {
RuntimeApiRequest::AvailabilityCores(tx)
},)
.await,
request_from_runtime(parent, ctx.sender(), |tx| {
RuntimeApiRequest::DisabledValidators(tx)
},)
.await,
)
.map_err(Error::JoinMultiple)?;

let validators: Vec<_> = try_runtime_api!(validators);
let (validator_groups, group_rotation_info) = try_runtime_api!(groups);
let session_index = try_runtime_api!(session_index);
let cores = try_runtime_api!(cores);
let disabled_validators = try_runtime_api!(disabled_validators);

let signing_context = SigningContext { parent_hash: parent, session_index };
let validator =
match Validator::construct(&validators, signing_context.clone(), keystore.clone()) {
Ok(v) => Some(v),
Err(util::Error::NotAValidator) => None,
Err(e) => {
gum::warn!(
target: LOG_TARGET,
err = ?e,
"Cannot participate in candidate backing",
);
let validator = match Validator::construct(
&validators,
&disabled_validators,
signing_context.clone(),
keystore.clone(),
) {
Ok(v) => Some(v),
Err(util::Error::NotAValidator) => None,
Err(e) => {
gum::warn!(
target: LOG_TARGET,
err = ?e,
"Cannot participate in candidate backing",
);

return Ok(None)
},
};
return Ok(None)
},
};

let mut groups = HashMap::new();
let n_cores = cores.len();
Expand Down Expand Up @@ -1043,7 +1053,7 @@ async fn construct_per_relay_parent_state<Context>(
}
}

let table_context = TableContext { groups, validators, validator };
let table_context = TableContext { validator, groups, validators, disabled_validators };
let table_config = TableConfig {
allow_multiple_seconded: match mode {
ProspectiveParachainsMode::Enabled { .. } => true,
Expand Down Expand Up @@ -1552,6 +1562,8 @@ async fn import_statement<Context>(

let stmt = primitive_statement_to_table(statement);

// TODO: check if `stmt.sender` is in disabled validators for the job

Ok(rp_state.table.import_statement(&rp_state.table_context, stmt))
}

Expand Down Expand Up @@ -1943,6 +1955,8 @@ async fn handle_second_message<Context>(
return Ok(())
}

// TODO: do nothing if we are disabled

// If the message is a `CandidateBackingMessage::Second`, sign and dispatch a
// Seconded statement only if we have not signed a Valid statement for the requested candidate.
//
Expand Down
11 changes: 11 additions & 0 deletions polkadot/node/core/backing/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,16 @@ async fn test_startup(virtual_overseer: &mut VirtualOverseer, test_state: &TestS
tx.send(Ok(test_state.availability_cores.clone())).unwrap();
}
);

// Check that subsystem job issues a request for the disabled validators.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::DisabledValidators(tx))
) if parent == test_state.relay_parent => {
tx.send(Ok(Vec::new())).unwrap();
}
);
}

// Test that a `CandidateBackingMessage::Second` issues validation work
Expand Down Expand Up @@ -1418,6 +1428,7 @@ fn candidate_backing_reorders_votes() {

let table_context = TableContext {
validator: None,
disabled_validators: Vec::new(),
groups: validator_groups,
validators: validator_public.clone(),
};
Expand Down
10 changes: 10 additions & 0 deletions polkadot/node/core/backing/src/tests/prospective_parachains.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,16 @@ async fn activate_leaf(
tx.send(Ok(test_state.availability_cores.clone())).unwrap();
}
);

// Check that the subsystem job issues a request for the disabled validators.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::DisabledValidators(tx))
) if parent == hash => {
tx.send(Ok(Vec::new())).unwrap();
}
);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1675,6 +1675,8 @@ async fn handle_incoming_message<'a, Context>(

let mut _span = handle_incoming_span.child("notify-backing");

// TODO: check if peer is disabled?

// When we receive a new message from a peer, we forward it to the
// candidate backing subsystem.
ctx.send_message(CandidateBackingMessage::Statement(relay_parent, statement_with_pvd))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1583,6 +1583,8 @@ async fn send_backing_fresh_statements<Context>(
})
.expect("statements refer to same candidate; qed");

// TODO: check if peer is disabled?

ctx.send_message(CandidateBackingMessage::Statement(*relay_parent, carrying_pvd))
.await;
}
Expand Down
19 changes: 16 additions & 3 deletions polkadot/node/subsystem-util/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ pub struct Validator {
signing_context: SigningContext,
key: ValidatorId,
index: ValidatorIndex,
disabled: bool,
}

impl Validator {
Expand All @@ -391,30 +392,37 @@ impl Validator {
// Note: request_validators and request_session_index_for_child do not and cannot
// run concurrently: they both have a mutable handle to the same sender.
// However, each of them returns a oneshot::Receiver, and those are resolved concurrently.
let (validators, session_index) = futures::try_join!(
let (validators, session_index, disabled_validators) = futures::try_join!(
request_validators(parent, sender).await,
request_session_index_for_child(parent, sender).await,
request_disabled_validators(parent, sender).await,
)?;

let signing_context = SigningContext { session_index: session_index?, parent_hash: parent };

let validators = validators?;

Self::construct(&validators, signing_context, keystore)
let disabled_validators = disabled_validators?;

Self::construct(&validators, &disabled_validators, signing_context, keystore)
}

/// Construct a validator instance without performing runtime fetches.
///
/// This can be useful if external code also needs the same data.
pub fn construct(
validators: &[ValidatorId],
disabled_validators: &[ValidatorIndex],
signing_context: SigningContext,
keystore: KeystorePtr,
) -> Result<Self, Error> {
let (key, index) =
signing_key_and_index(validators, &keystore).ok_or(Error::NotAValidator)?;

Ok(Validator { signing_context, key, index })
let disabled =
disabled_validators.iter().find(|d: &&ValidatorIndex| **d == index).is_some();

Ok(Validator { signing_context, key, index, disabled })
}

/// Get this validator's id.
Expand All @@ -427,6 +435,11 @@ impl Validator {
self.index
}

/// Get the enabled/disabled state of this validator
pub fn disabled(&self) -> bool {
self.disabled
}

/// Get the current signing context.
pub fn signing_context(&self) -> &SigningContext {
&self.signing_context
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/parachains/src/session_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ impl<T: Config> Pallet<T> {
for idx in old_earliest_stored_session..new_earliest_stored_session {
Sessions::<T>::remove(&idx);
// Idx will be missing for a few sessions after the runtime upgrade.
// But it shouldn'be be a problem.
// But it shouldn't be a problem.
AccountKeys::<T>::remove(&idx);
SessionExecutorParams::<T>::remove(&idx);
}
Expand Down

0 comments on commit b5c6f07

Please sign in to comment.