Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling of disabled validators in backing subsystem #1259

Merged
merged 44 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
800d8e5
Add `disabled_validators` in staging runtime api
tdimitrov Aug 10, 2023
936216d
Cumulus changes
tdimitrov Sep 4, 2023
7165ef9
Handling of disabled validators in backing subsystem
tdimitrov Aug 31, 2023
24b5df5
Tests
tdimitrov Aug 31, 2023
eaca5ed
Merge branch 'master' into tsv-disabling-node-side
tdimitrov Sep 4, 2023
e4dee17
Fix compilation errors
tdimitrov Sep 4, 2023
23023c4
Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing
tdimitrov Sep 5, 2023
8db76f5
spelling
tdimitrov Sep 5, 2023
e2846b2
clippy
tdimitrov Sep 5, 2023
8604b69
fmt
tdimitrov Sep 5, 2023
78afa76
clippy
tdimitrov Sep 5, 2023
9e39e58
Fix tests
tdimitrov Sep 5, 2023
0d0129c
Merge branch 'master' into tsv-disabling-node-side
tdimitrov Sep 11, 2023
23fbb41
Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing
tdimitrov Sep 11, 2023
91d1a8d
Demote log level of a trace
tdimitrov Sep 11, 2023
3d7efab
Code review feedback
tdimitrov Sep 13, 2023
e68fa6a
tests
tdimitrov Sep 13, 2023
5e7655f
Merge branch 'master' into tsv-disabling-node-side
tdimitrov Sep 16, 2023
c6bca9c
Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing
tdimitrov Sep 16, 2023
6fb4b7f
merge master and resolve conflicts (v8)
ordian Sep 27, 2023
70dc6b6
implement disabled_validators correctly
ordian Sep 27, 2023
bca3c83
add a CAVEAT comment
ordian Sep 27, 2023
f717d0b
cargo fmt
ordian Sep 27, 2023
2a5af89
Clarify docs
ordian Sep 27, 2023
2c53894
Merge branch 'master' into tsv-disabling-node-side
tdimitrov Sep 28, 2023
26866fa
Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing
ordian Sep 28, 2023
a3bcb2f
Merge branch 'master' into tsv-disabling-node-side
ordian Oct 3, 2023
863fc7d
Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing
ordian Oct 3, 2023
c44093d
Merge branch 'master' into tsv-disabling-backing
ordian Oct 16, 2023
1e561e9
Merge branch 'tsv-disabling' into tsv-disabling-backing
tdimitrov Oct 16, 2023
cf671f4
Update polkadot/node/core/backing/src/lib.rs
tdimitrov Oct 17, 2023
8807981
Don't kikoff validation work if we are not a validator
tdimitrov Oct 17, 2023
55f8e01
Merge branch 'tsv-disabling' into tsv-disabling-backing
tdimitrov Oct 17, 2023
b5609e1
Merge branch 'tsv-disabling' into tsv-disabling-backing
tdimitrov Oct 18, 2023
617e477
Extract `has_required_runtime` from provisioner to subsystem-util
tdimitrov Oct 19, 2023
d7f4315
Handle old runtimes
tdimitrov Oct 19, 2023
718f04b
Fix tests
tdimitrov Oct 19, 2023
275ce02
Use the specialized runtime request function for disabled validators
tdimitrov Oct 19, 2023
f2b17f1
Fix log level
tdimitrov Oct 19, 2023
7872f8f
Merge branch 'tsv-disabling' into tsv-disabling-backing
tdimitrov Oct 20, 2023
3c89059
Extract `get_disabled_validators_with_fallback` in a separate module …
tdimitrov Oct 20, 2023
15b5069
Small style fix
tdimitrov Oct 20, 2023
96c1358
Use `get_disabled_validators_with_fallback` in `Validator` from subsy…
tdimitrov Oct 20, 2023
1a6648a
Fix comments
tdimitrov Oct 20, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,13 @@ impl RuntimeApiSubsystemClient for BlockChainRpcClient {
Ok(self.rpc_client.parachain_host_minimum_backing_votes(at, session_index).await?)
}

async fn disabled_validators(
&self,
at: Hash,
) -> Result<Vec<polkadot_primitives::ValidatorIndex>, ApiError> {
Ok(self.rpc_client.parachain_host_disabled_validators(at).await?)
}

async fn staging_async_backing_params(&self, at: Hash) -> Result<AsyncBackingParams, ApiError> {
Ok(self.rpc_client.parachain_host_staging_async_backing_params(at).await?)
}
Expand Down
8 changes: 8 additions & 0 deletions cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,14 @@ impl RelayChainRpcClient {
.await
}

pub async fn parachain_host_disabled_validators(
&self,
at: RelayHash,
) -> Result<Vec<ValidatorIndex>, RelayChainError> {
self.call_remote_runtime_function("ParachainHost_disabled_validators", at, None::<()>)
.await
}

