Skip to content
This repository has been 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 all 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: 0 additions & 1 deletion roadmap/implementers-guide/src/disputes-flow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ stateDiagram-v2
[*] --> WaitForDisputeVote: backing Vote received
WaitForBackingVote --> Open: negative Vote received
WaitForDisputeVote --> Open: backing Vote received
Open --> Concluded: Timeout without supermajority
Open --> Concluded: Incoming Vote via Gossip
Open --> Open: No ⅔ supermajority
Open --> [*]
Expand Down
2 changes: 1 addition & 1 deletion roadmap/implementers-guide/src/protocol-disputes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,6 @@ Validators are rewarded for providing statements to the chain as well as for par

## Dispute Conclusion

Disputes, roughly, are over when one side reaches a ⅔ supermajority. They may also conclude after a timeout, without either side witnessing supermajority, which will only happen if the majority of validators are unable to vote for some reason. Furthermore, disputes on-chain will stay open for some fixed amount of time even after concluding, to accept new votes.
Disputes, roughly, are over when one side reaches a ⅔ supermajority. They may also never conclude without either side witnessing supermajority, which will only happen if the majority of validators are unable to vote for some reason. Furthermore, disputes on-chain will stay open for some fixed amount of time even after concluding, to accept new votes.

Late votes, after the dispute already reached a ⅔ supermajority, must be rewarded (albeit a smaller amount) as well.
2 changes: 1 addition & 1 deletion roadmap/implementers-guide/src/runtime/disputes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ Frozen: Option<BlockNumber>,

## Block Initialization

1. Iterate through all disputes. If any have not concluded and started more than `config.dispute_conclusion_by_timeout_period` blocks ago, set them to `Concluded` and mildly punish all validators associated, as they have failed to distribute available data.
This is currently a `no op`.

## Routines

Expand Down
2 changes: 0 additions & 2 deletions roadmap/implementers-guide/src/types/runtime.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ struct HostConfiguration {
pub dispute_post_conclusion_acceptance_period: BlockNumber,
/// The maximum number of dispute spam slots
pub dispute_max_spam_slots: u32,
/// How long it takes for a dispute to conclude by time-out, if no supermajority is reached.
pub dispute_conclusion_by_time_out_period: BlockNumber,
/// The amount of consensus slots that must pass between submitting an assignment and
/// submitting an approval vote before a validator is considered a no-show.
/// Must be at least 1.
Expand Down
1 change: 1 addition & 0 deletions runtime/kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1481,6 +1481,7 @@ pub type Migrations = (
Runtime,
NominationPoolsMigrationV4OldPallet,
>,
parachains_configuration::migration::v5::MigrateToV5<Runtime>,
);

/// Unchecked extrinsic type as expected by this runtime.
Expand Down
19 changes: 0 additions & 19 deletions runtime/parachains/src/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,6 @@ pub struct HostConfiguration<BlockNumber> {
pub dispute_period: SessionIndex,
/// How long after dispute conclusion to accept statements.
pub dispute_post_conclusion_acceptance_period: BlockNumber,
/// How long it takes for a dispute to conclude by time-out, if no supermajority is reached.
pub dispute_conclusion_by_time_out_period: BlockNumber,
/// The amount of consensus slots that must pass between submitting an assignment and
/// submitting an approval vote before a validator is considered a no-show.
///
Expand Down Expand Up @@ -262,7 +260,6 @@ impl<BlockNumber: Default + From<u32>> Default for HostConfiguration<BlockNumber
max_validators: None,
dispute_period: 6,
dispute_post_conclusion_acceptance_period: 100.into(),
dispute_conclusion_by_time_out_period: 200.into(),
n_delay_tranches: Default::default(),
zeroth_delay_tranche_width: Default::default(),
needed_approvals: Default::default(),
Expand Down Expand Up @@ -765,22 +762,6 @@ pub mod pallet {
})
}

/// Set the dispute conclusion by time out period.
#[pallet::call_index(17)]
#[pallet::weight((
T::WeightInfo::set_config_with_block_number(),
DispatchClass::Operational,
))]
pub fn set_dispute_conclusion_by_time_out_period(
origin: OriginFor<T>,
new: T::BlockNumber,
) -> DispatchResult {
ensure_root(origin)?;
Self::schedule_config_update(|config| {
config.dispute_conclusion_by_time_out_period = new;
})
}

/// Set the no show slots, in number of number of consensus slots.
/// Must be at least 1.
#[pallet::call_index(18)]
Expand Down
193 changes: 107 additions & 86 deletions runtime/parachains/src/configuration/migration.rs
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 0 additions & 6 deletions runtime/parachains/src/configuration/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ fn setting_pending_config_members() {
max_validators: None,
dispute_period: 239,
dispute_post_conclusion_acceptance_period: 10,
dispute_conclusion_by_time_out_period: 512,
no_show_slots: 240,
n_delay_tranches: 241,
zeroth_delay_tranche_width: 242,
Expand Down Expand Up @@ -389,11 +388,6 @@ fn setting_pending_config_members() {
new_config.dispute_post_conclusion_acceptance_period,
)
.unwrap();
Configuration::set_dispute_conclusion_by_time_out_period(
RuntimeOrigin::root(),
new_config.dispute_conclusion_by_time_out_period,
)
.unwrap();
Configuration::set_no_show_slots(RuntimeOrigin::root(), new_config.no_show_slots).unwrap();
Configuration::set_n_delay_tranches(RuntimeOrigin::root(), new_config.n_delay_tranches)
.unwrap();
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
sandreim marked this conversation as resolved.
Show resolved Hide resolved
{
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
1 change: 1 addition & 0 deletions runtime/polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,7 @@ pub type Migrations = (
Runtime,
NominationPoolsMigrationV4OldPallet,
>,
parachains_configuration::migration::v5::MigrateToV5<Runtime>,
);

/// Unchecked extrinsic type as expected by this runtime.
Expand Down
2 changes: 1 addition & 1 deletion runtime/rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1494,7 +1494,7 @@ pub type UncheckedExtrinsic =
/// All migrations that will run on the next runtime upgrade.
///
/// Should be cleared after every release.
pub type Migrations = ();
pub type Migrations = parachains_configuration::migration::v5::MigrateToV5<Runtime>;

/// Executive: handles dispatch to the various modules.
pub type Executive = frame_executive::Executive<
Expand Down
1 change: 1 addition & 0 deletions runtime/westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,7 @@ pub type Migrations = (
Runtime,
NominationPoolsMigrationV4OldPallet,
>,
parachains_configuration::migration::v5::MigrateToV5<Runtime>,
);

/// Unchecked extrinsic type as expected by this runtime.
Expand Down