Skip to content

Commit

Permalink
Babe: bad epoch index with skipped epochs and warp sync (paritytech#1…
Browse files Browse the repository at this point in the history 
…3135)

* Detect and correct epoch-index for skipped epochs

* Code refactory

* Epoch index should be also be fixed for secondary claims with VRF

* Fix typo

* Make clippy happy

* Fix typo

* Trigger pipeline
  • Loading branch information
@davxy
davxy authored Jan 25, 2023
1 parent 1314eec commit e393874
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 67 deletions.
21 changes: 16 additions & 5 deletions client/consensus/babe/src/authorship.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

use super::Epoch;
use codec::Encode;
use sc_consensus_epochs::Epoch as EpochT;
use schnorrkel::{keys::PublicKey, vrf::VRFInOut};
use sp_application_crypto::AppKey;
use sp_consensus_babe::{
Expand Down Expand Up @@ -135,18 +136,23 @@ fn claim_secondary_slot(
keystore: &SyncCryptoStorePtr,
author_secondary_vrf: bool,
) -> Option<(PreDigest, AuthorityId)> {
let Epoch { authorities, randomness, epoch_index, .. } = epoch;
let Epoch { authorities, randomness, mut epoch_index, .. } = epoch;

if authorities.is_empty() {
return None
}

if epoch.end_slot() <= slot {
// Slot doesn't strictly belong to the epoch, create a clone with fixed values.
epoch_index = epoch.clone_for_slot(slot).epoch_index;
}

let expected_author = secondary_slot_author(slot, authorities, *randomness)?;

for (authority_id, authority_index) in keys {
if authority_id == expected_author {
let pre_digest = if author_secondary_vrf {
let transcript_data = make_transcript_data(randomness, slot, *epoch_index);
let transcript_data = make_transcript_data(randomness, slot, epoch_index);
let result = SyncCryptoStore::sr25519_vrf_sign(
&**keystore,
AuthorityId::ID,
Expand Down Expand Up @@ -238,11 +244,16 @@ fn claim_primary_slot(
keystore: &SyncCryptoStorePtr,
keys: &[(AuthorityId, usize)],
) -> Option<(PreDigest, AuthorityId)> {
let Epoch { authorities, randomness, epoch_index, .. } = epoch;
let Epoch { authorities, randomness, mut epoch_index, .. } = epoch;

if epoch.end_slot() <= slot {
// Slot doesn't strictly belong to the epoch, create a clone with fixed values.
epoch_index = epoch.clone_for_slot(slot).epoch_index;
}

for (authority_id, authority_index) in keys {
let transcript = make_transcript(randomness, slot, *epoch_index);
let transcript_data = make_transcript_data(randomness, slot, *epoch_index);
let transcript = make_transcript(randomness, slot, epoch_index);
let transcript_data = make_transcript_data(randomness, slot, epoch_index);
let result = SyncCryptoStore::sr25519_vrf_sign(
&**keystore,
AuthorityId::ID,
Expand Down
75 changes: 45 additions & 30 deletions client/consensus/babe/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,9 @@ impl From<sp_consensus_babe::Epoch> for Epoch {
}

impl Epoch {
/// Create the genesis epoch (epoch #0). This is defined to start at the slot of
/// the first block, so that has to be provided.
/// Create the genesis epoch (epoch #0).
///
/// This is defined to start at the slot of the first block, so that has to be provided.
pub fn genesis(genesis_config: &BabeConfiguration, slot: Slot) -> Epoch {
Epoch {
epoch_index: 0,
Expand All @@ -224,6 +225,38 @@ impl Epoch {
},
}
}

/// Clone and tweak epoch information to refer to the specified slot.
///
/// All the information which depends on the slot value is recomputed and assigned
/// to the returned epoch instance.
///
/// The `slot` must be greater than or equal the original epoch start slot,
/// if is less this operation is equivalent to a simple clone.
pub fn clone_for_slot(&self, slot: Slot) -> Epoch {
let mut epoch = self.clone();

let skipped_epochs = *slot.saturating_sub(self.start_slot) / self.duration;

let epoch_index = epoch.epoch_index.checked_add(skipped_epochs).expect(
"epoch number is u64; it should be strictly smaller than number of slots; \
slots relate in some way to wall clock time; \
if u64 is not enough we should crash for safety; qed.",
);

let start_slot = skipped_epochs
.checked_mul(epoch.duration)
.and_then(|skipped_slots| epoch.start_slot.checked_add(skipped_slots))
.expect(
"slot number is u64; it should relate in some way to wall clock time; \
if u64 is not enough we should crash for safety; qed.",
);

epoch.epoch_index = epoch_index;
epoch.start_slot = Slot::from(start_slot);

epoch
}
}

/// Errors encountered by the babe authorship task.
Expand Down Expand Up @@ -1540,45 +1573,27 @@ where
};

if viable_epoch.as_ref().end_slot() <= slot {
// some epochs must have been skipped as our current slot
// fits outside the current epoch. we will figure out
// which epoch it belongs to and we will re-use the same
// data for that epoch
let mut epoch_data = viable_epoch.as_mut();
let skipped_epochs =
*slot.saturating_sub(epoch_data.start_slot) / epoch_data.duration;

// NOTE: notice that we are only updating a local copy of the `Epoch`, this
// Some epochs must have been skipped as our current slot fits outside the
// current epoch. We will figure out which epoch it belongs to and we will
// re-use the same data for that epoch.
// Notice that we are only updating a local copy of the `Epoch`, this
// makes it so that when we insert the next epoch into `EpochChanges` below
// (after incrementing it), it will use the correct epoch index and start slot.
// we do not update the original epoch that will be re-used because there might
// We do not update the original epoch that will be re-used because there might
// be other forks (that we haven't imported) where the epoch isn't skipped, and
// to import those forks we want to keep the original epoch data. not updating
// to import those forks we want to keep the original epoch data. Not updating
// the original epoch works because when we search the tree for which epoch to
// use for a given slot, we will search in-depth with the predicate
// `epoch.start_slot <= slot` which will still match correctly without updating
// `start_slot` to the correct value as below.
let epoch_index = epoch_data.epoch_index.checked_add(skipped_epochs).expect(
"epoch number is u64; it should be strictly smaller than number of slots; \
slots relate in some way to wall clock time; \
if u64 is not enough we should crash for safety; qed.",
);

let start_slot = skipped_epochs
.checked_mul(epoch_data.duration)
.and_then(|skipped_slots| epoch_data.start_slot.checked_add(skipped_slots))
.expect(
"slot number is u64; it should relate in some way to wall clock time; \
if u64 is not enough we should crash for safety; qed.",
);
let epoch = viable_epoch.as_mut();
let prev_index = epoch.epoch_index;
*epoch = epoch.clone_for_slot(slot);

warn!(
target: LOG_TARGET,
"👶 Epoch(s) skipped: from {} to {}", epoch_data.epoch_index, epoch_index,
"👶 Epoch(s) skipped: from {} to {}", prev_index, epoch.epoch_index,
);

epoch_data.epoch_index = epoch_index;
epoch_data.start_slot = Slot::from(start_slot);
}

log!(
Expand Down
141 changes: 111 additions & 30 deletions client/consensus/babe/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

use super::*;
use authorship::claim_slot;
use log::debug;
use rand_chacha::{
rand_core::{RngCore, SeedableRng},
ChaChaRng,
Expand All @@ -35,9 +34,10 @@ use sp_application_crypto::key_types::BABE;
use sp_consensus::{DisableProofRecording, NoNetwork as DummyOracle, Proposal};
use sp_consensus_babe::{
inherents::InherentDataProvider, make_transcript, make_transcript_data, AllowedSlots,
AuthorityPair, Slot,
AuthorityId, AuthorityPair, Slot,
};
use sp_consensus_slots::SlotDuration;
use sp_consensus_vrf::schnorrkel::VRFOutput;
use sp_core::crypto::Pair;
use sp_keyring::Sr25519Keyring;
use sp_keystore::{
Expand Down Expand Up @@ -553,52 +553,133 @@ fn sig_is_not_pre_digest() {
}

#[test]
fn can_author_block() {
sp_tracing::try_init_simple();
fn claim_epoch_slots() {
// We don't require the full claim information, thus as a shorter alias we're
// going to use a simple integer value. Generally not verbose enough, but good enough
// to be used within the narrow scope of a single test.
// 0 -> None (i.e. unable to claim the slot),
// 1 -> Primary
// 2 -> Secondary
// 3 -> Secondary with VRF
const EPOCH_DURATION: u64 = 10;

let authority = Sr25519Keyring::Alice;
let keystore = create_keystore(authority);

let mut i = 0;
let epoch = Epoch {
let mut epoch = Epoch {
start_slot: 0.into(),
authorities: vec![(authority.public().into(), 1)],
randomness: [0; 32],
epoch_index: 1,
duration: 100,
duration: EPOCH_DURATION,
config: BabeEpochConfiguration {
c: (3, 10),
allowed_slots: AllowedSlots::PrimaryAndSecondaryPlainSlots,
},
};

let mut config = crate::BabeConfiguration {
slot_duration: 1000,
epoch_length: 100,
c: (3, 10),
authorities: Vec::new(),
let claim_slot_wrap = |s, e| match claim_slot(Slot::from(s as u64), &e, &keystore) {
None => 0,
Some((PreDigest::Primary(_), _)) => 1,
Some((PreDigest::SecondaryPlain(_), _)) => 2,
Some((PreDigest::SecondaryVRF(_), _)) => 3,
};

// With secondary mechanism we should be able to claim all slots.
let claims: Vec<_> = (0..EPOCH_DURATION)
.into_iter()
.map(|slot| claim_slot_wrap(slot, epoch.clone()))
.collect();
assert_eq!(claims, [1, 2, 2, 1, 2, 2, 2, 2, 2, 1]);

// With secondary with VRF mechanism we should be able to claim all the slots.
epoch.config.allowed_slots = AllowedSlots::PrimaryAndSecondaryVRFSlots;
let claims: Vec<_> = (0..EPOCH_DURATION)
.into_iter()
.map(|slot| claim_slot_wrap(slot, epoch.clone()))
.collect();
assert_eq!(claims, [1, 3, 3, 1, 3, 3, 3, 3, 3, 1]);

// Otherwise with only vrf-based primary slots we are able to claim a subset of the slots.
epoch.config.allowed_slots = AllowedSlots::PrimarySlots;
let claims: Vec<_> = (0..EPOCH_DURATION)
.into_iter()
.map(|slot| claim_slot_wrap(slot, epoch.clone()))
.collect();
assert_eq!(claims, [1, 0, 0, 1, 0, 0, 0, 0, 0, 1]);
}

#[test]
fn claim_vrf_check() {
let authority = Sr25519Keyring::Alice;
let keystore = create_keystore(authority);

let public = authority.public();

let epoch = Epoch {
start_slot: 0.into(),
authorities: vec![(public.into(), 1)],
randomness: [0; 32],
allowed_slots: AllowedSlots::PrimaryAndSecondaryPlainSlots,
epoch_index: 1,
duration: 10,
config: BabeEpochConfiguration {
c: (3, 10),
allowed_slots: AllowedSlots::PrimaryAndSecondaryVRFSlots,
},
};

// with secondary slots enabled it should never be empty
match claim_slot(i.into(), &epoch, &keystore) {
None => i += 1,
Some(s) => debug!(target: LOG_TARGET, "Authored block {:?}", s.0),
}
// We leverage the predictability of claim types given a constant randomness.

// otherwise with only vrf-based primary slots we might need to try a couple
// of times.
config.allowed_slots = AllowedSlots::PrimarySlots;
loop {
match claim_slot(i.into(), &epoch, &keystore) {
None => i += 1,
Some(s) => {
debug!(target: LOG_TARGET, "Authored block {:?}", s.0);
break
},
}
}
// We expect a Primary claim for slot 0

let pre_digest = match claim_slot(0.into(), &epoch, &keystore).unwrap().0 {
PreDigest::Primary(d) => d,
v => panic!("Unexpected pre-digest variant {:?}", v),
};
let transcript = make_transcript_data(&epoch.randomness.clone(), 0.into(), epoch.epoch_index);
let sign = SyncCryptoStore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
.unwrap()
.unwrap();
assert_eq!(pre_digest.vrf_output, VRFOutput(sign.output));

// We expect a SecondaryVRF claim for slot 1
let pre_digest = match claim_slot(1.into(), &epoch, &keystore).unwrap().0 {
PreDigest::SecondaryVRF(d) => d,
v => panic!("Unexpected pre-digest variant {:?}", v),
};
let transcript = make_transcript_data(&epoch.randomness.clone(), 1.into(), epoch.epoch_index);
let sign = SyncCryptoStore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
.unwrap()
.unwrap();
assert_eq!(pre_digest.vrf_output, VRFOutput(sign.output));

// Check that correct epoch index has been used if epochs are skipped (primary VRF)
let slot = Slot::from(103);
let claim = match claim_slot(slot, &epoch, &keystore).unwrap().0 {
PreDigest::Primary(d) => d,
v => panic!("Unexpected claim variant {:?}", v),
};
let fixed_epoch = epoch.clone_for_slot(slot);
let transcript = make_transcript_data(&epoch.randomness.clone(), slot, fixed_epoch.epoch_index);
let sign = SyncCryptoStore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
.unwrap()
.unwrap();
assert_eq!(fixed_epoch.epoch_index, 11);
assert_eq!(claim.vrf_output, VRFOutput(sign.output));

// Check that correct epoch index has been used if epochs are skipped (secondary VRF)
let slot = Slot::from(100);
let pre_digest = match claim_slot(slot, &epoch, &keystore).unwrap().0 {
PreDigest::SecondaryVRF(d) => d,
v => panic!("Unexpected claim variant {:?}", v),
};
let fixed_epoch = epoch.clone_for_slot(slot);
let transcript = make_transcript_data(&epoch.randomness.clone(), slot, fixed_epoch.epoch_index);
let sign = SyncCryptoStore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
.unwrap()
.unwrap();
assert_eq!(fixed_epoch.epoch_index, 11);
assert_eq!(pre_digest.vrf_output, VRFOutput(sign.output));
}

// Propose and import a new BABE block on top of the given parent.
Expand Down
17 changes: 15 additions & 2 deletions client/consensus/babe/src/verification.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use crate::{
babe_err, find_pre_digest, BlockT, Epoch, Error, LOG_TARGET,
};
use log::{debug, trace};
use sc_consensus_epochs::Epoch as EpochT;
use sc_consensus_slots::CheckedHeader;
use sp_consensus_babe::{
digests::{
Expand Down Expand Up @@ -155,10 +156,16 @@ fn check_primary_header<B: BlockT + Sized>(
c: (u64, u64),
) -> Result<(), Error<B>> {
let author = &epoch.authorities[pre_digest.authority_index as usize].0;
let mut epoch_index = epoch.epoch_index;

if epoch.end_slot() <= pre_digest.slot {
// Slot doesn't strictly belong to this epoch, create a clone with fixed values.
epoch_index = epoch.clone_for_slot(pre_digest.slot).epoch_index;
}

if AuthorityPair::verify(&signature, pre_hash, author) {
let (inout, _) = {
let transcript = make_transcript(&epoch.randomness, pre_digest.slot, epoch.epoch_index);
let transcript = make_transcript(&epoch.randomness, pre_digest.slot, epoch_index);

schnorrkel::PublicKey::from_bytes(author.as_slice())
.and_then(|p| {
Expand Down Expand Up @@ -228,8 +235,14 @@ fn check_secondary_vrf_header<B: BlockT>(
return Err(Error::InvalidAuthor(expected_author.clone(), author.clone()))
}

let mut epoch_index = epoch.epoch_index;
if epoch.end_slot() <= pre_digest.slot {
// Slot doesn't strictly belong to this epoch, create a clone with fixed values.
epoch_index = epoch.clone_for_slot(pre_digest.slot).epoch_index;
}

if AuthorityPair::verify(&signature, pre_hash.as_ref(), author) {
let transcript = make_transcript(&epoch.randomness, pre_digest.slot, epoch.epoch_index);
let transcript = make_transcript(&epoch.randomness, pre_digest.slot, epoch_index);

schnorrkel::PublicKey::from_bytes(author.as_slice())
.and_then(|p| p.vrf_verify(transcript, &pre_digest.vrf_output, &pre_digest.vrf_proof))
Expand Down

0 comments on commit e393874

Please sign in to comment.