Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

attempt to fix zombienet disputes #7618

Merged
merged 5 commits into from
Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 11 additions & 11 deletions node/malus/integrationtests/0001-dispute-valid-block.zndsl
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ bob: reports block height is at least 2
bob: reports peers count is at least 2
charlie: reports block height is at least 2
charlie: reports peers count is at least 2
alice: reports parachain_candidate_disputes_total is at least 1 within 250 seconds
bob: reports parachain_candidate_disputes_total is at least 1 within 90 seconds
charlie: reports parachain_candidate_disputes_total is at least 1 within 90 seconds
alice: reports parachain_candidate_dispute_votes{validity="valid"} is at least 1 within 90 seconds
bob: reports parachain_candidate_dispute_votes{validity="valid"} is at least 2 within 90 seconds
charlie: reports parachain_candidate_dispute_votes{validity="valid"} is at least 2 within 90 seconds
alice: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
alice: reports parachain_candidate_dispute_concluded{validity="invalid"} is 0 within 90 seconds
bob: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
charlie: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
charlie: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
alice: reports polkadot_parachain_candidate_disputes_total is at least 1 within 250 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at a test run where it worked well without polkadot prefix. Do we support both ? CC @pepoviola

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, should works with and without the prefix (polkadot).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it as I was observing some failures to read the metric

