Storing multiple Justifications per block (paritytech#7640)

* 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 <pierre.krieger1708@gmail.com>

* Apply suggestions from code review

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

* network: tweaks to review suggestions

* network: revert change to BlockData for backwards compatibility

* Apply suggestion from code review

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

* 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 <pierre.krieger1708@gmail.com>
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: André Silva <andrerfosilva@gmail.com>

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<Block: BlockT> {
 		&mut self,
 		header: Block::Header,
 		body: Option<Vec<Block::Extrinsic>>,
-		justification: Option<Justification>,
+		justifications: Option<Justifications>,
 		state: NewBlockState,
 	) -> sp_blockchain::Result<()>;
 
@@ -197,6 +197,7 @@ pub trait BlockImportOperation<Block: BlockT> {
 		id: BlockId<Block>,
 		justification: Option<Justification>,
 	) -> 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<Block>) -> sp_blockchain::Result<()>;
@@ -230,7 +231,6 @@ pub trait Finalizer<Block: BlockT, B: Backend<Block>> {
 		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<Block: BlockT, B: Backend<Block>> {
 		justification: Option<Justification>,
 		notify: bool,
 	) -> sp_blockchain::Result<()>;
-
 }
 
 /// Provides access to an auxiliary database.
@@ -432,6 +431,15 @@ pub trait Backend<Block: BlockT>: AuxStore + Send + Sync {
 		justification: Option<Justification>,
 	) -> 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<Block>,
+		justification: Justification,
+	) -> sp_blockchain::Result<()>;
+
 	/// Returns reference to blockchain backend.
 	fn blockchain(&self) -> &Self::Blockchain;
 

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<Block: BlockT> {
 	/// Get block status.
 	fn block_status(&self, id: &BlockId<Block>) -> sp_blockchain::Result<sp_consensus::BlockStatus>;
 
-	/// Get block justification set by id.
-	fn justification(&self, id: &BlockId<Block>) -> sp_blockchain::Result<Option<Justification>>;
+	/// Get block justifications for the block with the given id.
+	fn justifications(&self, id: &BlockId<Block>) -> sp_blockchain::Result<Option<Justifications>>;
 
 	/// Get block hash by number.
 	fn block_hash(&self, number: NumberFor<Block>) -> sp_blockchain::Result<Option<Block::Hash>>;

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<B: BlockT> {
 
 #[derive(PartialEq, Eq, Clone)]
 enum StoredBlock<B: BlockT> {
-	Header(B::Header, Option<Justification>),
-	Full(B, Option<Justification>),
+	Header(B::Header, Option<Justifications>),
+	Full(B, Option<Justifications>),
 }
 
 impl<B: BlockT> StoredBlock<B> {
-	fn new(header: B::Header, body: Option<Vec<B::Extrinsic>>, just: Option<Justification>) -> Self {
+	fn new(header: B::Header, body: Option<Vec<B::Extrinsic>>, just: Option<Justifications>) -> Self {
 		match body {
 			Some(body) => StoredBlock::Full(B::new(header, body), just),
 			None => StoredBlock::Header(header, just),
@@ -70,7 +70,7 @@ impl<B: BlockT> StoredBlock<B> {
 		}
 	}
 
-	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<B: BlockT> StoredBlock<B> {
 		}
 	}
 
-	fn into_inner(self) -> (B::Header, Option<Vec<B::Extrinsic>>, Option<Justification>) {
+	fn into_inner(self) -> (B::Header, Option<Vec<B::Extrinsic>>, Option<Justifications>) {
 		match self {
 			StoredBlock::Header(header, just) => (header, None, just),
 			StoredBlock::Full(block, just) => {
@@ -164,7 +164,7 @@ impl<Block: BlockT> Blockchain<Block> {
 		&self,
 		hash: Block::Hash,
 		header: <Block as BlockT>::Header,
-		justification: Option<Justification>,
+		justifications: Option<Justifications>,
 		body: Option<Vec<<Block as BlockT>::Extrinsic>>,
 		new_state: NewBlockState,
 	) -> sp_blockchain::Result<()> {
@@ -176,7 +176,7 @@ impl<Block: BlockT> Blockchain<Block> {
 		{
 			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<Block: BlockT> Blockchain<Block> {
 			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<Block>, 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<u8>, Option<Vec<u8>>)>) {
 		let mut storage = self.storage.write();
 		for (k, v) in ops {
@@ -365,9 +393,9 @@ impl<Block: BlockT> blockchain::Backend<Block> for Blockchain<Block> {
 		}))
 	}
 
-	fn justification(&self, id: BlockId<Block>) -> sp_blockchain::Result<Option<Justification>> {
+	fn justifications(&self, id: BlockId<Block>) -> sp_blockchain::Result<Option<Justifications>> {
 		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<Block: BlockT> backend::BlockImportOperation<Block> for BlockImportOperatio
 		&mut self,
 		header: <Block as BlockT>::Header,
 		body: Option<Vec<<Block as BlockT>::Extrinsic>>,
-		justification: Option<Justification>,
+		justifications: Option<Justifications>,
 		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<Block: BlockT> backend::Backend<Block> for Backend<Block> where Block::Hash
 		self.blockchain.finalize_header(block, justification)
 	}
 
+	fn append_justification(
+		&self,
+		block: BlockId<Block>,
+		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<Block> {
+		let blockchain = Blockchain::<Block>::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(_)),
+		));
+	}
+}

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<B: BlockT, C, P, CAW> Verifier<B> for AuraVerifier<C, P, CAW> where
 		&mut self,
 		origin: BlockOrigin,
 		header: B::Header,
-		justification: Option<Justification>,
+		justifications: Option<Justifications>,
 		mut body: Option<Vec<B::Extrinsic>>,
 	) -> Result<(BlockImportParams<B, ()>, Option<Vec<(CacheKeyId, Vec<u8>)>>), String> {
 		let mut inherent_data = self.inherent_data_providers
@@ -317,7 +317,7 @@ impl<B: BlockT, C, P, CAW> Verifier<B> for AuraVerifier<C, P, CAW> 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);
 

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<Justification>,
+		justifications: Option<Justifications>,
 		mut body: Option<Vec<Block::Extrinsic>>,
 	) -> Result<(BlockImportParams<Block, ()>, Option<Vec<(CacheKeyId, Vec<u8>)>>), 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::<Block> { epoch_descriptor }) as Box<dyn Any>,

client/consensus/babe/src/tests.rs

@@ -235,12 +235,12 @@ impl Verifier<TestBlock> for TestVerifier {
 		&mut self,
 		origin: BlockOrigin,
 		mut header: TestHeader,
-		justification: Option<Justification>,
+		justifications: Option<Justifications>,
 		body: Option<Vec<TestExtrinsic>>,
 	) -> Result<(BlockImportParams<TestBlock, ()>, Option<Vec<(CacheKeyId, Vec<u8>)>>), 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)
 	}
 }