diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 838a6fb90219a..377fb3b5e5ca6 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -111,7 +111,6 @@ pub fn new_partial(config: &Configuration) -> Result: Send + Sync { - /// Read GRANDPA_AUTHORITIES_KEY from storage at given block. - fn authorities(&self, block: &BlockId) -> ClientResult; -} - -/// Implementation of AuthoritySetForFinalityProver. -impl AuthoritySetForFinalityProver - for Arc + Send + Sync> -where - BE: Backend + Send + Sync + 'static, -{ - fn authorities(&self, block: &BlockId) -> ClientResult { - let storage_key = StorageKey(GRANDPA_AUTHORITIES_KEY.to_vec()); - self.storage(block, &storage_key)? - .and_then(|encoded| VersionedAuthorityList::decode(&mut encoded.0.as_slice()).ok()) - .map(|versioned| versioned.into()) - .ok_or(ClientError::InvalidAuthoritiesSet) - } -} - /// Finality proof provider for serving network requests. pub struct FinalityProofProvider { backend: Arc, - authority_provider: Arc>, shared_authority_set: Option>>, } @@ -95,17 +71,12 @@ where /// - backend for accessing blockchain data; /// - authority_provider for calling and proving runtime methods. /// - shared_authority_set for accessing authority set data - pub fn new

( + pub fn new( backend: Arc, - authority_provider: P, shared_authority_set: Option>>, - ) -> Self - where - P: AuthoritySetForFinalityProver + 'static, - { + ) -> Self { FinalityProofProvider { backend, - authority_provider: Arc::new(authority_provider), shared_authority_set, } } @@ -117,14 +88,9 @@ where /// - shared_authority_set for accessing authority set data pub fn new_for_service( backend: Arc, - storage_provider: Arc + Send + Sync>, shared_authority_set: Option>>, ) -> Arc { - Arc::new(Self::new( - backend, - storage_provider, - shared_authority_set, - )) + Arc::new(Self::new(backend, shared_authority_set)) } } @@ -136,9 +102,10 @@ where { /// 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) - -> Result>, FinalityProofError> - { + pub fn prove_finality( + &self, + block: NumberFor + ) -> Result>, FinalityProofError> { let authority_set_changes = if let Some(changes) = self .shared_authority_set .as_ref() @@ -151,7 +118,6 @@ where prove_finality::<_, _, GrandpaJustification>( &*self.backend.blockchain(), - &*self.authority_provider, authority_set_changes, block, ) @@ -204,7 +170,6 @@ type AuthoritySetProof

= Vec>; fn prove_finality( blockchain: &B, - authorities_provider: &dyn AuthoritySetForFinalityProver, authority_set_changes: AuthoritySetChanges>, block: NumberFor, ) -> Result>, FinalityProofError> @@ -227,7 +192,7 @@ where // 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 (set_id, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) { + let (_, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) { id } else { trace!( @@ -253,21 +218,6 @@ where return Ok(None); }; - - // Check if the justification is generated by the requested authority set - let block_authorities = authorities_provider.authorities(&BlockId::Number(block))?; - let justification_check_result = - J::decode_and_verify(&justification, set_id, &block_authorities); - if justification_check_result.is_err() { - trace!( - target: "afg", - "Can not provide finality proof with requested set id #{}\ - (possible forced change?). Returning empty proof.", - set_id, - ); - return Ok(None); - } - // Collect all headers from the requested block until the last block of the set let unknown_headers = { let mut headers = Vec::new(); @@ -276,14 +226,6 @@ where if current >= last_block_for_set || headers.len() >= MAX_UNKNOWN_HEADERS { break; } - if block_authorities != authorities_provider.authorities(&BlockId::Number(current))? { - trace!( - target: "afg", - "Encountered new authorities when collecting unknown headers. \ - Returning empty proof", - ); - return Ok(None); - } headers.push(blockchain.expect_header(BlockId::Number(current))?); current += One::one(); } @@ -653,23 +595,15 @@ impl WarpSyncFragmentCache
{ #[cfg(test)] pub(crate) mod tests { use super::*; - use substrate_test_runtime_client::runtime::{Block, Header, H256}; + use crate::authorities::AuthoritySetChanges; + use sp_core::crypto::Public; + use sp_finality_grandpa::AuthorityList; use sc_client_api::NewBlockState; use sc_client_api::in_mem::Blockchain as InMemoryBlockchain; - use sp_core::crypto::Public; - use crate::authorities::AuthoritySetChanges; + use substrate_test_runtime_client::runtime::{Block, Header, H256}; pub(crate) type FinalityProof = super::FinalityProof
; - impl AuthoritySetForFinalityProver for GetAuthorities - where - GetAuthorities: Send + Sync + Fn(BlockId) -> ClientResult, - { - fn authorities(&self, block: &BlockId) -> ClientResult { - self(*block) - } - } - #[derive(Debug, PartialEq, Encode, Decode)] pub struct TestJustification(pub (u64, AuthorityList), pub Vec); @@ -733,7 +667,7 @@ pub(crate) mod tests { } #[test] - fn finality_proof_is_none_if_no_more_last_finalized_blocks() { + fn finality_proof_fails_if_no_more_last_finalized_blocks() { let blockchain = test_blockchain(); blockchain .insert(header(4).hash(), header(4), Some(vec![1]), None, NewBlockState::Best) @@ -748,7 +682,6 @@ pub(crate) mod tests { // The last finalized block is 3, so we cannot provide further justifications. let proof_of_4 = prove_finality::<_, _, TestJustification>( &blockchain, - &|_| unreachable!("Should return before calling GetAuthorities"), authority_set_changes, *header(4).number(), ); @@ -769,7 +702,6 @@ pub(crate) mod tests { // => we can't prove finality of 3 let proof_of_3 = prove_finality::<_, _, TestJustification>( &blockchain, - &|_| unreachable!("Should return before calling GetAuthorities"), authority_set_changes, *header(3).number(), ) @@ -777,136 +709,6 @@ pub(crate) mod tests { assert_eq!(proof_of_3, None); } - #[test] - fn finality_proof_is_none_if_justification_is_generated_by_unknown_set() { - // This is the case for forced change: set_id has been forcibly increased, - // or when our stored authority set changes is incomplete - let blockchain = test_blockchain(); - let auth = vec![(AuthorityId::from_slice(&[42u8; 32]), 1u64)]; - let just4 = TestJustification((0, auth), vec![4]).encode(); - blockchain - .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) - .unwrap(); - - let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(0, 4); - - let proof_of_3 = prove_finality::<_, _, TestJustification>( - &blockchain, - &|_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), - authority_set_changes, - *header(3).number(), - ) - .unwrap(); - assert!(proof_of_3.is_none()); - } - - #[test] - fn finality_proof_is_none_if_authority_set_id_is_incorrect() { - let blockchain = test_blockchain(); - let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let just4 = TestJustification((0, auth.clone()), vec![4]).encode(); - blockchain - .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) - .unwrap(); - - let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(0, 1); - authority_set_changes.append(1, 4); - - // We call `prove_finality` with the wrong `authorities_set_id`, since the Justification for - // block 4 contains set id 0. - let proof_of_3 = prove_finality::<_, _, TestJustification>( - &blockchain, - &|_| Ok(auth.clone()), - authority_set_changes, - *header(3).number(), - ) - .unwrap(); - assert!(proof_of_3.is_none()); - } - - #[test] - fn finality_proof_is_none_for_next_set_id_with_new_the_authority_set() { - let blockchain = test_blockchain(); - let auth1 = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let auth2 = vec![(AuthorityId::from_slice(&[2u8; 32]), 1u64)]; - let just5 = TestJustification((0, auth1.clone()), vec![5]).encode(); - let just6 = TestJustification((1, auth2.clone()), vec![6]).encode(); - blockchain - .insert(header(4).hash(), header(4), None, None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(5).hash(), header(5), Some(just5), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(6).hash(), header(6), Some(just6), None, NewBlockState::Final) - .unwrap(); - - let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(0, 1); - authority_set_changes.append(1, 6); - - // Trying to prove block 4 using block 6 fails as the authority set has changed - let proof_of_4 = prove_finality::<_, _, TestJustification>( - &blockchain, - &|block_id| match block_id { - BlockId::Number(4) => Ok(auth1.clone()), - _ => unimplemented!("No other authorities should be proved: {:?}", block_id), - }, - authority_set_changes, - *header(4).number(), - ) - .unwrap(); - assert!(proof_of_4.is_none()); - } - - #[test] - fn finality_proof_is_none_if_the_authority_set_changes_and_changes_back() { - let blockchain = test_blockchain(); - let auth1 = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let auth2 = vec![(AuthorityId::from_slice(&[2u8; 32]), 1u64)]; - let just5 = TestJustification((0, auth1.clone()), vec![5]).encode(); - let just6 = TestJustification((1, auth2.clone()), vec![6]).encode(); - let just7 = TestJustification((2, auth1.clone()), vec![7]).encode(); - blockchain - .insert(header(4).hash(), header(4), None, None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(5).hash(), header(5), Some(just5), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(6).hash(), header(6), Some(just6), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(7).hash(), header(7), Some(just7), None, NewBlockState::Final) - .unwrap(); - - // Set authority set changes so that they don't contain the switch, and switch back, of the - // authorities. As well as incorrect set_id to avoid the guard against that. - // This should trigger the check for walking through the headers and checking for authority - // set changes that are missed. - let mut authority_set_changes = AuthoritySetChanges::empty(); - authority_set_changes.append(0, 1); - authority_set_changes.append(1, 2); - authority_set_changes.append(2, 7); - - let proof_of_4 = - prove_finality::<_, _, TestJustification>( - &blockchain, - &|block_id| match block_id { - BlockId::Number(4) => Ok(auth1.clone()), - BlockId::Number(5) => Ok(auth1.clone()), - BlockId::Number(6) => Ok(auth2.clone()), - _ => unimplemented!("No other authorities should be proved: {:?}", block_id), - }, - authority_set_changes, - *header(4).number(), - ) - .unwrap(); - assert!(proof_of_4.is_none()); - } - #[test] fn finality_proof_check_fails_when_proof_decode_fails() { // When we can't decode proof from Vec @@ -947,7 +749,7 @@ pub(crate) mod tests { } #[test] - fn finality_proof_using_authority_set_changes_is_none_with_undefined_start() { + fn finality_proof_using_authority_set_changes_fails_with_undefined_start() { let blockchain = test_blockchain(); let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; let just4 = TestJustification((0, auth.clone()), vec![4]).encode(); @@ -972,11 +774,6 @@ pub(crate) mod tests { let proof_of_5 = prove_finality::<_, _, TestJustification>( &blockchain, - &|block_id| match block_id { - BlockId::Number(5) => Ok(auth.clone()), - BlockId::Number(6) => Ok(auth.clone()), - _ => unimplemented!("No other authorities should be proved: {:?}", block_id), - }, authority_set_changes, *header(5).number(), ); @@ -1009,11 +806,6 @@ pub(crate) mod tests { let proof_of_5: FinalityProof = Decode::decode( &mut &prove_finality::<_, _, TestJustification>( &blockchain, - &|block_id| match block_id { - BlockId::Number(5) => Ok(auth.clone()), - BlockId::Number(6) => Ok(auth.clone()), - _ => unimplemented!("No other authorities should be proved: {:?}", block_id), - }, authority_set_changes, *header(5).number(), )