bob: reports polkadot_parachain_candidate_disputes_total is at least 1 within 90 seconds
charlie: reports polkadot_parachain_candidate_disputes_total is at least 1 within 90 seconds
alice: reports polkadot_parachain_candidate_dispute_votes{validity="valid"} is at least 1 within 90 seconds
bob: reports polkadot_parachain_candidate_dispute_votes{validity="valid"} is at least 2 within 90 seconds
charlie: reports polkadot_parachain_candidate_dispute_votes{validity="valid"} is at least 2 within 90 seconds
alice: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
alice: reports polkadot_parachain_candidate_dispute_concluded{validity="invalid"} is 0 within 90 seconds
bob: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
charlie: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
charlie: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds
73 changes: 64 additions & 9 deletions node/malus/src/variants/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use polkadot_node_subsystem::{

use polkadot_primitives::{
CandidateCommitments, CandidateDescriptor, CandidateReceipt, PersistedValidationData,
PvfExecTimeoutKind,
};

use futures::channel::oneshot;
Expand All @@ -48,6 +49,55 @@ pub enum FakeCandidateValidation {
BackingAndApprovalValid,
}

impl FakeCandidateValidation {
fn misbehaves_valid(&self) -> bool {
use FakeCandidateValidation::*;

match *self {
BackingValid | ApprovalValid | BackingAndApprovalValid => true,
_ => false,
}
}

fn misbehaves_invalid(&self) -> bool {
use FakeCandidateValidation::*;

match *self {
BackingInvalid | ApprovalInvalid | BackingAndApprovalInvalid => true,
_ => false,
}
}

fn includes_backing(&self) -> bool {
use FakeCandidateValidation::*;

match *self {
BackingInvalid | BackingAndApprovalInvalid | BackingValid | BackingAndApprovalValid =>
true,
_ => false,
}
}

fn includes_approval(&self) -> bool {
use FakeCandidateValidation::*;

match *self {
ApprovalInvalid |
BackingAndApprovalInvalid |
ApprovalValid |
BackingAndApprovalValid => true,
_ => false,
}
}

fn should_misbehave(&self, timeout: PvfExecTimeoutKind) -> bool {
match timeout {
PvfExecTimeoutKind::Backing => self.includes_backing(),
PvfExecTimeoutKind::Approval => self.includes_approval(),
}
}
}

/// Candidate invalidity details
#[derive(clap::ValueEnum, Clone, Copy, Debug, PartialEq)]
#[value(rename_all = "kebab-case")]
Expand Down Expand Up @@ -162,11 +212,20 @@ where
pub fn create_fake_candidate_commitments(
persisted_validation_data: &PersistedValidationData,
) -> CandidateCommitments {
// Backing rejects candidates which output the same head as the parent,
// therefore we must create a new head which is not equal to the parent.
let mut head_data = persisted_validation_data.parent_head.clone();
if head_data.0.is_empty() {
head_data.0.push(0);
} else {
head_data.0[0] = head_data.0[0].wrapping_add(1);
};

CandidateCommitments {
upward_messages: Default::default(),
horizontal_messages: Default::default(),
new_validation_code: None,
head_data: persisted_validation_data.parent_head.clone(),
head_data,
processed_downward_messages: 0,
hrmp_watermark: persisted_validation_data.relay_parent_number,
}
Expand Down Expand Up @@ -224,8 +283,7 @@ where
),
} => {
match self.fake_validation {
FakeCandidateValidation::ApprovalValid |
FakeCandidateValidation::BackingAndApprovalValid => {
x if x.misbehaves_valid() && x.should_misbehave(timeout) => {
// Behave normally if the `PoV` is not known to be malicious.
if pov.block_data.0.as_slice() != MALICIOUS_POV {
return Some(FromOrchestra::Communication {
Expand Down Expand Up @@ -278,8 +336,7 @@ where
},
}
},
FakeCandidateValidation::ApprovalInvalid |
FakeCandidateValidation::BackingAndApprovalInvalid => {
x if x.misbehaves_invalid() && x.should_misbehave(timeout) => {
// Set the validation result to invalid with probability `p` and trigger a
// dispute
let behave_maliciously = self.distribution.sample(&mut rand::thread_rng());
Expand Down Expand Up @@ -342,8 +399,7 @@ where
),
} => {
match self.fake_validation {
FakeCandidateValidation::BackingValid |
FakeCandidateValidation::BackingAndApprovalValid => {
x if x.misbehaves_valid() && x.should_misbehave(timeout) => {
// Behave normally if the `PoV` is not known to be malicious.
if pov.block_data.0.as_slice() != MALICIOUS_POV {
return Some(FromOrchestra::Communication {
Expand Down Expand Up @@ -385,8 +441,7 @@ where
}),
}
},
FakeCandidateValidation::BackingInvalid |
FakeCandidateValidation::BackingAndApprovalInvalid => {
x if x.misbehaves_invalid() && x.should_misbehave(timeout) => {
// Maliciously set the validation result to invalid for a valid candidate
// with probability `p`
let behave_maliciously = self.distribution.sample(&mut rand::thread_rng());
Expand Down
59 changes: 33 additions & 26 deletions parachain/test-parachains/undying/collator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ use std::{
},
time::Duration,
};
use test_parachain_undying::{execute, hash_state, BlockData, GraveyardState, HeadData};
use test_parachain_undying::{
execute, hash_state, BlockData, GraveyardState, HeadData, StateMismatch,
};

/// Default PoV size which also drives state size.
const DEFAULT_POV_SIZE: usize = 1000;
Expand All @@ -45,7 +47,7 @@ fn calculate_head_and_state_for_number(
number: u64,
graveyard_size: usize,
pvf_complexity: u32,
) -> (HeadData, GraveyardState) {
) -> Result<(HeadData, GraveyardState), StateMismatch> {
let index = 0u64;
let mut graveyard = vec![0u8; graveyard_size * graveyard_size];
let zombies = 0;
Expand All @@ -62,13 +64,12 @@ fn calculate_head_and_state_for_number(

while head.number < number {
let block = BlockData { state, tombstones: 1_000, iterations: pvf_complexity };
let (new_head, new_state) =
execute(head.hash(), head.clone(), block).expect("Produces valid block");
let (new_head, new_state) = execute(head.hash(), head.clone(), block)?;
head = new_head;
state = new_state;
}

(head, state)
Ok((head, state))
}

/// The state of the undying parachain.
Expand Down Expand Up @@ -122,39 +123,35 @@ impl State {
/// Advance the state and produce a new block based on the given `parent_head`.
///
/// Returns the new [`BlockData`] and the new [`HeadData`].
fn advance(&mut self, parent_head: HeadData) -> (BlockData, HeadData) {
fn advance(&mut self, parent_head: HeadData) -> Result<(BlockData, HeadData), StateMismatch> {
self.best_block = parent_head.number;

let state = if let Some(head_data) = self.number_to_head.get(&self.best_block) {
self.head_to_state.get(head_data).cloned().unwrap_or_else(|| {
calculate_head_and_state_for_number(
parent_head.number,
self.graveyard_size,
self.pvf_complexity,
)
.1
})
let state = if let Some(state) = self
.number_to_head
.get(&self.best_block)
.and_then(|head_data| self.head_to_state.get(head_data).cloned())
{
state
} else {
let (_, state) = calculate_head_and_state_for_number(
parent_head.number,
self.graveyard_size,
self.pvf_complexity,
);
)?;
state
};

// Start with prev state and transaction to execute (place 1000 tombstones).
let block = BlockData { state, tombstones: 1000, iterations: self.pvf_complexity };

let (new_head, new_state) =
execute(parent_head.hash(), parent_head, block.clone()).expect("Produces valid block");
let (new_head, new_state) = execute(parent_head.hash(), parent_head, block.clone())?;

let new_head_arc = Arc::new(new_head.clone());

self.head_to_state.insert(new_head_arc.clone(), new_state);
self.number_to_head.insert(new_head.number, new_head_arc);

(block, new_head)
Ok((block, new_head))
}
}

Expand Down Expand Up @@ -233,10 +230,21 @@ impl Collator {
let seconded_collations = self.seconded_collations.clone();

Box::new(move |relay_parent, validation_data| {
let parent = HeadData::decode(&mut &validation_data.parent_head.0[..])
.expect("Decodes parent head");
let parent = match HeadData::decode(&mut &validation_data.parent_head.0[..]) {
Err(err) => {
log::error!("Requested to build on top of malformed head-data: {:?}", err);
return futures::future::ready(None).boxed()
},
Ok(p) => p,
};

let (block_data, head_data) = state.lock().unwrap().advance(parent);
let (block_data, head_data) = match state.lock().unwrap().advance(parent.clone()) {
Err(err) => {
log::error!("Unable to build on top of {:?}: {:?}", parent, err);
return futures::future::ready(None).boxed()
},
Ok(x) => x,
};

log::info!(
"created a new collation on relay-parent({}): {:?}",
Expand Down Expand Up @@ -280,7 +288,6 @@ impl Collator {
"Seconded statement should match our collation: {:?}",
res.statement.payload()
);
std::process::exit(-1);
}

seconded_collations.fetch_add(1, Ordering::Relaxed);
Expand Down Expand Up @@ -394,10 +401,10 @@ mod tests {
let collator = Collator::new(1_000, 1);
let graveyard_size = collator.state.lock().unwrap().graveyard_size;

let mut head = calculate_head_and_state_for_number(10, graveyard_size, 1).0;
let mut head = calculate_head_and_state_for_number(10, graveyard_size, 1).unwrap().0;

for i in 1..10 {
head = collator.state.lock().unwrap().advance(head).1;
head = collator.state.lock().unwrap().advance(head).unwrap().1;
assert_eq!(10 + i, head.number);
}

Expand All @@ -414,7 +421,7 @@ mod tests {
.clone();

for _ in 1..20 {
second_head = collator.state.lock().unwrap().advance(second_head.clone()).1;
second_head = collator.state.lock().unwrap().advance(second_head.clone()).unwrap().1;
}

assert_eq!(second_head, head);
Expand Down
6 changes: 3 additions & 3 deletions zombienet_tests/functional/0002-parachains-disputes.zndsl
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ alice: parachain 2003 block height is at least 10 within 200 seconds

# Check if disputes are initiated and concluded.
# TODO: check if disputes are concluded faster than initiated.
eve: reports parachain_candidate_disputes_total is at least 10 within 15 seconds
eve: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 10 within 15 seconds
eve: reports parachain_candidate_dispute_concluded{validity="invalid"} is 0 within 15 seconds
eve: reports polkadot_parachain_candidate_disputes_total is at least 10 within 15 seconds
eve: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is at least 10 within 15 seconds
eve: reports polkadot_parachain_candidate_dispute_concluded{validity="invalid"} is 0 within 15 seconds

# As of <https://github.com/paritytech/polkadot/pull/6249>, we don't slash on disputes
# with `valid` outcome, so there is no offence reported.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ honest-validator-2: reports polkadot_parachain_disputes_finality_lag is lower th
sleep 30 seconds

# Check that garbage parachain blocks included by malicious validators are being disputed.
honest-validator-0: reports parachain_candidate_disputes_total is at least 2 within 15 seconds
honest-validator-1: reports parachain_candidate_disputes_total is at least 2 within 15 seconds
honest-validator-2: reports parachain_candidate_disputes_total is at least 2 within 15 seconds
honest-validator-0: reports polkadot_parachain_candidate_disputes_total is at least 2 within 15 seconds
honest-validator-1: reports polkadot_parachain_candidate_disputes_total is at least 2 within 15 seconds
honest-validator-2: reports polkadot_parachain_candidate_disputes_total is at least 2 within 15 seconds

# Disputes should always end as "invalid"
honest-validator-0: reports parachain_candidate_dispute_concluded{validity="invalid"} is at least 2 within 15 seconds
honest-validator-1: reports parachain_candidate_dispute_concluded{validity="valid"} is 0 within 15 seconds
honest-validator-0: reports polkadot_parachain_candidate_dispute_concluded{validity="invalid"} is at least 2 within 15 seconds
honest-validator-1: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is 0 within 15 seconds

# Check participating in the losing side of a dispute logged
malus-validator: log line contains "Voted for a candidate that was concluded invalid." within 180 seconds
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ honest-validator-0: parachain 1000 is registered within 100 seconds
honest-validator-0: parachain 1000 block height is at least 1 within 300 seconds

# There should be disputes initiated
honest-validator-0: reports parachain_candidate_disputes_total is at least 2 within 200 seconds
honest-validator-0: reports polkadot_parachain_candidate_disputes_total is at least 2 within 200 seconds

# Stop issuing disputes
malus-validator-0: pause
Expand All @@ -29,9 +29,9 @@ honest-validator-0: reports block height minus finalised block is at least 10 wi
alice: resume

# Disputes should start concluding now
honest-validator-0: reports parachain_candidate_dispute_concluded{validity="invalid"} is at least 1 within 200 seconds
honest-validator-0: reports polkadot_parachain_candidate_dispute_concluded{validity="invalid"} is at least 1 within 200 seconds
# Disputes should always end as "invalid"
honest-validator-0: reports parachain_candidate_dispute_concluded{validity="valid"} is 0
honest-validator-0: reports polkadot_parachain_candidate_dispute_concluded{validity="valid"} is 0

# Check an unsigned extrinsic is submitted
honest-validator: log line contains "Successfully reported pending slash" within 180 seconds
20 changes: 10 additions & 10 deletions zombienet_tests/misc/0001-paritydb.zndsl
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ validator-8: reports polkadot_parachain_approval_checking_finality_lag is 0
validator-9: reports polkadot_parachain_approval_checking_finality_lag is 0

# Check lag - dispute conclusion
validator-0: reports parachain_candidate_disputes_total is 0
validator-1: reports parachain_candidate_disputes_total is 0
validator-2: reports parachain_candidate_disputes_total is 0
validator-3: reports parachain_candidate_disputes_total is 0
validator-4: reports parachain_candidate_disputes_total is 0
validator-5: reports parachain_candidate_disputes_total is 0
validator-6: reports parachain_candidate_disputes_total is 0
validator-7: reports parachain_candidate_disputes_total is 0
validator-8: reports parachain_candidate_disputes_total is 0
validator-9: reports parachain_candidate_disputes_total is 0
validator-0: reports polkadot_parachain_candidate_disputes_total is 0
validator-1: reports polkadot_parachain_candidate_disputes_total is 0
validator-2: reports polkadot_parachain_candidate_disputes_total is 0
validator-3: reports polkadot_parachain_candidate_disputes_total is 0
validator-4: reports polkadot_parachain_candidate_disputes_total is 0
validator-5: reports polkadot_parachain_candidate_disputes_total is 0
validator-6: reports polkadot_parachain_candidate_disputes_total is 0
validator-7: reports polkadot_parachain_candidate_disputes_total is 0
validator-8: reports polkadot_parachain_candidate_disputes_total is 0
validator-9: reports polkadot_parachain_candidate_disputes_total is 0