Small cleanup

client/consensus/sassafras/src/authorship.rs

@@ -55,7 +55,7 @@ pub(crate) fn claim_slot(
 	let (authority_idx, ticket_aux) = match ticket {
 		Some(ticket) => {
 			log::debug!(target: "sassafras", "🌳 [TRY PRIMARY]");
-			let (authority_idx, ticket_aux) = epoch.tickets_aux.get(&ticket)?.clone();
+			let (authority_idx, ticket_aux) = epoch.tickets_aux.get(&ticket.output)?.clone();
 			log::debug!(target: "sassafras", "🌳 Ticket = [ticket: {:02x?}, auth: {}, attempt: {}]",
                 &ticket.output.as_bytes()[0..8], authority_idx, ticket_aux.attempt);
 			(authority_idx, Some(ticket_aux))
@@ -145,8 +145,10 @@ fn generate_epoch_tickets(epoch: &mut Epoch, keystore: &SyncCryptoStorePtr) -> V
 
 		for attempt in 0..max_attempts {
 			if let Some((ticket, ticket_aux)) = make_ticket(attempt) {
+				epoch
+					.tickets_aux
+					.insert(ticket.output, (authority_idx as AuthorityIndex, ticket_aux));
 				tickets.push(ticket);
-				epoch.tickets_aux.insert(ticket, (authority_idx as AuthorityIndex, ticket_aux));
 			}
 		}
 	}

client/consensus/sassafras/src/lib.rs

@@ -194,7 +194,7 @@ pub struct Epoch {
 	/// Epoch configuration
 	pub config: SassafrasConfiguration,
 	/// Tickets auxiliary data.
-	pub tickets_aux: BTreeMap<Ticket, (AuthorityIndex, TicketAux)>,
+	pub tickets_aux: BTreeMap<VRFOutput, (AuthorityIndex, TicketAux)>,
 }
 
 impl EpochT for Epoch {

client/consensus/sassafras/src/verification.rs

@@ -23,24 +23,26 @@ use super::*;
 // Allowed slot drift.
 const MAX_SLOT_DRIFT: u64 = 1;
 
-/// Sassafras verification parameters
-pub struct VerificationParams<'a, B: 'a + BlockT> {
+/// Verification parameters
+struct VerificationParams<'a, B: 'a + BlockT> {
 	/// The header being verified.
-	pub header: B::Header,
+	header: B::Header,
 	/// The pre-digest of the header being verified.
-	pub pre_digest: PreDigest,
+	pre_digest: &'a PreDigest,
 	/// The slot number of the current time.
-	pub slot_now: Slot,
+	slot_now: Slot,
 	/// Epoch descriptor of the epoch this block _should_ be under, if it's valid.
-	pub epoch: &'a Epoch,
+	epoch: &'a Epoch,
 	/// Expected ticket for this block.
-	pub ticket: Option<Ticket>,
+	ticket: Option<Ticket>,
 }
 
-pub struct VerifiedHeaderInfo {
-	pub authority_id: AuthorityId,
-	pub pre_digest: DigestItem,
-	pub seal: DigestItem,
+/// Verified information
+struct VerifiedHeaderInfo {
+	/// Authority index.
+	authority_id: AuthorityId,
+	/// Seal found within the header.
+	seal: DigestItem,
 }
 
 /// Check a header has been signed by the right key. If the slot is too far in
@@ -52,7 +54,7 @@ pub struct VerifiedHeaderInfo {
 ///
 /// The given header can either be from a primary or secondary slot assignment,
 /// with each having different validation logic.
-pub fn check_header<B: BlockT + Sized>(
+fn check_header<B: BlockT + Sized>(
 	params: VerificationParams<B>,
 ) -> Result<CheckedHeader<B::Header, VerifiedHeaderInfo>, Error<B>> {
 	let VerificationParams { mut header, pre_digest, slot_now, epoch, ticket } = params;
@@ -92,7 +94,7 @@ pub fn check_header<B: BlockT + Sized>(
 			let transcript =
 				make_ticket_transcript(&config.randomness, ticket_aux.attempt, epoch.epoch_idx);
 			schnorrkel::PublicKey::from_bytes(authority_id.as_slice())
-				.and_then(|p| p.vrf_verify(transcript, &ticket, &ticket_aux.proof))
+				.and_then(|p| p.vrf_verify(transcript, &ticket.output, &ticket_aux.proof))
 				.map_err(|s| sassafras_err(Error::VRFVerificationFailed(s)))?;
 		},
 		(None, None) => {
@@ -120,11 +122,7 @@ pub fn check_header<B: BlockT + Sized>(
 		.and_then(|p| p.vrf_verify(transcript, &pre_digest.vrf_output, &pre_digest.vrf_proof))
 		.map_err(|s| sassafras_err(Error::VRFVerificationFailed(s)))?;
 
-	let info = VerifiedHeaderInfo {
-		authority_id,
-		pre_digest: CompatibleDigestItem::sassafras_pre_digest(pre_digest),
-		seal,
-	};
+	let info = VerifiedHeaderInfo { authority_id, seal };
 
 	Ok(CheckedHeader::Checked(header, info))
 }
@@ -301,29 +299,27 @@ where
 {
 	async fn verify(
 		&mut self,
-		mut block: BlockImportParams<Block, ()>,
+		mut import_params: BlockImportParams<Block, ()>,
 	) -> BlockVerificationResult<Block> {
 		trace!(
 			target: "sassafras",
 			"🌳 Verifying origin: {:?} header: {:?} justification(s): {:?} body: {:?}",
-			block.origin,
-			block.header,
-			block.justifications,
-			block.body,
+			import_params.origin,
+			import_params.header,
+			import_params.justifications,
+			import_params.body,
 		);
 
-		if block.with_state() {
+		if import_params.with_state() {
 			// When importing whole state we don't calculate epoch descriptor, but rather
 			// read it from the state after import. We also skip all verifications
 			// because there's no parent state and we trust the sync module to verify
 			// that the state is correct and finalized.
-			return Ok((block, Default::default()))
+			return Ok((import_params, Default::default()))
 		}
 
-		trace!(target: "sassafras", "🌳 We have {:?} logs in this header", block.header.digest().logs().len());
-
-		let hash = block.header.hash();
-		let parent_hash = *block.header.parent_hash();
+		let hash = import_params.header.hash();
+		let parent_hash = *import_params.header.parent_hash();
 
 		let create_inherent_data_providers = self
 			.create_inherent_data_providers
@@ -338,9 +334,9 @@ where
 			.header_metadata(parent_hash)
 			.map_err(Error::<Block>::FetchParentHeader)?;
 
-		let pre_digest = find_pre_digest::<Block>(&block.header)?;
+		let pre_digest = find_pre_digest::<Block>(&import_params.header)?;
 
-		let (check_header, epoch_descriptor) = {
+		let (checked_header, epoch_descriptor) = {
 			let epoch_changes = self.epoch_changes.shared_data();
 			let epoch_descriptor = epoch_changes
 				.epoch_descriptor_for_child_of(
@@ -361,35 +357,30 @@ where
 				.slot_ticket(&BlockId::Hash(parent_hash), pre_digest.slot)
 				.map_err(|err| err.to_string())?;
 
-			let v_params = VerificationParams {
-				header: block.header.clone(),
-				pre_digest,
+			let verification_params = VerificationParams {
+				header: import_params.header.clone(),
+				pre_digest: &pre_digest,
 				slot_now,
 				epoch: viable_epoch.as_ref(),
 				ticket,
 			};
+			let checked_header = check_header::<Block>(verification_params)?;
 
-			(check_header::<Block>(v_params)?, epoch_descriptor)
+			(checked_header, epoch_descriptor)
 		};
 
-		match check_header {
+		match checked_header {
 			CheckedHeader::Checked(pre_header, verified_info) => {
-				let sassafras_pre_digest = verified_info
-					.pre_digest
-					.as_sassafras_pre_digest()
-					.expect("check_header always returns a pre-digest digest item; qed");
-				let slot = sassafras_pre_digest.slot;
-
 				// The header is valid but let's check if there was something else already
 				// proposed at the same slot by the given author. If there was, we will
 				// report the equivocation to the runtime.
 				if let Err(err) = self
 					.check_and_report_equivocation(
 						slot_now,
-						slot,
-						&block.header,
+						pre_digest.slot,
+						&import_params.header,
 						&verified_info.authority_id,
-						&block.origin,
+						&import_params.origin,
 					)
 					.await
 				{
@@ -399,24 +390,24 @@ where
 				// If the body is passed through, we need to use the runtime to check that the
 				// internally-set timestamp in the inherents actually matches the slot set in the
 				// seal.
-				if let Some(inner_body) = block.body {
+				if let Some(inner_body) = import_params.body {
 					let mut inherent_data = create_inherent_data_providers
 						.create_inherent_data()
 						.map_err(Error::<Block>::CreateInherents)?;
-					inherent_data.sassafras_replace_inherent_data(slot);
-					let new_block = Block::new(pre_header.clone(), inner_body);
+					inherent_data.sassafras_replace_inherent_data(pre_digest.slot);
+					let block = Block::new(pre_header.clone(), inner_body);
 
 					self.check_inherents(
-						new_block.clone(),
+						block.clone(),
 						BlockId::Hash(parent_hash),
 						inherent_data,
 						create_inherent_data_providers,
-						block.origin.into(),
+						import_params.origin.into(),
 					)
 					.await?;
 
-					let (_, inner_body) = new_block.deconstruct();
-					block.body = Some(inner_body);
+					let (_, inner_body) = block.deconstruct();
+					import_params.body = Some(inner_body);
 				}
 
 				trace!(target: "sassafras", "🌳 Checked {:?}; importing.", pre_header);
@@ -427,15 +418,15 @@ where
 					"pre_header" => ?pre_header,
 				);
 
-				block.header = pre_header;
-				block.post_digests.push(verified_info.seal);
-				block.insert_intermediate(
+				import_params.header = pre_header;
+				import_params.post_hash = Some(hash);
+				import_params.post_digests.push(verified_info.seal);
+				import_params.insert_intermediate(
 					INTERMEDIATE_KEY,
 					SassafrasIntermediate::<Block> { epoch_descriptor },
 				);
-				block.post_hash = Some(hash);
 
-				Ok((block, Default::default()))
+				Ok((import_params, Default::default()))
 			},
 			CheckedHeader::Deferred(a, b) => {
 				debug!(target: "sassafras", "🌳 Checking {:?} failed; {:?}, {:?}.", hash, a, b);

frame/sassafras/src/lib.rs

@@ -55,7 +55,7 @@ use frame_system::offchain::{SendTransactionTypes, SubmitTransaction};
 use sp_consensus_sassafras::{
 	digests::{ConsensusLog, NextEpochDescriptor, PreDigest},
 	AuthorityId, EquivocationProof, Randomness, SassafrasAuthorityWeight,
-	SassafrasEpochConfiguration, Slot, Ticket, VRFOutput, VRFProof, SASSAFRAS_ENGINE_ID,
+	SassafrasEpochConfiguration, Slot, Ticket, VRFOutput, SASSAFRAS_ENGINE_ID,
 };
 use sp_io::hashing;
 use sp_runtime::{
@@ -302,12 +302,12 @@ pub mod pallet {
 				let mut metadata = TicketsMeta::<T>::get();
 				if metadata.segments_count != 0 {
 					let epoch_idx = EpochIndex::<T>::get() + 1;
-					let epoch_key = (epoch_idx & 1) as u8;
+					let epoch_tag = (epoch_idx & 1) as u8;
 					if metadata.segments_count != 0 {
 						let slots_left = epoch_duration.checked_sub(current_slot_idx).unwrap_or(1);
 						Self::sort_tickets(
 							u32::max(1, metadata.segments_count / slots_left as u32),
-							epoch_key,
+							epoch_tag,
 							&mut metadata,
 						);
 						TicketsMeta::<T>::set(metadata);
@@ -560,15 +560,15 @@ impl<T: Config> Pallet<T> {
 		};
 		Self::deposit_consensus(ConsensusLog::NextEpochData(next_epoch));
 
-		let epoch_key = (epoch_idx & 1) as u8;
+		let epoch_tag = (epoch_idx & 1) as u8;
 		let mut tickets_metadata = TicketsMeta::<T>::get();
 		// Optionally finish sorting
 		if tickets_metadata.segments_count != 0 {
-			Self::sort_tickets(tickets_metadata.segments_count, epoch_key, &mut tickets_metadata);
+			Self::sort_tickets(tickets_metadata.segments_count, epoch_tag, &mut tickets_metadata);
 		}
 		// Clear the prev (equal to the next) epoch tickets counter.
-		let next_epoch_key = epoch_key ^ 1;
-		tickets_metadata.tickets_count[next_epoch_key as usize] = 0;
+		let next_epoch_tag = epoch_tag ^ 1;
+		tickets_metadata.tickets_count[next_epoch_tag as usize] = 0;
 		TicketsMeta::<T>::set(tickets_metadata);
 	}
 
@@ -689,63 +689,63 @@ impl<T: Config> Pallet<T> {
 			ticket_idx as u32
 		};
 
-		let mut epoch_key = (epoch_idx & 1) as u8;
+		let mut epoch_tag = (epoch_idx & 1) as u8;
 
 		if duration <= slot_idx && slot_idx < 2 * duration {
 			// Try to get a ticket for the next epoch. Since its state values were not enacted yet,
 			// we may have to finish sorting the tickets.
-			epoch_key ^= 1;
+			epoch_tag ^= 1;
 			slot_idx -= duration;
 			if tickets_meta.segments_count != 0 {
-				Self::sort_tickets(tickets_meta.segments_count, epoch_key, &mut tickets_meta);
+				Self::sort_tickets(tickets_meta.segments_count, epoch_tag, &mut tickets_meta);
 				TicketsMeta::<T>::set(tickets_meta.clone());
 			}
 		} else if slot_idx >= 2 * duration {
 			return None
 		}
 
 		let ticket_idx = get_ticket_idx(slot_idx);
-		if ticket_idx < tickets_meta.tickets_count[epoch_key as usize] {
-			Tickets::<T>::get((epoch_key, ticket_idx))
+		if ticket_idx < tickets_meta.tickets_count[epoch_tag as usize] {
+			Tickets::<T>::get((epoch_tag, ticket_idx))
 		} else {
 			None
 		}
 	}
 
 	// Lexicographically sort the tickets who belongs to the next epoch.
-	// The tickets are fetched from at most `max_iter` segments received via the `submit_tickets`
-	// extrinsic. The resulting sorted vector is truncated and if all the segments where sorted
-	// it is saved to be as the next epoch tickets.
-	// Else the result is saved to be used by next calls.
-	fn sort_tickets(max_iter: u32, epoch_key: u8, metadata: &mut TicketsMetadata) {
+	//
+	// Tickets are fetched from at most `max_iter` segments received via the `submit_tickets`
+	// extrinsic.
+	//
+	// The resulting sorted vector is optionally truncated to contain at most `MaxTickets`
+	// entries. If all the segments were consumed then the sorted vector is saved as the
+	// next epoch tickets, else it is saved to be used by next calls to this function.
+	fn sort_tickets(max_iter: u32, epoch_tag: u8, metadata: &mut TicketsMetadata) {
 		let mut segments_count = metadata.segments_count;
 		let max_iter = max_iter.min(segments_count);
 		let max_tickets = T::MaxTickets::get() as usize;
 
+		// Fetch the partial result.
 		let mut new_segment = NextTicketsSegments::<T>::take(u32::MAX).into_inner();
 
 		let mut require_sort = max_iter != 0;
 
 		let mut sup = if new_segment.len() >= max_tickets {
-			new_segment[new_segment.len() - 1].clone()
+			new_segment[new_segment.len() - 1].output
 		} else {
-			Ticket {
-				output: VRFOutput::try_from([0xFF; 32])
-					.expect("This is a valid vrf output value; qed"),
-				proof: VRFProof::try_from([0xFF; 64])
-					.expect("This is a valid vrf proof value; qed"),
-			}
+			VRFOutput::try_from([0xFF; 32]).expect("This is a valid vrf output value; qed")
 		};
 
+		// Consume at most `max_iter` segments.
 		for _ in 0..max_iter {
 			let segment = NextTicketsSegments::<T>::take(segments_count);
 
-			segment.into_iter().filter(|t| t < &sup).for_each(|t| new_segment.push(t));
+			segment.into_iter().filter(|t| t.output < sup).for_each(|t| new_segment.push(t));
 			if new_segment.len() > max_tickets {
 				require_sort = false;
 				new_segment.sort_unstable();
 				new_segment.truncate(max_tickets);
-				sup = new_segment[new_segment.len() - 1].clone();
+				sup = new_segment[max_tickets - 1].output;
 			}
 
 			segments_count -= 1;
@@ -756,13 +756,14 @@ impl<T: Config> Pallet<T> {
 		}
 
 		if segments_count == 0 {
-			// Sort is over, write to the map.
+			// Sort is over, write to next epoch map.
 			// TODO-SASS-P3: is there a better way to write a map from a vector?
 			new_segment.iter().enumerate().for_each(|(i, t)| {
-				Tickets::<T>::insert((epoch_key, i as u32), t);
+				Tickets::<T>::insert((epoch_tag, i as u32), t);
 			});
-			metadata.tickets_count[epoch_key as usize] = new_segment.len() as u32;
+			metadata.tickets_count[epoch_tag as usize] = new_segment.len() as u32;
 		} else {
+			// Keep the partial result for next invocations.
 			NextTicketsSegments::<T>::insert(u32::MAX, BoundedVec::truncate_from(new_segment));
 		}