#[allow(missing_docs)]
pub async fn parachain_host_staging_async_backing_params(
&self,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use constants::{
bridge_hub_polkadot, bridge_hub_rococo, collectives, kusama, penpal, polkadot, rococo, westend,
};
use impls::{RococoWococoMessageHandler, WococoRococoMessageHandler};
use polkadot_primitives::runtime_api::runtime_decl_for_parachain_host::ParachainHostV6;

// Substrate
use frame_support::traits::OnInitialize;
Expand Down
71 changes: 55 additions & 16 deletions polkadot/node/core/backing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,19 @@ 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 {
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
self.disabled_validators
.iter()
.any(|disabled_val_idx| *disabled_val_idx == *validator_idx)
}

pub fn local_validator_is_disabled(&self) -> Option<bool> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to return Option ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.validator is an Option. Afair the code can be executed in cases where the local node is not a validator?

I decided to return an Option here too and handle it at the caller.

self.validator.as_ref().map(|v| v.disabled())
}
}

impl TableContextTrait for TableContext {
Expand Down Expand Up @@ -992,14 +1005,18 @@ async fn construct_per_relay_parent_state<Context>(

let parent = relay_parent;

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

Expand All @@ -1009,22 +1026,27 @@ async fn construct_per_relay_parent_state<Context>(
let cores = try_runtime_api!(cores);
let minimum_backing_votes =
try_runtime_api!(request_min_backing_votes(parent, session_index, ctx.sender()).await);
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 @@ -1054,7 +1076,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 @@ -1564,7 +1586,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!(
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
target: LOG_TARGET,
sender_validator_idx = ?stmt.sender,
"Not importing statement because the sender is disabled"
);
return Ok(None)
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
} 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 @@ -1958,6 +1990,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 processing 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
63 changes: 62 additions & 1 deletion polkadot/node/core/backing/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use sp_keyring::Sr25519Keyring;
use sp_keystore::Keystore;
use sp_tracing as _;
use statement_table::v2::Misbehavior;
use std::collections::HashMap;
use std::{collections::HashMap, time::Duration};

mod prospective_parachains;

Expand Down Expand Up @@ -81,6 +81,7 @@ struct TestState {
signing_context: SigningContext,
relay_parent: Hash,
minimum_backing_votes: u32,
disabled_validators: Vec<ValidatorIndex>,
}

impl TestState {
Expand Down Expand Up @@ -152,6 +153,7 @@ impl Default for TestState {
signing_context,
relay_parent,
minimum_backing_votes: LEGACY_MIN_BACKING_VOTES,
disabled_validators: Vec::new(),
}
}
}
Expand Down Expand Up @@ -287,6 +289,16 @@ async fn test_startup(virtual_overseer: &mut VirtualOverseer, test_state: &TestS
}
);

// 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(test_state.disabled_validators.clone())).unwrap();
}
);

// Check if subsystem job issues a request for the minimum backing votes.
assert_matches!(
virtual_overseer.recv().await,
Expand Down Expand Up @@ -1596,6 +1608,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 Expand Up @@ -2256,3 +2269,51 @@ fn new_leaf_view_doesnt_clobber_old() {
virtual_overseer
});
}

// Test that a disabled local validator doesn't do any work on `CandidateBackingMessage::Second`
#[test]
fn disabled_validator_doesnt_distribute_statement() {
tdimitrov marked this conversation as resolved.
Show resolved Hide resolved
let mut test_state = TestState::default();
test_state.disabled_validators.push(ValidatorIndex(0));

test_harness(test_state.keystore.clone(), |mut virtual_overseer| async move {
test_startup(&mut virtual_overseer, &test_state).await;

let pov = PoV { block_data: BlockData(vec![42, 43, 44]) };
let pvd = dummy_pvd();
let validation_code = ValidationCode(vec![1, 2, 3]);

let expected_head_data = test_state.head_data.get(&test_state.chain_ids[0]).unwrap();

let pov_hash = pov.hash();
let candidate = TestCandidateBuilder {
para_id: test_state.chain_ids[0],
relay_parent: test_state.relay_parent,
pov_hash,
head_data: expected_head_data.clone(),
erasure_root: make_erasure_root(&test_state, pov.clone(), pvd.clone()),
persisted_validation_data_hash: pvd.hash(),
validation_code: validation_code.0.clone(),
}
.build();

let second = CandidateBackingMessage::Second(
test_state.relay_parent,
candidate.to_plain(),
pvd.clone(),
pov.clone(),
);

virtual_overseer.send(FromOrchestra::Communication { msg: second }).await;

// Ensure backing subsystem is not doing any work
assert_matches!(virtual_overseer.recv().timeout(Duration::from_secs(1)).await, None);

virtual_overseer
.send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves(
ActiveLeavesUpdate::stop_work(test_state.relay_parent),
)))
.await;
virtual_overseer
});
}
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 @@ -185,6 +185,16 @@ async fn activate_leaf(
}
);

// 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();
}
);

