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

Babe: bad epoch index with skipped epochs and warp sync #13135

Merged
merged 8 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
13 changes: 10 additions & 3 deletions client/consensus/babe/src/authorship.rs
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 @@ -238,11 +239,17 @@ 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.
let fixed_epoch = epoch.clone_for_slot(slot);
epoch_index = fixed_epoch.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
68 changes: 42 additions & 26 deletions client/consensus/babe/src/lib.rs
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,13 +1573,12 @@ 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;
// 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 epoch = viable_epoch.as_mut();
let prev_index = epoch.epoch_index;
*epoch = epoch.clone_for_slot(slot);
michalkucharczyk marked this conversation as resolved.
Show resolved Hide resolved

// NOTE: notice that we are only updating a local copy of the `Epoch`, this
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be moved up or changed a little bit the wording :D

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean the comment?

// makes it so that when we insert the next epoch into `EpochChanges` below
Expand All @@ -1558,27 +1590,11 @@ where
// 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.",
);

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
11 changes: 10 additions & 1 deletion client/consensus/babe/src/verification.rs
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 @@ -156,9 +157,17 @@ fn check_primary_header<B: BlockT + Sized>(
) -> 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.
let fixed_epoch = epoch.clone_for_slot(pre_digest.slot);
epoch_index = fixed_epoch.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