Skip to content

Commit

Permalink
refactor: adversarial controls for block production (near#10107)
Browse files Browse the repository at this point in the history 
Adversarial controls are needed to test malicious behaviour on chain. At
least `malicious_chain.py` checks that a peer producing unexpected
blocks is banned.

However, the logic was not documented and messy, here I want to make it
cleaner. Current flags are, from what I see:
* adv_produce_blocks - ALWAYS set to true if someone called RPC to
trigger malicious behaviour
* adv_produce_blocks_only_valid - ALSO true if we want to produce valid
blocks. Looks like we skip only `height <= known_height` condition in
this case, but I don't want to look into impact of this for now.

I want to replace two flags with one self-descriptive enum and simplify
logic - e.g. by checking that node is adversarial from the beginning.

Also, rename `should_reschedule_block` with `can_produce_block`. I
prefer a method to return `true` in the default behaviour. It also seems
that `false` means some issue with code logic, but I didn't look deeper.

## Testing

Adversarial pytests: https://nayduck.near.org/#/run/3258

---------

Co-authored-by: Longarithm <the.aleksandr.logunov@gmail.com>
  • Loading branch information
@Longarithm
Longarithm and Longarithm authored Nov 6, 2023
1 parent 2a98e2f commit e8c1dd8
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 72 deletions.
116 changes: 49 additions & 67 deletions chain/client/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,19 @@ const BLOCK_HORIZON: u64 = 500;
/// number of blocks at the epoch start for which we will log more detailed info
pub const EPOCH_START_INFO_BLOCKS: u64 = 500;

/// Defines whether in case of adversarial block production invalid blocks can
/// be produced.
#[derive(PartialEq, Eq)]
pub enum AdvProduceBlocksMode {
All,
OnlyValid,
}

pub struct Client {
/// Adversarial controls
#[cfg(feature = "test_features")]
pub adv_produce_blocks: bool,
/// Adversarial controls - should be enabled only to test disruptive
/// behaviour on chain.
#[cfg(feature = "test_features")]
pub adv_produce_blocks_only_valid: bool,
pub adv_produce_blocks: Option<AdvProduceBlocksMode>,
#[cfg(feature = "test_features")]
pub produce_invalid_chunks: bool,
#[cfg(feature = "test_features")]
Expand Down Expand Up @@ -319,9 +326,7 @@ impl Client {
);
Ok(Self {
#[cfg(feature = "test_features")]
adv_produce_blocks: false,
#[cfg(feature = "test_features")]
adv_produce_blocks_only_valid: false,
adv_produce_blocks: None,
#[cfg(feature = "test_features")]
produce_invalid_chunks: false,
#[cfg(feature = "test_features")]
Expand Down Expand Up @@ -444,77 +449,57 @@ impl Client {
Ok(())
}

/// Check that this block height is not known yet.
fn known_block_height(&self, next_height: BlockHeight, known_height: BlockHeight) -> bool {
#[cfg(feature = "test_features")]
{
if self.adv_produce_blocks {
return false;
}
}

next_height <= known_height
}

/// Check that we are next block producer.
fn is_me_block_producer(
/// Checks couple conditions whether Client can produce new block on height
/// `height` on top of block with `prev_header`.
/// Needed to skip several checks in case of adversarial controls enabled.
/// TODO: consider returning `Result<(), Error>` as `Ok(false)` looks like
/// faulty logic.
fn can_produce_block(
&self,
prev_header: &BlockHeader,
height: BlockHeight,
account_id: &AccountId,
next_block_proposer: &AccountId,
) -> bool {
) -> Result<bool, Error> {
#[cfg(feature = "test_features")]
{
if self.adv_produce_blocks_only_valid {
return account_id == next_block_proposer;
}
if self.adv_produce_blocks {
return true;
if self.adv_produce_blocks == Some(AdvProduceBlocksMode::All) {
return Ok(true);
}
}

account_id == next_block_proposer
}

fn should_reschedule_block(
&self,
prev_hash: &CryptoHash,
prev_prev_hash: &CryptoHash,
height: BlockHeight,
known_height: BlockHeight,
account_id: &AccountId,
next_block_proposer: &AccountId,
) -> Result<bool, Error> {
if self.known_block_height(height, known_height) {
return Ok(true);
}

if !self.is_me_block_producer(account_id, next_block_proposer) {
info!(target: "client", "Produce block: chain at {}, not block producer for next block.", height);
return Ok(true);
// If we are not block proposer, skip block production.
if account_id != next_block_proposer {
info!(target: "client", height, "Skipping block production, not block producer for next block.");
return Ok(false);
}

#[cfg(feature = "test_features")]
{
if self.adv_produce_blocks {
return Ok(false);
if self.adv_produce_blocks == Some(AdvProduceBlocksMode::OnlyValid) {
return Ok(true);
}
}

if self.epoch_manager.is_next_block_epoch_start(&prev_hash)? {
// If height is known already, don't produce new block for this height.
let known_height = self.chain.store().get_latest_known()?.height;
if height <= known_height {
return Ok(false);
}

// If we are to start new epoch with this block, check if the previous
// block is caught up. If it is not the case, we wouldn't be able to
// apply the following block, so we also skip block production.
let prev_hash = prev_header.hash();
if self.epoch_manager.is_next_block_epoch_start(prev_hash)? {
let prev_prev_hash = prev_header.prev_hash();
if !self.chain.prev_block_is_caught_up(prev_prev_hash, prev_hash)? {
// Currently state for the chunks we are interested in this epoch
// are not yet caught up (e.g. still state syncing).
// We reschedule block production.
// Alex's comment:
// The previous block is not caught up for the next epoch relative to the previous
// block, which is the current epoch for this block, so this block cannot be applied
// at all yet, block production must to be rescheduled
debug!(target: "client", "Produce block: prev block is not caught up");
return Ok(true);
debug!(target: "client", height, "Skipping block production, prev block is not caught up");
return Ok(false);
}
}

Ok(false)
Ok(true)
}

pub fn get_chunk_headers_ready_for_inclusion(
Expand Down Expand Up @@ -585,7 +570,6 @@ impl Client {
height: BlockHeight,
prev_hash: CryptoHash,
) -> Result<Option<Block>, Error> {
let known_height = self.chain.store().get_latest_known()?.height;
let validator_signer = self
.validator_signer
.as_ref()
Expand All @@ -598,19 +582,16 @@ impl Client {

let prev = self.chain.get_block_header(&prev_hash)?;
let prev_height = prev.height();
let prev_prev_hash = *prev.prev_hash();
let prev_epoch_id = prev.epoch_id().clone();
let prev_next_bp_hash = *prev.next_bp_hash();

// Check and update the doomslug tip here. This guarantees that our endorsement will be in the
// doomslug witness. Have to do it before checking the ability to produce a block.
let _ = self.check_and_update_doomslug_tip()?;

if self.should_reschedule_block(
&prev_hash,
&prev_prev_hash,
if !self.can_produce_block(
&prev,
height,
known_height,
validator_signer.validator_id(),
&next_block_proposer,
)? {
Expand All @@ -628,8 +609,9 @@ impl Client {
#[cfg(not(feature = "test_features"))]
return Ok(None);
#[cfg(feature = "test_features")]
if !self.adv_produce_blocks || self.adv_produce_blocks_only_valid {
return Ok(None);
match self.adv_produce_blocks {
None | Some(AdvProduceBlocksMode::OnlyValid) => return Ok(None),
Some(AdvProduceBlocksMode::All) => {}
}
}

Expand Down
13 changes: 8 additions & 5 deletions chain/client/src/client_actor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::adapter::{
BlockApproval, BlockHeadersResponse, BlockResponse, ProcessTxRequest, ProcessTxResponse,
RecvChallenge, SetNetworkInfo, StateResponse,
};
use crate::client::{Client, EPOCH_START_INFO_BLOCKS};
use crate::client::{AdvProduceBlocksMode, Client, EPOCH_START_INFO_BLOCKS};
use crate::config_updater::ConfigUpdater;
use crate::debug::new_network_info_view;
use crate::info::{display_sync_status, InfoHelper};
Expand Down Expand Up @@ -310,9 +310,12 @@ impl Handler<WithSpanContext<NetworkAdversarialMessage>> for ClientActor {
num_blocks,
only_valid,
) => {
info!(target: "adversary", "Producing {} blocks", num_blocks);
this.client.adv_produce_blocks = true;
this.client.adv_produce_blocks_only_valid = only_valid;
info!(target: "adversary", num_blocks, "Starting adversary blocks production");
if only_valid {
this.client.adv_produce_blocks = Some(AdvProduceBlocksMode::OnlyValid);
} else {
this.client.adv_produce_blocks = Some(AdvProduceBlocksMode::All);
}
let start_height =
this.client.chain.mut_store().get_latest_known().unwrap().height + 1;
let mut blocks_produced = 0;
Expand All @@ -325,7 +328,7 @@ impl Handler<WithSpanContext<NetworkAdversarialMessage>> for ClientActor {
continue;
}
let block = block.expect("block should exist after produced");
info!(target: "adversary", "Producing {} block out of {}, height = {}", blocks_produced, num_blocks, height);
info!(target: "adversary", blocks_produced, num_blocks, height, "Producing adversary block");
this.network_adapter.send(
PeerManagerMessageRequest::NetworkRequests(
NetworkRequests::Block { block: block.clone() },
Expand Down

0 comments on commit e8c1dd8

Please sign in to comment.