// Check if subsystem job issues a request for the minimum backing votes.
assert_matches!(
virtual_overseer.recv().await,
Expand Down
18 changes: 18 additions & 0 deletions polkadot/node/core/runtime-api/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ pub(crate) struct RequestResultCache {
key_ownership_proof:
LruMap<(Hash, ValidatorId), Option<vstaging::slashing::OpaqueKeyOwnershipProof>>,
minimum_backing_votes: LruMap<SessionIndex, u32>,
disabled_validators: LruMap<Hash, Vec<ValidatorIndex>>,

staging_para_backing_state: LruMap<(Hash, ParaId), Option<vstaging::BackingState>>,
staging_async_backing_params: LruMap<Hash, vstaging::AsyncBackingParams>,
Expand Down Expand Up @@ -99,6 +100,7 @@ impl Default for RequestResultCache {
unapplied_slashes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
key_ownership_proof: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
minimum_backing_votes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
disabled_validators: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),

staging_para_backing_state: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
staging_async_backing_params: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
Expand Down Expand Up @@ -448,6 +450,21 @@ impl RequestResultCache {
self.minimum_backing_votes.insert(session_index, minimum_backing_votes);
}

pub(crate) fn disabled_validators(
&mut self,
relay_parent: &Hash,
) -> Option<&Vec<ValidatorIndex>> {
self.disabled_validators.get(relay_parent).map(|v| &*v)
}

pub(crate) fn cache_disabled_validators(
&mut self,
relay_parent: Hash,
disabled_validators: Vec<ValidatorIndex>,
) {
self.disabled_validators.insert(relay_parent, disabled_validators);
}

pub(crate) fn staging_para_backing_state(
&mut self,
key: (Hash, ParaId),
Expand Down Expand Up @@ -524,6 +541,7 @@ pub(crate) enum RequestResult {
vstaging::slashing::OpaqueKeyOwnershipProof,
Option<()>,
),
DisabledValidators(Hash, Vec<ValidatorIndex>),

StagingParaBackingState(Hash, ParaId, Option<vstaging::BackingState>),
StagingAsyncBackingParams(Hash, vstaging::AsyncBackingParams),
Expand Down
10 changes: 10 additions & 0 deletions polkadot/node/core/runtime-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ where
.requests_cache
.cache_key_ownership_proof((relay_parent, validator_id), key_ownership_proof),
SubmitReportDisputeLost(_, _, _, _) => {},
DisabledValidators(relay_parent, disabled_validators) =>
self.requests_cache.cache_disabled_validators(relay_parent, disabled_validators),

StagingParaBackingState(relay_parent, para_id, constraints) => self
.requests_cache
Expand Down Expand Up @@ -297,6 +299,8 @@ where
Request::SubmitReportDisputeLost(dispute_proof, key_ownership_proof, sender)
},
),
Request::DisabledValidators(sender) => query!(disabled_validators(), sender)
.map(|sender| Request::DisabledValidators(sender)),

Request::StagingParaBackingState(para, sender) =>
query!(staging_para_backing_state(para), sender)
Expand Down Expand Up @@ -569,6 +573,12 @@ where
ver = Request::MINIMUM_BACKING_VOTES_RUNTIME_REQUIREMENT,
sender
),
Request::DisabledValidators(sender) => query!(
DisabledValidators,
disabled_validators(),
ver = Request::DISABLED_VALIDATORS_RUNTIME_REQUIREMENT,
sender
),

Request::StagingParaBackingState(para, sender) => {
query!(
Expand Down
4 changes: 4 additions & 0 deletions polkadot/node/core/runtime-api/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ impl RuntimeApiSubsystemClient for MockSubsystemClient {
Ok(self.authorities.clone())
}

async fn disabled_validators(&self, _: Hash) -> Result<Vec<ValidatorIndex>, ApiError> {
todo!("Not required for tests")
}

async fn staging_async_backing_params(
&self,
_: Hash,
Expand Down
6 changes: 6 additions & 0 deletions polkadot/node/subsystem-types/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,9 @@ pub enum RuntimeApiRequest {
),
/// Get the minimum required backing votes.
MinimumBackingVotes(SessionIndex, RuntimeApiSender<u32>),
/// Returns all disabled validators at a given block height.
/// `V6`
DisabledValidators(RuntimeApiSender<Vec<ValidatorIndex>>),

/// Get the backing state of the given para.
/// This is a staging API that will not be available on production runtimes.
Expand Down Expand Up @@ -726,6 +729,9 @@ impl RuntimeApiRequest {
/// `MinimumBackingVotes`
pub const MINIMUM_BACKING_VOTES_RUNTIME_REQUIREMENT: u32 = 6;

/// `DisabledValidators`
pub const DISABLED_VALIDATORS_RUNTIME_REQUIREMENT: u32 = 6;

/// Minimum version for backing state, required for async backing.
///
/// 99 for now, should be adjusted to VSTAGING/actual runtime version once released.
Expand Down
Loading
Loading