From 211be1d5f477fa025f7813238b95a1524ad4212f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 17 Mar 2021 22:18:16 +0100 Subject: [PATCH] Storing multiple Justifications per block (#7640) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * primitives/runtime: initial changes on supporting multiple Justifications * primitives/runtime: make Justifications strongly typed * Encode/decode Justifications * primitives/runtime: add Justification type * backend: apply_finality and finalize_block takes a single Justification * manual-seal: create engine id and let rpc take encoded justification * backend: skeleton functions for appending justifications * backend: initial implementation append_justification Initial implementation of append_justification on the Backend trait, and also remove unused skeleton functions for append_justificaton on Finaziler trait. k * backend: guard against duplicate consensus engine id * client/db: add check for block finality * client/api: add append_justification to in_mem db * client/light: add no-op append_justification * network: fix decode call for Justification * network: only send a single Justification in BlockData * network: minor comment update * protocol: update field names to distinguish single justification * client: further field renames to plural * client: update function names to plural justifications * client/db: upgrade existing database for new format * network: remove dependency on grandpa crate * db: fix check for finalized block * grandpa: check for multiple grandpa justifications hwne importing * backend: update Finalizer trait to take multiple Justifications * db: remove debugging statements in migration code * manual-seal: update note about engine id * db: fix check for finalized block * client: update variable name to reflect it is now plural * grandpa: fix incorrect empty Justications in test * primitives: make Justifications opaque to avoid being empty * network: fix detecting empty Justification * runtime: doc strings for Justifications functions * runtime: add into_justifications * primitives: check for duplicates in when adding to Justifications * network/test: use real grandpa engine id in test * client: fix reviewer comments * primitives: rename Justifications::push to append * backend: revert changes to Finalizer trait * backend: revert mark_finalized * backend: revert changes to finalize_block * backend: revert finalized_blocks * db: add a quick early return for performance * client: minor reviewer comments * service/test: use local ConsensusEngineId * network: add link to issue for sending multiple Justifications * Apply suggestions from code review Co-authored-by: Pierre Krieger * Apply suggestions from code review Co-authored-by: Pierre Krieger * network: tweaks to review suggestions * network: revert change to BlockData for backwards compatibility * Apply suggestion from code review Co-authored-by: Pierre Krieger * Apply suggestions from code review Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * primitives: update doc comment for Justifications * client/db/upgrade: avoid grandpa crate dependency * consensus: revert to single Justification for import_justification * primitives: improve justifications docs * style cleanups * use and_then * client: rename JUSTIFICATIONS db column * network: revert to using FRNK in network-test Co-authored-by: Pierre Krieger Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: André Silva --- client/api/src/backend.rs | 16 ++- client/api/src/client.rs | 6 +- client/api/src/in_mem.rs | 125 ++++++++++++++++-- client/consensus/aura/src/import_queue.rs | 6 +- client/consensus/babe/src/lib.rs | 10 +- client/consensus/babe/src/tests.rs | 4 +- client/consensus/manual-seal/src/lib.rs | 10 +- client/consensus/manual-seal/src/rpc.rs | 8 +- client/consensus/pow/src/lib.rs | 6 +- client/db/src/changes_tries_storage.rs | 3 +- client/db/src/lib.rs | 116 +++++++++++++--- client/db/src/upgrade.rs | 41 +++++- .../finality-grandpa-warp-sync/src/proof.rs | 21 ++- client/finality-grandpa/src/environment.rs | 13 +- client/finality-grandpa/src/finality_proof.rs | 93 ++++++------- client/finality-grandpa/src/import.rs | 19 ++- client/finality-grandpa/src/tests.rs | 16 +-- client/light/src/backend.rs | 14 +- client/light/src/blockchain.rs | 4 +- client/network/src/behaviour.rs | 4 +- client/network/src/block_request_handler.rs | 24 +++- client/network/src/gossip/tests.rs | 4 +- client/network/src/protocol.rs | 13 +- client/network/src/protocol/message.rs | 4 +- client/network/src/protocol/sync.rs | 30 +++-- client/network/src/service.rs | 4 +- client/network/src/service/tests.rs | 4 +- client/network/test/src/block_import.rs | 4 +- client/network/test/src/lib.rs | 16 +-- client/network/test/src/sync.rs | 63 ++++++--- client/rpc/src/chain/chain_light.rs | 2 +- client/rpc/src/chain/tests.rs | 2 +- client/service/src/chain_ops/import_blocks.rs | 2 +- client/service/src/client/client.rs | 22 +-- client/service/test/src/client/light.rs | 7 +- client/service/test/src/client/mod.rs | 19 +-- primitives/blockchain/src/backend.rs | 6 +- .../consensus/common/src/block_import.rs | 10 +- .../consensus/common/src/import_queue.rs | 20 +-- .../common/src/import_queue/basic_queue.rs | 33 ++--- primitives/runtime/src/generic/block.rs | 4 +- primitives/runtime/src/lib.rs | 60 ++++++++- test-utils/client/src/client_ext.rs | 17 ++- 43 files changed, 635 insertions(+), 270 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index e41b250269a11..3108ba899467b 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -22,7 +22,7 @@ use std::sync::Arc; use std::collections::{HashMap, HashSet}; use sp_core::ChangesTrieConfigurationRange; use sp_core::offchain::OffchainStorage; -use sp_runtime::{generic::BlockId, Justification, Storage}; +use sp_runtime::{generic::BlockId, Justification, Justifications, Storage}; use sp_runtime::traits::{Block as BlockT, NumberFor, HashFor}; use sp_state_machine::{ ChangesTrieState, ChangesTrieStorage as StateChangesTrieStorage, ChangesTrieTransaction, @@ -148,7 +148,7 @@ pub trait BlockImportOperation { &mut self, header: Block::Header, body: Option>, - justification: Option, + justifications: Option, state: NewBlockState, ) -> sp_blockchain::Result<()>; @@ -197,6 +197,7 @@ pub trait BlockImportOperation { id: BlockId, justification: Option, ) -> sp_blockchain::Result<()>; + /// Mark a block as new head. If both block import and set head are specified, set head /// overrides block import's best block rule. fn mark_head(&mut self, id: BlockId) -> sp_blockchain::Result<()>; @@ -230,7 +231,6 @@ pub trait Finalizer> { notify: bool, ) -> sp_blockchain::Result<()>; - /// Finalize a block. /// /// This will implicitly finalize all blocks up to it and @@ -250,7 +250,6 @@ pub trait Finalizer> { justification: Option, notify: bool, ) -> sp_blockchain::Result<()>; - } /// Provides access to an auxiliary database. @@ -432,6 +431,15 @@ pub trait Backend: AuxStore + Send + Sync { justification: Option, ) -> sp_blockchain::Result<()>; + /// Append justification to the block with the given Id. + /// + /// This should only be called for blocks that are already finalized. + fn append_justification( + &self, + block: BlockId, + justification: Justification, + ) -> sp_blockchain::Result<()>; + /// Returns reference to blockchain backend. fn blockchain(&self) -> &Self::Blockchain; diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 990a7908b62bb..97fe77c8d81e1 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -23,7 +23,7 @@ use sp_core::storage::StorageKey; use sp_runtime::{ traits::{Block as BlockT, NumberFor}, generic::{BlockId, SignedBlock}, - Justification, + Justifications, }; use sp_consensus::BlockOrigin; @@ -90,8 +90,8 @@ pub trait BlockBackend { /// Get block status. fn block_status(&self, id: &BlockId) -> sp_blockchain::Result; - /// Get block justification set by id. - fn justification(&self, id: &BlockId) -> sp_blockchain::Result>; + /// Get block justifications for the block with the given id. + fn justifications(&self, id: &BlockId) -> sp_blockchain::Result>; /// Get block hash by number. fn block_hash(&self, number: NumberFor) -> sp_blockchain::Result>; diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index b7060cf1d9b1b..c3d266954278e 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -27,7 +27,7 @@ use sp_core::{ }; use sp_runtime::generic::BlockId; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, Zero, NumberFor, HashFor}; -use sp_runtime::{Justification, Storage}; +use sp_runtime::{Justification, Justifications, Storage}; use sp_state_machine::{ ChangesTrieTransaction, InMemoryBackend, Backend as StateBackend, StorageCollection, ChildStorageCollection, @@ -51,12 +51,12 @@ struct PendingBlock { #[derive(PartialEq, Eq, Clone)] enum StoredBlock { - Header(B::Header, Option), - Full(B, Option), + Header(B::Header, Option), + Full(B, Option), } impl StoredBlock { - fn new(header: B::Header, body: Option>, just: Option) -> Self { + fn new(header: B::Header, body: Option>, just: Option) -> Self { match body { Some(body) => StoredBlock::Full(B::new(header, body), just), None => StoredBlock::Header(header, just), @@ -70,7 +70,7 @@ impl StoredBlock { } } - fn justification(&self) -> Option<&Justification> { + fn justifications(&self) -> Option<&Justifications> { match *self { StoredBlock::Header(_, ref j) | StoredBlock::Full(_, ref j) => j.as_ref() } @@ -83,7 +83,7 @@ impl StoredBlock { } } - fn into_inner(self) -> (B::Header, Option>, Option) { + fn into_inner(self) -> (B::Header, Option>, Option) { match self { StoredBlock::Header(header, just) => (header, None, just), StoredBlock::Full(block, just) => { @@ -164,7 +164,7 @@ impl Blockchain { &self, hash: Block::Hash, header: ::Header, - justification: Option, + justifications: Option, body: Option::Extrinsic>>, new_state: NewBlockState, ) -> sp_blockchain::Result<()> { @@ -176,7 +176,7 @@ impl Blockchain { { let mut storage = self.storage.write(); storage.leaves.import(hash.clone(), number.clone(), header.parent_hash().clone()); - storage.blocks.insert(hash.clone(), StoredBlock::new(header, body, justification)); + storage.blocks.insert(hash.clone(), StoredBlock::new(header, body, justifications)); if let NewBlockState::Final = new_state { storage.finalized_hash = hash; @@ -285,16 +285,44 @@ impl Blockchain { let block = storage.blocks.get_mut(&hash) .expect("hash was fetched from a block in the db; qed"); - let block_justification = match block { + let block_justifications = match block { StoredBlock::Header(_, ref mut j) | StoredBlock::Full(_, ref mut j) => j }; - *block_justification = justification; + *block_justifications = justification.map(Justifications::from); } Ok(()) } + fn append_justification(&self, id: BlockId, justification: Justification) + -> sp_blockchain::Result<()> + { + let hash = self.expect_block_hash_from_id(&id)?; + let mut storage = self.storage.write(); + + let block = storage + .blocks + .get_mut(&hash) + .expect("hash was fetched from a block in the db; qed"); + + let block_justifications = match block { + StoredBlock::Header(_, ref mut j) | StoredBlock::Full(_, ref mut j) => j + }; + + if let Some(stored_justifications) = block_justifications { + if !stored_justifications.append(justification) { + return Err(sp_blockchain::Error::BadJustification( + "Duplicate consensus engine ID".into() + )); + } + } else { + *block_justifications = Some(Justifications::from(justification)); + }; + + Ok(()) + } + fn write_aux(&self, ops: Vec<(Vec, Option>)>) { let mut storage = self.storage.write(); for (k, v) in ops { @@ -365,9 +393,9 @@ impl blockchain::Backend for Blockchain { })) } - fn justification(&self, id: BlockId) -> sp_blockchain::Result> { + fn justifications(&self, id: BlockId) -> sp_blockchain::Result> { Ok(self.id(id).and_then(|hash| self.storage.read().blocks.get(&hash).and_then(|b| - b.justification().map(|x| x.clone())) + b.justifications().map(|x| x.clone())) )) } @@ -508,12 +536,12 @@ impl backend::BlockImportOperation for BlockImportOperatio &mut self, header: ::Header, body: Option::Extrinsic>>, - justification: Option, + justifications: Option, state: NewBlockState, ) -> sp_blockchain::Result<()> { assert!(self.pending_block.is_none(), "Only one block per operation is allowed"); self.pending_block = Some(PendingBlock { - block: StoredBlock::new(header, body, justification), + block: StoredBlock::new(header, body, justifications), state, }); Ok(()) @@ -696,6 +724,14 @@ impl backend::Backend for Backend where Block::Hash self.blockchain.finalize_header(block, justification) } + fn append_justification( + &self, + block: BlockId, + justification: Justification, + ) -> sp_blockchain::Result<()> { + self.blockchain.append_justification(block, justification) + } + fn blockchain(&self) -> &Self::Blockchain { &self.blockchain } @@ -766,3 +802,64 @@ pub fn check_genesis_storage(storage: &Storage) -> sp_blockchain::Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use crate::{NewBlockState, in_mem::Blockchain}; + use sp_api::{BlockId, HeaderT}; + use sp_runtime::{ConsensusEngineId, Justifications}; + use sp_blockchain::Backend; + use substrate_test_runtime::{Block, Header, H256}; + + pub const ID1: ConsensusEngineId = *b"TST1"; + pub const ID2: ConsensusEngineId = *b"TST2"; + + fn header(number: u64) -> Header { + let parent_hash = match number { + 0 => Default::default(), + _ => header(number - 1).hash(), + }; + Header::new(number, H256::from_low_u64_be(0), H256::from_low_u64_be(0), parent_hash, Default::default()) + } + + fn test_blockchain() -> Blockchain { + let blockchain = Blockchain::::new(); + let just0 = Some(Justifications::from((ID1, vec![0]))); + let just1 = Some(Justifications::from((ID1, vec![1]))); + let just2 = None; + let just3 = Some(Justifications::from((ID1, vec![3]))); + blockchain.insert(header(0).hash(), header(0), just0, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(1).hash(), header(1), just1, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(2).hash(), header(2), just2, None, NewBlockState::Best).unwrap(); + blockchain.insert(header(3).hash(), header(3), just3, None, NewBlockState::Final).unwrap(); + blockchain + } + + #[test] + fn append_and_retrieve_justifications() { + let blockchain = test_blockchain(); + let last_finalized = blockchain.last_finalized().unwrap(); + let block = BlockId::Hash(last_finalized); + + blockchain.append_justification(block, (ID2, vec![4])).unwrap(); + let justifications = { + let mut just = Justifications::from((ID1, vec![3])); + just.append((ID2, vec![4])); + just + }; + assert_eq!(blockchain.justifications(block).unwrap(), Some(justifications)); + } + + #[test] + fn store_duplicate_justifications_is_forbidden() { + let blockchain = test_blockchain(); + let last_finalized = blockchain.last_finalized().unwrap(); + let block = BlockId::Hash(last_finalized); + + blockchain.append_justification(block, (ID2, vec![0])).unwrap(); + assert!(matches!( + blockchain.append_justification(block, (ID2, vec![1])), + Err(sp_blockchain::Error::BadJustification(_)), + )); + } +} diff --git a/client/consensus/aura/src/import_queue.rs b/client/consensus/aura/src/import_queue.rs index d3ed2bea3e115..c33d937d93a2b 100644 --- a/client/consensus/aura/src/import_queue.rs +++ b/client/consensus/aura/src/import_queue.rs @@ -39,7 +39,7 @@ use sp_consensus::{ use sc_client_api::{backend::AuxStore, BlockOf}; use sp_blockchain::{well_known_cache_keys::{self, Id as CacheKeyId}, ProvideCache, HeaderBackend}; use sp_block_builder::BlockBuilder as BlockBuilderApi; -use sp_runtime::{generic::{BlockId, OpaqueDigestItemId}, Justification}; +use sp_runtime::{generic::{BlockId, OpaqueDigestItemId}, Justifications}; use sp_runtime::traits::{Block as BlockT, Header, DigestItemFor, Zero}; use sp_api::ProvideRuntimeApi; use sp_core::crypto::Pair; @@ -238,7 +238,7 @@ impl Verifier for AuraVerifier where &mut self, origin: BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, mut body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { let mut inherent_data = self.inherent_data_providers @@ -317,7 +317,7 @@ impl Verifier for AuraVerifier where let mut import_block = BlockImportParams::new(origin, pre_header); import_block.post_digests.push(seal); import_block.body = body; - import_block.justification = justification; + import_block.justifications = justifications; import_block.fork_choice = Some(ForkChoiceStrategy::LongestChain); import_block.post_hash = Some(hash); diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 3d72c436361c5..727ee29221b22 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -84,7 +84,7 @@ use sp_core::crypto::Public; use sp_application_crypto::AppKey; use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore}; use sp_runtime::{ - generic::{BlockId, OpaqueDigestItemId}, Justification, + generic::{BlockId, OpaqueDigestItemId}, Justifications, traits::{Block as BlockT, Header, DigestItemFor, Zero}, }; use sp_api::{ProvideRuntimeApi, NumberFor}; @@ -1097,15 +1097,15 @@ where &mut self, origin: BlockOrigin, header: Block::Header, - justification: Option, + justifications: Option, mut body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { trace!( target: "babe", - "Verifying origin: {:?} header: {:?} justification: {:?} body: {:?}", + "Verifying origin: {:?} header: {:?} justification(s): {:?} body: {:?}", origin, header, - justification, + justifications, body, ); @@ -1194,7 +1194,7 @@ where let mut import_block = BlockImportParams::new(origin, pre_header); import_block.post_digests.push(verified_info.seal); import_block.body = body; - import_block.justification = justification; + import_block.justifications = justifications; import_block.intermediates.insert( Cow::from(INTERMEDIATE_KEY), Box::new(BabeIntermediate:: { epoch_descriptor }) as Box, diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index d3e51b020326c..70b4cd7b0b61d 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -235,12 +235,12 @@ impl Verifier for TestVerifier { &mut self, origin: BlockOrigin, mut header: TestHeader, - justification: Option, + justifications: Option, body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { // apply post-sealing mutations (i.e. stripping seal, if desired). (self.mutator)(&mut header, Stage::PostSeal); - self.inner.verify(origin, header, justification, body) + self.inner.verify(origin, header, justifications, body) } } diff --git a/client/consensus/manual-seal/src/lib.rs b/client/consensus/manual-seal/src/lib.rs index 64de70939503c..870640c1f2012 100644 --- a/client/consensus/manual-seal/src/lib.rs +++ b/client/consensus/manual-seal/src/lib.rs @@ -27,7 +27,7 @@ use sp_consensus::{ }; use sp_blockchain::HeaderBackend; use sp_inherents::InherentDataProviders; -use sp_runtime::{traits::Block as BlockT, Justification}; +use sp_runtime::{traits::Block as BlockT, Justifications, ConsensusEngineId}; use sc_client_api::backend::{Backend as ClientBackend, Finalizer}; use sc_transaction_pool::txpool; use std::{sync::Arc, marker::PhantomData}; @@ -49,6 +49,9 @@ pub use self::{ }; use sp_api::{ProvideRuntimeApi, TransactionFor}; +/// The `ConsensusEngineId` of Manual Seal. +pub const MANUAL_SEAL_ENGINE_ID: ConsensusEngineId = [b'm', b'a', b'n', b'l']; + /// The verifier for the manual seal engine; instantly finalizes. struct ManualSealVerifier; @@ -57,11 +60,11 @@ impl Verifier for ManualSealVerifier { &mut self, origin: BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { let mut import_params = BlockImportParams::new(origin, header); - import_params.justification = justification; + import_params.justifications = justifications; import_params.body = body; import_params.finalized = false; import_params.fork_choice = Some(ForkChoiceStrategy::LongestChain); @@ -193,6 +196,7 @@ pub async fn run_manual_seal( ).await; } EngineCommand::FinalizeBlock { hash, sender, justification } => { + let justification = justification.map(|j| (MANUAL_SEAL_ENGINE_ID, j)); finalize_block( FinalizeBlockParams { hash, diff --git a/client/consensus/manual-seal/src/rpc.rs b/client/consensus/manual-seal/src/rpc.rs index 293d4487a5d59..eb056f22fed8b 100644 --- a/client/consensus/manual-seal/src/rpc.rs +++ b/client/consensus/manual-seal/src/rpc.rs @@ -28,7 +28,7 @@ use futures::{ SinkExt }; use serde::{Deserialize, Serialize}; -use sp_runtime::Justification; +use sp_runtime::EncodedJustification; pub use self::gen_client::Client as ManualSealClient; /// Future's type for jsonrpc @@ -62,7 +62,7 @@ pub enum EngineCommand { /// sender to report errors/success to the rpc. sender: Sender<()>, /// finalization justification - justification: Option, + justification: Option, } } @@ -83,7 +83,7 @@ pub trait ManualSealApi { fn finalize_block( &self, hash: Hash, - justification: Option + justification: Option ) -> FutureResult; } @@ -131,7 +131,7 @@ impl ManualSealApi for ManualSeal { Box::new(future.map_err(Error::from).compat()) } - fn finalize_block(&self, hash: Hash, justification: Option) -> FutureResult { + fn finalize_block(&self, hash: Hash, justification: Option) -> FutureResult { let mut sink = self.import_block_channel.clone(); let future = async move { let (sender, receiver) = oneshot::channel(); diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index 482bc80170fe8..d1df2875a1cb6 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -44,7 +44,7 @@ use parking_lot::Mutex; use sc_client_api::{BlockOf, backend::AuxStore, BlockchainEvents}; use sp_blockchain::{HeaderBackend, ProvideCache, well_known_cache_keys::Id as CacheKeyId}; use sp_block_builder::BlockBuilder as BlockBuilderApi; -use sp_runtime::{Justification, RuntimeString}; +use sp_runtime::{Justifications, RuntimeString}; use sp_runtime::generic::{BlockId, Digest, DigestItem}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; use sp_api::ProvideRuntimeApi; @@ -457,7 +457,7 @@ impl Verifier for PowVerifier where &mut self, origin: BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { let hash = header.hash(); @@ -470,7 +470,7 @@ impl Verifier for PowVerifier where let mut import_block = BlockImportParams::new(origin, checked_header); import_block.post_digests.push(seal); import_block.body = body; - import_block.justification = justification; + import_block.justifications = justifications; import_block.intermediates.insert( Cow::from(INTERMEDIATE_KEY), Box::new(intermediate) as Box diff --git a/client/db/src/changes_tries_storage.rs b/client/db/src/changes_tries_storage.rs index 6233eab3ea396..8051adc1832bc 100644 --- a/client/db/src/changes_tries_storage.rs +++ b/client/db/src/changes_tries_storage.rs @@ -955,7 +955,8 @@ mod tests { let block0 = insert_header_with_configuration_change(&backend, 0, Default::default(), None, config0); let config1 = Some(ChangesTrieConfiguration::new(2, 6)); let block1 = insert_header_with_configuration_change(&backend, 1, block0, changes(0), config1); - backend.finalize_block(BlockId::Number(1), Some(vec![42])).unwrap(); + let just1 = Some((*b"TEST", vec![42])); + backend.finalize_block(BlockId::Number(1), just1).unwrap(); let config2 = Some(ChangesTrieConfiguration::new(2, 7)); let block2 = insert_header_with_configuration_change(&backend, 2, block1, changes(1), config2); let config2_1 = Some(ChangesTrieConfiguration::new(2, 8)); diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 6654083939dae..acda057938e92 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -57,10 +57,11 @@ use sc_client_api::{ UsageInfo, MemoryInfo, IoInfo, MemorySize, backend::{NewBlockState, PrunableStateChangesTrieStorage, ProvideChtRoots}, leaves::{LeafSet, FinalizationDisplaced}, cht, + utils::is_descendent_of, }; use sp_blockchain::{ Result as ClientResult, Error as ClientError, - well_known_cache_keys, HeaderBackend, + well_known_cache_keys, Backend as _, HeaderBackend, }; use codec::{Decode, Encode}; use hash_db::Prefix; @@ -70,7 +71,7 @@ use sp_core::{Hasher, ChangesTrieConfiguration}; use sp_core::offchain::OffchainOverlayedChange; use sp_core::storage::{well_known_keys, ChildInfo}; use sp_arithmetic::traits::Saturating; -use sp_runtime::{generic::{DigestItem, BlockId}, Justification, Storage}; +use sp_runtime::{generic::{DigestItem, BlockId}, Justification, Justifications, Storage}; use sp_runtime::traits::{ Block as BlockT, Header as HeaderT, NumberFor, Zero, One, SaturatedConversion, HashFor, }; @@ -351,7 +352,7 @@ pub(crate) mod columns { pub const KEY_LOOKUP: u32 = 3; pub const HEADER: u32 = 4; pub const BODY: u32 = 5; - pub const JUSTIFICATION: u32 = 6; + pub const JUSTIFICATIONS: u32 = 6; pub const CHANGES_TRIE: u32 = 7; pub const AUX: u32 = 8; /// Offchain workers local storage @@ -363,7 +364,7 @@ pub(crate) mod columns { struct PendingBlock { header: Block::Header, - justification: Option, + justifications: Option, body: Option>, leaf_state: NewBlockState, } @@ -535,8 +536,8 @@ impl sc_client_api::blockchain::Backend for BlockchainDb) -> ClientResult> { - match read_db(&*self.db, columns::KEY_LOOKUP, columns::JUSTIFICATION, id)? { + fn justifications(&self, id: BlockId) -> ClientResult> { + match read_db(&*self.db, columns::KEY_LOOKUP, columns::JUSTIFICATIONS, id)? { Some(justification) => match Decode::decode(&mut &justification[..]) { Ok(justification) => Ok(Some(justification)), Err(err) => return Err(sp_blockchain::Error::Backend( @@ -716,7 +717,7 @@ impl sc_client_api::backend::BlockImportOperation for Bloc &mut self, header: Block::Header, body: Option>, - justification: Option, + justifications: Option, leaf_state: NewBlockState, ) -> ClientResult<()> { assert!(self.pending_block.is_none(), "Only one block per operation is allowed"); @@ -726,7 +727,7 @@ impl sc_client_api::backend::BlockImportOperation for Bloc self.pending_block = Some(PendingBlock { header, body, - justification, + justifications, leaf_state, }); Ok(()) @@ -1130,9 +1131,9 @@ impl Backend { if let Some(justification) = justification { transaction.set_from_vec( - columns::JUSTIFICATION, + columns::JUSTIFICATIONS, &utils::number_and_hash_to_lookup_key(number, hash)?, - justification.encode(), + Justifications::from(justification).encode(), ); } Ok((*hash, number, false, true)) @@ -1241,8 +1242,8 @@ impl Backend { }, } } - if let Some(justification) = pending_block.justification { - transaction.set_from_vec(columns::JUSTIFICATION, &lookup_key, justification.encode()); + if let Some(justifications) = pending_block.justifications { + transaction.set_from_vec(columns::JUSTIFICATIONS, &lookup_key, justifications.encode()); } if number.is_zero() { @@ -1409,7 +1410,7 @@ impl Backend { self.storage.db.commit(transaction)?; - // Apply all in-memory state shanges. + // Apply all in-memory state changes. // Code beyond this point can't fail. if let Some(( @@ -1668,6 +1669,50 @@ impl sc_client_api::backend::Backend for Backend { Ok(()) } + fn append_justification( + &self, + block: BlockId, + justification: Justification, + ) -> ClientResult<()> { + let mut transaction: Transaction = Transaction::new(); + let hash = self.blockchain.expect_block_hash_from_id(&block)?; + let header = self.blockchain.expect_header(block)?; + let number = *header.number(); + + // Check if the block is finalized first. + let is_descendent_of = is_descendent_of(&self.blockchain, None); + let last_finalized = self.blockchain.last_finalized()?; + + // We can do a quick check first, before doing a proper but more expensive check + if number > self.blockchain.info().finalized_number + || (hash != last_finalized && !is_descendent_of(&hash, &last_finalized)?) + { + return Err(ClientError::NotInFinalizedChain); + } + + let justifications = + if let Some(mut stored_justifications) = self.blockchain.justifications(block)? { + if !stored_justifications.append(justification) { + return Err(ClientError::BadJustification( + "Duplicate consensus engine ID".into() + )); + } + stored_justifications + } else { + Justifications::from(justification) + }; + + transaction.set_from_vec( + columns::JUSTIFICATIONS, + &utils::number_and_hash_to_lookup_key(number, hash)?, + justifications.encode(), + ); + + self.storage.db.commit(transaction)?; + + Ok(()) + } + fn changes_trie_storage(&self) -> Option<&dyn PrunableStateChangesTrieStorage> { Some(&self.changes_tries_storage) } @@ -1918,12 +1963,16 @@ pub(crate) mod tests { use sp_core::H256; use sc_client_api::backend::{Backend as BTrait, BlockImportOperation as Op}; use sc_client_api::blockchain::Backend as BLBTrait; + use sp_runtime::ConsensusEngineId; use sp_runtime::testing::{Header, Block as RawBlock, ExtrinsicWrapper}; use sp_runtime::traits::{Hash, BlakeTwo256}; use sp_runtime::generic::DigestItem; use sp_state_machine::{TrieMut, TrieDBMut}; use sp_blockchain::{lowest_common_ancestor, tree_route}; + const CONS0_ENGINE_ID: ConsensusEngineId = *b"CON0"; + const CONS1_ENGINE_ID: ConsensusEngineId = *b"CON1"; + pub(crate) type Block = RawBlock>; pub fn prepare_changes(changes: Vec<(Vec, Vec)>) -> (H256, MemoryDB) { @@ -2511,12 +2560,47 @@ pub(crate) mod tests { let block0 = insert_header(&backend, 0, Default::default(), None, Default::default()); let _ = insert_header(&backend, 1, block0, None, Default::default()); - let justification = Some(vec![1, 2, 3]); + let justification = Some((CONS0_ENGINE_ID, vec![1, 2, 3])); backend.finalize_block(BlockId::Number(1), justification.clone()).unwrap(); assert_eq!( - backend.blockchain().justification(BlockId::Number(1)).unwrap(), - justification, + backend.blockchain().justifications(BlockId::Number(1)).unwrap(), + justification.map(Justifications::from), + ); + } + + #[test] + fn test_append_justification_to_finalized_block() { + use sc_client_api::blockchain::{Backend as BlockChainBackend}; + + let backend = Backend::::new_test(10, 10); + + let block0 = insert_header(&backend, 0, Default::default(), None, Default::default()); + let _ = insert_header(&backend, 1, block0, None, Default::default()); + + let just0 = (CONS0_ENGINE_ID, vec![1, 2, 3]); + backend.finalize_block( + BlockId::Number(1), + Some(just0.clone().into()), + ).unwrap(); + + let just1 = (CONS1_ENGINE_ID, vec![4, 5]); + backend.append_justification(BlockId::Number(1), just1.clone()).unwrap(); + + let just2 = (CONS1_ENGINE_ID, vec![6, 7]); + assert!(matches!( + backend.append_justification(BlockId::Number(1), just2), + Err(ClientError::BadJustification(_)) + )); + + let justifications = { + let mut just = Justifications::from(just0); + just.append(just1); + just + }; + assert_eq!( + backend.blockchain().justifications(BlockId::Number(1)).unwrap(), + Some(justifications), ); } diff --git a/client/db/src/upgrade.rs b/client/db/src/upgrade.rs index b6e49edba1978..6c7cbbb4a1af6 100644 --- a/client/db/src/upgrade.rs +++ b/client/db/src/upgrade.rs @@ -23,17 +23,19 @@ use std::io::{Read, Write, ErrorKind}; use std::path::{Path, PathBuf}; use sp_runtime::traits::Block as BlockT; -use crate::utils::DatabaseType; +use crate::{columns, utils::DatabaseType}; use kvdb_rocksdb::{Database, DatabaseConfig}; +use codec::Encode; /// Version file name. const VERSION_FILE_NAME: &'static str = "db_version"; /// Current db version. -const CURRENT_VERSION: u32 = 2; +const CURRENT_VERSION: u32 = 3; /// Number of columns in v1. const V1_NUM_COLUMNS: u32 = 11; +const V2_NUM_COLUMNS: u32 = 12; /// Upgrade database to current version. pub fn upgrade_db(db_path: &Path, db_type: DatabaseType) -> sp_blockchain::Result<()> { @@ -42,7 +44,11 @@ pub fn upgrade_db(db_path: &Path, db_type: DatabaseType) -> sp_bl let db_version = current_version(db_path)?; match db_version { 0 => Err(sp_blockchain::Error::Backend(format!("Unsupported database version: {}", db_version)))?, - 1 => migrate_1_to_2::(db_path, db_type)?, + 1 => { + migrate_1_to_2::(db_path, db_type)?; + migrate_2_to_3::(db_path, db_type)? + }, + 2 => migrate_2_to_3::(db_path, db_type)?, CURRENT_VERSION => (), _ => Err(sp_blockchain::Error::Backend(format!("Future database version: {}", db_version)))?, } @@ -62,6 +68,31 @@ fn migrate_1_to_2(db_path: &Path, _db_type: DatabaseType) -> sp_b db.add_column().map_err(db_err) } +/// Migration from version2 to version3: +/// - The format of the stored Justification changed to support multiple Justifications. +fn migrate_2_to_3(db_path: &Path, _db_type: DatabaseType) -> sp_blockchain::Result<()> { + let db_path = db_path.to_str() + .ok_or_else(|| sp_blockchain::Error::Backend("Invalid database path".into()))?; + let db_cfg = DatabaseConfig::with_columns(V2_NUM_COLUMNS); + let db = Database::open(&db_cfg, db_path).map_err(db_err)?; + + // Get all the keys we need to update + let keys: Vec<_> = db.iter(columns::JUSTIFICATIONS).map(|entry| entry.0).collect(); + + // Read and update each entry + let mut transaction = db.transaction(); + for key in keys { + if let Some(justification) = db.get(columns::JUSTIFICATIONS, &key).map_err(db_err)? { + // Tag each Justification with the hardcoded ID for GRANDPA. Avoid the dependency on the GRANDPA crate + let justifications = sp_runtime::Justifications::from((*b"FRNK", justification)); + transaction.put_vec(columns::JUSTIFICATIONS, &key, justifications.encode()); + } + } + db.write(transaction).map_err(db_err)?; + + Ok(()) +} + /// Reads current database version from the file at given path. /// If the file does not exist returns 0. fn current_version(path: &Path) -> sp_blockchain::Result { @@ -141,8 +172,8 @@ mod tests { } #[test] - fn upgrade_from_1_to_2_works() { - for version_from_file in &[None, Some(1)] { + fn upgrade_to_3_works() { + for version_from_file in &[None, Some(1), Some(2)] { let db_dir = tempfile::TempDir::new().unwrap(); let db_path = db_dir.path(); create_db(db_path, *version_from_file); diff --git a/client/finality-grandpa-warp-sync/src/proof.rs b/client/finality-grandpa-warp-sync/src/proof.rs index 1b447d2ef720c..e6fb989abc9d8 100644 --- a/client/finality-grandpa-warp-sync/src/proof.rs +++ b/client/finality-grandpa-warp-sync/src/proof.rs @@ -20,7 +20,7 @@ use sc_finality_grandpa::{ find_scheduled_change, AuthoritySetChanges, BlockNumberOps, GrandpaJustification, }; use sp_blockchain::Backend as BlockchainBackend; -use sp_finality_grandpa::{AuthorityList, SetId}; +use sp_finality_grandpa::{AuthorityList, SetId, GRANDPA_ENGINE_ID}; use sp_runtime::{ generic::BlockId, traits::{Block as BlockT, NumberFor}, @@ -108,11 +108,14 @@ impl WarpSyncProof { break; } - let justification = backend.justification(BlockId::Number(*last_block))?.expect( - "header is last in set and contains standard change signal; \ - must have justification; \ - qed.", - ); + let justification = backend + .justifications(BlockId::Number(*last_block))? + .and_then(|just| just.into_justification(GRANDPA_ENGINE_ID)) + .expect( + "header is last in set and contains standard change signal; \ + must have justification; \ + qed.", + ); let justification = GrandpaJustification::::decode(&mut &justification[..])?; @@ -171,6 +174,7 @@ mod tests { use sc_finality_grandpa::{AuthoritySetChanges, GrandpaJustification}; use sp_blockchain::HeaderBackend; use sp_consensus::BlockOrigin; + use sp_finality_grandpa::GRANDPA_ENGINE_ID; use sp_keyring::Ed25519Keyring; use sp_runtime::{generic::BlockId, traits::Header as _}; use std::sync::Arc; @@ -272,7 +276,10 @@ mod tests { let justification = GrandpaJustification::from_commit(&client, 42, commit).unwrap(); client - .finalize_block(BlockId::Hash(target_hash), Some(justification.encode())) + .finalize_block( + BlockId::Hash(target_hash), + Some((GRANDPA_ENGINE_ID, justification.encode())) + ) .unwrap(); authority_set_changes.push((current_set_id, n)); diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 5bb525549b18c..27ff1e57b670c 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -55,7 +55,7 @@ use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; use sp_finality_grandpa::{ - AuthorityId, AuthoritySignature, Equivocation, EquivocationProof, + AuthorityId, AuthoritySignature, Equivocation, EquivocationProof, GRANDPA_ENGINE_ID, GrandpaApi, RoundNumber, SetId, }; use prometheus_endpoint::{register, Counter, Gauge, PrometheusError, U64}; @@ -1326,10 +1326,13 @@ where // ideally some handle to a synchronization oracle would be used // to avoid unconditionally notifying. - client.apply_finality(import_op, BlockId::Hash(hash), justification, true).map_err(|e| { - warn!(target: "afg", "Error applying finality to block {:?}: {:?}", (hash, number), e); - e - })?; + let justification = justification.map(|j| (GRANDPA_ENGINE_ID, j.clone())); + client + .apply_finality(import_op, BlockId::Hash(hash), justification, true) + .map_err(|e| { + warn!(target: "afg", "Error applying finality to block {:?}: {:?}", (hash, number), e); + e + })?; telemetry!( telemetry; CONSENSUS_INFO; diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index b79b3190739d6..80ba8cee9101e 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -43,11 +43,11 @@ use finality_grandpa::BlockNumberOps; use parity_scale_codec::{Encode, Decode}; use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; use sp_runtime::{ - Justification, generic::BlockId, + EncodedJustification, generic::BlockId, traits::{NumberFor, Block as BlockT, Header as HeaderT, One}, }; use sc_client_api::backend::Backend; -use sp_finality_grandpa::AuthorityId; +use sp_finality_grandpa::{AuthorityId, GRANDPA_ENGINE_ID}; use crate::authorities::AuthoritySetChanges; use crate::justification::GrandpaJustification; @@ -190,8 +190,10 @@ where // Get the Justification stored at the last block of the set let last_block_for_set_id = BlockId::Number(last_block_for_set); let justification = - if let Some(justification) = blockchain.justification(last_block_for_set_id)? { - justification + if let Some(grandpa_justification) = blockchain.justifications(last_block_for_set_id)? + .and_then(|justifications| justifications.into_justification(GRANDPA_ENGINE_ID)) + { + grandpa_justification } else { trace!( target: "afg", @@ -257,7 +259,7 @@ pub trait ProvableJustification: Encode + Decode { /// Decode and verify justification. fn decode_and_verify( - justification: &Justification, + justification: &EncodedJustification, set_id: u64, authorities: &[(AuthorityId, u64)], ) -> ClientResult { @@ -286,6 +288,7 @@ pub(crate) mod tests { use super::*; use crate::authorities::AuthoritySetChanges; use sp_core::crypto::Public; + use sp_runtime::Justifications; use sp_finality_grandpa::AuthorityList; use sc_client_api::NewBlockState; use sc_client_api::in_mem::Blockchain as InMemoryBlockchain; @@ -330,31 +333,27 @@ pub(crate) mod tests { } fn test_blockchain() -> InMemoryBlockchain { + use sp_finality_grandpa::GRANDPA_ENGINE_ID as ID; let blockchain = InMemoryBlockchain::::new(); - blockchain - .insert(header(0).hash(), header(0), Some(vec![0]), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(1).hash(), header(1), Some(vec![1]), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(2).hash(), header(2), None, None, NewBlockState::Best) - .unwrap(); - blockchain - .insert(header(3).hash(), header(3), Some(vec![3]), None, NewBlockState::Final) - .unwrap(); + let just0 = Some(Justifications::from((ID, vec![0]))); + let just1 = Some(Justifications::from((ID, vec![1]))); + let just2 = None; + let just3 = Some(Justifications::from((ID, vec![3]))); + blockchain.insert(header(0).hash(), header(0), just0, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(1).hash(), header(1), just1, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(2).hash(), header(2), just2, None, NewBlockState::Best).unwrap(); + blockchain.insert(header(3).hash(), header(3), just3, None, NewBlockState::Final).unwrap(); blockchain } #[test] fn finality_proof_fails_if_no_more_last_finalized_blocks() { + use sp_finality_grandpa::GRANDPA_ENGINE_ID as ID; let blockchain = test_blockchain(); - blockchain - .insert(header(4).hash(), header(4), Some(vec![1]), None, NewBlockState::Best) - .unwrap(); - blockchain - .insert(header(5).hash(), header(5), Some(vec![2]), None, NewBlockState::Best) - .unwrap(); + let just1 = Some(Justifications::from((ID, vec![1]))); + let just2 = Some(Justifications::from((ID, vec![2]))); + blockchain.insert(header(4).hash(), header(4), just1, None, NewBlockState::Best).unwrap(); + blockchain.insert(header(5).hash(), header(5), just2, None, NewBlockState::Best).unwrap(); let mut authority_set_changes = AuthoritySetChanges::empty(); authority_set_changes.append(0, 5); @@ -430,22 +429,17 @@ pub(crate) mod tests { #[test] fn finality_proof_using_authority_set_changes_fails_with_undefined_start() { + use sp_finality_grandpa::GRANDPA_ENGINE_ID as ID; let blockchain = test_blockchain(); let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let just4 = TestJustification((0, auth.clone()), vec![4]).encode(); - let just7 = TestJustification((1, auth.clone()), vec![7]).encode(); - blockchain - .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(5).hash(), header(5), None, None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(6).hash(), header(6), None, None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(7).hash(), header(7), Some(just7.clone()), None, NewBlockState::Final) - .unwrap(); + let grandpa_just4 = TestJustification((0, auth.clone()), vec![4]).encode(); + let grandpa_just7 = TestJustification((1, auth.clone()), vec![7]).encode(); + let just4 = Some(Justifications::from((ID, grandpa_just4))); + let just7 = Some(Justifications::from((ID, grandpa_just7))); + blockchain.insert(header(4).hash(), header(4), just4, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(5).hash(), header(5), None, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(6).hash(), header(6), None, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(7).hash(), header(7), just7, None, NewBlockState::Final).unwrap(); // We have stored the correct block number for the relevant set, but as we are missing the // block for the preceding set the start is not well-defined. @@ -462,22 +456,17 @@ pub(crate) mod tests { #[test] fn finality_proof_using_authority_set_changes_works() { + use sp_finality_grandpa::GRANDPA_ENGINE_ID as ID; let blockchain = test_blockchain(); let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let just4 = TestJustification((0, auth.clone()), vec![4]).encode(); - let just7 = TestJustification((1, auth.clone()), vec![7]).encode(); - blockchain - .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(5).hash(), header(5), None, None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(6).hash(), header(6), None, None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(7).hash(), header(7), Some(just7.clone()), None, NewBlockState::Final) - .unwrap(); + let grandpa_just4 = TestJustification((0, auth.clone()), vec![4]).encode(); + let grandpa_just7 = TestJustification((1, auth.clone()), vec![7]).encode(); + let just4 = Some(Justifications::from((ID, grandpa_just4))); + let just7 = Some(Justifications::from((ID, grandpa_just7.clone()))); + blockchain.insert(header(4).hash(), header(4), just4, None, NewBlockState::Final) .unwrap(); + blockchain.insert(header(5).hash(), header(5), None, None, NewBlockState::Final) .unwrap(); + blockchain.insert(header(6).hash(), header(6), None, None, NewBlockState::Final).unwrap(); + blockchain.insert(header(7).hash(), header(7), just7, None, NewBlockState::Final).unwrap(); let mut authority_set_changes = AuthoritySetChanges::empty(); authority_set_changes.append(0, 4); @@ -497,7 +486,7 @@ pub(crate) mod tests { proof_of_5, FinalityProof { block: header(7).hash(), - justification: just7, + justification: grandpa_just7, unknown_headers: vec![header(6)], } ); diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 22d7b7fd5bcc8..6814d5dfb6195 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -451,7 +451,7 @@ impl BlockImport let pending_changes = self.make_authorities_changes(&mut block, hash, initial_sync)?; // we don't want to finalize on `inner.import_block` - let mut justification = block.justification.take(); + let mut justifications = block.justifications.take(); let import_result = (&*self.inner).import_block(block, new_cache); let mut imported_aux = { @@ -513,17 +513,20 @@ impl BlockImport // need to apply first, drop any justification that might have been provided with // the block to make sure we request them from `sync` which will ensure they'll be // applied in-order. - justification.take(); + justifications.take(); }, _ => {}, } - match justification { + let grandpa_justification = justifications + .and_then(|just| just.into_justification(GRANDPA_ENGINE_ID)); + + match grandpa_justification { Some(justification) => { let import_res = self.import_justification( hash, number, - justification, + (GRANDPA_ENGINE_ID, justification), needs_justification, initial_sync, ); @@ -637,8 +640,14 @@ where enacts_change: bool, initial_sync: bool, ) -> Result<(), ConsensusError> { + if justification.0 != GRANDPA_ENGINE_ID { + return Err(ConsensusError::ClientImport( + "GRANDPA can only import GRANDPA Justifications.".into(), + )); + } + let justification = GrandpaJustification::decode_and_verify_finalizes( - &justification, + &justification.1, (hash, number), self.authority_set.set_id(), &self.authority_set.current_authorities(), diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 6824a8ed04273..42d0a10d34e08 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -39,7 +39,7 @@ use sp_consensus::{ import_queue::BoxJustificationImport, }; use std::{collections::{HashMap, HashSet}, pin::Pin}; -use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; +use sp_runtime::{Justifications, traits::{Block as BlockT, Header as HeaderT}}; use sp_runtime::generic::{BlockId, DigestItem}; use sp_core::H256; use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore}; @@ -369,7 +369,7 @@ fn finalize_3_voters_no_observers() { // normally there's no justification for finalized blocks assert!( - net.lock().peer(0).client().justification(&BlockId::Number(20)).unwrap().is_none(), + net.lock().peer(0).client().justifications(&BlockId::Number(20)).unwrap().is_none(), "Extra justification for block#1", ); } @@ -613,7 +613,7 @@ fn justification_is_generated_periodically() { // when block#32 (justification_period) is finalized, justification // is required => generated for i in 0..3 { - assert!(net.lock().peer(i).client().justification(&BlockId::Number(32)).unwrap().is_some()); + assert!(net.lock().peer(i).client().justifications(&BlockId::Number(32)).unwrap().is_some()); } } @@ -658,12 +658,12 @@ fn sync_justifications_on_change_blocks() { // the first 3 peers are grandpa voters and therefore have already finalized // block 21 and stored a justification for i in 0..3 { - assert!(net.lock().peer(i).client().justification(&BlockId::Number(21)).unwrap().is_some()); + assert!(net.lock().peer(i).client().justifications(&BlockId::Number(21)).unwrap().is_some()); } // the last peer should get the justification by syncing from other peers futures::executor::block_on(futures::future::poll_fn(move |cx| { - if net.lock().peer(3).client().justification(&BlockId::Number(21)).unwrap().is_none() { + if net.lock().peer(3).client().justifications(&BlockId::Number(21)).unwrap().is_none() { net.lock().poll(cx); Poll::Pending } else { @@ -868,7 +868,7 @@ fn test_bad_justification() { let block = || { let block = block.clone(); let mut import = BlockImportParams::new(BlockOrigin::File, block.header); - import.justification = Some(Vec::new()); + import.justifications = Some(Justifications::from((GRANDPA_ENGINE_ID, Vec::new()))); import.body = Some(block.extrinsics); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); @@ -1583,7 +1583,7 @@ fn imports_justification_for_regular_blocks_on_import() { // we import the block with justification attached let mut import = BlockImportParams::new(BlockOrigin::File, block.header); - import.justification = Some(justification.encode()); + import.justifications = Some((GRANDPA_ENGINE_ID, justification.encode()).into()); import.body = Some(block.extrinsics); import.fork_choice = Some(ForkChoiceStrategy::LongestChain); @@ -1600,7 +1600,7 @@ fn imports_justification_for_regular_blocks_on_import() { // the justification should be imported and available from the client assert!( - client.justification(&BlockId::Hash(block_hash)).unwrap().is_some(), + client.justifications(&BlockId::Hash(block_hash)).unwrap().is_some(), ); } diff --git a/client/light/src/backend.rs b/client/light/src/backend.rs index 27e0754eb552e..52ace4fd94753 100644 --- a/client/light/src/backend.rs +++ b/client/light/src/backend.rs @@ -32,7 +32,7 @@ use sp_state_machine::{ Backend as StateBackend, TrieBackend, InMemoryBackend, ChangesTrieTransaction, StorageCollection, ChildStorageCollection, }; -use sp_runtime::{generic::BlockId, Justification, Storage}; +use sp_runtime::{generic::BlockId, Justification, Justifications, Storage}; use sp_runtime::traits::{Block as BlockT, NumberFor, Zero, Header, HashFor}; use sp_blockchain::{Error as ClientError, Result as ClientResult}; use sc_client_api::{ @@ -199,6 +199,14 @@ impl ClientBackend for Backend> self.blockchain.storage().finalize_header(block) } + fn append_justification( + &self, + _block: BlockId, + _justification: Justification, + ) -> ClientResult<()> { + Ok(()) + } + fn blockchain(&self) -> &Blockchain { &self.blockchain } @@ -278,7 +286,7 @@ impl BlockImportOperation for ImportOperation &mut self, header: Block::Header, _body: Option>, - _justification: Option, + _justifications: Option, state: NewBlockState, ) -> ClientResult<()> { self.leaf_state = state; @@ -356,7 +364,7 @@ impl BlockImportOperation for ImportOperation fn mark_finalized( &mut self, block: BlockId, - _justification: Option, + _justifications: Option, ) -> ClientResult<()> { self.finalized_blocks.push(block); Ok(()) diff --git a/client/light/src/blockchain.rs b/client/light/src/blockchain.rs index bcabc365676a5..062b3a9866d08 100644 --- a/client/light/src/blockchain.rs +++ b/client/light/src/blockchain.rs @@ -21,7 +21,7 @@ use std::sync::Arc; -use sp_runtime::{Justification, generic::BlockId}; +use sp_runtime::{Justifications, generic::BlockId}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor, Zero}; use sp_blockchain::{ @@ -109,7 +109,7 @@ impl BlockchainBackend for Blockchain where Block: BlockT, S Err(ClientError::NotAvailableOnLightClient) } - fn justification(&self, _id: BlockId) -> ClientResult> { + fn justifications(&self, _id: BlockId) -> ClientResult> { Err(ClientError::NotAvailableOnLightClient) } diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index 0eebd1713cc81..a73685ed3bf32 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -37,7 +37,7 @@ use libp2p::swarm::{ use log::debug; use prost::Message; use sp_consensus::{BlockOrigin, import_queue::{IncomingBlock, Origin}}; -use sp_runtime::{traits::{Block as BlockT, NumberFor}, Justification}; +use sp_runtime::{traits::{Block as BlockT, NumberFor}, Justifications}; use std::{ borrow::Cow, collections::{HashSet, VecDeque}, @@ -84,7 +84,7 @@ pub struct Behaviour { /// Event generated by `Behaviour`. pub enum BehaviourOut { BlockImport(BlockOrigin, Vec>), - JustificationImport(Origin, B::Hash, NumberFor, Justification), + JustificationImport(Origin, B::Hash, NumberFor, Justifications), /// Started a random iterative Kademlia discovery query. RandomKademliaStarted(ProtocolId), diff --git a/client/network/src/block_request_handler.rs b/client/network/src/block_request_handler.rs index 148bc01302f7f..2cc888c220f62 100644 --- a/client/network/src/block_request_handler.rs +++ b/client/network/src/block_request_handler.rs @@ -275,12 +275,28 @@ impl BlockRequestHandler { let number = *header.number(); let hash = header.hash(); let parent_hash = *header.parent_hash(); - let justification = if get_justification { - self.client.justification(&BlockId::Hash(hash))? + let justifications = if get_justification { + self.client.justifications(&BlockId::Hash(hash))? } else { None }; - let is_empty_justification = justification.as_ref().map(|j| j.is_empty()).unwrap_or(false); + + // TODO: In a follow up PR tracked by https://github.com/paritytech/substrate/issues/8172 + // we want to send/receive all justifications. + // For now we keep compatibility by selecting precisely the GRANDPA one, and not just + // the first one. When sending we could have just taken the first one, since we don't + // expect there to be any other kind currently, but when receiving we need to add the + // engine ID tag. + // The ID tag is hardcoded here to avoid depending on the GRANDPA crate, and will be + // removed when resolving the above issue. + let justification = justifications.and_then(|just| just.into_justification(*b"FRNK")); + + let is_empty_justification = justification + .as_ref() + .map(|j| j.is_empty()) + .unwrap_or(false); + + let justification = justification.unwrap_or_default(); let body = if get_body { match self.client.block_body(&BlockId::Hash(hash))? { @@ -306,7 +322,7 @@ impl BlockRequestHandler { body, receipt: Vec::new(), message_queue: Vec::new(), - justification: justification.unwrap_or_default(), + justification, is_empty_justification, }; diff --git a/client/network/src/gossip/tests.rs b/client/network/src/gossip/tests.rs index 89ad5fcf047dc..cd637f162721e 100644 --- a/client/network/src/gossip/tests.rs +++ b/client/network/src/gossip/tests.rs @@ -52,7 +52,7 @@ fn build_test_full_node(network_config: config::NetworkConfiguration) &mut self, origin: sp_consensus::BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option>, ) -> Result< ( @@ -79,7 +79,7 @@ fn build_test_full_node(network_config: config::NetworkConfiguration) let mut import = sp_consensus::BlockImportParams::new(origin, header); import.body = body; import.finalized = self.0; - import.justification = justification; + import.justifications = justifications; import.fork_choice = Some(sp_consensus::ForkChoiceStrategy::LongestChain); Ok((import, maybe_keys)) } diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 7f321775b160e..11e1197998350 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -43,9 +43,10 @@ use sp_consensus::{ block_validation::BlockAnnounceValidator, import_queue::{BlockImportResult, BlockImportError, IncomingBlock, Origin} }; -use sp_runtime::{generic::BlockId, Justification}; -use sp_runtime::traits::{ - Block as BlockT, Header as HeaderT, NumberFor, Zero, CheckedSub +use sp_runtime::{ + Justifications, + generic::BlockId, + traits::{Block as BlockT, Header as HeaderT, NumberFor, Zero, CheckedSub}, }; use sp_arithmetic::traits::SaturatedConversion; use sync::{ChainSync, SyncState}; @@ -612,8 +613,8 @@ impl Protocol { if request.fields == message::BlockAttributes::JUSTIFICATION { match self.sync.on_block_justification(peer_id, block_response) { Ok(sync::OnBlockJustification::Nothing) => CustomMessageOutcome::None, - Ok(sync::OnBlockJustification::Import { peer, hash, number, justification }) => - CustomMessageOutcome::JustificationImport(peer, hash, number, justification), + Ok(sync::OnBlockJustification::Import { peer, hash, number, justifications }) => + CustomMessageOutcome::JustificationImport(peer, hash, number, justifications), Err(sync::BadPeer(id, repu)) => { self.behaviour.disconnect_peer(&id, HARDCODED_PEERSETS_SYNC); self.peerset_handle.report_peer(id, repu); @@ -1134,7 +1135,7 @@ fn prepare_block_request( #[must_use] pub enum CustomMessageOutcome { BlockImport(BlockOrigin, Vec>), - JustificationImport(Origin, B::Hash, NumberFor, Justification), + JustificationImport(Origin, B::Hash, NumberFor, Justifications), /// Notification protocols have been opened with a remote. NotificationStreamOpened { remote: PeerId, diff --git a/client/network/src/protocol/message.rs b/client/network/src/protocol/message.rs index ed2721032801c..01e9a5d7215af 100644 --- a/client/network/src/protocol/message.rs +++ b/client/network/src/protocol/message.rs @@ -148,7 +148,7 @@ pub struct RemoteReadResponse { pub mod generic { use bitflags::bitflags; use codec::{Encode, Decode, Input, Output}; - use sp_runtime::Justification; + use sp_runtime::EncodedJustification; use super::{ RemoteReadResponse, Transactions, Direction, RequestId, BlockAttributes, RemoteCallResponse, ConsensusEngineId, @@ -233,7 +233,7 @@ pub mod generic { /// Block message queue if requested. pub message_queue: Option>, /// Justification if requested. - pub justification: Option, + pub justification: Option, } /// Identifies starting point of a block sequence. diff --git a/client/network/src/protocol/sync.rs b/client/network/src/protocol/sync.rs index 35f840152217f..37f9a451b67d3 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -44,7 +44,7 @@ use extra_requests::ExtraRequests; use libp2p::PeerId; use log::{debug, trace, warn, info, error}; use sp_runtime::{ - Justification, + EncodedJustification, Justifications, generic::BlockId, traits::{ Block as BlockT, Header as HeaderT, NumberFor, Zero, One, CheckedSub, SaturatedConversion, @@ -425,7 +425,7 @@ pub enum OnBlockJustification { peer: PeerId, hash: B::Hash, number: NumberFor, - justification: Justification + justifications: Justifications } } @@ -823,11 +823,13 @@ impl ChainSync { .drain(self.best_queued_number + One::one()) .into_iter() .map(|block_data| { + let justifications = + legacy_justification_mapping(block_data.block.justification); IncomingBlock { hash: block_data.block.hash, header: block_data.block.header, body: block_data.block.body, - justification: block_data.block.justification, + justifications, origin: block_data.origin, allow_missing_state: true, import_existing: false, @@ -846,7 +848,7 @@ impl ChainSync { hash: b.hash, header: b.header, body: b.body, - justification: b.justification, + justifications: legacy_justification_mapping(b.justification), origin: Some(who.clone()), allow_missing_state: true, import_existing: false, @@ -955,7 +957,7 @@ impl ChainSync { hash: b.hash, header: b.header, body: b.body, - justification: b.justification, + justifications: legacy_justification_mapping(b.justification), origin: Some(who.clone()), allow_missing_state: true, import_existing: false, @@ -1039,8 +1041,11 @@ impl ChainSync { None }; - if let Some((peer, hash, number, j)) = self.extra_justifications.on_response(who, justification) { - return Ok(OnBlockJustification::Import { peer, hash, number, justification: j }) + if let Some((peer, hash, number, j)) = self + .extra_justifications + .on_response(who, legacy_justification_mapping(justification)) + { + return Ok(OnBlockJustification::Import { peer, hash, number, justifications: j }) } } @@ -1597,6 +1602,14 @@ impl ChainSync { } } +// This is purely during a backwards compatible transitionary period and should be removed +// once we can assume all nodes can send and receive multiple Justifications +// The ID tag is hardcoded here to avoid depending on the GRANDPA crate. +// TODO: https://github.com/paritytech/substrate/issues/8172 +fn legacy_justification_mapping(justification: Option) -> Option { + justification.map(|just| (*b"FRNK", just).into()) +} + #[derive(Debug)] pub(crate) struct Metrics { pub(crate) queued_blocks: u32, @@ -2396,7 +2409,8 @@ mod test { ); let finalized_block = blocks[MAX_BLOCKS_TO_LOOK_BACKWARDS as usize * 2 - 1].clone(); - client.finalize_block(BlockId::Hash(finalized_block.hash()), Some(Vec::new())).unwrap(); + let just = (*b"TEST", Vec::new()); + client.finalize_block(BlockId::Hash(finalized_block.hash()), Some(just)).unwrap(); sync.update_chain_info(&info.best_hash, info.best_number); let peer_id1 = PeerId::random(); diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 74ce9316fc41c..54a5559d2eaf9 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1452,11 +1452,11 @@ impl Future for NetworkWorker { } this.import_queue.import_blocks(origin, blocks); }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::JustificationImport(origin, hash, nb, justification))) => { + Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::JustificationImport(origin, hash, nb, justifications))) => { if let Some(metrics) = this.metrics.as_ref() { metrics.import_queue_justifications_submitted.inc(); } - this.import_queue.import_justification(origin, hash, nb, justification); + this.import_queue.import_justifications(origin, hash, nb, justifications); }, Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::InboundRequest { protocol, result, .. })) => { if let Some(metrics) = this.metrics.as_ref() { diff --git a/client/network/src/service/tests.rs b/client/network/src/service/tests.rs index 660eac82c4c60..fd8cf4c3d105f 100644 --- a/client/network/src/service/tests.rs +++ b/client/network/src/service/tests.rs @@ -52,7 +52,7 @@ fn build_test_full_node(config: config::NetworkConfiguration) &mut self, origin: sp_consensus::BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option>, ) -> Result< ( @@ -79,7 +79,7 @@ fn build_test_full_node(config: config::NetworkConfiguration) let mut import = sp_consensus::BlockImportParams::new(origin, header); import.body = body; import.finalized = self.0; - import.justification = justification; + import.justifications = justifications; import.fork_choice = Some(sp_consensus::ForkChoiceStrategy::LongestChain); Ok((import, maybe_keys)) } diff --git a/client/network/test/src/block_import.rs b/client/network/test/src/block_import.rs index 4000e53420b4a..200c7357c4244 100644 --- a/client/network/test/src/block_import.rs +++ b/client/network/test/src/block_import.rs @@ -35,13 +35,13 @@ fn prepare_good_block() -> (TestClient, Hash, u64, PeerId, IncomingBlock) let (hash, number) = (client.block_hash(1).unwrap().unwrap(), 1); let header = client.header(&BlockId::Number(1)).unwrap(); - let justification = client.justification(&BlockId::Number(1)).unwrap(); + let justifications = client.justifications(&BlockId::Number(1)).unwrap(); let peer_id = PeerId::random(); (client, hash, number, peer_id.clone(), IncomingBlock { hash, header, body: Some(Vec::new()), - justification, + justifications, origin: Some(peer_id.clone()), allow_missing_state: false, import_existing: false, diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index c8b442d0dd563..1c237f94700c3 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -63,7 +63,7 @@ use sp_core::H256; use sc_network::config::ProtocolConfig; use sp_runtime::generic::{BlockId, OpaqueDigestItemId}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; -use sp_runtime::Justification; +use sp_runtime::{Justification, Justifications}; use substrate_test_runtime_client::{self, AccountKeyring}; use sc_service::client::Client; pub use sc_network::config::EmptyTransactionPool; @@ -109,7 +109,7 @@ impl Verifier for PassThroughVerifier { &mut self, origin: BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option> ) -> Result<(BlockImportParams, Option)>>), String> { let maybe_keys = header.digest() @@ -120,7 +120,7 @@ impl Verifier for PassThroughVerifier { let mut import = BlockImportParams::new(origin, header); import.body = body; import.finalized = self.finalized; - import.justification = justification; + import.justifications = justifications; import.fork_choice = Some(self.fork_choice.clone()); Ok((import, maybe_keys)) @@ -184,10 +184,10 @@ impl PeersClient { } } - pub fn justification(&self, block: &BlockId) -> ClientResult> { + pub fn justifications(&self, block: &BlockId) -> ClientResult> { match *self { - PeersClient::Full(ref client, ref _backend) => client.justification(block), - PeersClient::Light(ref client, ref _backend) => client.justification(block), + PeersClient::Full(ref client, ref _backend) => client.justifications(block), + PeersClient::Light(ref client, ref _backend) => client.justifications(block), } } @@ -577,11 +577,11 @@ impl Verifier for VerifierAdapter { &mut self, origin: BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option> ) -> Result<(BlockImportParams, Option)>>), String> { let hash = header.hash(); - self.verifier.lock().verify(origin, header, justification, body).map_err(|e| { + self.verifier.lock().verify(origin, header, justifications, body).map_err(|e| { self.failed_verifications.lock().insert(hash, e.clone()); e }) diff --git a/client/network/test/src/sync.rs b/client/network/test/src/sync.rs index b11dbaca75e14..953639dcc0e22 100644 --- a/client/network/test/src/sync.rs +++ b/client/network/test/src/sync.rs @@ -22,6 +22,7 @@ use futures::{Future, executor::block_on}; use super::*; use sp_consensus::block_validation::Validation; use substrate_test_runtime::Header; +use sp_runtime::Justifications; fn test_ancestor_search_when_common_is(n: usize) { sp_tracing::try_init_simple(); @@ -248,13 +249,14 @@ fn sync_justifications() { net.block_until_sync(); // there's currently no justification for block #10 - assert_eq!(net.peer(0).client().justification(&BlockId::Number(10)).unwrap(), None); - assert_eq!(net.peer(1).client().justification(&BlockId::Number(10)).unwrap(), None); + assert_eq!(net.peer(0).client().justifications(&BlockId::Number(10)).unwrap(), None); + assert_eq!(net.peer(1).client().justifications(&BlockId::Number(10)).unwrap(), None); // we finalize block #10, #15 and #20 for peer 0 with a justification - net.peer(0).client().finalize_block(BlockId::Number(10), Some(Vec::new()), true).unwrap(); - net.peer(0).client().finalize_block(BlockId::Number(15), Some(Vec::new()), true).unwrap(); - net.peer(0).client().finalize_block(BlockId::Number(20), Some(Vec::new()), true).unwrap(); + let just = (*b"FRNK", Vec::new()); + net.peer(0).client().finalize_block(BlockId::Number(10), Some(just.clone()), true).unwrap(); + net.peer(0).client().finalize_block(BlockId::Number(15), Some(just.clone()), true).unwrap(); + net.peer(0).client().finalize_block(BlockId::Number(20), Some(just.clone()), true).unwrap(); let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap(); let h2 = net.peer(1).client().header(&BlockId::Number(15)).unwrap().unwrap(); @@ -269,10 +271,20 @@ fn sync_justifications() { net.poll(cx); for height in (10..21).step_by(5) { - if net.peer(0).client().justification(&BlockId::Number(height)).unwrap() != Some(Vec::new()) { + if net + .peer(0) + .client() + .justifications(&BlockId::Number(height)) + .unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) + { return Poll::Pending; } - if net.peer(1).client().justification(&BlockId::Number(height)).unwrap() != Some(Vec::new()) { + if net + .peer(1) + .client() + .justifications(&BlockId::Number(height)) + .unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) + { return Poll::Pending; } } @@ -295,7 +307,8 @@ fn sync_justifications_across_forks() { // for both and finalize the small fork instead. net.block_until_sync(); - net.peer(0).client().finalize_block(BlockId::Hash(f1_best), Some(Vec::new()), true).unwrap(); + let just = (*b"FRNK", Vec::new()); + net.peer(0).client().finalize_block(BlockId::Hash(f1_best), Some(just), true).unwrap(); net.peer(1).request_justification(&f1_best, 10); net.peer(1).request_justification(&f2_best, 11); @@ -303,8 +316,16 @@ fn sync_justifications_across_forks() { block_on(futures::future::poll_fn::<(), _>(|cx| { net.poll(cx); - if net.peer(0).client().justification(&BlockId::Number(10)).unwrap() == Some(Vec::new()) && - net.peer(1).client().justification(&BlockId::Number(10)).unwrap() == Some(Vec::new()) + if net + .peer(0) + .client() + .justifications(&BlockId::Number(10)) + .unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) + && net + .peer(1) + .client() + .justifications(&BlockId::Number(10)) + .unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) { Poll::Ready(()) } else { @@ -696,8 +717,9 @@ fn can_sync_to_peers_with_wrong_common_block() { net.block_until_connected(); // both peers re-org to the same fork without notifying each other - net.peer(0).client().finalize_block(BlockId::Hash(fork_hash), Some(Vec::new()), true).unwrap(); - net.peer(1).client().finalize_block(BlockId::Hash(fork_hash), Some(Vec::new()), true).unwrap(); + let just = Some((*b"FRNK", Vec::new())); + net.peer(0).client().finalize_block(BlockId::Hash(fork_hash), just.clone(), true).unwrap(); + net.peer(1).client().finalize_block(BlockId::Hash(fork_hash), just, true).unwrap(); let final_hash = net.peer(0).push_blocks(1, false); net.block_until_sync(); @@ -948,8 +970,8 @@ fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { net.block_until_sync(); // there's currently no justification for block #10 - assert_eq!(net.peer(0).client().justification(&BlockId::Number(10)).unwrap(), None); - assert_eq!(net.peer(1).client().justification(&BlockId::Number(10)).unwrap(), None); + assert_eq!(net.peer(0).client().justifications(&BlockId::Number(10)).unwrap(), None); + assert_eq!(net.peer(1).client().justifications(&BlockId::Number(10)).unwrap(), None); let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap(); @@ -967,12 +989,21 @@ fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { } // Finalize the block and make the justification available. - net.peer(0).client().finalize_block(BlockId::Number(10), Some(Vec::new()), true).unwrap(); + net.peer(0).client().finalize_block( + BlockId::Number(10), + Some((*b"FRNK", Vec::new())), + true, + ).unwrap(); block_on(futures::future::poll_fn::<(), _>(|cx| { net.poll(cx); - if net.peer(1).client().justification(&BlockId::Number(10)).unwrap() != Some(Vec::new()) { + if net + .peer(1) + .client() + .justifications(&BlockId::Number(10)) + .unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) + { return Poll::Pending; } diff --git a/client/rpc/src/chain/chain_light.rs b/client/rpc/src/chain/chain_light.rs index 41d4d02e33c9d..a3f3db9b7116c 100644 --- a/client/rpc/src/chain/chain_light.rs +++ b/client/rpc/src/chain/chain_light.rs @@ -106,7 +106,7 @@ impl ChainBackend for LightChain( hash, header: Some(header), body: Some(extrinsics), - justification: signed_block.justification, + justifications: signed_block.justifications, origin: None, allow_missing_state: false, import_existing: force, diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 07e8e005fa1a8..81c98b8b1e2b9 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -41,7 +41,7 @@ use sc_telemetry::{ SUBSTRATE_INFO, }; use sp_runtime::{ - Justification, BuildStorage, + Justification, Justifications, BuildStorage, generic::{BlockId, SignedBlock, DigestItem}, traits::{ Block as BlockT, Header as HeaderT, Zero, NumberFor, @@ -625,7 +625,7 @@ impl Client where let BlockImportParams { origin, header, - justification, + justifications, post_digests, body, storage_changes, @@ -637,7 +637,7 @@ impl Client where .. } = import_block; - assert!(justification.is_some() && finalized || justification.is_none()); + assert!(justifications.is_some() && finalized || justifications.is_none()); if !intermediates.is_empty() { return Err(Error::IncompletePipeline) @@ -665,7 +665,7 @@ impl Client where origin, hash, import_headers, - justification, + justifications, body, storage_changes, new_cache, @@ -704,7 +704,7 @@ impl Client where origin: BlockOrigin, hash: Block::Hash, import_headers: PrePostHeader, - justification: Option, + justifications: Option, body: Option>, storage_changes: Option, Block>>, new_cache: HashMap>, @@ -820,7 +820,7 @@ impl Client where operation.op.set_block_data( import_headers.post().clone(), body, - justification, + justifications, leaf_state, )?; @@ -1926,9 +1926,9 @@ impl BlockBackend for Client } fn block(&self, id: &BlockId) -> sp_blockchain::Result>> { - Ok(match (self.header(id)?, self.body(id)?, self.justification(id)?) { - (Some(header), Some(extrinsics), justification) => - Some(SignedBlock { block: Block::new(header, extrinsics), justification }), + Ok(match (self.header(id)?, self.body(id)?, self.justifications(id)?) { + (Some(header), Some(extrinsics), justifications) => + Some(SignedBlock { block: Block::new(header, extrinsics), justifications }), _ => None, }) } @@ -1937,8 +1937,8 @@ impl BlockBackend for Client Client::block_status(self, id) } - fn justification(&self, id: &BlockId) -> sp_blockchain::Result> { - self.backend.blockchain().justification(*id) + fn justifications(&self, id: &BlockId) -> sp_blockchain::Result> { + self.backend.blockchain().justifications(*id) } fn block_hash(&self, number: NumberFor) -> sp_blockchain::Result> { diff --git a/client/service/test/src/client/light.rs b/client/service/test/src/client/light.rs index f5b2d4aac83d3..02d54a24c3135 100644 --- a/client/service/test/src/client/light.rs +++ b/client/service/test/src/client/light.rs @@ -28,8 +28,9 @@ use sc_light::{ }; use std::sync::Arc; use sp_runtime::{ - traits::{BlakeTwo256, HashFor, NumberFor}, - generic::BlockId, traits::{Block as _, Header as HeaderT}, Digest, + generic::BlockId, + traits::{BlakeTwo256, Block as _, HashFor, Header as HeaderT, NumberFor}, + Digest, Justifications, }; use std::collections::HashMap; use parking_lot::Mutex; @@ -377,7 +378,7 @@ fn execution_proof_is_generated_and_checked() { remote_client.import_justified( BlockOrigin::Own, remote_client.new_block(digest).unwrap().build().unwrap().block, - Default::default(), + Justifications::from((*b"TEST", Default::default())), ).unwrap(); } diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index 122782ee51ef5..d8a09734bebb6 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -36,8 +36,9 @@ use sc_client_db::{ }; use sc_block_builder::BlockBuilderProvider; use sc_service::client::{self, Client, LocalCallExecutor, new_in_mem}; -use sp_runtime::traits::{ - BlakeTwo256, Block as BlockT, Header as HeaderT, +use sp_runtime::{ + ConsensusEngineId, + traits::{BlakeTwo256, Block as BlockT, Header as HeaderT}, }; use substrate_test_runtime::TestAPI; use sp_state_machine::backend::Backend as _; @@ -51,12 +52,14 @@ use sp_consensus::{ }; use sp_storage::StorageKey; use sp_trie::{TrieConfiguration, trie_types::Layout}; -use sp_runtime::{generic::BlockId, DigestItem}; +use sp_runtime::{generic::BlockId, DigestItem, Justifications}; use hex_literal::hex; mod light; mod db; +const TEST_ENGINE_ID: ConsensusEngineId = *b"TEST"; + native_executor_instance!( Executor, substrate_test_runtime_client::runtime::api::dispatch, @@ -1016,7 +1019,7 @@ fn import_with_justification() { client.import(BlockOrigin::Own, a2.clone()).unwrap(); // A2 -> A3 - let justification = vec![1, 2, 3]; + let justification = Justifications::from((TEST_ENGINE_ID, vec![1, 2, 3])); let a3 = client.new_block_at( &BlockId::Hash(a2.hash()), Default::default(), @@ -1030,17 +1033,17 @@ fn import_with_justification() { ); assert_eq!( - client.justification(&BlockId::Hash(a3.hash())).unwrap(), + client.justifications(&BlockId::Hash(a3.hash())).unwrap(), Some(justification), ); assert_eq!( - client.justification(&BlockId::Hash(a1.hash())).unwrap(), + client.justifications(&BlockId::Hash(a1.hash())).unwrap(), None, ); assert_eq!( - client.justification(&BlockId::Hash(a2.hash())).unwrap(), + client.justifications(&BlockId::Hash(a2.hash())).unwrap(), None, ); } @@ -1088,7 +1091,7 @@ fn importing_diverged_finalized_block_should_trigger_reorg() { ); // importing B1 as finalized should trigger a re-org and set it as new best - let justification = vec![1, 2, 3]; + let justification = Justifications::from((TEST_ENGINE_ID, vec![1, 2, 3])); client.import_justified(BlockOrigin::Own, b1.clone(), justification).unwrap(); assert_eq!( diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index b5efcfb02198a..6ee836acb6441 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -21,7 +21,7 @@ use std::sync::Arc; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use sp_runtime::generic::BlockId; -use sp_runtime::Justification; +use sp_runtime::Justifications; use log::warn; use parking_lot::RwLock; @@ -84,8 +84,8 @@ pub trait HeaderBackend: Send + Sync { pub trait Backend: HeaderBackend + HeaderMetadata { /// Get block body. Returns `None` if block is not found. fn body(&self, id: BlockId) -> Result::Extrinsic>>>; - /// Get block justification. Returns `None` if justification does not exist. - fn justification(&self, id: BlockId) -> Result>; + /// Get block justifications. Returns `None` if no justification exists. + fn justifications(&self, id: BlockId) -> Result>; /// Get last finalized block hash. fn last_finalized(&self) -> Result; /// Returns data cache reference, if it is enabled on this backend. diff --git a/primitives/consensus/common/src/block_import.rs b/primitives/consensus/common/src/block_import.rs index 00f84501dbb32..9b7995a2b00bd 100644 --- a/primitives/consensus/common/src/block_import.rs +++ b/primitives/consensus/common/src/block_import.rs @@ -18,7 +18,7 @@ //! Block import helpers. use sp_runtime::traits::{Block as BlockT, DigestItemFor, Header as HeaderT, NumberFor, HashFor}; -use sp_runtime::Justification; +use sp_runtime::{Justification, Justifications}; use serde::{Serialize, Deserialize}; use std::borrow::Cow; use std::collections::HashMap; @@ -128,8 +128,8 @@ pub struct BlockImportParams { /// re-executed in a runtime that checks digest equivalence -- the /// post-runtime digests are pushed back on after. pub header: Block::Header, - /// Justification provided for this block from the outside. - pub justification: Option, + /// Justification(s) provided for this block from the outside. + pub justifications: Option, /// Digest items that have been added after the runtime for external /// work, like a consensus signature. pub post_digests: Vec>, @@ -174,7 +174,7 @@ impl BlockImportParams { ) -> Self { Self { origin, header, - justification: None, + justifications: None, post_digests: Vec::new(), body: None, storage_changes: None, @@ -219,7 +219,7 @@ impl BlockImportParams { BlockImportParams { origin: self.origin, header: self.header, - justification: self.justification, + justifications: self.justifications, post_digests: self.post_digests, body: self.body, storage_changes: None, diff --git a/primitives/consensus/common/src/import_queue.rs b/primitives/consensus/common/src/import_queue.rs index 83f6271941fab..b6067645a8920 100644 --- a/primitives/consensus/common/src/import_queue.rs +++ b/primitives/consensus/common/src/import_queue.rs @@ -28,7 +28,7 @@ use std::collections::HashMap; -use sp_runtime::{Justification, traits::{Block as BlockT, Header as _, NumberFor}}; +use sp_runtime::{Justifications, traits::{Block as BlockT, Header as _, NumberFor}}; use crate::{ error::Error as ConsensusError, @@ -68,8 +68,8 @@ pub struct IncomingBlock { pub header: Option<::Header>, /// Block body if requested. pub body: Option::Extrinsic>>, - /// Justification if requested. - pub justification: Option, + /// Justification(s) if requested. + pub justifications: Option, /// The peer, we received this from pub origin: Option, /// Allow importing the block skipping state verification if parent state is missing. @@ -90,7 +90,7 @@ pub trait Verifier: Send + Sync { &mut self, origin: BlockOrigin, header: B::Header, - justification: Option, + justifications: Option, body: Option>, ) -> Result<(BlockImportParams, Option)>>), String>; } @@ -102,13 +102,13 @@ pub trait Verifier: Send + Sync { pub trait ImportQueue: Send { /// Import bunch of blocks. fn import_blocks(&mut self, origin: BlockOrigin, blocks: Vec>); - /// Import a block justification. - fn import_justification( + /// Import block justifications. + fn import_justifications( &mut self, who: Origin, hash: B::Hash, number: NumberFor, - justification: Justification + justifications: Justifications ); /// Polls for actions to perform on the network. /// @@ -182,8 +182,8 @@ pub(crate) fn import_single_block_metered, Transaction ) -> Result>, BlockImportError> { let peer = block.origin; - let (header, justification) = match (block.header, block.justification) { - (Some(header), justification) => (header, justification), + let (header, justifications) = match (block.header, block.justifications) { + (Some(header), justifications) => (header, justifications), (None, _) => { if let Some(ref peer) = peer { debug!(target: "sync", "Header {} was not provided by {} ", block.hash, peer); @@ -238,7 +238,7 @@ pub(crate) fn import_single_block_metered, Transaction } let started = wasm_timer::Instant::now(); - let (mut import_block, maybe_keys) = verifier.verify(block_origin, header, justification, block.body) + let (mut import_block, maybe_keys) = verifier.verify(block_origin, header, justifications, block.body) .map_err(|msg| { if let Some(ref peer) = peer { trace!(target: "sync", "Verifying {}({}) from {} failed: {}", number, hash, peer, msg); diff --git a/primitives/consensus/common/src/import_queue/basic_queue.rs b/primitives/consensus/common/src/import_queue/basic_queue.rs index f1b42e1460e59..eb2b4b1fa7fcd 100644 --- a/primitives/consensus/common/src/import_queue/basic_queue.rs +++ b/primitives/consensus/common/src/import_queue/basic_queue.rs @@ -18,7 +18,7 @@ use std::{pin::Pin, time::Duration, marker::PhantomData}; use futures::{prelude::*, task::Context, task::Poll}; use futures_timer::Delay; -use sp_runtime::{Justification, traits::{Block as BlockT, Header as HeaderT, NumberFor}}; +use sp_runtime::{Justification, Justifications, traits::{Block as BlockT, Header as HeaderT, NumberFor}}; use sp_utils::mpsc::{TracingUnboundedSender, tracing_unbounded, TracingUnboundedReceiver}; use prometheus_endpoint::Registry; @@ -112,22 +112,24 @@ impl ImportQueue for BasicQueue } } - fn import_justification( + fn import_justifications( &mut self, who: Origin, hash: B::Hash, number: NumberFor, - justification: Justification, + justifications: Justifications, ) { - let res = self.justification_sender.unbounded_send( - worker_messages::ImportJustification(who, hash, number, justification), - ); - - if res.is_err() { - log::error!( - target: "sync", - "import_justification: Background import task is no longer alive" + for justification in justifications { + let res = self.justification_sender.unbounded_send( + worker_messages::ImportJustification(who, hash, number, justification), ); + + if res.is_err() { + log::error!( + target: "sync", + "import_justification: Background import task is no longer alive" + ); + } } } @@ -281,7 +283,7 @@ impl BlockImportWorker { who: Origin, hash: B::Hash, number: NumberFor, - justification: Justification + justification: Justification, ) { let started = wasm_timer::Instant::now(); let success = self.justification_import.as_mut().map(|justification_import| { @@ -442,7 +444,7 @@ mod tests { &mut self, origin: BlockOrigin, header: Header, - _justification: Option, + _justifications: Option, _body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { Ok((BlockImportParams::new(origin, header), None)) @@ -541,7 +543,7 @@ mod tests { hash, header: Some(header), body: None, - justification: None, + justifications: None, origin: None, allow_missing_state: false, import_existing: false, @@ -554,12 +556,11 @@ mod tests { let mut import_justification = || { let hash = Hash::random(); - block_on(finality_sender.send(worker_messages::ImportJustification( libp2p::PeerId::random(), hash, 1, - Vec::new(), + (*b"TEST", Vec::new()), ))) .unwrap(); diff --git a/primitives/runtime/src/generic/block.rs b/primitives/runtime/src/generic/block.rs index 7b2a10297f9c6..1b30d43ccaca7 100644 --- a/primitives/runtime/src/generic/block.rs +++ b/primitives/runtime/src/generic/block.rs @@ -30,7 +30,7 @@ use crate::traits::{ self, Member, Block as BlockT, Header as HeaderT, MaybeSerialize, MaybeMallocSizeOf, NumberFor, }; -use crate::Justification; +use crate::Justifications; /// Something to identify a block. #[derive(PartialEq, Eq, Clone, RuntimeDebug)] @@ -112,5 +112,5 @@ pub struct SignedBlock { /// Full block. pub block: Block, /// Block justification. - pub justification: Option, + pub justifications: Option, } diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index c8b93a083be4e..4508f84eefc38 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -96,7 +96,65 @@ pub use either::Either; /// the block itself would allow swapping justifications to change the block's hash /// (and thus fork the chain). Sending a `Justification` alongside a block instead /// bypasses this problem. -pub type Justification = Vec; +/// +/// Each justification is provided as an encoded blob, and is tagged with an ID +/// to identify the consensus engine that generated the proof (we might have +/// multiple justifications from different engines for the same block). +pub type Justification = (ConsensusEngineId, EncodedJustification); + +/// The encoded justification specific to a consensus engine. +pub type EncodedJustification = Vec; + +/// Collection of justifications for a given block, multiple justifications may +/// be provided by different consensus engines for the same block. +#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] +#[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)] +pub struct Justifications(Vec); + +impl Justifications { + /// Return an iterator over the justifications. + pub fn iter(&self) -> impl Iterator { + self.0.iter() + } + + /// Append a justification. Returns false if a justification with the same + /// `ConsensusEngineId` already exists, in which case the justification is + /// not inserted. + pub fn append(&mut self, justification: Justification) -> bool { + if self.get(justification.0).is_some() { + return false; + } + self.0.push(justification); + true + } + + /// Return the encoded justification for the given consensus engine, if it + /// exists. + pub fn get(&self, engine_id: ConsensusEngineId) -> Option<&EncodedJustification> { + self.iter().find(|j| j.0 == engine_id).map(|j| &j.1) + } + + /// Return a copy of the encoded justification for the given consensus + /// engine, if it exists. + pub fn into_justification(self, engine_id: ConsensusEngineId) -> Option { + self.into_iter().find(|j| j.0 == engine_id).map(|j| j.1) + } +} + +impl IntoIterator for Justifications { + type Item = Justification; + type IntoIter = sp_std::vec::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} + +impl From for Justifications { + fn from(justification: Justification) -> Self { + Self(vec![justification]) + } +} use traits::{Verify, Lazy}; diff --git a/test-utils/client/src/client_ext.rs b/test-utils/client/src/client_ext.rs index db3e42f7e01c2..aa4856f6baf66 100644 --- a/test-utils/client/src/client_ext.rs +++ b/test-utils/client/src/client_ext.rs @@ -24,7 +24,7 @@ use sp_consensus::{ BlockImportParams, BlockImport, BlockOrigin, Error as ConsensusError, ForkChoiceStrategy, }; -use sp_runtime::Justification; +use sp_runtime::{Justification, Justifications}; use sp_runtime::traits::{Block as BlockT}; use sp_runtime::generic::BlockId; use codec::alloc::collections::hash_map::HashMap; @@ -51,15 +51,14 @@ pub trait ClientBlockImportExt: Sized { fn import_as_best(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; /// Import a block and finalize it. - fn import_as_final(&mut self, origin: BlockOrigin, block: Block) - -> Result<(), ConsensusError>; + fn import_as_final(&mut self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; - /// Import block with justification, finalizes block. + /// Import block with justification(s), finalizes block. fn import_justified( &mut self, origin: BlockOrigin, block: Block, - justification: Justification + justifications: Justifications, ) -> Result<(), ConsensusError>; } @@ -119,11 +118,11 @@ impl ClientBlockImportExt for std::sync::A &mut self, origin: BlockOrigin, block: Block, - justification: Justification, + justifications: Justifications, ) -> Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); - import.justification = Some(justification); + import.justifications = Some(justifications); import.body = Some(extrinsics); import.finalized = true; import.fork_choice = Some(ForkChoiceStrategy::LongestChain); @@ -168,11 +167,11 @@ impl ClientBlockImportExt for Client Result<(), ConsensusError> { let (header, extrinsics) = block.deconstruct(); let mut import = BlockImportParams::new(origin, header); - import.justification = Some(justification); + import.justifications = Some(justifications); import.body = Some(extrinsics); import.finalized = true; import.fork_choice = Some(ForkChoiceStrategy::LongestChain);