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

feat: avoid processing state witness multiple times #11111

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions chain/chain-primitives/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ pub enum Error {
InvalidChunkState(Box<ChunkState>),
#[error("Invalid Chunk State Witness: {0}")]
InvalidChunkStateWitness(String),
#[error("Duplicate Chunk State Witness: {0}")]
DuplicateChunkStateWitness(String),
#[error("Invalid Chunk Endorsement")]
InvalidChunkEndorsement,
/// Invalid chunk mask
Expand Down Expand Up @@ -270,6 +272,7 @@ impl Error {
| Error::InvalidChunkProofs(_)
| Error::InvalidChunkState(_)
| Error::InvalidChunkStateWitness(_)
| Error::DuplicateChunkStateWitness(_)
| Error::InvalidChunkEndorsement
| Error::InvalidChunkMask
| Error::InvalidStateRoot
Expand Down Expand Up @@ -344,6 +347,7 @@ impl Error {
Error::InvalidChunkProofs(_) => "invalid_chunk_proofs",
Error::InvalidChunkState(_) => "invalid_chunk_state",
Error::InvalidChunkStateWitness(_) => "invalid_chunk_state_witness",
Error::DuplicateChunkStateWitness(_) => "duplicate_chunk_state_witness",
Error::InvalidChunkEndorsement => "invalid_chunk_endorsement",
Error::InvalidChunkMask => "invalid_chunk_mask",
Error::InvalidStateRoot => "invalid_state_root",
Expand Down
30 changes: 27 additions & 3 deletions chain/client/src/stateless_validation/chunk_validator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ use near_primitives::merkle::merklize;
use near_primitives::receipt::Receipt;
use near_primitives::sharding::{ChunkHash, ReceiptProof, ShardChunkHeader};
use near_primitives::stateless_validation::{
ChunkEndorsement, ChunkStateWitness, ChunkStateWitnessAck, ChunkStateWitnessSize,
SignedEncodedChunkStateWitness,
ChunkEndorsement, ChunkProductionKey, ChunkStateWitness, ChunkStateWitnessAck,
ChunkStateWitnessSize, SignedEncodedChunkStateWitness,
};
use near_primitives::transaction::SignedTransaction;
use near_primitives::types::chunk_extra::ChunkExtra;
Expand All @@ -46,6 +46,8 @@ use std::sync::Arc;
// Keeping a threshold of 5 block producers should be sufficient for most scenarios.
const NUM_NEXT_BLOCK_PRODUCERS_TO_SEND_CHUNK_ENDORSEMENT: u64 = 5;

const RECEIVED_STATE_WITNESSES_CACHE_SIZE: usize = 100;

/// A module that handles chunk validation logic. Chunk validation refers to a
/// critical process of stateless validation, where chunk validators (certain
/// validators selected to validate the chunk) verify that the chunk's state
Expand All @@ -60,6 +62,7 @@ pub struct ChunkValidator {
chunk_endorsement_tracker: Arc<ChunkEndorsementTracker>,
orphan_witness_pool: OrphanStateWitnessPool,
validation_spawner: Arc<dyn AsyncComputationSpawner>,
received_witnesses: lru::LruCache<ChunkProductionKey, ()>,
}

impl ChunkValidator {
Expand All @@ -80,6 +83,7 @@ impl ChunkValidator {
chunk_endorsement_tracker,
orphan_witness_pool: OrphanStateWitnessPool::new(orphan_witness_pool_size),
validation_spawner,
received_witnesses: lru::LruCache::new(RECEIVED_STATE_WITNESSES_CACHE_SIZE),
}
}

Expand All @@ -89,7 +93,7 @@ impl ChunkValidator {
/// The chunk is validated asynchronously, if you want to wait for the processing to finish
/// you can use the `processing_done_tracker` argument (but it's optional, it's safe to pass None there).
pub fn start_validating_chunk(
&self,
&mut self,
state_witness: ChunkStateWitness,
chain: &Chain,
processing_done_tracker: Option<ProcessingDoneTracker>,
Expand All @@ -103,6 +107,8 @@ impl ChunkValidator {
)));
}

self.check_duplicate_witness(&state_witness)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm IMO this should be placed after pre_validate_chunk_state_witness().

Otherwise a malicious chunk producer can do something like:

  1. Submit valid ChunkStateWitness
  2. Submit 100 junk ChunkStateWitnesses to fill the cache with junk (possibly from other random nodes to avoid getting banned)
  3. Go to (1), the cache doesn't contain the valid ChunkStateWitness so it'll be processed again.

This way a malicious chunk producer could force the validator to process the valid ChunkStateWitness an arbitrary number of times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not be possible because of the checks as part of partially_validate_state_witness. In particular chunk producer could only submit one witness for every height it suppose to produce a chunk on. On top of that it should be on top of a not-yet-final block which is known to us (otherwise it goes to orphan pool). So chunk producer could only submit junk for a very narrow set of heights hence the cache size of 100.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I didn't notice that apart from pre_validate_chunk_state_witness we also do partially_validate_state_witness at the beginning of process_chunk_state_witness . That makes the situation better 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

But could a malicious chunk producer submit 100 fake witnesses (with existing prev_block_hash) for the next 100 heights for which it's a chunk producer? I think we don't limit the distance from the tip for non-orphan witnesses?

Copy link
Contributor Author

@pugachAG pugachAG Apr 19, 2024

Choose a reason for hiding this comment

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

Only if we don't have a final block for those 100 heights. If that happens then the blockchain is in a very degraded state and we have bigger problems to worry about 🙃 Keeping Flat Storage deltas in memory was built on the same assumptions by the way.

Copy link
Contributor

Choose a reason for hiding this comment

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

The scenario that I'm imagining would look something like this:
There's a final block F at height 100
There's a chunk producer P, responsible for producing chunks at heights 102, 104, 106, 108, 110, ...

P is malicious and constructs a fake witness for each of those heights:

  • Witness {height: 102, prev_block_hash: F, receipts: lotsofwork}
  • Witness {height: 104, prev_block_hash: F, receipts: lotsofwork}
  • Witness {height: 106, prev_block_hash: F, receipts: lotsofwork}
  • ...

Then P sends all of those witnesses to validator V.

Wouldn't V process all of those witnesses? P could fill the whole cache with them, or just make V do a lot of useless work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see, that is indeed a valid scenario, good catch!
Moving this PR to draft until I have a solution that addresses that.


let pre_validation_result = pre_validate_chunk_state_witness(
&state_witness,
chain,
Expand Down Expand Up @@ -142,6 +148,24 @@ impl ChunkValidator {
});
Ok(())
}

/// Verifies that we haven't already processed state witness for the corresponding
/// (shard_id, epoch_id, height_created). This protects against malicious chunk
/// producers wasting stateless validator resources by making it apply chunk multiple
/// times.
fn check_duplicate_witness(&mut self, state_witness: &ChunkStateWitness) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we store the chunk hash as "value" for LRU cache and add a check to see if we have received the same chunk hash? If the chunk hash of the witness is the same it can probably be deemed as a non-malicious behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically submitting the witness for the same chunk (hence with the same chunk hash) could also be an attack vector. For example another stateless validator could send a witness it received to us multiple times and in this case it is not coming from the chunk producer.
On top of that currently we don't really distinguish between "definitely malicious behaviours" and "potentially malicious behaviours", in both cases we just ignore the witness. So I suggest to keep it simple for now and extend it later if we ever need it.

let chunk_production_key = ChunkProductionKey::from_witness(&state_witness);
if self.received_witnesses.contains(&chunk_production_key) {
return Err(Error::DuplicateChunkStateWitness(format!(
"Already received witness for height {} shard {} epoch {:?}",
chunk_production_key.height_created,
chunk_production_key.shard_id,
chunk_production_key.epoch_id,
)));
}
self.received_witnesses.push(chunk_production_key, ());
Ok(())
}
}

/// Checks that proposed `transactions` are valid for a chunk with `chunk_header`.
Expand Down
21 changes: 21 additions & 0 deletions core/primitives/src/stateless_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,27 @@ impl ChunkValidatorAssignments {
}
}

/// This struct contains combination of fields that uniquely identify chunk production.
/// It means that for a given instance only one chunk could be produced.
/// The main use case it to track processed state witness in order to protect stateless
/// validator against malicious chunk producers.
#[derive(Hash, PartialEq, Eq)]
pub struct ChunkProductionKey {
pub shard_id: ShardId,
pub epoch_id: EpochId,
pub height_created: BlockHeight,
}

impl ChunkProductionKey {
pub fn from_witness(witness: &ChunkStateWitness) -> Self {
Self {
shard_id: witness.chunk_header.shard_id(),
epoch_id: witness.epoch_id.clone(),
height_created: witness.chunk_header.height_created(),
}
}
}

fn decompress_with_limit(data: &[u8], limit: usize) -> std::io::Result<Vec<u8>> {
let mut buf = Vec::new().limit(limit).writer();
match zstd::stream::copy_decode(data, &mut buf) {
Expand Down