From d390eb51791c612135f8612c40bd6f9f04789bcc Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 17 Jul 2020 03:27:49 +0200 Subject: [PATCH] inclusion: split CandidatePendingAvailability according to the guide (#1413) * inclusion: split PendingAvailability storage into descriptor and commitments * inclusion: fix tests * implementers-guide: update CandidatePendingAvailability type * inclusion: simplify process_candidates a bit * implementers-guide: more updates to the inclusion module * inclusion: fix copy-paste errors in tests * inclusion: revert some of the changes * inclusion: lazy commitments loading and a test * guide: revert enact_candidate changes * inclusion: test process_bitfield for no commitments * Grammar Co-authored-by: Robert Habermeier Co-authored-by: Robert Habermeier --- .../src/runtime/inclusion.md | 5 +- runtime/parachains/src/inclusion.rs | 201 +++++++++++++++--- 2 files changed, 170 insertions(+), 36 deletions(-) diff --git a/roadmap/implementers-guide/src/runtime/inclusion.md b/roadmap/implementers-guide/src/runtime/inclusion.md index d201e3ed51b2..7f093261d304 100644 --- a/roadmap/implementers-guide/src/runtime/inclusion.md +++ b/roadmap/implementers-guide/src/runtime/inclusion.md @@ -14,7 +14,7 @@ struct AvailabilityBitfield { struct CandidatePendingAvailability { core: CoreIndex, // availability core - receipt: CandidateReceipt, + descriptor: CandidateDescriptor, availability_votes: Bitfield, // one bit per validator. relay_parent_number: BlockNumber, // number of the relay-parent. backed_in_number: BlockNumber, @@ -65,7 +65,6 @@ All failed checks should lead to an unrecoverable error making the block invalid 1. If the core assignment includes a specific collator, ensure the backed candidate is issued by that collator. 1. Ensure that any code upgrade scheduled by the candidate does not happen within `config.validation_upgrade_frequency` of `Paras::last_code_upgrade(para_id, true)`, if any, comparing against the value of `Paras::FutureCodeUpgrades` for the given para ID. 1. Check the collator's signature on the candidate data. - 1. Transform each [`CommittedCandidateReceipt`](../types/candidate.md#committed-candidate-receipt) into the corresponding [`CandidateReceipt`](../types/candidate.md#candidate-receipt), setting the commitments aside. 1. check the backing of the candidate using the signatures and the bitfields, comparing against the validators assigned to the groups, fetched with the `group_validators` lookup. 1. check that the upward messages, when combined with the existing queue size, are not exceeding `config.max_upward_queue_count` and `config.watermark_upward_queue_size` parameters. 1. create an entry in the `PendingAvailability` map for each backed candidate with a blank `availability_votes` bitfield. @@ -81,7 +80,7 @@ All failed checks should lead to an unrecoverable error making the block invalid ```rust fn collect_pending(f: impl Fn(CoreIndex, BlockNumber) -> bool) -> Vec { // sweep through all paras pending availability. if the predicate returns true, when given the core index and - // the block number the candidate has been pending availability since, then clean up the corresponding storage for that candidate. + // the block number the candidate has been pending availability since, then clean up the corresponding storage for that candidate and the commitments. // return a vector of cleaned-up core IDs. } ``` diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index a2480f1c8034..10c019bbe1f1 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -22,14 +22,15 @@ use sp_std::prelude::*; use primitives::v1::{ - ValidatorId, CommittedCandidateReceipt, ValidatorIndex, Id as ParaId, + ValidatorId, CandidateCommitments, CandidateDescriptor, ValidatorIndex, Id as ParaId, AvailabilityBitfield as AvailabilityBitfield, SignedAvailabilityBitfields, SigningContext, - BackedCandidate, CoreIndex, GroupIndex, CoreAssignment, + BackedCandidate, CoreIndex, GroupIndex, CoreAssignment, CommittedCandidateReceipt, }; use frame_support::{ decl_storage, decl_module, decl_error, ensure, dispatch::DispatchResult, IterableStorageMap, weights::Weight, traits::Get, + debug, }; use codec::{Encode, Decode}; use bitvec::{order::Lsb0 as BitOrderLsb0, vec::BitVec}; @@ -58,8 +59,8 @@ pub struct AvailabilityBitfieldRecord { pub struct CandidatePendingAvailability { /// The availability core this is assigned to. core: CoreIndex, - /// The candidate receipt itself. - receipt: CommittedCandidateReceipt, + /// The candidate descriptor. + descriptor: CandidateDescriptor, /// The received availability votes. One bit per validator. availability_votes: BitVec, /// The block number of the relay-parent of the receipt. @@ -80,6 +81,10 @@ decl_storage! { PendingAvailability: map hasher(twox_64_concat) ParaId => Option>; + /// The commitments of candidates pending availability, by ParaId. + PendingAvailabilityCommitments: map hasher(twox_64_concat) ParaId + => Option; + /// The current validators, by their parachain session keys. Validators get(fn validators) config(validators): Vec; @@ -146,6 +151,7 @@ impl Module { ) { // unlike most drain methods, drained elements are not cleared on `Drop` of the iterator // and require consumption. + for _ in ::drain() { } for _ in >::drain() { } for _ in >::drain() { } @@ -227,14 +233,14 @@ impl Module { for (bit_idx, _) in signed_bitfield.payload().0.iter().enumerate().filter(|(_, is_av)| **is_av) { - let record = assigned_paras_record[bit_idx] + let (_, pending_availability) = assigned_paras_record[bit_idx] .as_mut() .expect("validator bitfields checked not to contain bits corresponding to unoccupied cores; qed"); // defensive check - this is constructed by loading the availability bitfield record, // which is always `Some` if the core is occupied - that's why we're here. let val_idx = signed_bitfield.validator_index() as usize; - if let Some(mut bit) = record.1.as_mut() + if let Some(mut bit) = pending_availability.as_mut() .and_then(|r| r.availability_votes.get_mut(val_idx)) { *bit = true; @@ -261,9 +267,25 @@ impl Module { { if pending_availability.availability_votes.count_ones() >= threshold { >::remove(¶_id); + let commitments = match ::take(¶_id) { + Some(commitments) => commitments, + None => { + debug::warn!(r#" + Inclusion::process_bitfields: + PendingAvailability and PendingAvailabilityCommitments + are out of sync, did someone mess with the storage? + "#); + continue; + } + }; + + let receipt = CommittedCandidateReceipt { + descriptor: pending_availability.descriptor, + commitments, + }; Self::enact_candidate( pending_availability.relay_parent_number, - pending_availability.receipt, + receipt, ); freed_cores.push(pending_availability.core); @@ -358,7 +380,7 @@ impl Module { for (i, assignment) in scheduled[skip..].iter().enumerate() { check_assignment_in_order(assignment)?; - if candidate.descriptor().para_id == assignment.para_id { + if para_id == assignment.para_id { if let Some(required_collator) = assignment.required_collator() { ensure!( required_collator == &candidate.descriptor().collator, @@ -367,7 +389,8 @@ impl Module { } ensure!( - >::get(&assignment.para_id).is_none(), + >::get(¶_id).is_none() && + ::get(¶_id).is_none(), Error::::CandidateScheduledBeforeParaFree, ); @@ -427,13 +450,15 @@ impl Module { // initialize all availability votes to 0. let availability_votes: BitVec = bitvec::bitvec![BitOrderLsb0, u8; 0; validators.len()]; + let (descriptor, commitments) = (candidate.candidate.descriptor, candidate.candidate.commitments); >::insert(¶_id, CandidatePendingAvailability { core, - receipt: candidate.candidate, + descriptor, availability_votes, relay_parent_number, backed_in_number: now, }); + ::insert(¶_id, commitments); } Ok(core_indices) @@ -482,6 +507,7 @@ impl Module { for para_id in cleaned_up_ids { >::remove(¶_id); + ::remove(¶_id); } cleaned_up_cores @@ -713,31 +739,38 @@ mod tests { let paras = vec![(chain_a, true), (chain_b, true), (thread_a, false)]; new_test_ext(genesis_config(paras)).execute_with(|| { + let default_candidate = TestCandidateBuilder::default().build(); >::insert(chain_a, CandidatePendingAvailability { core: CoreIndex::from(0), - receipt: Default::default(), + descriptor: default_candidate.descriptor.clone(), availability_votes: default_availability_votes(), relay_parent_number: 0, backed_in_number: 0, }); + PendingAvailabilityCommitments::insert(chain_a, default_candidate.commitments.clone()); - >::insert(chain_b, CandidatePendingAvailability { + >::insert(&chain_b, CandidatePendingAvailability { core: CoreIndex::from(1), - receipt: Default::default(), + descriptor: default_candidate.descriptor, availability_votes: default_availability_votes(), relay_parent_number: 0, backed_in_number: 0, }); + PendingAvailabilityCommitments::insert(chain_b, default_candidate.commitments); run_to_block(5, |_| None); assert!(>::get(&chain_a).is_some()); assert!(>::get(&chain_b).is_some()); + assert!(::get(&chain_a).is_some()); + assert!(::get(&chain_b).is_some()); Inclusion::collect_pending(|core, _since| core == CoreIndex::from(0)); assert!(>::get(&chain_a).is_none()); assert!(>::get(&chain_b).is_some()); + assert!(::get(&chain_a).is_none()); + assert!(::get(&chain_b).is_some()); }); } @@ -868,13 +901,15 @@ mod tests { assert_eq!(core_lookup(CoreIndex::from(0)), Some(chain_a)); + let default_candidate = TestCandidateBuilder::default().build(); >::insert(chain_a, CandidatePendingAvailability { core: CoreIndex::from(0), - receipt: Default::default(), + descriptor: default_candidate.descriptor, availability_votes: default_availability_votes(), relay_parent_number: 0, backed_in_number: 0, }); + PendingAvailabilityCommitments::insert(chain_a, default_candidate.commitments); *bare_bitfield.0.get_mut(0).unwrap() = true; let signed = sign_bitfield( @@ -888,6 +923,42 @@ mod tests { vec![signed], &core_lookup, ).is_ok()); + + >::remove(chain_a); + PendingAvailabilityCommitments::remove(chain_a); + } + + // bitfield signed with pending bit signed, but no commitments. + { + let mut bare_bitfield = default_bitfield(); + + assert_eq!(core_lookup(CoreIndex::from(0)), Some(chain_a)); + + let default_candidate = TestCandidateBuilder::default().build(); + >::insert(chain_a, CandidatePendingAvailability { + core: CoreIndex::from(0), + descriptor: default_candidate.descriptor, + availability_votes: default_availability_votes(), + relay_parent_number: 0, + backed_in_number: 0, + }); + + *bare_bitfield.0.get_mut(0).unwrap() = true; + let signed = sign_bitfield( + &validators[0], + 0, + bare_bitfield, + &signing_context, + ); + + // no core is freed + assert_eq!( + Inclusion::process_bitfields( + vec![signed], + &core_lookup, + ), + Ok(vec![]), + ); } }); } @@ -924,29 +995,35 @@ mod tests { _ => panic!("Core out of bounds for 2 parachains and 1 parathread core."), }; + let candidate_a = TestCandidateBuilder { + para_id: chain_a, + head_data: vec![1, 2, 3, 4].into(), + ..Default::default() + }.build(); + >::insert(chain_a, CandidatePendingAvailability { core: CoreIndex::from(0), - receipt: TestCandidateBuilder { - para_id: chain_a, - head_data: vec![1, 2, 3, 4].into(), - ..Default::default() - }.build(), + descriptor: candidate_a.descriptor, availability_votes: default_availability_votes(), relay_parent_number: 0, backed_in_number: 0, }); + PendingAvailabilityCommitments::insert(chain_a, candidate_a.commitments); + + let candidate_b = TestCandidateBuilder { + para_id: chain_b, + head_data: vec![5, 6, 7, 8].into(), + ..Default::default() + }.build(); >::insert(chain_b, CandidatePendingAvailability { core: CoreIndex::from(1), - receipt: TestCandidateBuilder { - para_id: chain_b, - head_data: vec![5, 6, 7, 8].into(), - ..Default::default() - }.build(), + descriptor: candidate_b.descriptor, availability_votes: default_availability_votes(), relay_parent_number: 0, backed_in_number: 0, }); + PendingAvailabilityCommitments::insert(chain_b, candidate_b.commitments); // this bitfield signals that a and b are available. let a_and_b_available = { @@ -996,6 +1073,8 @@ mod tests { // chain A had 4 signing off, which is >= threshold. // chain B has 3 signing off, which is < threshold. assert!(>::get(&chain_a).is_none()); + assert!(::get(&chain_a).is_none()); + assert!(::get(&chain_b).is_some()); assert_eq!( >::get(&chain_b).unwrap().availability_votes, { @@ -1053,7 +1132,7 @@ mod tests { let chain_a_assignment = CoreAssignment { core: CoreIndex::from(0), - para_id: chain_b, + para_id: chain_a, kind: AssignmentKind::Parachain, group_idx: GroupIndex::from(0), }; @@ -1067,7 +1146,7 @@ mod tests { let thread_a_assignment = CoreAssignment { core: CoreIndex::from(2), - para_id: chain_b, + para_id: thread_a, kind: AssignmentKind::Parathread(thread_collator.clone(), 0), group_idx: GroupIndex::from(2), }; @@ -1296,13 +1375,15 @@ mod tests { BackingKind::Threshold, ); + let candidate = TestCandidateBuilder::default().build(); >::insert(&chain_a, CandidatePendingAvailability { core: CoreIndex::from(0), - receipt: Default::default(), + descriptor: candidate.descriptor, availability_votes: default_availability_votes(), relay_parent_number: 3, backed_in_number: 4, }); + ::insert(&chain_a, candidate.commitments); assert!(Inclusion::process_candidates( vec![backed], @@ -1311,6 +1392,41 @@ mod tests { ).is_err()); >::remove(&chain_a); + ::remove(&chain_a); + } + + // messed up commitments storage - do not panic - reject. + { + let mut candidate = TestCandidateBuilder { + para_id: chain_a, + relay_parent: System::parent_hash(), + pov_hash: Hash::from([1; 32]), + ..Default::default() + }.build(); + + collator_sign_candidate( + Sr25519Keyring::One, + &mut candidate, + ); + + // this is not supposed to happen + ::insert(&chain_a, candidate.commitments.clone()); + + let backed = back_candidate( + candidate, + &validators, + group_validators(GroupIndex::from(0)).unwrap().as_ref(), + &signing_context, + BackingKind::Threshold, + ); + + assert!(Inclusion::process_candidates( + vec![backed], + vec![chain_a_assignment.clone()], + &group_validators, + ).is_err()); + + ::remove(&chain_a); } // interfering code upgrade - reject @@ -1483,34 +1599,46 @@ mod tests { >::get(&chain_a), Some(CandidatePendingAvailability { core: CoreIndex::from(0), - receipt: candidate_a, + descriptor: candidate_a.descriptor, availability_votes: default_availability_votes(), relay_parent_number: System::block_number() - 1, backed_in_number: System::block_number(), }) ); + assert_eq!( + ::get(&chain_a), + Some(candidate_a.commitments), + ); assert_eq!( >::get(&chain_b), Some(CandidatePendingAvailability { core: CoreIndex::from(1), - receipt: candidate_b, + descriptor: candidate_b.descriptor, availability_votes: default_availability_votes(), relay_parent_number: System::block_number() - 1, backed_in_number: System::block_number(), }) ); + assert_eq!( + ::get(&chain_b), + Some(candidate_b.commitments), + ); assert_eq!( >::get(&thread_a), Some(CandidatePendingAvailability { core: CoreIndex::from(2), - receipt: candidate_c, + descriptor: candidate_c.descriptor, availability_votes: default_availability_votes(), relay_parent_number: System::block_number() - 1, backed_in_number: System::block_number(), }) ); + assert_eq!( + ::get(&thread_a), + Some(candidate_c.commitments), + ); }); } @@ -1568,21 +1696,24 @@ mod tests { }, ); + let candidate = TestCandidateBuilder::default().build(); >::insert(&chain_a, CandidatePendingAvailability { core: CoreIndex::from(0), - receipt: Default::default(), + descriptor: candidate.descriptor.clone(), availability_votes: default_availability_votes(), relay_parent_number: 5, backed_in_number: 6, }); + ::insert(&chain_a, candidate.commitments.clone()); >::insert(&chain_b, CandidatePendingAvailability { core: CoreIndex::from(1), - receipt: Default::default(), + descriptor: candidate.descriptor, availability_votes: default_availability_votes(), relay_parent_number: 6, backed_in_number: 7, }); + ::insert(&chain_b, candidate.commitments); run_to_block(11, |_| None); @@ -1595,6 +1726,8 @@ mod tests { assert!(>::get(&chain_a).is_some()); assert!(>::get(&chain_b).is_some()); + assert!(::get(&chain_a).is_some()); + assert!(::get(&chain_b).is_some()); run_to_block(12, |n| match n { 12 => Some(SessionChangeNotification { @@ -1617,10 +1750,12 @@ mod tests { assert!(>::get(&chain_a).is_none()); assert!(>::get(&chain_b).is_none()); + assert!(::get(&chain_a).is_none()); + assert!(::get(&chain_b).is_none()); assert!(>::iter().collect::>().is_empty()); assert!(>::iter().collect::>().is_empty()); - + assert!(::iter().collect::>().is_empty()); }); } }