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

inherent disputes: remove per block initializer and disputes timeout event #6937

Merged
merged 15 commits into from
Mar 24, 2023
Merged

inherent disputes: remove per block initializer and disputes timeout event #6937

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4e0f65b
Limit disputes weight and remove initializer code
sandreim Mar 22, 2023
de17b48
const
sandreim Mar 22, 2023
882e7d3
remove timeout test
sandreim Mar 22, 2023
a5ba764
review feedback #1
sandreim Mar 22, 2023
91efd2b
Remove the new weight limiting for disputes
sandreim Mar 23, 2023
4790096
cargo lock
sandreim Mar 23, 2023
96a5804
Remove dispute_conclusion_by_time_out_period
sandreim Mar 23, 2023
4aa0170
Enable migrations
sandreim Mar 23, 2023
0c52e6b
Update guide
sandreim Mar 23, 2023
0b98d33
Fix comment
sandreim Mar 23, 2023
2cc2574
More guide fixes
sandreim Mar 23, 2023
93d9b3a
Merge branch 'master' of github.com:paritytech/polkadot into sandreim…
sandreim Mar 24, 2023
3abaf1f
Also migrate pending configs
sandreim Mar 24, 2023
eb00f89
fix build
sandreim Mar 24, 2023
589733c
fix test
sandreim Mar 24, 2023
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions runtime/parachains/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ sp-core = { git = "https://github.com/paritytech/substrate", branch = "master",
sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master", optional = true }
sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false, optional = true }
sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false, optional = true }
sp-arithmetic = { package = "sp-arithmetic", git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }

pallet-authority-discovery = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
pallet-authorship = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
Expand Down
27 changes: 3 additions & 24 deletions runtime/parachains/src/disputes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

