Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicate Grandpa consensus log reading logic #2245

Merged
merged 1 commit into from
Jul 7, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
52 changes: 11 additions & 41 deletions modules/grandpa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,12 @@
pub use storage_types::StoredAuthoritySet;

use bp_header_chain::{
justification::GrandpaJustification, ChainWithGrandpa, HeaderChain, InitializationData,
StoredHeaderData, StoredHeaderDataBuilder,
justification::GrandpaJustification, ChainWithGrandpa, GrandpaConsensusLogReader, HeaderChain,
InitializationData, StoredHeaderData, StoredHeaderDataBuilder,
};
use bp_runtime::{BlockNumberOf, HashOf, HasherOf, HeaderId, HeaderOf, OwnedBridgeModule};
use finality_grandpa::voter_set::VoterSet;
use frame_support::{dispatch::PostDispatchInfo, ensure, DefaultNoBound};
use sp_consensus_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
use sp_runtime::{
traits::{Header as HeaderT, Zero},
SaturatedConversion,
Expand Down Expand Up @@ -443,11 +442,17 @@ pub mod pallet {

// We don't support forced changes - at that point governance intervention is required.
ensure!(
super::find_forced_change(header).is_none(),
GrandpaConsensusLogReader::<BridgedBlockNumber<T, I>>::find_forced_change(
header.digest()
)
.is_none(),
<Error<T, I>>::UnsupportedScheduledChange
);

if let Some(change) = super::find_scheduled_change(header) {
if let Some(change) =
GrandpaConsensusLogReader::<BridgedBlockNumber<T, I>>::find_scheduled_change(
header.digest(),
) {
// GRANDPA only includes a `delay` for forced changes, so this isn't valid.
ensure!(change.delay == Zero::zero(), <Error<T, I>>::UnsupportedScheduledChange);

Expand Down Expand Up @@ -616,42 +621,6 @@ impl<T: Config<I>, I: 'static> HeaderChain<BridgedChain<T, I>> for GrandpaChainH
}
}

pub(crate) fn find_scheduled_change<H: HeaderT>(
header: &H,
) -> Option<sp_consensus_grandpa::ScheduledChange<H::Number>> {
use sp_runtime::generic::OpaqueDigestItemId;

let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID);

let filter_log = |log: ConsensusLog<H::Number>| match log {
ConsensusLog::ScheduledChange(change) => Some(change),
_ => None,
};

// find the first consensus digest with the right ID which converts to
// the right kind of consensus log.
header.digest().convert_first(|l| l.try_to(id).and_then(filter_log))
}

/// Checks the given header for a consensus digest signaling a **forced** scheduled change and
/// extracts it.
pub(crate) fn find_forced_change<H: HeaderT>(
header: &H,
) -> Option<(H::Number, sp_consensus_grandpa::ScheduledChange<H::Number>)> {
use sp_runtime::generic::OpaqueDigestItemId;

let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID);

let filter_log = |log: ConsensusLog<H::Number>| match log {
ConsensusLog::ForcedChange(delay, change) => Some((delay, change)),
_ => None,
};

// find the first consensus digest with the right ID which converts to
// the right kind of consensus log.
header.digest().convert_first(|l| l.try_to(id).and_then(filter_log))
}

/// (Re)initialize bridge with given header for using it in `pallet-bridge-messages` benchmarks.
#[cfg(feature = "runtime-benchmarks")]
pub fn initialize_for_benchmarks<T: Config<I>, I: 'static>(header: BridgedHeader<T, I>) {
Expand Down Expand Up @@ -685,6 +654,7 @@ mod tests {
storage::generator::StorageValue,
};
use frame_system::{EventRecord, Phase};
use sp_consensus_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
use sp_core::Get;
use sp_runtime::{Digest, DigestItem, DispatchError};

Expand Down
17 changes: 15 additions & 2 deletions primitives/header-chain/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ pub trait ConsensusLogReader {
pub struct GrandpaConsensusLogReader<Number>(sp_std::marker::PhantomData<Number>);

impl<Number: Codec> GrandpaConsensusLogReader<Number> {
pub fn find_authorities_change(
pub fn find_scheduled_change(
digest: &Digest,
) -> Option<sp_consensus_grandpa::ScheduledChange<Number>> {
// find the first consensus digest with the right ID which converts to
Expand All @@ -165,11 +165,24 @@ impl<Number: Codec> GrandpaConsensusLogReader<Number> {
_ => None,
})
}

pub fn find_forced_change(
digest: &Digest,
) -> Option<(Number, sp_consensus_grandpa::ScheduledChange<Number>)> {
// find the first consensus digest with the right ID which converts to
// the right kind of consensus log.
digest
.convert_first(|log| log.consensus_try_to(&GRANDPA_ENGINE_ID))
.and_then(|log| match log {
ConsensusLog::ForcedChange(delay, change) => Some((delay, change)),
_ => None,
})
}
}

impl<Number: Codec> ConsensusLogReader for GrandpaConsensusLogReader<Number> {
fn schedules_authorities_change(digest: &Digest) -> bool {
GrandpaConsensusLogReader::<Number>::find_authorities_change(digest).is_some()
GrandpaConsensusLogReader::<Number>::find_scheduled_change(digest).is_some()
}
}

Expand Down
7 changes: 3 additions & 4 deletions relays/lib-substrate-relay/src/finality/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,9 @@ impl<C: ChainWithGrandpa> Engine<C> for Grandpa<C> {
// If initial header changes the GRANDPA authorities set, then we need previous authorities
// to verify justification.
let mut authorities_for_verification = initial_authorities_set.clone();
let scheduled_change =
GrandpaConsensusLogReader::<BlockNumberOf<C>>::find_authorities_change(
initial_header.digest(),
);
let scheduled_change = GrandpaConsensusLogReader::<BlockNumberOf<C>>::find_scheduled_change(
initial_header.digest(),
);
assert!(
scheduled_change.as_ref().map(|c| c.delay.is_zero()).unwrap_or(true),
"GRANDPA authorities change at {} scheduled to happen in {:?} blocks. We expect\
Expand Down