diff --git a/Cargo.lock b/Cargo.lock index f9902c2802fa6..d2dadcaf56926 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2913,9 +2913,9 @@ dependencies = [ [[package]] name = "jsonrpsee-http-client" -version = "0.2.0-alpha.5" +version = "0.2.0-alpha.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7e3a49473ea266be8e9f23e20a7bfa4349109b42319d72cc0b8a101e18fa6466" +checksum = "2737440f37efa10e5ef7beeec43d059d29dc92640978be21fcdcef481a2edb0d" dependencies = [ "async-trait", "fnv", @@ -2932,9 +2932,9 @@ dependencies = [ [[package]] name = "jsonrpsee-proc-macros" -version = "0.2.0-alpha.5" +version = "0.2.0-alpha.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b0cbaee9ca6440e191545a68c7bf28db0ff918359a904e37a6e7cf7edd132f5a" +checksum = "5784ee8bb31988fa2c7a755fe31b0e21aa51894a67e5c99b6d4470f0253bf31a" dependencies = [ "Inflector", "proc-macro-crate 1.0.0", @@ -2945,9 +2945,9 @@ dependencies = [ [[package]] name = "jsonrpsee-types" -version = "0.2.0-alpha.5" +version = "0.2.0-alpha.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e4ce2de6884fb4abee16eca02329a1eec1eb8df8aed751a8e929083820c78ce7" +checksum = "bab3dabceeeeb865897661d532d47202eaae71cd2c606f53cb69f1fbc0555a51" dependencies = [ "async-trait", "beef", @@ -2961,9 +2961,9 @@ dependencies = [ [[package]] name = "jsonrpsee-utils" -version = "0.2.0-alpha.5" +version = "0.2.0-alpha.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3b22199cccd81d9ef601be86bedc5bef67aeacbbfddace031d4931c60fca96e9" +checksum = "d63cf4d423614e71fd144a8691208539d2b23d8373e069e2fbe023c5eba5e922" dependencies = [ "futures-util", "hyper 0.13.10", diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 056460ac9ed80..194911e1f104a 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -18,6 +18,10 @@ //! Utilities for dealing with authorities, authority sets, and handoffs. +use std::cmp::Ord; +use std::fmt::Debug; +use std::ops::Add; + use fork_tree::ForkTree; use parking_lot::MappedMutexGuard; use finality_grandpa::voter_set::VoterSet; @@ -27,9 +31,7 @@ use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList}; use sc_consensus::shared_data::{SharedData, SharedDataLocked}; -use std::cmp::Ord; -use std::fmt::Debug; -use std::ops::Add; +use crate::SetId; /// Error type returned on operations on the `AuthoritySet`. #[derive(Debug, derive_more::Display)] @@ -684,6 +686,20 @@ impl + Clone> PendingChange { #[derive(Debug, Encode, Decode, Clone, PartialEq)] pub struct AuthoritySetChanges(Vec<(u64, N)>); +/// The response when querying for a set id for a specific block. Either we get a set id +/// together with a block number for the last block in the set, or that the requested block is in the +/// latest set, or that we don't know what set id the given block belongs to. +#[derive(Debug, PartialEq)] +pub enum AuthoritySetChangeId { + /// The requested block is in the latest set. + Latest, + /// Tuple containing the set id and the last block number of that set. + Set(SetId, N), + /// We don't know which set id the request block belongs to (this can only happen due to missing + /// data). + Unknown, +} + impl From> for AuthoritySetChanges { fn from(changes: Vec<(u64, N)>) -> AuthoritySetChanges { AuthoritySetChanges(changes) @@ -699,7 +715,15 @@ impl AuthoritySetChanges { self.0.push((set_id, block_number)); } - pub(crate) fn get_set_id(&self, block_number: N) -> Option<(u64, N)> { + pub(crate) fn get_set_id(&self, block_number: N) -> AuthoritySetChangeId { + if self.0 + .last() + .map(|last_auth_change| last_auth_change.1 < block_number) + .unwrap_or(false) + { + return AuthoritySetChangeId::Latest; + } + let idx = self.0 .binary_search_by_key(&block_number, |(_, n)| n.clone()) .unwrap_or_else(|b| b); @@ -711,16 +735,16 @@ impl AuthoritySetChanges { let (prev_set_id, _) = self.0[idx - 1usize]; if set_id != prev_set_id + 1u64 { // Without the preceding set_id we don't have a well-defined start. - return None; + return AuthoritySetChangeId::Unknown; } } else if set_id != 0 { // If this is the first index, yet not the first set id then it's not well-defined // that we are in the right set id. - return None; + return AuthoritySetChangeId::Unknown; } - Some((set_id, block_number)) + AuthoritySetChangeId::Set(set_id, block_number) } else { - None + AuthoritySetChangeId::Unknown } } @@ -1660,11 +1684,11 @@ mod tests { authority_set_changes.append(1, 81); authority_set_changes.append(2, 121); - assert_eq!(authority_set_changes.get_set_id(20), Some((0, 41))); - assert_eq!(authority_set_changes.get_set_id(40), Some((0, 41))); - assert_eq!(authority_set_changes.get_set_id(41), Some((0, 41))); - assert_eq!(authority_set_changes.get_set_id(42), Some((1, 81))); - assert_eq!(authority_set_changes.get_set_id(141), None); + assert_eq!(authority_set_changes.get_set_id(20), AuthoritySetChangeId::Set(0, 41)); + assert_eq!(authority_set_changes.get_set_id(40), AuthoritySetChangeId::Set(0, 41)); + assert_eq!(authority_set_changes.get_set_id(41), AuthoritySetChangeId::Set(0, 41)); + assert_eq!(authority_set_changes.get_set_id(42), AuthoritySetChangeId::Set(1, 81)); + assert_eq!(authority_set_changes.get_set_id(141), AuthoritySetChangeId::Latest); } #[test] @@ -1674,11 +1698,11 @@ mod tests { authority_set_changes.append(3, 81); authority_set_changes.append(4, 121); - assert_eq!(authority_set_changes.get_set_id(20), None); - assert_eq!(authority_set_changes.get_set_id(40), None); - assert_eq!(authority_set_changes.get_set_id(41), None); - assert_eq!(authority_set_changes.get_set_id(42), Some((3, 81))); - assert_eq!(authority_set_changes.get_set_id(141), None); + assert_eq!(authority_set_changes.get_set_id(20), AuthoritySetChangeId::Unknown); + assert_eq!(authority_set_changes.get_set_id(40), AuthoritySetChangeId::Unknown); + assert_eq!(authority_set_changes.get_set_id(41), AuthoritySetChangeId::Unknown); + assert_eq!(authority_set_changes.get_set_id(42), AuthoritySetChangeId::Set(3, 81)); + assert_eq!(authority_set_changes.get_set_id(141), AuthoritySetChangeId::Latest); } #[test] diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 80ba8cee9101e..6735d91ba8b75 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -36,23 +36,23 @@ //! finality proof (that finalizes some block C that is ancestor of the B and descendant //! of the U) could be returned. -use log::trace; +use log::{trace, warn}; use std::sync::Arc; -use finality_grandpa::BlockNumberOps; use parity_scale_codec::{Encode, Decode}; -use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; +use sp_blockchain::{Backend as BlockchainBackend, HeaderBackend}; +use sp_finality_grandpa::GRANDPA_ENGINE_ID; use sp_runtime::{ - EncodedJustification, generic::BlockId, + generic::BlockId, traits::{NumberFor, Block as BlockT, Header as HeaderT, One}, }; use sc_client_api::backend::Backend; -use sp_finality_grandpa::{AuthorityId, GRANDPA_ENGINE_ID}; -use crate::authorities::AuthoritySetChanges; -use crate::justification::GrandpaJustification; -use crate::SharedAuthoritySet; -use crate::VoterSet; +use crate::{ + SharedAuthoritySet, best_justification, + authorities::{AuthoritySetChangeId, AuthoritySetChanges}, + justification::GrandpaJustification, +}; const MAX_UNKNOWN_HEADERS: usize = 100_000; @@ -97,14 +97,13 @@ where impl FinalityProofProvider where Block: BlockT, - NumberFor: BlockNumberOps, B: Backend + Send + Sync + 'static, { /// Prove finality for the given block number by returning a Justification for the last block of /// the authority set. pub fn prove_finality( &self, - block: NumberFor + block: NumberFor, ) -> Result>, FinalityProofError> { let authority_set_changes = if let Some(changes) = self .shared_authority_set @@ -116,8 +115,8 @@ where return Ok(None); }; - prove_finality::<_, _, GrandpaJustification>( - &*self.backend.blockchain(), + prove_finality( + &*self.backend, authority_set_changes, block, ) @@ -151,19 +150,19 @@ pub enum FinalityProofError { Client(sp_blockchain::Error), } -fn prove_finality( - blockchain: &B, +fn prove_finality( + backend: &B, authority_set_changes: AuthoritySetChanges>, block: NumberFor, ) -> Result>, FinalityProofError> where Block: BlockT, - B: BlockchainBackend, - J: ProvableJustification, + B: Backend, { - // Early-return if we sure that there are no blocks finalized AFTER begin block - let info = blockchain.info(); - if info.finalized_number <= block { + // Early-return if we are sure that there are no blocks finalized that cover the requested + // block. + let info = backend.blockchain().info(); + if info.finalized_number < block { let err = format!( "Requested finality proof for descendant of #{} while we only have finalized #{}.", block, @@ -173,45 +172,60 @@ where return Err(FinalityProofError::BlockNotYetFinalized); } - // Get set_id the block belongs to, and the last block of the set which should contain a - // Justification we can use to prove the requested block. - let (_, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) { - id - } else { - trace!( - target: "afg", - "AuthoritySetChanges does not cover the requested block #{}. \ - Maybe the subscription API is more appropriate.", - block, - ); - return Err(FinalityProofError::BlockNotInAuthoritySetChanges); - }; - - // 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(grandpa_justification) = blockchain.justifications(last_block_for_set_id)? - .and_then(|justifications| justifications.into_justification(GRANDPA_ENGINE_ID)) - { - grandpa_justification - } else { - trace!( + let (justification, just_block) = match authority_set_changes.get_set_id(block) { + AuthoritySetChangeId::Latest => { + if let Some(justification) = best_justification(backend)? + .map(|j: GrandpaJustification| (j.encode(), j.target().0)) + { + justification + } else { + trace!( + target: "afg", + "No justification found for the latest finalized block. \ + Returning empty proof.", + ); + return Ok(None); + } + } + AuthoritySetChangeId::Set(_, last_block_for_set) => { + let last_block_for_set_id = BlockId::Number(last_block_for_set); + let justification = if let Some(grandpa_justification) = backend + .blockchain() + .justifications(last_block_for_set_id)? + .and_then(|justifications| justifications.into_justification(GRANDPA_ENGINE_ID)) + { + grandpa_justification + } else { + trace!( + target: "afg", + "No justification found when making finality proof for {}. \ + Returning empty proof.", + block, + ); + return Ok(None); + }; + (justification, last_block_for_set) + } + AuthoritySetChangeId::Unknown => { + warn!( target: "afg", - "No justification found when making finality proof for {}. Returning empty proof.", + "AuthoritySetChanges does not cover the requested block #{} due to missing data. \ + You need to resync to populate AuthoritySetChanges properly.", block, ); - return Ok(None); - }; + return Err(FinalityProofError::BlockNotInAuthoritySetChanges); + } + }; // Collect all headers from the requested block until the last block of the set let unknown_headers = { let mut headers = Vec::new(); let mut current = block + One::one(); loop { - if current >= last_block_for_set || headers.len() >= MAX_UNKNOWN_HEADERS { + if current > just_block || headers.len() >= MAX_UNKNOWN_HEADERS { break; } - headers.push(blockchain.expect_header(BlockId::Number(current))?); + headers.push(backend.blockchain().expect_header(BlockId::Number(current))?); current += One::one(); } headers @@ -219,7 +233,7 @@ where Ok(Some( FinalityProof { - block: blockchain.expect_block_hash_from_id(&last_block_for_set_id)?, + block: backend.blockchain().expect_block_hash_from_id(&BlockId::Number(just_block))?, justification, unknown_headers, } @@ -227,96 +241,48 @@ where )) } -/// Check GRANDPA proof-of-finality for the given block. -/// -/// Returns the vector of headers that MUST be validated + imported -/// AND if at least one of those headers is invalid, all other MUST be considered invalid. -/// -/// This is currently not used, and exists primarily as an example of how to check finality proofs. -#[cfg(test)] -fn check_finality_proof( - current_set_id: u64, - current_authorities: sp_finality_grandpa::AuthorityList, - remote_proof: Vec, -) -> ClientResult> -where - J: ProvableJustification
, -{ - let proof = FinalityProof::
::decode(&mut &remote_proof[..]) - .map_err(|_| ClientError::BadJustification("failed to decode finality proof".into()))?; - - let justification: J = Decode::decode(&mut &proof.justification[..]) - .map_err(|_| ClientError::JustificationDecode)?; - justification.verify(current_set_id, ¤t_authorities)?; - - Ok(proof) -} - -/// Justification used to prove block finality. -pub trait ProvableJustification: Encode + Decode { - /// Verify justification with respect to authorities set and authorities set id. - fn verify(&self, set_id: u64, authorities: &[(AuthorityId, u64)]) -> ClientResult<()>; - - /// Decode and verify justification. - fn decode_and_verify( - justification: &EncodedJustification, - set_id: u64, - authorities: &[(AuthorityId, u64)], - ) -> ClientResult { - let justification = - Self::decode(&mut &**justification).map_err(|_| ClientError::JustificationDecode)?; - justification.verify(set_id, authorities)?; - Ok(justification) - } -} - -impl ProvableJustification for GrandpaJustification -where - NumberFor: BlockNumberOps, -{ - fn verify(&self, set_id: u64, authorities: &[(AuthorityId, u64)]) -> ClientResult<()> { - let authorities = VoterSet::new(authorities.iter().cloned()).ok_or( - ClientError::Consensus(sp_consensus::Error::InvalidAuthoritiesSet), - )?; - - GrandpaJustification::verify_with_voter_set(self, set_id, &authorities) - } -} - #[cfg(test)] pub(crate) mod tests { use super::*; - use crate::authorities::AuthoritySetChanges; + use crate::{ + authorities::AuthoritySetChanges, BlockNumberOps, ClientError, SetId, + }; + use futures::executor::block_on; + use sc_block_builder::BlockBuilderProvider; + use sc_client_api::{apply_aux, LockImportRun}; + use sp_consensus::BlockOrigin; 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; - use substrate_test_runtime_client::runtime::{Block, Header, H256}; - - pub(crate) type FinalityProof = super::FinalityProof
; - - #[derive(Debug, PartialEq, Encode, Decode)] - pub struct TestJustification(pub (u64, AuthorityList), pub Vec); - - impl ProvableJustification
for TestJustification { - fn verify(&self, set_id: u64, authorities: &[(AuthorityId, u64)]) -> ClientResult<()> { - if (self.0).0 != set_id || (self.0).1 != authorities { - return Err(ClientError::BadJustification("test".into())); - } + use sp_finality_grandpa::{AuthorityId, GRANDPA_ENGINE_ID as ID}; + use sp_keyring::Ed25519Keyring; + use substrate_test_runtime_client::{ + runtime::{Block, Header, H256}, + Backend as TestBackend, ClientBlockImportExt, ClientExt, DefaultTestClientBuilderExt, + TestClient, TestClientBuilder, TestClientBuilderExt, + }; - Ok(()) - } + /// Check GRANDPA proof-of-finality for the given block. + /// + /// Returns the vector of headers that MUST be validated + imported + /// AND if at least one of those headers is invalid, all other MUST be considered invalid. + fn check_finality_proof( + current_set_id: SetId, + current_authorities: sp_finality_grandpa::AuthorityList, + remote_proof: Vec, + ) -> sp_blockchain::Result> + where + NumberFor: BlockNumberOps, + { + let proof = super::FinalityProof::::decode(&mut &remote_proof[..]) + .map_err(|_| ClientError::BadJustification("failed to decode finality proof".into()))?; + + let justification: GrandpaJustification = Decode::decode(&mut &proof.justification[..]) + .map_err(|_| ClientError::JustificationDecode)?; + justification.verify(current_set_id, ¤t_authorities)?; + + Ok(proof) } - #[derive(Debug, PartialEq, Encode, Decode)] - pub struct TestBlockJustification(TestJustification, u64, H256); - - impl ProvableJustification
for TestBlockJustification { - fn verify(&self, set_id: u64, authorities: &[(AuthorityId, u64)]) -> ClientResult<()> { - self.0.verify(set_id, authorities) - } - } + pub(crate) type FinalityProof = super::FinalityProof
; fn header(number: u64) -> Header { let parent_hash = match number { @@ -332,57 +298,64 @@ pub(crate) mod tests { ) } - fn test_blockchain() -> InMemoryBlockchain { - use sp_finality_grandpa::GRANDPA_ENGINE_ID as ID; - let blockchain = InMemoryBlockchain::::new(); - 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 + fn test_blockchain( + number_of_blocks: u64, + to_finalize: &[u64], + ) -> (Arc, Arc, Vec) { + let builder = TestClientBuilder::new(); + let backend = builder.backend(); + let mut client = Arc::new(builder.build()); + + let mut blocks = Vec::new(); + for _ in 0..number_of_blocks { + let block = client.new_block(Default::default()).unwrap().build().unwrap().block; + block_on(client.import(BlockOrigin::Own, block.clone())).unwrap(); + blocks.push(block); + } + + for block in to_finalize { + client.finalize_block(BlockId::Number(*block), None).unwrap(); + } + (client, backend, blocks) + } + + fn store_best_justification(client: &TestClient, just: &GrandpaJustification) { + client.lock_import_and_run(|import_op| { + crate::aux_schema::update_best_justification( + just, + |insert| apply_aux(import_op, insert, &[]), + ) + }) + .unwrap(); } #[test] fn finality_proof_fails_if_no_more_last_finalized_blocks() { - use sp_finality_grandpa::GRANDPA_ENGINE_ID as ID; - let blockchain = test_blockchain(); - 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 (_, backend, _) = test_blockchain(6, &[4]); + let authority_set_changes = AuthoritySetChanges::empty(); - let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(0, 5); - - // The last finalized block is 3, so we cannot provide further justifications. - let proof_of_4 = prove_finality::<_, _, TestJustification>( - &blockchain, + // The last finalized block is 4, so we cannot provide further justifications. + let proof_of_5 = prove_finality( + &*backend, authority_set_changes, - *header(4).number(), + 5, ); - assert!(matches!(proof_of_4, Err(FinalityProofError::BlockNotYetFinalized))); + assert!(matches!(proof_of_5, Err(FinalityProofError::BlockNotYetFinalized))); } #[test] fn finality_proof_is_none_if_no_justification_known() { - let blockchain = test_blockchain(); - blockchain - .insert(header(4).hash(), header(4), None, None, NewBlockState::Final) - .unwrap(); + let (_, backend, _) = test_blockchain(6, &[4]); let mut authority_set_changes = AuthoritySetChanges::empty(); authority_set_changes.append(0, 4); // Block 4 is finalized without justification // => we can't prove finality of 3 - let proof_of_3 = prove_finality::<_, _, TestJustification>( - &blockchain, + let proof_of_3 = prove_finality( + &*backend, authority_set_changes, - *header(3).number(), + 3, ) .unwrap(); assert_eq!(proof_of_3, None); @@ -391,7 +364,7 @@ pub(crate) mod tests { #[test] fn finality_proof_check_fails_when_proof_decode_fails() { // When we can't decode proof from Vec - check_finality_proof::<_, TestJustification>( + check_finality_proof::( 1, vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], vec![42], @@ -402,92 +375,208 @@ pub(crate) mod tests { #[test] fn finality_proof_check_fails_when_proof_is_empty() { // When decoded proof has zero length - check_finality_proof::<_, TestJustification>( + check_finality_proof::( 1, vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], - Vec::::new().encode(), + Vec::>::new().encode(), ) .unwrap_err(); } #[test] - fn finality_proof_check_works() { - let auth = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; + fn finality_proof_check_fails_with_incomplete_justification() { + let (client, _, blocks) = test_blockchain(8, &[4, 5, 8]); + + // Create a commit without precommits + let commit = finality_grandpa::Commit { + target_hash: blocks[7].hash(), + target_number: *blocks[7].header().number(), + precommits: Vec::new(), + }; + let grandpa_just = GrandpaJustification::from_commit(&client, 8, commit).unwrap(); + let finality_proof = FinalityProof { block: header(2).hash(), - justification: TestJustification((1, auth.clone()), vec![7]).encode(), + justification: grandpa_just.encode(), unknown_headers: Vec::new(), }; - let proof = check_finality_proof::<_, TestJustification>( + + check_finality_proof::( 1, - auth.clone(), + vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], finality_proof.encode(), - ) - .unwrap(); - assert_eq!(proof, finality_proof); + ).unwrap_err(); + } + + fn create_commit( + block: Block, + round: u64, + set_id: SetId, + auth: &[Ed25519Keyring] + ) -> finality_grandpa::Commit + where + Id: From, + S: From, + { + let mut precommits = Vec::new(); + + for voter in auth { + let precommit = finality_grandpa::Precommit { + target_hash: block.hash(), + target_number: *block.header().number(), + }; + + let msg = finality_grandpa::Message::Precommit(precommit.clone()); + let encoded = sp_finality_grandpa::localized_payload(round, set_id, &msg); + let signature = voter.sign(&encoded[..]).into(); + + let signed_precommit = finality_grandpa::SignedPrecommit { + precommit, + signature, + id: voter.public().into(), + }; + precommits.push(signed_precommit); + } + + finality_grandpa::Commit { + target_hash: block.hash(), + target_number: *block.header().number(), + precommits, + } + } + + #[test] + fn finality_proof_check_works_with_correct_justification() { + let (client, _, blocks) = test_blockchain(8, &[4, 5, 8]); + + let alice = Ed25519Keyring::Alice; + let set_id = 1; + let round = 8; + let commit = create_commit(blocks[7].clone(), round, set_id, &[alice]); + let grandpa_just = GrandpaJustification::from_commit(&client, round, commit).unwrap(); + + let finality_proof = FinalityProof { + block: header(2).hash(), + justification: grandpa_just.encode(), + unknown_headers: Vec::new(), + }; + assert_eq!( + finality_proof, + check_finality_proof::( + set_id, + vec![(alice.public().into(), 1u64)], + finality_proof.encode(), + ) + .unwrap(), + ); } #[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 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(); + let (_, backend, _) = test_blockchain(8, &[4, 5, 8]); // 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. let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(1, 7); + authority_set_changes.append(1, 8); - let proof_of_5 = prove_finality::<_, _, TestJustification>( - &blockchain, + let proof_of_6 = prove_finality( + &*backend, authority_set_changes, - *header(5).number(), + 6, ); - assert!(matches!(proof_of_5, Err(FinalityProofError::BlockNotInAuthoritySetChanges))); + assert!(matches!(proof_of_6, Err(FinalityProofError::BlockNotInAuthoritySetChanges))); } #[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 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 (client, backend, blocks) = test_blockchain(8, &[4, 5]); + let block7 = &blocks[6]; + let block8 = &blocks[7]; + + let round = 8; + let commit = create_commit(block8.clone(), round, 1, &[Ed25519Keyring::Alice]); + let grandpa_just8 = GrandpaJustification::from_commit(&client, round, commit).unwrap(); + client.finalize_block( + BlockId::Number(8), + Some((ID, grandpa_just8.encode().clone())) + ) + .unwrap(); + + // Authority set change at block 8, so the justification stored there will be used in the + // FinalityProof for block 6 let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(0, 4); - authority_set_changes.append(1, 7); + authority_set_changes.append(0, 5); + authority_set_changes.append(1, 8); + + let proof_of_6: FinalityProof = Decode::decode( + &mut &prove_finality( + &*backend, + authority_set_changes.clone(), + 6, + ) + .unwrap() + .unwrap()[..], + ) + .unwrap(); + assert_eq!( + proof_of_6, + FinalityProof { + block: block8.hash(), + justification: grandpa_just8.encode(), + unknown_headers: vec![block7.header().clone(), block8.header().clone()], + }, + ); + } + + #[test] + fn finality_proof_in_last_set_fails_without_latest() { + let (_, backend, _) = test_blockchain(8, &[4, 5, 8]); + + // No recent authority set change, so we are in the latest set, and we will try to pickup + // the best stored justification, for which there is none in this case. + let mut authority_set_changes = AuthoritySetChanges::empty(); + authority_set_changes.append(0, 5); + + assert!(matches!( + prove_finality(&*backend, authority_set_changes, 6), + Ok(None), + )); + } + + #[test] + fn finality_proof_in_last_set_using_latest_justification_works() { + let (client, backend, blocks) = test_blockchain(8, &[4, 5, 8]); + let block7 = &blocks[6]; + let block8 = &blocks[7]; + + let round = 8; + let commit = create_commit(block8.clone(), round, 1, &[Ed25519Keyring::Alice]); + let grandpa_just8 = GrandpaJustification::from_commit(&client, round, commit).unwrap(); + store_best_justification(&client, &grandpa_just8); + + // No recent authority set change, so we are in the latest set, and will pickup the best + // stored justification + let mut authority_set_changes = AuthoritySetChanges::empty(); + authority_set_changes.append(0, 5); - let proof_of_5: FinalityProof = Decode::decode( - &mut &prove_finality::<_, _, TestJustification>( - &blockchain, + let proof_of_6: FinalityProof = Decode::decode( + &mut &prove_finality( + &*backend, authority_set_changes, - *header(5).number(), + 6, ) .unwrap() .unwrap()[..], ) .unwrap(); assert_eq!( - proof_of_5, + proof_of_6, FinalityProof { - block: header(7).hash(), - justification: grandpa_just7, - unknown_headers: vec![header(6)], + block: block8.hash(), + justification: grandpa_just8.encode(), + unknown_headers: vec![block7.header().clone(), block8.header().clone()], } ); } diff --git a/client/network/src/block_request_handler.rs b/client/network/src/block_request_handler.rs index 332635dbe7901..633b6b5935edc 100644 --- a/client/network/src/block_request_handler.rs +++ b/client/network/src/block_request_handler.rs @@ -80,6 +80,7 @@ struct SeenRequestsKey { max_blocks: usize, direction: Direction, attributes: BlockAttributes, + support_multiple_justifications: bool, } impl Hash for SeenRequestsKey { @@ -180,12 +181,15 @@ impl BlockRequestHandler { let attributes = BlockAttributes::from_be_u32(request.fields)?; + let support_multiple_justifications = request.support_multiple_justifications; + let key = SeenRequestsKey { peer: *peer, max_blocks, direction, from: from_block_id.clone(), attributes, + support_multiple_justifications, }; let mut reputation_changes = Vec::new(); @@ -221,6 +225,7 @@ impl BlockRequestHandler { from_block_id, direction, max_blocks, + support_multiple_justifications, )?; // If any of the blocks contains nay data, we can consider it as successful request. @@ -259,6 +264,7 @@ impl BlockRequestHandler { mut block_id: BlockId, direction: Direction, max_blocks: usize, + support_multiple_justifications: bool, ) -> Result { let get_header = attributes.contains(BlockAttributes::HEADER); let get_body = attributes.contains(BlockAttributes::BODY); @@ -277,22 +283,33 @@ impl BlockRequestHandler { None }; - // 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 (justifications, justification, is_empty_justification) = + if support_multiple_justifications { + let justifications = match justifications { + Some(v) => v.encode(), + None => Vec::new(), + }; + (justifications, Vec::new(), false) + } else { + // 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 once we remove the backwards compatibility. + // See: https://github.com/paritytech/substrate/issues/8172 + 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(); + + (Vec::new(), justification, is_empty_justification) + }; let body = if get_body { match self.client.block_body(&BlockId::Hash(hash))? { @@ -320,6 +337,7 @@ impl BlockRequestHandler { message_queue: Vec::new(), justification, is_empty_justification, + justifications, }; total_size += block_data.body.len(); diff --git a/client/network/src/light_client_requests/sender.rs b/client/network/src/light_client_requests/sender.rs index 652f465d6f250..bf832ea13aedf 100644 --- a/client/network/src/light_client_requests/sender.rs +++ b/client/network/src/light_client_requests/sender.rs @@ -722,6 +722,7 @@ impl Request { to_block: Default::default(), direction: schema::v1::Direction::Ascending as i32, max_blocks: 1, + support_multiple_justifications: true, }; let mut buf = Vec::with_capacity(rq.encoded_len()); diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 5f37880653a39..a1a0d774f0f65 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -592,6 +592,11 @@ impl Protocol { } else { None }, + justifications: if !block_data.justifications.is_empty() { + Some(DecodeAll::decode_all(&mut block_data.justifications.as_ref())?) + } else { + None + }, }) }).collect::, codec::Error>>(); @@ -920,6 +925,7 @@ impl Protocol { receipt: None, message_queue: None, justification: None, + justifications: None, }, ], }, @@ -1135,6 +1141,7 @@ fn prepare_block_request( to_block: request.to.map(|h| h.encode()).unwrap_or_default(), direction: request.direction as i32, max_blocks: request.max.unwrap_or(0), + support_multiple_justifications: true, }; CustomMessageOutcome::BlockRequest { diff --git a/client/network/src/protocol/message.rs b/client/network/src/protocol/message.rs index 7564804400fb3..dc6beac99aa01 100644 --- a/client/network/src/protocol/message.rs +++ b/client/network/src/protocol/message.rs @@ -168,7 +168,7 @@ impl generic::BlockAnnounce { pub mod generic { use bitflags::bitflags; use codec::{Encode, Decode, Input, Output}; - use sp_runtime::EncodedJustification; + use sp_runtime::{EncodedJustification, Justifications}; use super::{ RemoteReadResponse, Transactions, Direction, RequestId, BlockAttributes, RemoteCallResponse, ConsensusEngineId, @@ -254,6 +254,8 @@ pub mod generic { pub message_queue: Option>, /// Justification if requested. pub justification: Option, + /// Justifications if requested. + pub justifications: 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 6e07bd4c96971..d3ab2912a9dc5 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -833,8 +833,9 @@ impl ChainSync { .drain(self.best_queued_number + One::one()) .into_iter() .map(|block_data| { - let justifications = - legacy_justification_mapping(block_data.block.justification); + let justifications = block_data.block.justifications.or( + legacy_justification_mapping(block_data.block.justification) + ); IncomingBlock { hash: block_data.block.hash, header: block_data.block.header, @@ -854,11 +855,14 @@ impl ChainSync { } validate_blocks::(&blocks, who, Some(request))?; blocks.into_iter().map(|b| { + let justifications = b.justifications.or( + legacy_justification_mapping(b.justification) + ); IncomingBlock { hash: b.hash, header: b.header, body: b.body, - justifications: legacy_justification_mapping(b.justification), + justifications, origin: Some(who.clone()), allow_missing_state: true, import_existing: false, @@ -963,11 +967,14 @@ impl ChainSync { // When request.is_none() this is a block announcement. Just accept blocks. validate_blocks::(&blocks, who, None)?; blocks.into_iter().map(|b| { + let justifications = b.justifications.or( + legacy_justification_mapping(b.justification) + ); IncomingBlock { hash: b.hash, header: b.header, body: b.body, - justifications: legacy_justification_mapping(b.justification), + justifications, origin: Some(who.clone()), allow_missing_state: true, import_existing: false, @@ -1043,7 +1050,7 @@ impl ChainSync { return Err(BadPeer(who, rep::BAD_JUSTIFICATION)); } - block.justification + block.justifications.or(legacy_justification_mapping(block.justification)) } else { // we might have asked the peer for a justification on a block that we assumed it // had but didn't (regardless of whether it had a justification for it or not). @@ -1058,7 +1065,7 @@ impl ChainSync { if let Some((peer, hash, number, j)) = self .extra_justifications - .on_response(who, legacy_justification_mapping(justification)) + .on_response(who, justification) { return Ok(OnBlockJustification::Import { peer, hash, number, justifications: j }) } @@ -1655,7 +1662,7 @@ 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 +// See: https://github.com/paritytech/substrate/issues/8172 fn legacy_justification_mapping(justification: Option) -> Option { justification.map(|just| (*b"FRNK", just).into()) } @@ -2163,6 +2170,7 @@ mod test { receipt: None, message_queue: None, justification: None, + justifications: None, } ).collect(), } diff --git a/client/network/src/protocol/sync/blocks.rs b/client/network/src/protocol/sync/blocks.rs index 60492f24ed8c3..81f9cffacaab4 100644 --- a/client/network/src/protocol/sync/blocks.rs +++ b/client/network/src/protocol/sync/blocks.rs @@ -228,6 +228,7 @@ mod test { message_queue: None, receipt: None, justification: None, + justifications: None, }).collect() } diff --git a/client/network/src/schema/api.v1.proto b/client/network/src/schema/api.v1.proto index a933c5811c109..23d585b05e9cd 100644 --- a/client/network/src/schema/api.v1.proto +++ b/client/network/src/schema/api.v1.proto @@ -29,6 +29,10 @@ message BlockRequest { Direction direction = 5; // Maximum number of blocks to return. An implementation defined maximum is used when unspecified. uint32 max_blocks = 6; // optional + // Indicate to the receiver that we support multiple justifications. If the responder also + // supports this it will populate the multiple justifications field in `BlockData` instead of + // the single justification field. + bool support_multiple_justifications = 7; // optional } // Response to `BlockRequest` @@ -56,5 +60,11 @@ message BlockData { // doesn't make in possible to differentiate between a lack of justification and an empty // justification. bool is_empty_justification = 7; // optional, false if absent + // Justifications if requested. + // Unlike the field for a single justification, this field does not required an associated + // boolean to differentiate between the lack of justifications and empty justification(s). This + // is because empty justifications, like all justifications, are paired with a non-empty + // consensus engine ID. + bytes justifications = 8; // optional } diff --git a/frame/support/src/traits/members.rs b/frame/support/src/traits/members.rs index 125f096fa92e1..8b9c2c90f541d 100644 --- a/frame/support/src/traits/members.rs +++ b/frame/support/src/traits/members.rs @@ -27,10 +27,20 @@ pub trait Contains { /// A `Contains` implementation which always returns `true`. pub struct All(PhantomData); -impl Contains for All { +impl Contains for All { fn contains(_: &T) -> bool { true } } +#[impl_trait_for_tuples::impl_for_tuples(30)] +impl Contains for Tuple { + fn contains(t: &T) -> bool { + for_tuples!( #( + if Tuple::contains(t) { return true } + )* ); + false + } +} + /// Create a type which implements the `Contains` trait for a particular type with syntax similar /// to `matches!`. #[macro_export] diff --git a/utils/frame/remote-externalities/Cargo.toml b/utils/frame/remote-externalities/Cargo.toml index 06789442a8c52..7d372e8648eea 100644 --- a/utils/frame/remote-externalities/Cargo.toml +++ b/utils/frame/remote-externalities/Cargo.toml @@ -13,8 +13,8 @@ readme = "README.md" targets = ["x86_64-unknown-linux-gnu"] [dependencies] -jsonrpsee-http-client = { version = "=0.2.0-alpha.5", default-features = false, features = ["tokio02"] } -jsonrpsee-proc-macros = "=0.2.0-alpha.5" +jsonrpsee-http-client = { version = "=0.2.0-alpha.6", default-features = false, features = ["tokio02"] } +jsonrpsee-proc-macros = "=0.2.0-alpha.6" hex-literal = "0.3.1" env_logger = "0.8.2"