use crate::{configuration, initializer::SessionChangeNotification, session_info};
use bitvec::{bitvec, order::Lsb0 as BitOrderLsb0};
use frame_support::{ensure, traits::Get, weights::Weight};
use frame_support::{ensure, weights::Weight};
use frame_system::pallet_prelude::*;
use parity_scale_codec::{Decode, Encode};
use primitives::{
Expand Down Expand Up @@ -503,9 +503,6 @@ pub mod pallet {
/// A dispute has concluded for or against a candidate.
/// `\[para id, candidate hash, dispute result\]`
DisputeConcluded(CandidateHash, DisputeResult),
/// A dispute has timed out due to insufficient participation.
/// `\[para id, candidate hash\]`
DisputeTimedOut(CandidateHash),
/// A dispute has concluded with supermajority against a candidate.
/// Block authors should no longer build on top of this head and should
/// instead revert the block at the given height. This should be the
Expand Down Expand Up @@ -911,26 +908,8 @@ impl StatementSetFilter {

impl<T: Config> Pallet<T> {
/// Called by the initializer to initialize the disputes module.
pub(crate) fn initializer_initialize(now: T::BlockNumber) -> Weight {
let config = <configuration::Pallet<T>>::config();

let mut weight = Weight::zero();
for (session_index, candidate_hash, mut dispute) in <Disputes<T>>::iter() {
weight += T::DbWeight::get().reads_writes(1, 0);

if dispute.concluded_at.is_none() &&
dispute.start + config.dispute_conclusion_by_time_out_period < now
{
Self::deposit_event(Event::DisputeTimedOut(candidate_hash));

dispute.concluded_at = Some(now);
Copy link
Member

Choose a reason for hiding this comment

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

As discussed with @eskimor we don't really need the timeout event

Could you post the gist of the discussion here? This code not only removes timeout events, but the notion of timing out for disputes. I'm lacking the context here. cc @burdges

Copy link
Member

Choose a reason for hiding this comment

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

Yes:

  1. Disputes that hit the runtime should actually never time out - they have been confirmed, so they should conclude.
  2. We don't have a timeout on the node side. So timing them out in the runtime is kind of pointless and out of sync with what the node would assume about the dispute.
  3. We drop disputes after 6 sessions anyway.
  4. We can not and should not slash anybody on a timed out dispute - the people who voted are the good guys (most likely).

Copy link
Member

Choose a reason for hiding this comment

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

We drop disputes after 6 sessions anyway.

Yes, but maybe we should be reverting a block if the dispute hasn't concluded instead of finalizing it after some time?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, what we could and probably should do: Revert immediately once f+1 validators voted against a candidate.

<Disputes<T>>::insert(session_index, candidate_hash, &dispute);

weight += T::DbWeight::get().writes(1);
}
}

weight
pub(crate) fn initializer_initialize(_now: T::BlockNumber) -> Weight {
Weight::zero()
}

/// Called by the initializer to finalize the disputes pallet.
Expand Down
125 changes: 0 additions & 125 deletions runtime/parachains/src/disputes/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,131 +329,6 @@ fn test_import_backing_votes() {
);
}

// Test that dispute timeout is handled correctly.
#[test]
fn test_dispute_timeout() {
let dispute_conclusion_by_time_out_period = 3;
let start = 10;

let mock_genesis_config = MockGenesisConfig {
configuration: crate::configuration::GenesisConfig {
config: HostConfiguration {
dispute_conclusion_by_time_out_period,
..Default::default()
},
..Default::default()
},
..Default::default()
};

new_test_ext(mock_genesis_config).execute_with(|| {
// We need 7 validators for the byzantine threshold to be 2
let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
let v1 = <ValidatorId as CryptoType>::Pair::generate().0;
let v2 = <ValidatorId as CryptoType>::Pair::generate().0;
let v3 = <ValidatorId as CryptoType>::Pair::generate().0;
let v4 = <ValidatorId as CryptoType>::Pair::generate().0;
let v5 = <ValidatorId as CryptoType>::Pair::generate().0;
let v6 = <ValidatorId as CryptoType>::Pair::generate().0;

run_to_block(start, |b| {
// a new session at each block
Some((
true,
b,
vec![
(&0, v0.public()),
(&1, v1.public()),
(&2, v2.public()),
(&3, v3.public()),
(&4, v4.public()),
(&5, v5.public()),
(&6, v6.public()),
],
Some(vec![
(&0, v0.public()),
(&1, v1.public()),
(&2, v2.public()),
(&3, v3.public()),
(&4, v4.public()),
(&5, v5.public()),
(&6, v6.public()),
]),
))
});

let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1));
let inclusion_parent = sp_core::H256::repeat_byte(0xff);

// v0 and v1 vote for 3, v2 against. We need f+1 votes (3) so that the dispute is
// confirmed. Otherwise It will be filtered out.
let session = start - 1;
let stmts = vec![DisputeStatementSet {
candidate_hash: candidate_hash.clone(),
session,
statements: vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(
inclusion_parent,
)),
ValidatorIndex(0),
v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload(
&SigningContext { session_index: start - 1, parent_hash: inclusion_parent },
)),
),
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(1),
v1.sign(
&ExplicitDisputeStatement {
valid: true,
candidate_hash: candidate_hash.clone(),
session: start - 1,
}
.signing_payload(),
),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(6),
v2.sign(
&ExplicitDisputeStatement {
valid: false,
candidate_hash: candidate_hash.clone(),
session: start - 1,
}
.signing_payload(),
),
),
],
}];

let stmts = filter_dispute_set(stmts);

assert_ok!(
Pallet::<Test>::process_checked_multi_dispute_data(&stmts),
vec![(9, candidate_hash.clone())],
);

// Run to timeout period
run_to_block(start + dispute_conclusion_by_time_out_period, |_| None);
assert!(<Disputes<Test>>::get(&session, &candidate_hash)
.expect("dispute should exist")
.concluded_at
.is_none());

