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

chain: don't skip early transaction validity checks in state witnesses #12627

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
47 changes: 43 additions & 4 deletions chain/chain/src/stateless_validation/chunk_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,18 @@ impl MainTransition {
pub struct PreValidationOutput {
pub main_transition_params: MainTransition,
pub implicit_transition_params: Vec<ImplicitTransitionParams>,
/// List of the transactions that are valid and should be processed by e.g.
/// `validate_chunk_state_witness`.
///
/// This list is exactly the length of the corresponding `ChunkStateWitness::transactions`
/// field. Element at the index N in this array corresponds to an element at index N in the
/// transactions list.
///
/// Transactions for which a `false` is stored here ought to be ignored/dropped/skipped.
///
/// All elements will be true for protocol versions where `RelaxedChunkValidation` is not
/// enabled.
pub transaction_validity_check_passed: Vec<bool>,
}

#[derive(Clone)]
Expand Down Expand Up @@ -369,11 +381,19 @@ pub fn pre_validate_chunk_state_witness(

let current_protocol_version =
epoch_manager.get_epoch_protocol_version(&state_witness.epoch_id)?;
if !checked_feature!(
let transaction_validity_check_passed = if checked_feature!(
"protocol_feature_relaxed_chunk_validation",
RelaxedChunkValidation,
current_protocol_version
) {
if last_chunk_block.header().is_genesis() {
vec![true; state_witness.transactions.len()]
Comment on lines +377 to +378
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to add this to make test that start at the genesis to pass. There is no "prev_block_header" in this instance. The code further below that relies on the previous block has a similar special case for the genesis.

This vector currently ends up not being used, but if it did, it seemed to me that considering all transactions "valid" is reasonable enough: the transactions can't be much older than any useful transaction_validity_period. On the other hand if there are additional checks added to transaction_validity_check in the future, this might no longer hold… So this is not a particularly resilient piece of code in my eyes.

} else {
let prev_block_header = store.get_block_header(last_chunk_block.header().prev_hash())?;
let mut check = chain.transaction_validity_check(prev_block_header);
state_witness.transactions.iter().map(|t| check(t)).collect::<Vec<_>>()
}
} else {
let new_transactions = &state_witness.new_transactions;
let (new_tx_root_from_state_witness, _) = merklize(&new_transactions);
let chunk_tx_root = state_witness.chunk_header.tx_root();
Expand Down Expand Up @@ -420,7 +440,8 @@ pub fn pre_validate_chunk_state_witness(
}
};
}
}
vec![true; state_witness.transactions.len()]
};

let main_transition_params = if last_chunk_block.header().is_genesis() {
let epoch_id = last_chunk_block.header().epoch_id();
Expand Down Expand Up @@ -462,7 +483,11 @@ pub fn pre_validate_chunk_state_witness(
})
};

Ok(PreValidationOutput { main_transition_params, implicit_transition_params })
Ok(PreValidationOutput {
main_transition_params,
implicit_transition_params,
transaction_validity_check_passed,
})
}

/// Validate that receipt proofs contain the receipts that should be applied during the
Expand Down Expand Up @@ -611,7 +636,21 @@ pub fn validate_chunk_state_witness(
let (mut chunk_extra, mut outgoing_receipts) =
match (pre_validation_output.main_transition_params, cache_result) {
(MainTransition::Genesis { chunk_extra, .. }, _) => (chunk_extra, vec![]),
(MainTransition::NewChunk(new_chunk_data), None) => {
(MainTransition::NewChunk(mut new_chunk_data), None) => {
let mut validity_iterator =
pre_validation_output.transaction_validity_check_passed.iter();
new_chunk_data.transactions.retain(|t| {
let valid = *validity_iterator.next().unwrap();
if !valid {
tracing::debug!(
target: "chain",
message="discarding invalid transaction",
tx=%t.get_hash()
);
}
valid
});

let chunk_header = new_chunk_data.chunk_header.clone();
let NewChunkResult { apply_result: mut main_apply_result, .. } = apply_new_chunk(
ApplyChunkReason::ValidateChunkStateWitness,
nagisa marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading