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

Commit

Permalink
Enforce target candidate is part of the signature
Browse files Browse the repository at this point in the history
Signed-off-by: Alexandru Gheorghe <[email protected]>
  • Loading branch information
alexggh committed Aug 2, 2023
1 parent fd4b906 commit 2271c71
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 40 deletions.
8 changes: 4 additions & 4 deletions node/core/approval-voting/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2268,9 +2268,9 @@ where
let candidate_hashes: Vec<CandidateHash> =
approved_candidates_info.iter().map(|candidate| candidate.1).collect();
// Signature check:
match DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(
candidate_hashes.clone(),
))
match DisputeStatement::Valid(
ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone()),
)
.check_signature(
&pubkey,
*candidate_hashes.first().expect("Checked above this is not empty; qed"),
Expand Down Expand Up @@ -3243,7 +3243,7 @@ fn sign_approval(
) -> Option<ValidatorSignature> {
let key = keystore.key_pair::<ValidatorPair>(public).ok().flatten()?;

let payload = ApprovalVoteMultipleCandidates(candidate_hashes).signing_payload(session_index);
let payload = ApprovalVoteMultipleCandidates(&candidate_hashes).signing_payload(session_index);

Some(key.sign(&payload[..]))
}
Expand Down
18 changes: 9 additions & 9 deletions node/core/approval-voting/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1975,9 +1975,9 @@ fn test_signing_a_single_candidate_is_backwards_compatible() {
session_index,
);

assert!(DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(vec![
candidate_hash
]))
assert!(DisputeStatement::Valid(
ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(vec![candidate_hash])
)
.check_signature(&Sr25519Keyring::Alice.public().into(), candidate_hash, session_index, &sig_c,)
.is_ok());

Expand All @@ -1990,9 +1990,9 @@ fn test_signing_a_single_candidate_is_backwards_compatible() {
)
.is_ok());

assert!(DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(vec![
candidate_hash
]))
assert!(DisputeStatement::Valid(
ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(vec![candidate_hash])
)
.check_signature(&Sr25519Keyring::Alice.public().into(), candidate_hash, session_index, &sig_a,)
.is_ok());

Expand All @@ -2002,9 +2002,9 @@ fn test_signing_a_single_candidate_is_backwards_compatible() {
session_index,
);

assert!(DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(
candidate_hashes.clone()
))
assert!(DisputeStatement::Valid(
ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone())
)
.check_signature(
&Sr25519Keyring::Alice.public().into(),
*candidate_hashes.first().expect("test"),
Expand Down
2 changes: 1 addition & 1 deletion node/core/dispute-coordinator/src/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ impl ImportResult {
{
let pub_key = &env.session_info().validators.get(index).expect("indices are validated by approval-voting subsystem; qed");
let session_index = env.session_index();
candidate_hashes.contains(&votes.candidate_receipt.hash()) && DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(candidate_hashes.clone()))
candidate_hashes.contains(&votes.candidate_receipt.hash()) && DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone()))
.check_signature(pub_key, *candidate_hashes.first().expect("Valid votes have at least one candidate; qed"), session_index, &sig)
.is_ok()
},
Expand Down
20 changes: 16 additions & 4 deletions node/primitives/src/disputes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ pub struct SignedDisputeStatement {
session_index: SessionIndex,
}

/// Errors encountered while signing a dispute statement
#[derive(Debug)]
pub enum SignedDisputeStatementError {
/// Encountered a keystore error while signing
KeyStoreError(KeystoreError),
/// Could not generate signing payload
PayloadError,
}