// Run to timeout + 1 in order to executive on_finalize(timeout)
run_to_block(start + dispute_conclusion_by_time_out_period + 1, |_| None);
assert_eq!(
<Disputes<Test>>::get(&session, &candidate_hash)
.expect("dispute should exist")
.concluded_at
.expect("dispute should have concluded"),
start + dispute_conclusion_by_time_out_period + 1
);
});
}

// Test pruning works
#[test]
fn test_initializer_on_new_session() {
Expand Down
42 changes: 22 additions & 20 deletions runtime/parachains/src/paras_inherent/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ use primitives::{
use rand::{seq::SliceRandom, SeedableRng};

use scale_info::TypeInfo;
use sp_arithmetic::Percent;
use sp_runtime::traits::{Header as HeaderT, One};
use sp_std::{
cmp::Ordering,
Expand Down Expand Up @@ -77,6 +78,7 @@ mod benchmarking;
mod tests;

const LOG_TARGET: &str = "runtime::inclusion-inherent";
const MAX_DISPUTES_WEIGHT_PERCENT: Percent = Percent::from_percent(70);

/// A bitfield concerning concluded disputes for candidates
/// associated to the core index equivalent to the bit position.
Expand Down Expand Up @@ -305,8 +307,8 @@ impl<T: Config> Pallet<T> {
full_check: FullCheck,
) -> DispatchResultWithPostInfo {
let ParachainsInherentData {
bitfields: mut signed_bitfields,
mut backed_candidates,
bitfields: signed_bitfields,
backed_candidates,
parent_header,
mut disputes,
} = data;
Expand All @@ -331,14 +333,29 @@ impl<T: Config> Pallet<T> {

let now = <frame_system::Pallet<T>>::block_number();

let mut candidates_weight = backed_candidates_weight::<T>(&backed_candidates);
let mut bitfields_weight = signed_bitfields_weight::<T>(signed_bitfields.len());
let candidates_weight = backed_candidates_weight::<T>(&backed_candidates);
let bitfields_weight = signed_bitfields_weight::<T>(signed_bitfields.len());
let disputes_weight = multi_dispute_statement_sets_weight::<T, _, _>(&disputes);

let current_session = <shared::Pallet<T>>::session_index();

let max_block_weight = <T as frame_system::Config>::BlockWeights::get().max_block;

// At most 60% of total block space will be dispute votes.
let mut max_disputes_weight = Weight::from_parts(
MAX_DISPUTES_WEIGHT_PERCENT.mul_floor(max_block_weight.ref_time()),
max_block_weight.proof_size(),
);

// Compute the excess weight value.
let spill = candidates_weight
.saturating_add(bitfields_weight)
.saturating_add(disputes_weight)
.saturating_sub(max_block_weight);

// Constrain max dispute weight to ensure block will not be overweight.
max_disputes_weight -= spill;

METRICS
.on_before_filter((candidates_weight + bitfields_weight + disputes_weight).ref_time());

Expand Down Expand Up @@ -366,28 +383,13 @@ impl<T: Config> Pallet<T> {
)
};

// In case of an overweight block, consume up to the entire block weight
// in disputes, since we will never process anything else, but invalidate
// the block. It's still reasonable to protect against a massive amount of disputes.
if candidates_weight
.saturating_add(bitfields_weight)
.saturating_add(disputes_weight)
.any_gt(max_block_weight)
{
log::warn!("Overweight para inherent data reached the runtime {:?}", parent_hash);
backed_candidates.clear();
candidates_weight = Weight::zero();
signed_bitfields.clear();
bitfields_weight = Weight::zero();
}

let entropy = compute_entropy::<T>(parent_hash);
let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into());

let (checked_disputes, checked_disputes_weight) = limit_and_sanitize_disputes::<T, _>(
disputes,
&dispute_set_validity_check,
max_block_weight,
max_disputes_weight,
&mut rng,
);
(
Expand Down