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

Gracefully handle filtering bitfields & candidates #4280

Merged
emostov merged 17 commits into from
Nov 15, 2021
Merged

Gracefully handle filtering bitfields & candidates #4280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
04d5d0a
Don't Ever error with sanitize + rand changes
emostov Nov 13, 2021
93b6918
Merge branch 'bernhard-inherent-filtering' into zeke-bernhard-inheren…
emostov Nov 13, 2021
f26d668
Do not return result for sanitize_bitfields / process bitfields
emostov Nov 13, 2021
f419eca
Update test for process_bitfields
emostov Nov 14, 2021
5c424a1
sanitize_backe_candidate return vec; not result
emostov Nov 14, 2021
8963ae9
Fix signature check test'
emostov Nov 14, 2021
91c42c4
Some small fixes
emostov Nov 14, 2021
98679a9
Some small fixes
emostov Nov 14, 2021
7d330e8
Rename const generice for sanitize bitfields
emostov Nov 15, 2021
8b239cf
Use is_empty instead of .len() == 0
emostov Nov 15, 2021
a414390
Use consts as args to const generic params
emostov Nov 15, 2021
e1c32e6
Assert result in happy path of sanitize_backed_candidates
emostov Nov 15, 2021
e0e59d6
Add trace logs for bitfields with issues
emostov Nov 15, 2021
d9ef2ca
Log CHECK_SIGS
emostov Nov 15, 2021
b063a11
Try merge feat branch in
emostov Nov 15, 2021
9a2dda9
Spacing
emostov Nov 15, 2021
ad9e675
Some small fixes
emostov Nov 15, 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
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

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

6 changes: 6 additions & 0 deletions primitives/src/v1/signed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,12 @@ impl<Payload: EncodeAs<RealPayload>, RealPayload: Encode> UncheckedSigned<Payloa
pub fn benchmark_signature(&self) -> ValidatorSignature {
self.signature.clone()
}

/// Set the signature. Only should be used for creating testing mocks.
#[cfg(feature = "std")]
pub fn set_signature(&mut self, signature: ValidatorSignature) {
self.signature = signature
}
}

impl<Payload, RealPayload> From<Signed<Payload, RealPayload>>
Expand Down
1 change: 0 additions & 1 deletion runtime/parachains/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ hex-literal = "0.3.3"
keyring = { package = "sp-keyring", git = "https://github.com/paritytech/substrate", branch = "master" }
frame-support-test = { git = "https://github.com/paritytech/substrate", branch = "master" }
sc-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
assert_matches = "1"

[features]
default = ["std"]
Expand Down
201 changes: 128 additions & 73 deletions runtime/parachains/src/inclusion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