/// Tracked votes on candidates, for the purposes of dispute resolution.
#[derive(Debug, Clone)]
pub struct CandidateVotes {
Expand Down Expand Up @@ -108,7 +117,7 @@ impl ValidCandidateVotes {
ValidDisputeStatementKind::BackingSeconded(_) => false,
ValidDisputeStatementKind::Explicit |
ValidDisputeStatementKind::ApprovalChecking |
ValidDisputeStatementKind::ApprovalCheckingV2(_) => {
ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(_) => {
occupied.insert((kind.clone(), sig));
kind != occupied.get().0
},
Expand Down Expand Up @@ -214,16 +223,19 @@ impl SignedDisputeStatement {
candidate_hash: CandidateHash,
session_index: SessionIndex,
validator_public: ValidatorId,
) -> Result<Option<Self>, KeystoreError> {
) -> Result<Option<Self>, SignedDisputeStatementError> {
let dispute_statement = if valid {
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)
} else {
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)
};

let data = dispute_statement.payload_data(candidate_hash, session_index);
let data = dispute_statement
.payload_data(candidate_hash, session_index)
.map_err(|_| SignedDisputeStatementError::PayloadError)?;
let signature = keystore
.sr25519_sign(ValidatorId::ID, validator_public.as_ref(), &data)?
.sr25519_sign(ValidatorId::ID, validator_public.as_ref(), &data)
.map_err(SignedDisputeStatementError::KeyStoreError)?
.map(|sig| Self {
dispute_statement,
candidate_hash,
Expand Down
43 changes: 27 additions & 16 deletions primitives/src/v5/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1069,9 +1069,9 @@ impl ApprovalVote {

/// A vote of approvalf for multiple candidates.
#[derive(Clone, RuntimeDebug)]
pub struct ApprovalVoteMultipleCandidates(pub Vec<CandidateHash>);
pub struct ApprovalVoteMultipleCandidates<'a>(pub &'a Vec<CandidateHash>);

impl ApprovalVoteMultipleCandidates {
impl<'a> ApprovalVoteMultipleCandidates<'a> {
/// Yields the signing payload for this approval vote.
pub fn signing_payload(&self, session_index: SessionIndex) -> Vec<u8> {
const MAGIC: [u8; 4] = *b"APPR";
Expand Down Expand Up @@ -1260,28 +1260,39 @@ pub enum DisputeStatement {

impl DisputeStatement {
/// Get the payload data for this type of dispute statement.
pub fn payload_data(&self, candidate_hash: CandidateHash, session: SessionIndex) -> Vec<u8> {
pub fn payload_data(
&self,
candidate_hash: CandidateHash,
session: SessionIndex,
) -> Result<Vec<u8>, ()> {
match self {
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) =>
ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload(),
Ok(ExplicitDisputeStatement { valid: true, candidate_hash, session }
.signing_payload()),
DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(
inclusion_parent,
)) => CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext {
)) => Ok(CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext {
session_index: session,
parent_hash: *inclusion_parent,
}),
})),
DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) =>
CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext {
Ok(CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext {
session_index: session,
parent_hash: *inclusion_parent,
}),
})),
DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) =>
ApprovalVote(candidate_hash).signing_payload(session),
DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(
candidate_hashes,
)) => ApprovalVoteMultipleCandidates(candidate_hashes.clone()).signing_payload(session),
Ok(ApprovalVote(candidate_hash).signing_payload(session)),
DisputeStatement::Valid(
ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes),
) =>
if candidate_hashes.contains(&candidate_hash) {
Ok(ApprovalVoteMultipleCandidates(candidate_hashes).signing_payload(session))
} else {
Err(())
},
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) =>
ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload(),
Ok(ExplicitDisputeStatement { valid: false, candidate_hash, session }
.signing_payload()),
}
}

Expand All @@ -1293,7 +1304,7 @@ impl DisputeStatement {
session: SessionIndex,
validator_signature: &ValidatorSignature,
) -> Result<(), ()> {
let payload = self.payload_data(candidate_hash, session);
let payload = self.payload_data(candidate_hash, session)?;

if validator_signature.verify(&payload[..], &validator_public) {
Ok(())
Expand Down Expand Up @@ -1325,7 +1336,7 @@ impl DisputeStatement {
Self::Valid(ValidDisputeStatementKind::BackingValid(_)) => true,
Self::Valid(ValidDisputeStatementKind::Explicit) |
Self::Valid(ValidDisputeStatementKind::ApprovalChecking) |
Self::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(_)) |
Self::Valid(ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(_)) |
Self::Invalid(_) => false,
}
}
Expand All @@ -1350,7 +1361,7 @@ pub enum ValidDisputeStatementKind {
/// TODO: Fixme this probably means we can't create this version
/// untill all nodes have been updated to support it.
#[codec(index = 4)]
ApprovalCheckingV2(Vec<CandidateHash>),
ApprovalCheckingMultipleCandidates(Vec<CandidateHash>),
}

/// Different kinds of statements of invalidity on a candidate.
Expand Down
17 changes: 11 additions & 6 deletions runtime/parachains/src/disputes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1330,24 +1330,29 @@ fn check_signature(
statement: &DisputeStatement,
validator_signature: &ValidatorSignature,
) -> Result<(), ()> {
let payload = match *statement {
let payload = match statement {
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) =>
ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload(),
DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(inclusion_parent)) =>
CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext {
session_index: session,
parent_hash: inclusion_parent,
parent_hash: *inclusion_parent,
}),
DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) =>
CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext {
session_index: session,
parent_hash: inclusion_parent,
parent_hash: *inclusion_parent,
}),
DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) =>
ApprovalVote(candidate_hash).signing_payload(session),
DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingV2(_)) =>
// TODO: Fixme
ApprovalVoteMultipleCandidates(vec![candidate_hash]).signing_payload(session),
DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(
candidates,
)) =>
if candidates.contains(&candidate_hash) {

This comment has been minimized.

Copy link
@rphmeier

rphmeier Aug 17, 2023

Contributor

This seems ~fine, though the linear complexity in the search isn't great IMO. The runtime is pretty resource constrained.

This comment has been minimized.

Copy link
@alexggh

alexggh Aug 18, 2023

Author Contributor

The size of this array should be in single digits numbers, since we do want the size of the use space to get out of hand , so in that case the linear complexity shouldn't be a concern and given we are doing some "heavy" work here like checking the signature, I don't think we would gain much with non-linear optimisation, don't you agree ?

ApprovalVoteMultipleCandidates(candidates).signing_payload(session)
} else {
return Err(())
},
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) =>
ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload(),
};
Expand Down

0 comments on commit 2271c71

Please sign in to comment.