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

paras_inherent: reject only candidates with concluded disputes #3969

Merged
merged 2 commits into from
Sep 29, 2021
Merged

paras_inherent: reject only candidates with concluded disputes #3969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78f67db
paras_inherent: reject only candidates with concluded disputes
ordian Sep 29, 2021
3f5dd43
remove unused Error variant
ordian Sep 29, 2021
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
23 changes: 11 additions & 12 deletions runtime/parachains/src/disputes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,8 @@ pub trait DisputesHandler<BlockNumber> {
included_in: BlockNumber,
);

/// Whether the given candidate could be invalid, i.e. there is an ongoing
/// or concluded dispute with supermajority-against.
fn could_be_invalid(session: SessionIndex, candidate_hash: CandidateHash) -> bool;
/// Whether the given candidate concluded invalid in a dispute with supermajority.
fn concluded_invalid(session: SessionIndex, candidate_hash: CandidateHash) -> bool;

/// Called by the initializer to initialize the configuration module.
fn initializer_initialize(now: BlockNumber) -> Weight;
Expand Down Expand Up @@ -165,7 +164,7 @@ impl<BlockNumber> DisputesHandler<BlockNumber> for () {
) {
}

fn could_be_invalid(_session: SessionIndex, _candidate_hash: CandidateHash) -> bool {
fn concluded_invalid(_session: SessionIndex, _candidate_hash: CandidateHash) -> bool {
false
}

Expand Down Expand Up @@ -201,8 +200,8 @@ impl<T: Config> DisputesHandler<T::BlockNumber> for pallet::Pallet<T> {
pallet::Pallet::<T>::note_included(session, candidate_hash, included_in)
}

fn could_be_invalid(session: SessionIndex, candidate_hash: CandidateHash) -> bool {
pallet::Pallet::<T>::could_be_invalid(session, candidate_hash)
fn concluded_invalid(session: SessionIndex, candidate_hash: CandidateHash) -> bool {
pallet::Pallet::<T>::concluded_invalid(session, candidate_hash)
}

fn initializer_initialize(now: T::BlockNumber) -> Weight {
Expand Down Expand Up @@ -1114,10 +1113,10 @@ impl<T: Config> Pallet<T> {
}
}

pub(crate) fn could_be_invalid(session: SessionIndex, candidate_hash: CandidateHash) -> bool {
pub(crate) fn concluded_invalid(session: SessionIndex, candidate_hash: CandidateHash) -> bool {
<Disputes<T>>::get(&session, &candidate_hash).map_or(false, |dispute| {
// A dispute that is ongoing or has concluded with supermajority-against.
dispute.concluded_at.is_none() || has_supermajority_against(&dispute)
// A dispute that has concluded with supermajority-against.
has_supermajority_against(&dispute)
})
}

Expand Down Expand Up @@ -2207,9 +2206,9 @@ mod tests {
]
);

assert_eq!(Pallet::<Test>::could_be_invalid(3, candidate_hash.clone()), false); // It has 5 votes for
assert_eq!(Pallet::<Test>::could_be_invalid(4, candidate_hash.clone()), true);
assert_eq!(Pallet::<Test>::could_be_invalid(5, candidate_hash.clone()), true);
assert!(!Pallet::<Test>::concluded_invalid(3, candidate_hash.clone()));
assert!(!Pallet::<Test>::concluded_invalid(4, candidate_hash.clone()));
assert!(Pallet::<Test>::concluded_invalid(5, candidate_hash.clone()));

// Ensure inclusion removes spam slots
assert_eq!(SpamSlots::<Test>::get(4), Some(vec![0, 0, 1, 1, 0, 0, 0]));
Expand Down
10 changes: 5 additions & 5 deletions runtime/parachains/src/paras_inherent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ pub mod pallet {
/// The hash of the submitted parent header doesn't correspond to the saved block hash of
/// the parent.
InvalidParentHeader,
/// Potentially invalid candidate.
CandidateCouldBeInvalid,
/// Disputed candidate that was concluded invalid.
CandidateConcludedInvalid,
}

/// Whether the paras inherent was included within this block.
Expand Down Expand Up @@ -238,14 +238,14 @@ pub mod pallet {
let backed_candidates = limit_backed_candidates::<T>(backed_candidates);
let backed_candidates_len = backed_candidates.len() as Weight;

// Refuse to back any candidates that are disputed or invalid.
// Refuse to back any candidates that were disputed and are concluded invalid.
for candidate in &backed_candidates {
ensure!(
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the point of this check in the first place. Candidates can only be disputed once included, so how can a candidate be disputed before it is even backed? I mean obviously because of a fake dispute, but in a legit way?

Copy link
Member

@eskimor eskimor Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Answering my own question - in theory via forks. Candidate is about to be backed on two forks, one is faster - where it gets backed and included and disputed, while the backing did not even happen on the other fork - especially with chain reversions/reorgs and such that could actually happen. The check is still not needed, I think, because if it gets backed, it will get included, then checked by approval voters and disputed again - correct?

Just testing my understanding here.... It could help with avoiding potential endless loops, any other motivation?

Copy link
Member

Choose a reason for hiding this comment

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

Avoiding endless loops is a good one, just curious whether I am missing something else.

Copy link
Member

@eskimor eskimor Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I should think more before I type: I actually think this can not happen at all. The disputes are recorded in chain state. So for a candidate to be disputed and considered for being backed it must have a relay parent that either is the block that contained the dispute or some later block. Even in the case of a chain reversion, the presence of the dispute, rules out the existence of such a candidate. As the dispute must have happened in one of the parents (even if transplanted), but if that's the case then the relay parent of the candidate cannot be the current head, as the dispute can not have happened for a candidate with a relay parent that has not existed yet - that's just not possible.

Candidate x with relay_parent: A

Relay Parent A: |Relay parent containing dispute for candidate x|

How?!

Copy link
Member

Choose a reason for hiding this comment

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

Talking to myself again: It could happen in the future, with contextual execution, then the relay parent of a candidate can be older. 💡 Which makes me realize, we should be really careful with contextual execution...

Copy link
Contributor

@rphmeier rphmeier Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand the point of this check in the first place. Candidates can only be disputed once included, so how can a candidate be disputed before it is even backed? I mean obviously because of a fake dispute, but in a legit way?

It's the entire point of remote disputes. Typical flow:

B1[C1 backed] <- B2[C1 included] <- B3[C1 disputed]
      |
      | (B2 reverted)
      |
       \>  B2'[C1 disputed and dropped]

An adversary with a few block authors could withhold a small chain and wait for the reversion to successfully back the bad block unless this check is respected:

B1[C1 backed] <- B2[C1 included] <- B3[C1 disputed]
      |
      | (B2 reverted)
      |
      |
B1'[withheld by adversary until dispute]  <- B2''[C1 disputed] <- B3''[C1 backed]

This would land B3'' in an impossible state where C1 has already been disputed and cannot be disputed again.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rphmeier for those really useful illustrations! Ok, I understand that we need to drop an already backed candidate in case we find it disputed, but isn't the above code just dropping candidates that enter backing state in this very block? Like, in my understanding the enter function gets called with inherents of the currently imported block.

If that is the case then B3''[C1 backed] with the meaning that C1 gets backed in this block cannot exist as C1 would need to have B2'' as relay parent which already contained the dispute of C1, therefore C1 cannot have B2'' as relay parent. In fact the relay parent of C1 must be B0 (common ancestor of both B1' and B1). (B1'[witheld by adversary until dispute] must had C1 already backed, the adversary cannot just transplant a candidate to a later block as then the relay parent would not match)

My reasoning is based on the assumption that a candidate must be based on the relay parent right before the block where it is backed, which reflects my current understanding of things 🤔 .

Copy link
Member

Choose a reason for hiding this comment

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

Which is indeed ensured:

polkadot/runtime/parachains/src/inclusion.rs

Line 454 in 7bf12d6

candidate.descriptor().relay_parent == parent_hash,

Copy link
Contributor

Choose a reason for hiding this comment

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

@eskimor Good point on the relay parent. The current code would still be affected if B1'[C1 disputed, C1 backed] if we assume that B1 and B1' have the same parent B0

Copy link
Member

Choose a reason for hiding this comment

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

Uuh, because disputes are imported before candidates - so disputes of the current block will be already available in the state, regardless of being added in the same block. 💡 Got it, thanks!

!T::DisputesHandler::could_be_invalid(
!T::DisputesHandler::concluded_invalid(
current_session,
candidate.candidate.hash(),
),
Error::<T>::CandidateCouldBeInvalid,
Error::<T>::CandidateConcludedInvalid,
);
}

Expand Down