use crate::{
configuration, disputes, dmp, hrmp, paras,
paras_inherent::{sanitize_bitfields, DisputedBitfield},
paras_inherent::{sanitize_bitfields, DisputedBitfield, VERIFY_SIGS},
scheduler::CoreAssignment,
shared, ump,
};
Expand Down Expand Up @@ -398,19 +398,19 @@ impl<T: Config> Pallet<T> {
signed_bitfields: UncheckedSignedAvailabilityBitfields,
disputed_bitfield: DisputedBitfield,
core_lookup: impl Fn(CoreIndex) -> Option<ParaId>,
) -> Result<Vec<(CoreIndex, CandidateHash)>, DispatchError> {
) -> Vec<(CoreIndex, CandidateHash)> {
Copy link
Copy Markdown
Contributor

@drahnr drahnr Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We should consider returning Result<Vec<(CoreIndex)>, Vec<(CoreIndex)>> to be able to easily evaluate if the sanitization caused any changes and be able to warn outside this fn call and also keep the assert_noop! storage check macro.

This would be a rather backwards change, so I am happy to discuss this.

Copy link
Copy Markdown
Contributor Author

@emostov emostov Nov 15, 2021

Choose a reason for hiding this comment

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

We can use assert_storage_noop! if we need to check for storage root changes https://crates.parity.io/frame_support/macro.assert_storage_noop.html

What storage changes though do you want the system to keep track of? It seems like the calling logic doesn't have any handling based on if storage changes are made here

Copy link
Copy Markdown
Contributor

@drahnr drahnr Nov 15, 2021

Choose a reason for hiding this comment

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

I can't follow. The Ok()/Err() would be independent of the storage_noop and we should verify that no storage was written in case we do error (which from the tests is never the case), which I missed. So please discard that part :]

let validators = shared::Pallet::<T>::active_validator_keys();
let session_index = shared::Pallet::<T>::session_index();
let parent_hash = frame_system::Pallet::<T>::parent_hash();

let checked_bitfields = sanitize_bitfields::<T, true>(
let checked_bitfields = sanitize_bitfields::<T, VERIFY_SIGS>(
signed_bitfields,
disputed_bitfield,
expected_bits,
parent_hash,
session_index,
&validators[..],
)?;
);

let freed_cores = Self::update_pending_availability_and_get_freed_cores::<_, true>(
expected_bits,
Expand All @@ -419,7 +419,7 @@ impl<T: Config> Pallet<T> {
core_lookup,
);

Ok(freed_cores)
freed_cores
}

/// Process candidates that have been backed. Provide the relay storage root, a set of candidates
Expand Down Expand Up @@ -1008,7 +1008,6 @@ pub(crate) mod tests {
paras_inherent::DisputedBitfield,
scheduler::AssignmentKind,
};
use assert_matches::assert_matches;
use frame_support::assert_noop;
use futures::executor::block_on;
use keyring::Sr25519Keyring;
Expand Down Expand Up @@ -1349,7 +1348,7 @@ pub(crate) mod tests {
}
let validator_public = validator_pubkeys(&validators);

new_test_ext(genesis_config(paras)).execute_with(|| {
new_test_ext(genesis_config(paras.clone())).execute_with(|| {
shared::Pallet::<Test>::set_active_validators_ascending(validator_public.clone());
shared::Pallet::<Test>::set_session_index(5);

Expand All @@ -1364,6 +1363,19 @@ pub(crate) mod tests {
_ => panic!("out of bounds for testing"),
};

// mark all candidates as pending availability
let set_pending_av = || {
for (p_id, _) in paras {
PendingAvailability::<Test>::insert(
p_id,
CandidatePendingAvailability {
availability_votes: default_availability_votes(),
..Default::default()
},
)
}
};
Copy link
Copy Markdown
Contributor

@drahnr drahnr Nov 15, 2021

Choose a reason for hiding this comment

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

👍


// too many bits in bitfield
{
let mut bare_bitfield = default_bitfield();
Expand All @@ -1376,14 +1388,14 @@ pub(crate) mod tests {
&signing_context,
));

assert_noop!(
assert_eq!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Error::<Test>::WrongBitfieldSize
vec![]
);
}

Expand All @@ -1398,48 +1410,77 @@ pub(crate) mod tests {
&signing_context,
));

assert_noop!(
assert_eq!(
ParaInclusion::process_bitfields(
expected_bits() + 1,
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Error::<Test>::WrongBitfieldSize
vec![]
);
}

// duplicate.
{
let bare_bitfield = default_bitfield();
set_pending_av.clone()();
let back_core_0_bitfield = {
let mut b = default_bitfield();
b.0.set(0, true);
b
};
let signed: UncheckedSignedAvailabilityBitfield = block_on(sign_bitfield(
&keystore,
&validators[0],
ValidatorIndex(0),
bare_bitfield,
back_core_0_bitfield,
&signing_context,
))
.into();

assert_noop!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.clone(), signed],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Error::<Test>::BitfieldDuplicateOrUnordered
assert_eq!(
<PendingAvailability<Test>>::get(chain_a)
.unwrap()
.availability_votes
.count_ones(),
0
);

// the threshold to free a core is 4 availability votes, but we only expect 1 valid
// valid bitfield.
assert!(ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.clone(), signed],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
)
.is_empty());

assert_eq!(
<PendingAvailability<Test>>::get(chain_a)
.unwrap()
.availability_votes
.count_ones(),
1
);

// clean up
PendingAvailability::<Test>::remove_all(None);
}

// out of order.
{
let bare_bitfield = default_bitfield();
Copy link
Copy Markdown
Contributor

@drahnr drahnr Nov 15, 2021

Choose a reason for hiding this comment

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

👍

set_pending_av.clone()();
let back_core_0_bitfield = {
let mut b = default_bitfield();
b.0.set(0, true);
b
};
let signed_0 = block_on(sign_bitfield(
&keystore,
&validators[0],
ValidatorIndex(0),
bare_bitfield.clone(),
back_core_0_bitfield.clone(),
&signing_context,
))
.into();
Expand All @@ -1448,20 +1489,38 @@ pub(crate) mod tests {
&keystore,
&validators[1],
ValidatorIndex(1),
bare_bitfield,
back_core_0_bitfield,
&signing_context,
))
.into();

assert_noop!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed_1, signed_0],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Error::<Test>::BitfieldDuplicateOrUnordered
assert_eq!(
<PendingAvailability<Test>>::get(chain_a)
.unwrap()
.availability_votes
.count_ones(),
0
);

