Skip to content

Commit

Permalink
Handling of disabled validators in backing subsystem
Browse files Browse the repository at this point in the history
  • Loading branch information
tdimitrov committed Sep 4, 2023
1 parent 936216d commit 7165ef9
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 19 deletions.
72 changes: 56 additions & 16 deletions polkadot/node/core/backing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,20 @@ struct TableContext {
validator: Option<Validator>,
groups: HashMap<ParaId, Vec<ValidatorIndex>>,
validators: Vec<ValidatorId>,
disabled_validators: Vec<ValidatorIndex>,
}

impl TableContext {
pub fn validator_is_disabled(&self, validator_idx: &ValidatorIndex) -> bool {
self.disabled_validators
.iter()
.find(|disabled_val_idx| *disabled_val_idx == validator_idx)
.is_some()
}

pub fn local_validator_is_disabled(&self) -> Option<bool> {
self.validator.as_ref().map(|v| v.disabled())
}
}

impl TableContextTrait for TableContext {
Expand Down Expand Up @@ -983,37 +997,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 +1066,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,7 +1575,17 @@ async fn import_statement<Context>(

let stmt = primitive_statement_to_table(statement);

Ok(rp_state.table.import_statement(&rp_state.table_context, stmt))
// Don't import statement if the sender is disabled
if rp_state.table_context.validator_is_disabled(&stmt.sender) {
gum::warn!(
target: LOG_TARGET,
sender_validator_idx = ?stmt.sender,
"Not importing statement because the sender is disabled"
);
return Ok(None)
} else {
Ok(rp_state.table.import_statement(&rp_state.table_context, stmt))
}
}

/// Handles a summary received from [`import_statement`] and dispatches `Backed` notifications and
Expand Down Expand Up @@ -1943,6 +1976,13 @@ async fn handle_second_message<Context>(
return Ok(())
}

// Just return if the local validator is disabled. If we are here the local node should be a
// validator but defensively use `unwrap_or(false)`) to continue processin in this case.
if rp_state.table_context.local_validator_is_disabled().unwrap_or(false) {
gum::warn!(target: LOG_TARGET, "Local validator is disabled. Don't validate and second");
return Ok(())
}

// 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
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

0 comments on commit 7165ef9

Please sign in to comment.