Skip to content

Commit

Permalink
feat: remove stateless validation panic in test environment (#12355)
Browse files Browse the repository at this point in the history
With async witness code distribution occasional chunk validation
failures are expected due to network delays or message loss.
  • Loading branch information
pugachAG authored Oct 31, 2024
1 parent 9f86687 commit 97d586c
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 39 deletions.
4 changes: 0 additions & 4 deletions chain/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,16 +444,12 @@ impl Client {
epoch_manager.clone(),
chain.chain_store().store().clone(),
);
// Chunk validator should panic if there is a validator error in non-production chains (eg. mocket and localnet).
let panic_on_validation_error = config.chain_id != near_primitives::chains::MAINNET
&& config.chain_id != near_primitives::chains::TESTNET;
let chunk_validator = ChunkValidator::new(
epoch_manager.clone(),
network_adapter.clone().into_sender(),
runtime_adapter.clone(),
config.orphan_state_witness_pool_size,
async_computation_spawner,
panic_on_validation_error,
);
let chunk_distribution_network = ChunkDistributionNetwork::from_config(&config);
Ok(Self {
Expand Down
42 changes: 13 additions & 29 deletions chain/client/src/stateless_validation/chunk_validator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,6 @@ pub struct ChunkValidator {
orphan_witness_pool: OrphanStateWitnessPool,
validation_spawner: Arc<dyn AsyncComputationSpawner>,
main_state_transition_result_cache: chunk_validation::MainStateTransitionCache,
/// If true, a chunk-witness validation error will lead to a panic.
/// This is used for non-production environments, eg. mocknet and localnet,
/// to quickly detect issues in validation code, and must NOT be set to true
/// for mainnet and testnet.
panic_on_validation_error: bool,
}

impl ChunkValidator {
Expand All @@ -56,7 +51,6 @@ impl ChunkValidator {
runtime_adapter: Arc<dyn RuntimeAdapter>,
orphan_witness_pool_size: usize,
validation_spawner: Arc<dyn AsyncComputationSpawner>,
panic_on_validation_error: bool,
) -> Self {
Self {
epoch_manager,
Expand All @@ -66,7 +60,6 @@ impl ChunkValidator {
validation_spawner,
main_state_transition_result_cache: chunk_validation::MainStateTransitionCache::default(
),
panic_on_validation_error,
}
}

Expand Down Expand Up @@ -126,7 +119,6 @@ impl ChunkValidator {
chunk_header.shard_id(),
)?;
let shard_uid = epoch_manager.shard_id_to_uid(last_header.shard_id(), &epoch_id)?;
let panic_on_validation_error = self.panic_on_validation_error;

if let Ok(prev_chunk_extra) = chain.get_chunk_extra(prev_block_hash, &shard_uid) {
match validate_chunk_with_chunk_extra(
Expand All @@ -147,14 +139,14 @@ impl ChunkValidator {
return Ok(());
}
Err(err) => {
if panic_on_validation_error {
panic!("Failed to validate chunk using existing chunk extra: {:?}", err);
} else {
tracing::error!(
"Failed to validate chunk using existing chunk extra: {:?}",
err
);
}
tracing::error!(
target: "client",
?err,
"Failed to validate chunk using existing chunk extra",
);
near_chain::stateless_validation::metrics::CHUNK_WITNESS_VALIDATION_FAILED_TOTAL
.with_label_values(&[&shard_id.to_string(), err.prometheus_label_value()])
.inc();
return Err(err);
}
}
Expand Down Expand Up @@ -187,24 +179,16 @@ impl ChunkValidator {
near_chain::stateless_validation::metrics::CHUNK_WITNESS_VALIDATION_FAILED_TOTAL
.with_label_values(&[&shard_id.to_string(), err.prometheus_label_value()])
.inc();
if panic_on_validation_error {
panic!("Failed to validate chunk: {:?}", err);
} else {
tracing::error!("Failed to validate chunk: {:?}", err);
}
tracing::error!(
target: "client",
?err,
"Failed to validate chunk"
);
}
}
});
Ok(())
}

/// TESTING ONLY: Used to override the value of panic_on_validation_error, for example,
/// when the chunks validation errors are expected when testing adversarial behavior and
/// the test should not panic for the invalid chunks witnesses.
#[cfg(feature = "test_features")]
pub fn set_should_panic_on_validation_error(&mut self, value: bool) {
self.panic_on_validation_error = value;
}
}

pub(crate) fn send_chunk_endorsement_to_block_producers(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,9 +373,6 @@ fn test_banning_chunk_producer_when_seeing_invalid_chunk() {
init_test_logger();
let mut test = AdversarialBehaviorTestData::new();
test.env.clients[7].produce_invalid_chunks = true;
for client in test.env.clients.iter_mut() {
client.chunk_validator.set_should_panic_on_validation_error(false);
}
test_banning_chunk_producer_when_seeing_invalid_chunk_base(test);
}

Expand All @@ -385,8 +382,5 @@ fn test_banning_chunk_producer_when_seeing_invalid_tx_in_chunk() {
init_test_logger();
let mut test = AdversarialBehaviorTestData::new();
test.env.clients[7].produce_invalid_tx_in_chunks = true;
for client in test.env.clients.iter_mut() {
client.chunk_validator.set_should_panic_on_validation_error(false);
}
test_banning_chunk_producer_when_seeing_invalid_chunk_base(test);
}

0 comments on commit 97d586c

Please sign in to comment.