// the threshold to free a core is 4 availability votes, but we only expect 1 valid
// valid bitfield because `signed_0` will get skipped for being out of order.
assert!(ParaInclusion::process_bitfields(
expected_bits(),
vec![signed_1, signed_0],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
)
.is_empty());

assert_eq!(
<PendingAvailability<Test>>::get(chain_a)
.unwrap()
.availability_votes
.count_ones(),
1
);

PendingAvailability::<Test>::remove_all(None);
}

// non-pending bit set.
Expand All @@ -1476,18 +1535,16 @@ pub(crate) mod tests {
&signing_context,
));

assert_matches!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Ok(_)
);
assert!(ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
)
.is_empty());
}

// empty bitfield signed: always OK, but kind of useless.
// empty bitfield signed: always ok, but kind of useless.
{
let bare_bitfield = default_bitfield();
let signed = block_on(sign_bitfield(
Expand All @@ -1498,15 +1555,13 @@ pub(crate) mod tests {
&signing_context,
));

assert_matches!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Ok(_)
);
assert!(ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
)
.is_empty());
}

// bitfield signed with pending bit signed.
Expand Down Expand Up @@ -1543,15 +1598,13 @@ pub(crate) mod tests {
&signing_context,
));

assert_matches!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Ok(_)
);
assert!(ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
)
.is_empty());

<PendingAvailability<Test>>::remove(chain_a);
PendingAvailabilityCommitments::<Test>::remove(chain_a);
Expand Down Expand Up @@ -1588,15 +1641,13 @@ pub(crate) mod tests {
));

// no core is freed
assert_eq!(
ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Ok(vec![])
);
assert!(ParaInclusion::process_bitfields(
expected_bits(),
vec![signed.into()],
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
)
.is_empty());
}
});
}
Expand Down Expand Up @@ -1652,15 +1703,18 @@ pub(crate) mod tests {
CandidatePendingAvailability {
core: CoreIndex::from(0),
hash: candidate_a.hash(),
descriptor: candidate_a.descriptor,
descriptor: candidate_a.clone().descriptor,
availability_votes: default_availability_votes(),
relay_parent_number: 0,
backed_in_number: 0,
backers: backing_bitfield(&[3, 4]),
backing_group: GroupIndex::from(0),
},
);
PendingAvailabilityCommitments::<Test>::insert(chain_a, candidate_a.commitments);
PendingAvailabilityCommitments::<Test>::insert(
chain_a,
candidate_a.clone().commitments,
);

let candidate_b = TestCandidateBuilder {
para_id: chain_b,
Expand Down Expand Up @@ -1732,14 +1786,15 @@ pub(crate) mod tests {
})
.collect();

assert_matches!(
// only chain A's core is freed.
assert_eq!(
ParaInclusion::process_bitfields(
expected_bits(),
signed_bitfields,
DisputedBitfield::zeros(expected_bits()),
&core_lookup,
),
Ok(_)
vec![(CoreIndex(0), candidate_a.hash())]
);

// chain A had 4 signing off, which is >= threshold.
Expand Down
Loading