Storing multiple Justifications per block (#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>

substrate/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));

substrate/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<Block: BlockT> {
 	header: Block::Header,
-	justification: Option<Justification>,
+	justifications: Option<Justifications>,
 	body: Option<Vec<Block::Extrinsic>>,
 	leaf_state: NewBlockState,
 }
@@ -535,8 +536,8 @@ impl<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
 		}
 	}
 
-	fn justification(&self, id: BlockId<Block>) -> ClientResult<Option<Justification>> {
-		match read_db(&*self.db, columns::KEY_LOOKUP, columns::JUSTIFICATION, id)? {
+	fn justifications(&self, id: BlockId<Block>) -> ClientResult<Option<Justifications>> {
+		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<Block: BlockT> sc_client_api::backend::BlockImportOperation<Block> for Bloc
 		&mut self,
 		header: Block::Header,
 		body: Option<Vec<Block::Extrinsic>>,
-		justification: Option<Justification>,
+		justifications: Option<Justifications>,
 		leaf_state: NewBlockState,
 	) -> ClientResult<()> {
 		assert!(self.pending_block.is_none(), "Only one block per operation is allowed");
@@ -726,7 +727,7 @@ impl<Block: BlockT> sc_client_api::backend::BlockImportOperation<Block> for Bloc
 		self.pending_block = Some(PendingBlock {
 			header,
 			body,
-			justification,
+			justifications,
 			leaf_state,
 		});
 		Ok(())
@@ -1130,9 +1131,9 @@ impl<Block: BlockT> Backend<Block> {
 
 		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<Block: BlockT> Backend<Block> {
 					},
 				}
 			}
-			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<Block: BlockT> Backend<Block> {
 
 		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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 		Ok(())
 	}
 
+	fn append_justification(
+		&self,
+		block: BlockId<Block>,
+		justification: Justification,
+	) -> ClientResult<()> {
+		let mut transaction: Transaction<DbHash> = 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<Block>> {
 		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<ExtrinsicWrapper<u64>>;
 
 	pub fn prepare_changes(changes: Vec<(Vec<u8>, Vec<u8>)>) -> (H256, MemoryDB<BlakeTwo256>) {
@@ -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::<Block>::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),
 		);
 	}
 

substrate/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<Block: BlockT>(db_path: &Path, db_type: DatabaseType) -> sp_blockchain::Result<()> {
@@ -42,7 +44,11 @@ pub fn upgrade_db<Block: BlockT>(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::<Block>(db_path, db_type)?,
+			1 => {
+				migrate_1_to_2::<Block>(db_path, db_type)?;
+				migrate_2_to_3::<Block>(db_path, db_type)?
+			},
+			2 => migrate_2_to_3::<Block>(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<Block: BlockT>(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<Block: BlockT>(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<u32> {
@@ -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);