Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Commit

Permalink
Introduce NimbusApi and deprecate AuthorFilterAPI (#21)
Browse files Browse the repository at this point in the history 
* primitives

* consensus worker

* author-inherent

* template runtime

* account set

* more runtime

* restore old api, remove versioning  on new api

* remove commented code

* spelling

* Remove old api from runtime

* Revert "Remove old api from runtime"

This reverts commit 6975319.

* checkpoint

* just panic in the runtime

* Deref properly (consensus now compiles)
  • Loading branch information
@JoshOrndorff
JoshOrndorff authored Dec 2, 2021
1 parent 3df70c3 commit 7d54dc6
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 77 deletions.
81 changes: 49 additions & 32 deletions nimbus-consensus/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use log::{info, warn, debug};
use parking_lot::Mutex;
use polkadot_client::ClientHandle;
use sc_client_api::Backend;
use sp_api::{ProvideRuntimeApi, BlockId, ApiExt};
use sp_api::{ProvideRuntimeApi, BlockId, ApiExt as _};
use sp_consensus::{
BlockOrigin, EnableProofRecording, Environment,
ProofRecording, Proposal, Proposer,
Expand All @@ -45,7 +45,7 @@ use tracing::error;
use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore};
use sp_core::crypto::Public;
use sp_std::convert::TryInto;
use nimbus_primitives::{AuthorFilterAPI, NIMBUS_KEY_ID, NimbusId};
use nimbus_primitives::{AuthorFilterAPI, NimbusApi, NIMBUS_KEY_ID, NimbusId};
mod import_queue;

const LOG_TARGET: &str = "filtering-consensus";
Expand Down Expand Up @@ -170,7 +170,9 @@ where
Proof = <EnableProofRecording as ProofRecording>::Proof,
>,
ParaClient: ProvideRuntimeApi<B> + Send + Sync,
// We require the client to provide both runtime apis, but only one will be called
ParaClient::Api: AuthorFilterAPI<B, NimbusId>,
ParaClient::Api: NimbusApi<B>,
CIDP: CreateInherentDataProviders<B, (PHash, PersistedValidationData, NimbusId)>,
{
async fn produce_candidate(
Expand All @@ -183,7 +185,7 @@ where
// those keys until we find one that is eligible. If none are eligible, we skip this slot.
// If multiple are eligible, we only author with the first one.

// Get allthe available keys
// Get all the available keys
let available_keys =
SyncCryptoStore::keys(&*self.keystore, NIMBUS_KEY_ID)
.expect("keystore should return the keys it has");
Expand All @@ -195,18 +197,40 @@ where
}

let at = BlockId::Hash(parent.hash());
// Get `AuthorFilterAPI` version.
let api_version = self.parachain_client.runtime_api()
.api_version::<dyn AuthorFilterAPI<B, NimbusId>>(&at)
.expect("Runtime api access to not error.");

if api_version.is_none() {
tracing::error!(
target: LOG_TARGET, "Could not find `AuthorFilterAPI` version.",
);
return None;
}
let api_version = api_version.unwrap();

// helper function for calling the various runtime apis and versions
let prediction_helper = |at, nimbus_id: NimbusId, slot: u32, parent| -> bool {

let has_nimbus_api = self
.parachain_client
.runtime_api()
.has_api::<dyn NimbusApi<B>>(at)
.expect("should be able to dynamically detect the api");

if has_nimbus_api {
NimbusApi::can_author(&*self.parachain_client.runtime_api(), at, nimbus_id, slot, parent)
.expect("NimbusAPI should not return error")
} else {
// There are two versions of the author filter, so we do that dynamically also.
let api_version = self.parachain_client.runtime_api()
.api_version::<dyn AuthorFilterAPI<B, NimbusId>>(&at)
.expect("Runtime api access to not error.")
.expect("Should be able to detect author filter version");

if api_version >= 2 {
AuthorFilterAPI::can_author(&*self.parachain_client.runtime_api(), at, nimbus_id, slot, parent)
.expect("Author API should not return error")
} else {
#[allow(deprecated)]
self.parachain_client.runtime_api().can_author_before_version_2(
&at,
nimbus_id,
slot,
)
.expect("Author API version 2 should not return error")
}
}
};

// Iterate keys until we find an eligible one, or run out of candidates.
// If we are skipping prediction, then we author withthe first key we find.
Expand All @@ -218,23 +242,12 @@ where

// Have to convert to a typed NimbusId to pass to the runtime API. Maybe this is a clue
// That I should be passing Vec<u8> across the wasm boundary?
if api_version >= 2 {
self.parachain_client.runtime_api().can_author(
&at,
NimbusId::from_slice(&type_public_pair.1),
validation_data.relay_parent_number,
parent,
)
.expect("Author API should not return error")
} else {
#[allow(deprecated)]
self.parachain_client.runtime_api().can_author_before_version_2(
&at,
NimbusId::from_slice(&type_public_pair.1),
validation_data.relay_parent_number,
)
.expect("Author API version 2 should not return error")
}
prediction_helper(
&at,
NimbusId::from_slice(&type_public_pair.1),
validation_data.relay_parent_number,
parent,
)
});

// If there are no eligible keys, print the log, and exit early.
Expand Down Expand Up @@ -393,6 +406,7 @@ where
// Rust bug: https://github.com/rust-lang/rust/issues/24159
sc_client_api::StateBackendFor<RBackend, PBlock>: sc_client_api::StateBackend<HashFor<PBlock>>,
ParaClient: ProvideRuntimeApi<Block> + Send + Sync + 'static,
ParaClient::Api: NimbusApi<Block>,
ParaClient::Api: AuthorFilterAPI<Block, NimbusId>,
CIDP: CreateInherentDataProviders<Block, (PHash, PersistedValidationData, NimbusId)> + 'static,
{
Expand Down Expand Up @@ -475,6 +489,7 @@ where
/// Build the nimbus consensus.
fn build(self) -> Box<dyn ParachainConsensus<Block>>
where
ParaClient::Api: NimbusApi<Block>,
ParaClient::Api: AuthorFilterAPI<Block, NimbusId>,
{
self.relay_chain_client.clone().execute_with(self)
Expand All @@ -497,6 +512,7 @@ where
BI: BlockImport<Block> + Send + Sync + 'static,
RBackend: Backend<PBlock> + 'static,
ParaClient: ProvideRuntimeApi<Block> + Send + Sync + 'static,
ParaClient::Api: NimbusApi<Block>,
ParaClient::Api: AuthorFilterAPI<Block, NimbusId>,
CIDP: CreateInherentDataProviders<Block, (PHash, PersistedValidationData, NimbusId)> + 'static,
{
Expand All @@ -509,6 +525,7 @@ where
PBackend::State: sp_api::StateBackend<sp_runtime::traits::BlakeTwo256>,
Api: polkadot_client::RuntimeApiCollection<StateBackend = PBackend::State>,
PClient: polkadot_client::AbstractClient<PBlock, PBackend, Api = Api> + 'static,
ParaClient::Api: NimbusApi<Block>,
ParaClient::Api: AuthorFilterAPI<Block, NimbusId>,
{
Box::new(NimbusConsensus::new(
Expand Down
27 changes: 19 additions & 8 deletions nimbus-primitives/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#![cfg_attr(not(feature = "std"), no_std)]

use sp_std::vec::Vec;
use parity_scale_codec::Codec;
use sp_application_crypto::KeyTypeId;
use sp_runtime::ConsensusEngineId;
use sp_runtime::traits::BlockNumberProvider;
Expand Down Expand Up @@ -95,13 +94,13 @@ impl<T> CanAuthor<T> for () {
/// different notions of AccoutId. It is also generic over the AuthorId to
/// support the usecase where the author inherent is used for beneficiary info
/// and contains an AccountId directly.
pub trait AccountLookup<AuthorId, AccountId> {
fn lookup_account(author: &AuthorId) -> Option<AccountId>;
pub trait AccountLookup<AccountId> {
fn lookup_account(author: &NimbusId) -> Option<AccountId>;
}

// A dummy impl used in simple tests
impl<AuthorId, AccountId> AccountLookup<AuthorId, AccountId> for () {
fn lookup_account(_: &AuthorId) -> Option<AccountId> {
impl<AccountId> AccountLookup<AccountId> for () {
fn lookup_account(_: &NimbusId) -> Option<AccountId> {
None
}
}
Expand Down Expand Up @@ -136,12 +135,24 @@ sp_application_crypto::with_pair! {


sp_api::decl_runtime_apis! {
/// The runtime api used to predict whether an author will be eligible in the given slot
/// The runtime api used to predict whether a Nimbus author will be eligible in the given slot
pub trait NimbusApi {
fn can_author(author: NimbusId, relay_parent: u32, parent_header: &Block::Header) -> bool;
}


// #[deprecated]
// The macro ended up always making the warning print
// so I decided to bail on that.

/// Deprecated Runtime API from earlier versions of Nimbus.
/// It is retained for now so that live chains can temporarily support both
/// for a smooth migration. It will be removed soon.
#[api_version(2)]
pub trait AuthorFilterAPI<AuthorId: Codec> {
pub trait AuthorFilterAPI<AuthorId: parity_scale_codec::Codec> {
#[changed_in(2)]
fn can_author(author: AuthorId, relay_parent: u32) -> bool;

fn can_author(author: AuthorId, relay_parent: u32, parent_header: &Block::Header) -> bool;
}
}
}
34 changes: 12 additions & 22 deletions pallets/author-inherent/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ use frame_support::{
use parity_scale_codec::{Decode, Encode};
use sp_inherents::{InherentIdentifier, IsFatalError};
use sp_runtime::{
ConsensusEngineId, DigestItem, RuntimeString, RuntimeAppPublic,
ConsensusEngineId, DigestItem, RuntimeString,
};
use log::debug;
use nimbus_primitives::{AccountLookup, CanAuthor, NIMBUS_ENGINE_ID, SlotBeacon, EventHandler, INHERENT_IDENTIFIER};
use nimbus_primitives::{AccountLookup, CanAuthor, NimbusId, NIMBUS_ENGINE_ID, SlotBeacon, EventHandler, INHERENT_IDENTIFIER};

mod exec;
pub use exec::BlockExecutor;
Expand All @@ -48,15 +48,10 @@ pub mod pallet {

#[pallet::config]
pub trait Config: frame_system::Config {
// This is copied from Aura. I wonder if I really need all those trait bounds. For now I'll leave them.
// TODO could I remove this type entirely and just always use NimbusId? Why didn't Aura do that?
/// The identifier type for an authority.
type AuthorId: Member + Parameter + RuntimeAppPublic + Default + MaybeSerializeDeserialize;

/// A type to convert between AuthorId and AccountId. This is useful when you want to associate
/// Block authoring behavior with an AccoutId for rewards or slashing. If you do not need to
/// hold an AccountID responsible for authoring use `()` which acts as an identity mapping.
type AccountLookup: AccountLookup<Self::AuthorId, Self::AccountId>;
type AccountLookup: AccountLookup<Self::AccountId>;

/// Other pallets that want to be informed about block authorship
type EventHandler: EventHandler<Self::AccountId>;
Expand All @@ -72,14 +67,10 @@ pub mod pallet {
type SlotBeacon: SlotBeacon;
}

// If the AccountId type supports it, then this pallet can be BoundToRuntimeAppPublic
impl<T> sp_runtime::BoundToRuntimeAppPublic for Pallet<T>
where
T: Config,
T::AuthorId: RuntimeAppPublic,
{
type Public = T::AuthorId;
impl<T> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
type Public = NimbusId;
}

#[pallet::error]
pub enum Error<T> {
/// Author already set in block.
Expand Down Expand Up @@ -107,7 +98,7 @@ pub mod pallet {
impl<T: Config> Pallet<T> {
/// Inherent to set the author of a block
#[pallet::weight((0, DispatchClass::Mandatory))]
pub fn set_author(origin: OriginFor<T>, author: T::AuthorId) -> DispatchResult {
pub fn set_author(origin: OriginFor<T>, author: NimbusId) -> DispatchResult {

ensure_none(origin)?;

Expand Down Expand Up @@ -155,7 +146,7 @@ pub mod pallet {

fn create_inherent(data: &InherentData) -> Option<Self::Call> {
let author_raw = data
.get_data::<T::AuthorId>(&INHERENT_IDENTIFIER);
.get_data::<NimbusId>(&INHERENT_IDENTIFIER);

debug!("In create_inherent (runtime side). data is");
debug!("{:?}", author_raw);
Expand All @@ -182,10 +173,10 @@ pub mod pallet {
}
}

/// To learn whether a given AuthorId can author, you call the author-inherent directly.
/// It will do the mapping lookup.
impl<T: Config> CanAuthor<T::AuthorId> for Pallet<T> {
fn can_author(author: &T::AuthorId, slot: &u32) -> bool {
/// To learn whether a given NimbusId can author, as opposed to an account id, you
/// can ask this pallet directly. It will do the mapping for you.
impl<T: Config> CanAuthor<NimbusId> for Pallet<T> {
fn can_author(author: &NimbusId, slot: &u32) -> bool {
let account = match T::AccountLookup::lookup_account(&author) {
Some(account) => account,
// Authors whose account lookups fail will not be eligible
Expand Down Expand Up @@ -241,7 +232,6 @@ mod tests {
testing::Header,
traits::{BlakeTwo256, IdentityLookup},
};
use nimbus_primitives::NimbusId;
use sp_core::Public;
const TEST_AUTHOR_ID: [u8; 32] = [0u8; 32];
const BOGUS_AUTHOR_ID: [u8; 32] = [1u8; 32];
Expand Down
16 changes: 10 additions & 6 deletions parachain-template/runtime/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,6 @@ impl cumulus_pallet_dmp_queue::Config for Runtime {
}

impl pallet_author_inherent::Config for Runtime {
type AuthorId = NimbusId;
// We start a new slot each time we see a new relay block.
type SlotBeacon = cumulus_pallet_parachain_system::RelaychainBlockNumberProvider<Self>;
type AccountLookup = PotentialAuthorSet;
Expand All @@ -551,9 +550,7 @@ impl pallet_author_slot_filter::Config for Runtime {
type PotentialAuthors = PotentialAuthorSet;
}

impl pallet_account_set::Config for Runtime {
type AuthorId = NimbusId;
}
impl pallet_account_set::Config for Runtime {}

/// Configure the pallet template in pallets/template.
impl pallet_template::Config for Runtime {
Expand Down Expand Up @@ -693,8 +690,8 @@ impl_runtime_apis! {
}
}

impl nimbus_primitives::AuthorFilterAPI<Block, nimbus_primitives::NimbusId> for Runtime {
fn can_author(author: nimbus_primitives::NimbusId, slot: u32, parent_header: &<Block as BlockT>::Header) -> bool {
impl nimbus_primitives::NimbusApi<Block> for Runtime {
fn can_author(author: NimbusId, slot: u32, parent_header: &<Block as BlockT>::Header) -> bool {
// This runtime uses an entropy source that is updated during block initialization
// Therefore we need to initialize it to match the state it will be in when the
// next block is being executed.
Expand All @@ -706,6 +703,13 @@ impl_runtime_apis! {
}
}

// We also implement the olf AuthorFilterAPI to meet the trait bounds on the client side.
impl nimbus_primitives::AuthorFilterAPI<Block, NimbusId> for Runtime {
fn can_author(_: NimbusId, _: u32, _: &<Block as BlockT>::Header) -> bool {
panic!("AuthorFilterAPI is no longer supported. Please update your client.")
}
}

#[cfg(feature = "runtime-benchmarks")]
impl frame_benchmarking::Benchmark<Block> for Runtime {
fn benchmark_metadata(extra: bool) -> (
Expand Down
15 changes: 6 additions & 9 deletions parachain-template/runtime/src/pallet_account_set.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,15 @@ pub mod pallet {
use log::warn;
use frame_support::pallet_prelude::*;
use sp_std::vec::Vec;
use nimbus_primitives::{AccountLookup, CanAuthor};
use nimbus_primitives::{AccountLookup, CanAuthor, NimbusId};

/// The Account Set pallet
#[pallet::pallet]
pub struct Pallet<T>(PhantomData<T>);

/// Configuration trait of this pallet.
#[pallet::config]
pub trait Config: frame_system::Config {
/// The identifier type for an author.
type AuthorId: Member + Parameter + MaybeSerializeDeserialize;
}
pub trait Config: frame_system::Config {}

/// The set of accounts that is stored in this pallet.
#[pallet::storage]
Expand All @@ -63,13 +60,13 @@ pub mod pallet {
#[pallet::getter(fn account_id_of)]
/// A mapping from the AuthorIds used in the consensus layer
/// to the AccountIds runtime.
type Mapping<T: Config> = StorageMap<_, Twox64Concat, T::AuthorId, T::AccountId, OptionQuery>;
type Mapping<T: Config> = StorageMap<_, Twox64Concat, NimbusId, T::AccountId, OptionQuery>;

#[pallet::genesis_config]
/// Genesis config for author mapping pallet
pub struct GenesisConfig<T: Config> {
/// The associations that should exist at chain genesis
pub mapping: Vec<(T::AccountId, T::AuthorId)>,
pub mapping: Vec<(T::AccountId, NimbusId)>,
}

#[cfg(feature = "std")]
Expand Down Expand Up @@ -101,8 +98,8 @@ pub mod pallet {
}
}

impl<T: Config> AccountLookup<T::AuthorId, T::AccountId> for Pallet<T> {
fn lookup_account(author: &T::AuthorId) -> Option<T::AccountId> {
impl<T: Config> AccountLookup<T::AccountId> for Pallet<T> {
fn lookup_account(author: &NimbusId) -> Option<T::AccountId> {
Mapping::<T>::get(&author)
}
}
Expand Down

0 comments on commit 7d54dc6

Please sign in to comment.