From 75d59d87358fb47f0695783cb2232a292530219b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 8 Oct 2020 15:55:50 +0200 Subject: [PATCH 01/28] grandpa: persist block number for last block of authority set --- .../src/authority_set_changes.rs | 39 +++++++++++++++++++ client/finality-grandpa/src/aux_schema.rs | 23 +++++++++++ client/finality-grandpa/src/environment.rs | 25 +++++++++++- client/finality-grandpa/src/import.rs | 6 +++ client/finality-grandpa/src/lib.rs | 4 ++ client/finality-grandpa/src/observer.rs | 5 +++ 6 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 client/finality-grandpa/src/authority_set_changes.rs diff --git a/client/finality-grandpa/src/authority_set_changes.rs b/client/finality-grandpa/src/authority_set_changes.rs new file mode 100644 index 0000000000000..532c83f332c43 --- /dev/null +++ b/client/finality-grandpa/src/authority_set_changes.rs @@ -0,0 +1,39 @@ +// Copyright 2018-2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +use std::sync::Arc; +use parity_scale_codec::{Encode, Decode}; + +// Tracks authority set changes. We store the block numbers for the last block of each authority +// set. +#[derive(Debug, Encode, Decode)] +pub(crate) struct AuthoritySetChanges { + authority_set_changes: Vec, +} + +impl AuthoritySetChanges { + pub(crate) fn empty() -> Self { + Self { + authority_set_changes: Vec::new(), + } + } + + pub(crate) fn append(&mut self, number: N) { + self.authority_set_changes.push(number) + } +} + +pub(crate) type SharedAuthoritySetChanges = Arc>>; diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 4ed96d058ac6b..9bbb31ef1f7a6 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -28,6 +28,7 @@ use log::{info, warn}; use sp_finality_grandpa::{AuthorityList, SetId, RoundNumber}; use crate::authorities::{AuthoritySet, SharedAuthoritySet, PendingChange, DelayKind}; +use crate::authority_set_changes::{AuthoritySetChanges, SharedAuthoritySetChanges}; use crate::consensus_changes::{SharedConsensusChanges, ConsensusChanges}; use crate::environment::{ CompletedRound, CompletedRounds, CurrentRounds, HasVoted, SharedVoterSetState, VoterSetState, @@ -39,6 +40,7 @@ const SET_STATE_KEY: &[u8] = b"grandpa_completed_round"; const CONCLUDED_ROUNDS: &[u8] = b"grandpa_concluded_rounds"; const AUTHORITY_SET_KEY: &[u8] = b"grandpa_voters"; const CONSENSUS_CHANGES_KEY: &[u8] = b"grandpa_consensus_changes"; +const AUTHORITY_SET_CHANGES_KEY: &[u8] = b"grandpa_authority_set_changes"; const CURRENT_VERSION: u32 = 2; @@ -122,6 +124,7 @@ pub(crate) fn load_decode(backend: &B, key: &[u8]) -> Cl /// Persistent data kept between runs. pub(crate) struct PersistentData { pub(crate) authority_set: SharedAuthoritySet>, + pub(crate) authority_set_changes: SharedAuthoritySetChanges>, pub(crate) consensus_changes: SharedConsensusChanges>, pub(crate) set_state: SharedVoterSetState, } @@ -274,6 +277,8 @@ pub(crate) fn load_persistent( let version: Option = load_decode(backend, VERSION_KEY)?; let consensus_changes = load_decode(backend, CONSENSUS_CHANGES_KEY)? .unwrap_or_else(ConsensusChanges::>::empty); + let authority_set_changes = load_decode(backend, AUTHORITY_SET_CHANGES_KEY)? + .unwrap_or_else(AuthoritySetChanges::>::empty); let make_genesis_round = move || RoundState::genesis((genesis_hash, genesis_number)); @@ -282,6 +287,7 @@ pub(crate) fn load_persistent( if let Some((new_set, set_state)) = migrate_from_version0::(backend, &make_genesis_round)? { return Ok(PersistentData { authority_set: new_set.into(), + authority_set_changes: Arc::new(authority_set_changes.into()), consensus_changes: Arc::new(consensus_changes.into()), set_state: set_state.into(), }); @@ -291,6 +297,7 @@ pub(crate) fn load_persistent( if let Some((new_set, set_state)) = migrate_from_version1::(backend, &make_genesis_round)? { return Ok(PersistentData { authority_set: new_set.into(), + authority_set_changes: Arc::new(authority_set_changes.into()), consensus_changes: Arc::new(consensus_changes.into()), set_state: set_state.into(), }); @@ -321,6 +328,7 @@ pub(crate) fn load_persistent( return Ok(PersistentData { authority_set: set.into(), + authority_set_changes: Arc::new(authority_set_changes.into()), consensus_changes: Arc::new(consensus_changes.into()), set_state: set_state.into(), }); @@ -358,6 +366,7 @@ pub(crate) fn load_persistent( Ok(PersistentData { authority_set: genesis_set.into(), + authority_set_changes: Arc::new(authority_set_changes.into()), set_state: genesis_state.into(), consensus_changes: Arc::new(consensus_changes.into()), }) @@ -421,6 +430,20 @@ pub(crate) fn write_concluded_round( backend.insert_aux(&[(&key[..], round_data.encode().as_slice())], &[]) } +pub(crate) fn update_authority_set_changes( + authority_set_changes: &AuthoritySetChanges, + write_aux: F, +) -> R +where + N: Encode, + F: FnOnce(&[(&'static [u8], &[u8])]) -> R, +{ + write_aux(&[( + AUTHORITY_SET_CHANGES_KEY, + authority_set_changes.encode().as_slice() + )]) +} + /// Update the consensus changes. pub(crate) fn update_consensus_changes( set: &ConsensusChanges, diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 9215dcb323516..4fef0ac09e865 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1,5 +1,4 @@ // This file is part of Substrate. - // Copyright (C) 2018-2020 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 @@ -49,6 +48,7 @@ use crate::{ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; +use crate::authority_set_changes::SharedAuthoritySetChanges; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; use crate::notification::GrandpaJustificationSender; @@ -417,6 +417,7 @@ pub(crate) struct Environment, SC, pub(crate) config: Config, pub(crate) authority_set: SharedAuthoritySet>, pub(crate) consensus_changes: SharedConsensusChanges>, + pub(crate) authority_set_changes: SharedAuthoritySetChanges>, pub(crate) network: crate::communication::NetworkBridge, pub(crate) set_id: SetId, pub(crate) voter_set_state: SharedVoterSetState, @@ -1071,6 +1072,7 @@ where finalize_block( self.client.clone(), &self.authority_set, + &self.authority_set_changes, &self.consensus_changes, Some(self.config.justification_period.into()), hash, @@ -1136,6 +1138,7 @@ impl From> for JustificationOrCommit< pub(crate) fn finalize_block( client: Arc, authority_set: &SharedAuthoritySet>, + authority_set_changes: &SharedAuthoritySetChanges>, consensus_changes: &SharedConsensusChanges>, justification_period: Option>, hash: Block::Hash, @@ -1175,6 +1178,8 @@ where // reverting in case of failure let mut old_consensus_changes = None; + let mut authority_set_changes = authority_set_changes.lock(); + let mut consensus_changes = consensus_changes.lock(); let canon_at_height = |canon_number| { // "true" because the block is finalized @@ -1292,6 +1297,23 @@ where // the authority set has changed. let (new_id, set_ref) = authority_set.current(); + // Persist the number of the last block of the session + if number > NumberFor::::zero() { + let parent_number = number - NumberFor::::one(); + authority_set_changes.append(parent_number); + let write_result = crate::aux_schema::update_authority_set_changes( + &*authority_set_changes, + |insert| apply_aux(import_op, insert, &[]), + ); + if let Err(e) = write_result { + warn!( + target: "afg", + "Failed to write updated authority set changes to disk: {}", + e + ); + } + } + if set_ref.len() > 16 { afg_log!(initial_sync, "👴 Applying GRANDPA set change to new set with {} authorities", @@ -1309,6 +1331,7 @@ where "authorities" => ?set_ref.to_vec(), "set_id" => ?new_id, ); + Some(NewAuthoritySet { canon_hash, canon_number, diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 04df95a3187e1..56fe5d6547756 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -41,6 +41,7 @@ use sp_runtime::traits::{ use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand}; use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingChange}; +use crate::authority_set_changes::SharedAuthoritySetChanges; use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; @@ -60,6 +61,7 @@ pub struct GrandpaBlockImport { inner: Arc, select_chain: SC, authority_set: SharedAuthoritySet>, + authority_set_changes: SharedAuthoritySetChanges>, send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, @@ -75,6 +77,7 @@ impl Clone for inner: self.inner.clone(), select_chain: self.select_chain.clone(), authority_set: self.authority_set.clone(), + authority_set_changes: self.authority_set_changes.clone(), send_voter_commands: self.send_voter_commands.clone(), consensus_changes: self.consensus_changes.clone(), authority_set_hard_forks: self.authority_set_hard_forks.clone(), @@ -560,6 +563,7 @@ impl GrandpaBlockImport, select_chain: SC, authority_set: SharedAuthoritySet>, + authority_set_changes: SharedAuthoritySetChanges>, send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, @@ -604,6 +608,7 @@ impl GrandpaBlockImport finality_grandpa::Chain> fn grandpa_observer( client: &Arc, authority_set: &SharedAuthoritySet>, + authority_set_changes: &SharedAuthoritySetChanges>, consensus_changes: &SharedConsensusChanges>, voters: &Arc>, justification_sender: &Option>, @@ -83,6 +85,7 @@ where Client: crate::ClientForGrandpa, { let authority_set = authority_set.clone(); + let authority_set_changes = authority_set_changes.clone(); let consensus_changes = consensus_changes.clone(); let client = client.clone(); let voters = voters.clone(); @@ -123,6 +126,7 @@ where match environment::finalize_block( client.clone(), &authority_set, + &authority_set_changes, &consensus_changes, None, finalized_hash, @@ -293,6 +297,7 @@ where let observer = grandpa_observer( &self.client, &self.persistent_data.authority_set, + &self.persistent_data.authority_set_changes, &self.persistent_data.consensus_changes, &voters, &self.justification_sender, From c3dac360eff21af96cc9366bf2f153abd245078d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 13 Oct 2020 11:35:24 +0200 Subject: [PATCH 02/28] grandpa: fix authority_set_changes field in tests --- client/finality-grandpa/src/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 83f1c498a1951..3603c621bf1d3 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1547,6 +1547,7 @@ where { let PersistentData { ref authority_set, + ref authority_set_changes, ref consensus_changes, ref set_state, .. @@ -1570,6 +1571,7 @@ where Environment { authority_set: authority_set.clone(), + authority_set_changes: authority_set_changes.clone(), config: config.clone(), consensus_changes: consensus_changes.clone(), client: link.client.clone(), From 363ba7b14ab55aba9cea3134a530795f4e930768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 16 Oct 2020 14:28:15 +0200 Subject: [PATCH 03/28] grandpa: fix date on copyright notice --- client/finality-grandpa/src/authority_set_changes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/authority_set_changes.rs b/client/finality-grandpa/src/authority_set_changes.rs index 532c83f332c43..8a99b72fe2f84 100644 --- a/client/finality-grandpa/src/authority_set_changes.rs +++ b/client/finality-grandpa/src/authority_set_changes.rs @@ -1,4 +1,4 @@ -// Copyright 2018-2020 Parity Technologies (UK) Ltd. +// Copyright 2020 Parity Technologies (UK) Ltd. // This file is part of Substrate. // Substrate is free software: you can redistribute it and/or modify From 1d54eb5638b3663020d235e8589fe53839e7a999 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 16 Oct 2020 14:31:00 +0200 Subject: [PATCH 04/28] grandpa-rpc: implement cleaner api for prove finality rpc --- client/finality-grandpa/rpc/src/finality.rs | 13 ++++++ client/finality-grandpa/rpc/src/lib.rs | 41 +++++++++++++++++-- .../src/authority_set_changes.rs | 26 ++++++++---- client/finality-grandpa/src/aux_schema.rs | 6 +++ client/finality-grandpa/src/finality_proof.rs | 30 +++++++++++++- 5 files changed, 104 insertions(+), 12 deletions(-) diff --git a/client/finality-grandpa/rpc/src/finality.rs b/client/finality-grandpa/rpc/src/finality.rs index 1f288b86a0e46..525e3699083a6 100644 --- a/client/finality-grandpa/rpc/src/finality.rs +++ b/client/finality-grandpa/rpc/src/finality.rs @@ -34,6 +34,11 @@ pub trait RpcFinalityProofProvider { end: Block::Hash, authorities_set_id: u64, ) -> Result, sp_blockchain::Error>; + + fn rpc_prove_finality2( + &self, + block: NumberFor, + ) -> Result, sp_blockchain::Error>; } impl RpcFinalityProofProvider for FinalityProofProvider @@ -51,4 +56,12 @@ where self.prove_finality(begin, end, authorities_set_id) .map(|x| x.map(|y| EncodedFinalityProofs(y.into()))) } + + fn rpc_prove_finality2( + &self, + block: NumberFor, + ) -> Result, sp_blockchain::Error> { + self.prove_finality2(block) + .map(|x| x.map(|y| EncodedFinalityProofs(y.into()))) + } } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 172473ad6518b..509a152907d4b 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -37,7 +37,7 @@ mod notification; mod report; use sc_finality_grandpa::GrandpaJustificationStream; -use sp_runtime::traits::Block as BlockT; +use sp_runtime::traits::{Block as BlockT, NumberFor}; use finality::{EncodedFinalityProofs, RpcFinalityProofProvider}; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; @@ -48,7 +48,7 @@ type FutureResult = /// Provides RPC methods for interacting with GRANDPA. #[rpc] -pub trait GrandpaApi { +pub trait GrandpaApi { /// RPC Metadata type Metadata; @@ -91,6 +91,14 @@ pub trait GrandpaApi { end: Hash, authorities_set_id: Option, ) -> FutureResult>; + + /// Prove finality for the given block number. + /// WIP: expand this + #[rpc(name = "grandpa_proveFinality2")] + fn prove_finality2( + &self, + block: N, + ) -> FutureResult>; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. @@ -127,7 +135,8 @@ impl } } -impl GrandpaApi +impl + GrandpaApi> for GrandpaRpcHandler where VoterState: ReportVoterState + Send + Sync + 'static, @@ -192,6 +201,25 @@ where .compat() ) } + + fn prove_finality2( + &self, + block: NumberFor, + ) -> FutureResult> { + let result = self + .finality_proof_provider + .rpc_prove_finality2(block); + let future = async move { result }.boxed(); + Box::new( + future + .map_err(|e| { + warn!("Error proving finality: {}", e); + error::Error::ProveFinalityFailed(e) + }) + .map_err(jsonrpc_core::Error::from) + .compat() + ) + } } #[cfg(test)] @@ -268,6 +296,13 @@ mod tests { ) -> Result, sp_blockchain::Error> { Ok(Some(EncodedFinalityProofs(self.finality_proofs.encode().into()))) } + + fn rpc_prove_finality2( + &self, + _block: NumberFor + ) -> Result, sp_blockchain::Error> { + Ok(Some(EncodedFinalityProofs(self.finality_proofs.encode().into()))) + } } impl ReportVoterState for TestVoterState { diff --git a/client/finality-grandpa/src/authority_set_changes.rs b/client/finality-grandpa/src/authority_set_changes.rs index 8a99b72fe2f84..da6f4bead3390 100644 --- a/client/finality-grandpa/src/authority_set_changes.rs +++ b/client/finality-grandpa/src/authority_set_changes.rs @@ -14,8 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use std::sync::Arc; -use parity_scale_codec::{Encode, Decode}; +use parity_scale_codec::{Decode, Encode}; +use std::{cmp::Ord, sync::Arc}; // Tracks authority set changes. We store the block numbers for the last block of each authority // set. @@ -24,16 +24,26 @@ pub(crate) struct AuthoritySetChanges { authority_set_changes: Vec, } -impl AuthoritySetChanges { +impl AuthoritySetChanges { pub(crate) fn empty() -> Self { Self { authority_set_changes: Vec::new(), } - } - - pub(crate) fn append(&mut self, number: N) { - self.authority_set_changes.push(number) - } + } + + pub(crate) fn append(&mut self, number: N) { + self.authority_set_changes.push(number) + } + + pub(crate) fn get_set_id(&self, number: N) -> (u64, N) { + let set_id = self + .authority_set_changes + .binary_search(&number) + .unwrap_or_else(|idx| idx); + let last_block_for_set_id = self.authority_set_changes[set_id]; + // WIP: avoid cast? + (set_id as u64, last_block_for_set_id) + } } pub(crate) type SharedAuthoritySetChanges = Arc>>; diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 9bbb31ef1f7a6..8ef0b94f93d3a 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -456,6 +456,12 @@ pub(crate) fn update_consensus_changes( write_aux(&[(CONSENSUS_CHANGES_KEY, set.encode().as_slice())]) } +pub(crate) fn load_authority_set_changes(backend: &B) + -> ClientResult>>> +{ + load_decode(backend, AUTHORITY_SET_CHANGES_KEY) +} + #[cfg(test)] pub(crate) fn load_authorities(backend: &B) -> Option> { diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 33dd69cc11d6e..9f14e7bcbb2d9 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -39,7 +39,9 @@ use std::sync::Arc; use log::{trace, warn}; -use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; +use sp_blockchain::{ + Backend as BlockchainBackend, Error as ClientError, HeaderBackend, Result as ClientResult, +}; use sc_client_api::{ backend::Backend, StorageProof, light::{FetchChecker, RemoteReadRequest}, @@ -204,6 +206,32 @@ impl FinalityProofProvider end, ) } + + /// Prove finality for the given block number. + /// WIP: expand this description + pub fn prove_finality2( + &self, + block: NumberFor, + ) -> Result>, ClientError> { + let blocks_where_set_changes = + match crate::aux_schema::load_authority_set_changes::<_, Block>(&*self.backend)? { + Some(blocks) => blocks, + None => return Ok(None), + }; + let (set_id, last_block_for_set) = blocks_where_set_changes.get_set_id(block); + + // Convert from block numbers to hashes + let last_block_for_set = self.backend.blockchain().hash(last_block_for_set).unwrap().unwrap(); + let block = self.backend.blockchain().hash(block).unwrap().unwrap(); + + prove_finality::<_, _, GrandpaJustification>( + &*self.backend.blockchain(), + &*self.authority_provider, + set_id, + block, + last_block_for_set, + ) + } } impl sc_network::config::FinalityProofProvider for FinalityProofProvider From 58477bc404ace3c1809e1d00be2ac0270a29d61c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 16 Oct 2020 16:29:19 +0200 Subject: [PATCH 05/28] grandpa-rpc: replace the old prove_finality with the new one --- client/finality-grandpa/rpc/src/finality.rs | 21 +------ client/finality-grandpa/rpc/src/lib.rs | 56 ++----------------- client/finality-grandpa/src/finality_proof.rs | 19 +------ 3 files changed, 7 insertions(+), 89 deletions(-) diff --git a/client/finality-grandpa/rpc/src/finality.rs b/client/finality-grandpa/rpc/src/finality.rs index 525e3699083a6..01006ed5f3a31 100644 --- a/client/finality-grandpa/rpc/src/finality.rs +++ b/client/finality-grandpa/rpc/src/finality.rs @@ -26,16 +26,7 @@ pub struct EncodedFinalityProofs(pub sp_core::Bytes); /// Local trait mainly to allow mocking in tests. pub trait RpcFinalityProofProvider { - /// Return finality proofs for the given authorities set id, if it is provided, otherwise the - /// current one will be used. fn rpc_prove_finality( - &self, - begin: Block::Hash, - end: Block::Hash, - authorities_set_id: u64, - ) -> Result, sp_blockchain::Error>; - - fn rpc_prove_finality2( &self, block: NumberFor, ) -> Result, sp_blockchain::Error>; @@ -48,20 +39,10 @@ where B: sc_client_api::backend::Backend + Send + Sync + 'static, { fn rpc_prove_finality( - &self, - begin: Block::Hash, - end: Block::Hash, - authorities_set_id: u64, - ) -> Result, sp_blockchain::Error> { - self.prove_finality(begin, end, authorities_set_id) - .map(|x| x.map(|y| EncodedFinalityProofs(y.into()))) - } - - fn rpc_prove_finality2( &self, block: NumberFor, ) -> Result, sp_blockchain::Error> { - self.prove_finality2(block) + self.prove_finality(block) .map(|x| x.map(|y| EncodedFinalityProofs(y.into()))) } } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 509a152907d4b..163fb4b6b212f 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -82,20 +82,10 @@ pub trait GrandpaApi { id: SubscriptionId ) -> jsonrpc_core::Result; - /// Prove finality for the range (begin; end] hash. Returns None if there are no finalized blocks - /// unknown in the range. If no authorities set is provided, the current one will be attempted. - #[rpc(name = "grandpa_proveFinality")] - fn prove_finality( - &self, - begin: Hash, - end: Hash, - authorities_set_id: Option, - ) -> FutureResult>; - /// Prove finality for the given block number. /// WIP: expand this - #[rpc(name = "grandpa_proveFinality2")] - fn prove_finality2( + #[rpc(name = "grandpa_proveFinality")] + fn prove_finality( &self, block: N, ) -> FutureResult>; @@ -179,36 +169,12 @@ where } fn prove_finality( - &self, - begin: Block::Hash, - end: Block::Hash, - authorities_set_id: Option, - ) -> FutureResult> { - // If we are not provided a set_id, try with the current one. - let authorities_set_id = authorities_set_id - .unwrap_or_else(|| self.authority_set.get().0); - let result = self - .finality_proof_provider - .rpc_prove_finality(begin, end, authorities_set_id); - let future = async move { result }.boxed(); - Box::new( - future - .map_err(|e| { - warn!("Error proving finality: {}", e); - error::Error::ProveFinalityFailed(e) - }) - .map_err(jsonrpc_core::Error::from) - .compat() - ) - } - - fn prove_finality2( &self, block: NumberFor, ) -> FutureResult> { let result = self .finality_proof_provider - .rpc_prove_finality2(block); + .rpc_prove_finality(block); let future = async move { result }.boxed(); Box::new( future @@ -289,15 +255,6 @@ mod tests { impl RpcFinalityProofProvider for TestFinalityProofProvider { fn rpc_prove_finality( - &self, - _begin: Block::Hash, - _end: Block::Hash, - _authoritites_set_id: u64, - ) -> Result, sp_blockchain::Error> { - Ok(Some(EncodedFinalityProofs(self.finality_proofs.encode().into()))) - } - - fn rpc_prove_finality2( &self, _block: NumberFor ) -> Result, sp_blockchain::Error> { @@ -566,11 +523,8 @@ mod tests { finality_proofs.clone(), ); - let request = "{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_proveFinality\",\"params\":[\ - \"0x0000000000000000000000000000000000000000000000000000000000000000\",\ - \"0x0000000000000000000000000000000000000000000000000000000000000001\",\ - 42\ - ],\"id\":1}"; + let request = + "{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_proveFinality\",\"params\":[42],\"id\":1}"; let meta = sc_rpc::Metadata::default(); let resp = io.handle_request_sync(request, meta); diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 9f14e7bcbb2d9..70512633174ca 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -190,26 +190,9 @@ impl FinalityProofProvider NumberFor: BlockNumberOps, B: Backend + Send + Sync + 'static, { - /// Prove finality for the range (begin; end] hash. Returns None if there are no finalized blocks - /// unknown in the range. - pub fn prove_finality( - &self, - begin: Block::Hash, - end: Block::Hash, - authorities_set_id: u64, - ) -> Result>, ClientError> { - prove_finality::<_, _, GrandpaJustification>( - &*self.backend.blockchain(), - &*self.authority_provider, - authorities_set_id, - begin, - end, - ) - } - /// Prove finality for the given block number. /// WIP: expand this description - pub fn prove_finality2( + pub fn prove_finality( &self, block: NumberFor, ) -> Result>, ClientError> { From 10e507ff6c56c439aeed0d0dfa70ecaef10f4c12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 16 Oct 2020 16:36:41 +0200 Subject: [PATCH 06/28] grandpa: undo accidental whitespace change --- client/finality-grandpa/src/environment.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 4fef0ac09e865..d69c005b2289e 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1331,7 +1331,6 @@ where "authorities" => ?set_ref.to_vec(), "set_id" => ?new_id, ); - Some(NewAuthoritySet { canon_hash, canon_number, From 83ff901361bf798f6246c858427af2e0ef1f0330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sat, 31 Oct 2020 22:23:09 +0100 Subject: [PATCH 07/28] grandpa-rpc: start work on redo of the finality_proof RPC API --- bin/node-template/node/src/service.rs | 16 +++- bin/node/cli/src/service.rs | 14 +++- client/finality-grandpa/rpc/src/lib.rs | 6 +- client/finality-grandpa/src/authorities.rs | 77 ++++++++++++++++++- .../src/authority_set_changes.rs | 49 ------------ client/finality-grandpa/src/aux_schema.rs | 37 ++------- client/finality-grandpa/src/environment.rs | 23 ------ client/finality-grandpa/src/finality_proof.rs | 43 ++++++++--- client/finality-grandpa/src/import.rs | 6 -- client/finality-grandpa/src/lib.rs | 4 - client/finality-grandpa/src/observer.rs | 5 -- client/finality-grandpa/src/tests.rs | 8 +- 12 files changed, 142 insertions(+), 146 deletions(-) delete mode 100644 client/finality-grandpa/src/authority_set_changes.rs diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index 90187061c9cf7..f1732c9726501 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -87,8 +87,13 @@ pub fn new_full(config: Configuration) -> Result { other: (block_import, grandpa_link), } = new_partial(&config)?; - let finality_proof_provider = - GrandpaFinalityProofProvider::new_for_service(backend.clone(), client.clone()); + let shared_authority_set = grandpa_link.shared_authority_set().clone(); + + let finality_proof_provider = GrandpaFinalityProofProvider::new_for_service( + backend.clone(), + client.clone(), + Some(shared_authority_set), + ); let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { @@ -255,8 +260,11 @@ pub fn new_light(config: Configuration) -> Result { sp_consensus::NeverCanAuthor, )?; - let finality_proof_provider = - GrandpaFinalityProofProvider::new_for_service(backend.clone(), client.clone()); + let finality_proof_provider = GrandpaFinalityProofProvider::new_for_service( + backend.clone(), + client.clone(), + None, + ); let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index ecf50dc14634b..05bb2d27d9731 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -110,8 +110,11 @@ pub fn new_partial(config: &Configuration) -> Result Result<( sp_consensus::NeverCanAuthor, )?; - let finality_proof_provider = - GrandpaFinalityProofProvider::new_for_service(backend.clone(), client.clone()); + let finality_proof_provider = GrandpaFinalityProofProvider::new_for_service( + backend.clone(), + client.clone(), + None, + ); let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 163fb4b6b212f..99c21b84bc9c7 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -83,7 +83,7 @@ pub trait GrandpaApi { ) -> jsonrpc_core::Result; /// Prove finality for the given block number. - /// WIP: expand this + /// WIP(JON): expand this #[rpc(name = "grandpa_proveFinality")] fn prove_finality( &self, @@ -172,9 +172,7 @@ where &self, block: NumberFor, ) -> FutureResult> { - let result = self - .finality_proof_provider - .rpc_prove_finality(block); + let result = self.finality_proof_provider.rpc_prove_finality(block); let future = async move { result }.boxed(); Box::new( future diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 2de169fc8285a..7cbc16b472572 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -114,6 +114,11 @@ where N: Add + Ord + Clone + Debug, pub fn clone_inner(&self) -> AuthoritySet { self.inner.read().clone() } + + /// WIP(JON) + pub fn authority_set_changes(&self) -> AuthoritySetChanges { + self.inner.read().authority_set_changes.clone() + } } impl From> for SharedAuthoritySet { @@ -152,12 +157,14 @@ pub struct AuthoritySet { /// is lower than the last finalized block (as signaled in the forced /// change) must be applied beforehand. pending_forced_changes: Vec>, + // WIP(JON) + pub(crate) authority_set_changes: AuthoritySetChanges, } impl AuthoritySet where H: PartialEq, - N: Ord, + N: Ord + Clone, { // authority sets must be non-empty and all weights must be greater than 0 fn invalid_authority_list(authorities: &AuthorityList) -> bool { @@ -175,6 +182,7 @@ where set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }) } @@ -184,6 +192,7 @@ where set_id: u64, pending_standard_changes: ForkTree>, pending_forced_changes: Vec>, + authority_set_changes: AuthoritySetChanges, ) -> Option { if Self::invalid_authority_list(&authorities) { return None; @@ -194,6 +203,7 @@ where set_id, pending_standard_changes, pending_forced_changes, + authority_set_changes, }) } @@ -461,6 +471,7 @@ where set_id: self.set_id + 1, pending_standard_changes: ForkTree::new(), // new set, new changes. pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }, )); @@ -532,6 +543,10 @@ where "block" => ?change.canon_height ); + // WIP(JON) + // Store the set_id together with the last block_number + self.authority_set_changes.append(finalized_number.clone(), self.set_id); + self.current_authorities = change.next_authorities; self.set_id += 1; @@ -631,6 +646,43 @@ impl + Clone> PendingChange { } } +// Tracks authority set changes. We store the block numbers for the first block of each authority +// set. +#[derive(Debug, Encode, Decode, Clone, PartialEq)] +pub struct AuthoritySetChanges { + // WIP(JON): reconsider this choice of container + // (block_number, set_id) + authority_set_changes: Vec<(N, u64)>, +} + +impl AuthoritySetChanges { + pub(crate) fn empty() -> Self { + Self { + authority_set_changes: Default::default(), + } + } + + pub(crate) fn append(&mut self, block_number: N, set_id: u64) { + println!("JON: AuthoritySetChanges::append()"); + // dbg!(&number); + // self.authority_set_changes.insert(block_number, set_id); + self.authority_set_changes.push((block_number, set_id)); + } + + pub(crate) fn get_set_id(&self, block_number: N) -> Option<(N, u64)> { + // self.authority_set_changes.range(block_number..).next() + // WIP(JON): just happy unwrap + let idx = self.authority_set_changes + .binary_search_by_key(&block_number, |(b, _n)| b.clone()) + .unwrap_or_else(|b| b); + if idx < self.authority_set_changes.len() { + Some(self.authority_set_changes[idx].clone()) + } else { + None + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -657,6 +709,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let change = |height| { @@ -704,6 +757,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let change_a = PendingChange { @@ -771,6 +825,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)]; @@ -846,6 +901,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)]; @@ -924,6 +980,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)]; @@ -990,6 +1047,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)]; @@ -1073,6 +1131,7 @@ mod tests { set_id: 1, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }, ) ); @@ -1086,6 +1145,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)]; @@ -1124,6 +1184,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; // effective at #15 @@ -1210,6 +1271,7 @@ mod tests { set_id: 3, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), } ), ); @@ -1224,6 +1286,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let new_set = current_authorities.clone(); @@ -1342,7 +1405,13 @@ mod tests { // empty authority lists are invalid assert_eq!(AuthoritySet::<(), ()>::genesis(vec![]), None); assert_eq!( - AuthoritySet::<(), ()>::new(vec![], 0, ForkTree::new(), Vec::new()), + AuthoritySet::<(), ()>::new( + vec![], + 0, + ForkTree::new(), + Vec::new(), + AuthoritySetChanges::empty(), + ), None, ); @@ -1361,7 +1430,8 @@ mod tests { invalid_authorities_weight.clone(), 0, ForkTree::new(), - Vec::new() + Vec::new(), + AuthoritySetChanges::empty(), ), None, ); @@ -1416,6 +1486,7 @@ mod tests { set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + authority_set_changes: AuthoritySetChanges::empty(), }; let new_set = current_authorities.clone(); diff --git a/client/finality-grandpa/src/authority_set_changes.rs b/client/finality-grandpa/src/authority_set_changes.rs deleted file mode 100644 index da6f4bead3390..0000000000000 --- a/client/finality-grandpa/src/authority_set_changes.rs +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2020 Parity Technologies (UK) Ltd. -// This file is part of Substrate. - -// Substrate is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Substrate is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Substrate. If not, see . - -use parity_scale_codec::{Decode, Encode}; -use std::{cmp::Ord, sync::Arc}; - -// Tracks authority set changes. We store the block numbers for the last block of each authority -// set. -#[derive(Debug, Encode, Decode)] -pub(crate) struct AuthoritySetChanges { - authority_set_changes: Vec, -} - -impl AuthoritySetChanges { - pub(crate) fn empty() -> Self { - Self { - authority_set_changes: Vec::new(), - } - } - - pub(crate) fn append(&mut self, number: N) { - self.authority_set_changes.push(number) - } - - pub(crate) fn get_set_id(&self, number: N) -> (u64, N) { - let set_id = self - .authority_set_changes - .binary_search(&number) - .unwrap_or_else(|idx| idx); - let last_block_for_set_id = self.authority_set_changes[set_id]; - // WIP: avoid cast? - (set_id as u64, last_block_for_set_id) - } -} - -pub(crate) type SharedAuthoritySetChanges = Arc>>; diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 8ef0b94f93d3a..35e2b82189670 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -27,8 +27,9 @@ use sp_runtime::traits::{Block as BlockT, NumberFor}; use log::{info, warn}; use sp_finality_grandpa::{AuthorityList, SetId, RoundNumber}; -use crate::authorities::{AuthoritySet, SharedAuthoritySet, PendingChange, DelayKind}; -use crate::authority_set_changes::{AuthoritySetChanges, SharedAuthoritySetChanges}; +use crate::authorities::{ + AuthoritySet, AuthoritySetChanges, SharedAuthoritySet, PendingChange, DelayKind, +}; use crate::consensus_changes::{SharedConsensusChanges, ConsensusChanges}; use crate::environment::{ CompletedRound, CompletedRounds, CurrentRounds, HasVoted, SharedVoterSetState, VoterSetState, @@ -40,7 +41,6 @@ const SET_STATE_KEY: &[u8] = b"grandpa_completed_round"; const CONCLUDED_ROUNDS: &[u8] = b"grandpa_concluded_rounds"; const AUTHORITY_SET_KEY: &[u8] = b"grandpa_voters"; const CONSENSUS_CHANGES_KEY: &[u8] = b"grandpa_consensus_changes"; -const AUTHORITY_SET_CHANGES_KEY: &[u8] = b"grandpa_authority_set_changes"; const CURRENT_VERSION: u32 = 2; @@ -104,6 +104,7 @@ where H: Clone + Debug + PartialEq, self.set_id, pending_standard_changes, Vec::new(), + AuthoritySetChanges::empty(), ); authority_set.expect("current_authorities is non-empty and weights are non-zero; qed.") @@ -124,7 +125,6 @@ pub(crate) fn load_decode(backend: &B, key: &[u8]) -> Cl /// Persistent data kept between runs. pub(crate) struct PersistentData { pub(crate) authority_set: SharedAuthoritySet>, - pub(crate) authority_set_changes: SharedAuthoritySetChanges>, pub(crate) consensus_changes: SharedConsensusChanges>, pub(crate) set_state: SharedVoterSetState, } @@ -277,8 +277,6 @@ pub(crate) fn load_persistent( let version: Option = load_decode(backend, VERSION_KEY)?; let consensus_changes = load_decode(backend, CONSENSUS_CHANGES_KEY)? .unwrap_or_else(ConsensusChanges::>::empty); - let authority_set_changes = load_decode(backend, AUTHORITY_SET_CHANGES_KEY)? - .unwrap_or_else(AuthoritySetChanges::>::empty); let make_genesis_round = move || RoundState::genesis((genesis_hash, genesis_number)); @@ -287,7 +285,6 @@ pub(crate) fn load_persistent( if let Some((new_set, set_state)) = migrate_from_version0::(backend, &make_genesis_round)? { return Ok(PersistentData { authority_set: new_set.into(), - authority_set_changes: Arc::new(authority_set_changes.into()), consensus_changes: Arc::new(consensus_changes.into()), set_state: set_state.into(), }); @@ -297,7 +294,6 @@ pub(crate) fn load_persistent( if let Some((new_set, set_state)) = migrate_from_version1::(backend, &make_genesis_round)? { return Ok(PersistentData { authority_set: new_set.into(), - authority_set_changes: Arc::new(authority_set_changes.into()), consensus_changes: Arc::new(consensus_changes.into()), set_state: set_state.into(), }); @@ -328,7 +324,6 @@ pub(crate) fn load_persistent( return Ok(PersistentData { authority_set: set.into(), - authority_set_changes: Arc::new(authority_set_changes.into()), consensus_changes: Arc::new(consensus_changes.into()), set_state: set_state.into(), }); @@ -366,7 +361,6 @@ pub(crate) fn load_persistent( Ok(PersistentData { authority_set: genesis_set.into(), - authority_set_changes: Arc::new(authority_set_changes.into()), set_state: genesis_state.into(), consensus_changes: Arc::new(consensus_changes.into()), }) @@ -430,20 +424,6 @@ pub(crate) fn write_concluded_round( backend.insert_aux(&[(&key[..], round_data.encode().as_slice())], &[]) } -pub(crate) fn update_authority_set_changes( - authority_set_changes: &AuthoritySetChanges, - write_aux: F, -) -> R -where - N: Encode, - F: FnOnce(&[(&'static [u8], &[u8])]) -> R, -{ - write_aux(&[( - AUTHORITY_SET_CHANGES_KEY, - authority_set_changes.encode().as_slice() - )]) -} - /// Update the consensus changes. pub(crate) fn update_consensus_changes( set: &ConsensusChanges, @@ -456,12 +436,6 @@ pub(crate) fn update_consensus_changes( write_aux(&[(CONSENSUS_CHANGES_KEY, set.encode().as_slice())]) } -pub(crate) fn load_authority_set_changes(backend: &B) - -> ClientResult>>> -{ - load_decode(backend, AUTHORITY_SET_CHANGES_KEY) -} - #[cfg(test)] pub(crate) fn load_authorities(backend: &B) -> Option> { @@ -540,6 +514,7 @@ mod test { set_id, ForkTree::new(), Vec::new(), + AuthoritySetChanges::empty(), ).unwrap(), ); @@ -584,6 +559,7 @@ mod test { set_id, ForkTree::new(), Vec::new(), + AuthoritySetChanges::empty(), ).unwrap(); let voter_set_state = V1VoterSetState::Live(round_number, round_state.clone()); @@ -630,6 +606,7 @@ mod test { set_id, ForkTree::new(), Vec::new(), + AuthoritySetChanges::empty(), ).unwrap(), ); diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 3839e3a639e79..21566b07f3a8d 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -48,7 +48,6 @@ use crate::{ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; -use crate::authority_set_changes::SharedAuthoritySetChanges; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; use crate::notification::GrandpaJustificationSender; @@ -417,7 +416,6 @@ pub(crate) struct Environment, SC, pub(crate) config: Config, pub(crate) authority_set: SharedAuthoritySet>, pub(crate) consensus_changes: SharedConsensusChanges>, - pub(crate) authority_set_changes: SharedAuthoritySetChanges>, pub(crate) network: crate::communication::NetworkBridge, pub(crate) set_id: SetId, pub(crate) voter_set_state: SharedVoterSetState, @@ -1080,7 +1078,6 @@ where finalize_block( self.client.clone(), &self.authority_set, - &self.authority_set_changes, &self.consensus_changes, Some(self.config.justification_period.into()), hash, @@ -1146,7 +1143,6 @@ impl From> for JustificationOrCommit< pub(crate) fn finalize_block( client: Arc, authority_set: &SharedAuthoritySet>, - authority_set_changes: &SharedAuthoritySetChanges>, consensus_changes: &SharedConsensusChanges>, justification_period: Option>, hash: Block::Hash, @@ -1186,8 +1182,6 @@ where // reverting in case of failure let mut old_consensus_changes = None; - let mut authority_set_changes = authority_set_changes.lock(); - let mut consensus_changes = consensus_changes.lock(); let canon_at_height = |canon_number| { // "true" because the block is finalized @@ -1305,23 +1299,6 @@ where // the authority set has changed. let (new_id, set_ref) = authority_set.current(); - // Persist the number of the last block of the session - if number > NumberFor::::zero() { - let parent_number = number - NumberFor::::one(); - authority_set_changes.append(parent_number); - let write_result = crate::aux_schema::update_authority_set_changes( - &*authority_set_changes, - |insert| apply_aux(import_op, insert, &[]), - ); - if let Err(e) = write_result { - warn!( - target: "afg", - "Failed to write updated authority set changes to disk: {}", - e - ); - } - } - if set_ref.len() > 16 { afg_log!(initial_sync, "👴 Applying GRANDPA set change to new set with {} authorities", diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 70512633174ca..92a25ba899a98 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -59,6 +59,7 @@ use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GR use crate::justification::GrandpaJustification; use crate::VoterSet; +use crate::SharedAuthoritySet; /// Maximum number of fragments that we want to return in a single prove_finality call. const MAX_FRAGMENTS_IN_PROOF: usize = 8; @@ -154,6 +155,7 @@ impl AuthoritySetForFinalityChecker for Arc { backend: Arc, authority_provider: Arc>, + shared_authority_set: Option>>, } impl FinalityProofProvider @@ -166,10 +168,15 @@ impl FinalityProofProvider pub fn new

( backend: Arc, authority_provider: P, + shared_authority_set: Option>>, ) -> Self where P: AuthoritySetForFinalityProver + 'static, { - FinalityProofProvider { backend, authority_provider: Arc::new(authority_provider) } + FinalityProofProvider { + backend, + authority_provider: Arc::new(authority_provider), + shared_authority_set, + } } /// Create new finality proof provider for the service using: @@ -179,8 +186,13 @@ impl FinalityProofProvider pub fn new_for_service( backend: Arc, storage_and_proof_provider: Arc>, + shared_authority_set: Option>>, ) -> Arc { - Arc::new(Self::new(backend, storage_and_proof_provider)) + Arc::new(Self::new( + backend, + storage_and_proof_provider, + shared_authority_set, + )) } } @@ -191,21 +203,30 @@ impl FinalityProofProvider B: Backend + Send + Sync + 'static, { /// Prove finality for the given block number. - /// WIP: expand this description + /// WIP(JON): expand this description pub fn prove_finality( &self, block: NumberFor, ) -> Result>, ClientError> { - let blocks_where_set_changes = - match crate::aux_schema::load_authority_set_changes::<_, Block>(&*self.backend)? { - Some(blocks) => blocks, - None => return Ok(None), - }; - let (set_id, last_block_for_set) = blocks_where_set_changes.get_set_id(block); + let (last_block_for_set, set_id) = match self.shared_authority_set + .as_ref() + .map(SharedAuthoritySet::authority_set_changes) + .and_then(|changes| changes.get_set_id(block)) + { + Some((last_block_for_set, set_id)) => (last_block_for_set, set_id), + None => return Ok(None), + }; // Convert from block numbers to hashes - let last_block_for_set = self.backend.blockchain().hash(last_block_for_set).unwrap().unwrap(); - let block = self.backend.blockchain().hash(block).unwrap().unwrap(); + let block = match self.backend.blockchain().hash(block)? { + Some(block) => block, + None => return Ok(None), + }; + + let last_block_for_set = match self.backend.blockchain().hash(last_block_for_set)? { + Some(block) => block, + None => return Ok(None), + }; prove_finality::<_, _, GrandpaJustification>( &*self.backend.blockchain(), diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 56fe5d6547756..04df95a3187e1 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -41,7 +41,6 @@ use sp_runtime::traits::{ use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand}; use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingChange}; -use crate::authority_set_changes::SharedAuthoritySetChanges; use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; @@ -61,7 +60,6 @@ pub struct GrandpaBlockImport { inner: Arc, select_chain: SC, authority_set: SharedAuthoritySet>, - authority_set_changes: SharedAuthoritySetChanges>, send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, @@ -77,7 +75,6 @@ impl Clone for inner: self.inner.clone(), select_chain: self.select_chain.clone(), authority_set: self.authority_set.clone(), - authority_set_changes: self.authority_set_changes.clone(), send_voter_commands: self.send_voter_commands.clone(), consensus_changes: self.consensus_changes.clone(), authority_set_hard_forks: self.authority_set_hard_forks.clone(), @@ -563,7 +560,6 @@ impl GrandpaBlockImport, select_chain: SC, authority_set: SharedAuthoritySet>, - authority_set_changes: SharedAuthoritySetChanges>, send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, @@ -608,7 +604,6 @@ impl GrandpaBlockImport finality_grandpa::Chain> fn grandpa_observer( client: &Arc, authority_set: &SharedAuthoritySet>, - authority_set_changes: &SharedAuthoritySetChanges>, consensus_changes: &SharedConsensusChanges>, voters: &Arc>, justification_sender: &Option>, @@ -85,7 +83,6 @@ where Client: crate::ClientForGrandpa, { let authority_set = authority_set.clone(); - let authority_set_changes = authority_set_changes.clone(); let consensus_changes = consensus_changes.clone(); let client = client.clone(); let voters = voters.clone(); @@ -126,7 +123,6 @@ where match environment::finalize_block( client.clone(), &authority_set, - &authority_set_changes, &consensus_changes, None, finalized_hash, @@ -297,7 +293,6 @@ where let observer = grandpa_observer( &self.client, &self.persistent_data.authority_set, - &self.persistent_data.authority_set_changes, &self.persistent_data.consensus_changes, &voters, &self.justification_sender, diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 5f057d0bcaa57..15007a59b034d 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -171,7 +171,11 @@ impl TestNetFactory for GrandpaTestNet { ) -> Option>> { match client { PeersClient::Full(_, ref backend) => { - Some(Arc::new(FinalityProofProvider::new(backend.clone(), self.test_config.clone()))) + Some(Arc::new(FinalityProofProvider::new( + backend.clone(), + self.test_config.clone(), + None, + ))) }, PeersClient::Light(_, _) => None, } @@ -1542,7 +1546,6 @@ where { let PersistentData { ref authority_set, - ref authority_set_changes, ref consensus_changes, ref set_state, .. @@ -1566,7 +1569,6 @@ where Environment { authority_set: authority_set.clone(), - authority_set_changes: authority_set_changes.clone(), config: config.clone(), consensus_changes: consensus_changes.clone(), client: link.client.clone(), From df5cca156ade665d1214ff8a58da867f2e24056d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 12 Nov 2020 00:07:03 +0100 Subject: [PATCH 08/28] grandpa: manual impl of Decode for AuthoritySet --- client/finality-grandpa/src/authorities.rs | 43 +++++++++++++++++++++- client/finality-grandpa/src/aux_schema.rs | 2 +- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 7cbc16b472572..3a386885fff70 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -21,7 +21,7 @@ use fork_tree::ForkTree; use parking_lot::RwLock; use finality_grandpa::voter_set::VoterSet; -use parity_scale_codec::{Encode, Decode}; +use parity_scale_codec::{Encode, Decode, Input}; use log::debug; use sc_telemetry::{telemetry, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList}; @@ -137,8 +137,18 @@ pub(crate) struct Status { pub(crate) new_set_block: Option<(H, N)>, } -/// A set of authorities. +// Same as `AuthoritySet`, but without the last field `authority_set_changes`. Only used during +// decoding. #[derive(Debug, Clone, Encode, Decode, PartialEq)] +struct LegacyAuthoritySet { + current_authorities: AuthorityList, + set_id: u64, + pending_standard_changes: ForkTree>, + pending_forced_changes: Vec>, +} + +/// A set of authorities. +#[derive(Debug, Clone, Encode, PartialEq)] pub struct AuthoritySet { /// The current active authorities. pub(crate) current_authorities: AuthorityList, @@ -161,6 +171,35 @@ pub struct AuthoritySet { pub(crate) authority_set_changes: AuthoritySetChanges, } +impl Decode for AuthoritySet +where + H: Decode, + N: Decode + Clone + Ord +{ + fn decode(value: &mut I) -> Result { + let legacy = LegacyAuthoritySet::decode(value)?; + let authority_set_changes = match >::decode(value) { + Ok(v) => v, + Err(_) => AuthoritySetChanges::empty(), + }; + + let LegacyAuthoritySet { + current_authorities, + set_id, + pending_standard_changes, + pending_forced_changes, + } = legacy; + + Ok(AuthoritySet { + current_authorities, + set_id, + pending_standard_changes, + pending_forced_changes, + authority_set_changes, + }) + } +} + impl AuthoritySet where H: PartialEq, diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 35e2b82189670..d17ce9ca01e3c 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -437,7 +437,7 @@ pub(crate) fn update_consensus_changes( } #[cfg(test)] -pub(crate) fn load_authorities(backend: &B) +pub(crate) fn load_authorities(backend: &B) -> Option> { load_decode::<_, AuthoritySet>(backend, AUTHORITY_SET_KEY) .expect("backend error") From 6ed408a4058692ffa8a89da4d28524529ce4af57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 12 Nov 2020 12:39:44 +0100 Subject: [PATCH 09/28] grandpa: add comment about appending changes for forced changes --- client/finality-grandpa/src/authorities.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 3a386885fff70..98ab9be09e6dd 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -503,6 +503,11 @@ where "block" => ?change.canon_height ); + // WIP(JON) + // We should store the set_id for this, but what do we use as finalized_number? + // Maybe none as it seems it wont be used for Justifications anyway? + //self.authority_set_changes.append(finalized_number.clone(), self.set_id); + new_set = Some(( median_last_finalized, AuthoritySet { From bde6188a14d84eca6fabe935490bac93568b5125 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 12 Nov 2020 14:01:32 +0100 Subject: [PATCH 10/28] grandpa: flip order in set changes, tidy up some comments --- client/finality-grandpa/rpc/src/lib.rs | 2 +- client/finality-grandpa/src/authorities.rs | 36 ++++++++----------- client/finality-grandpa/src/finality_proof.rs | 4 +-- 3 files changed, 18 insertions(+), 24 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 99c21b84bc9c7..251eff8605220 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -83,7 +83,7 @@ pub trait GrandpaApi { ) -> jsonrpc_core::Result; /// Prove finality for the given block number. - /// WIP(JON): expand this + /// WIP(JON): expand this description #[rpc(name = "grandpa_proveFinality")] fn prove_finality( &self, diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 98ab9be09e6dd..2fb9e74920350 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -115,7 +115,7 @@ where N: Add + Ord + Clone + Debug, self.inner.read().clone() } - /// WIP(JON) + /// WIP(JON) expand this description pub fn authority_set_changes(&self) -> AuthoritySetChanges { self.inner.read().authority_set_changes.clone() } @@ -167,7 +167,7 @@ pub struct AuthoritySet { /// is lower than the last finalized block (as signaled in the forced /// change) must be applied beforehand. pending_forced_changes: Vec>, - // WIP(JON) + // WIP(JON) expand this description pub(crate) authority_set_changes: AuthoritySetChanges, } @@ -503,10 +503,10 @@ where "block" => ?change.canon_height ); - // WIP(JON) - // We should store the set_id for this, but what do we use as finalized_number? - // Maybe none as it seems it wont be used for Justifications anyway? - //self.authority_set_changes.append(finalized_number.clone(), self.set_id); + // WIP(JON): We should store the set_id for this, but what do we use as + // finalized_number? Maybe none as it seems it wont be used for Justifications + // anyway? + //self.authority_set_changes.append(self.set_id, finalized_number.clone()); new_set = Some(( median_last_finalized, @@ -587,9 +587,8 @@ where "block" => ?change.canon_height ); - // WIP(JON) - // Store the set_id together with the last block_number - self.authority_set_changes.append(finalized_number.clone(), self.set_id); + // Store the set_id together with the last block_number for the set + self.authority_set_changes.append(self.set_id, finalized_number.clone()); self.current_authorities = change.next_authorities; self.set_id += 1; @@ -694,9 +693,7 @@ impl + Clone> PendingChange { // set. #[derive(Debug, Encode, Decode, Clone, PartialEq)] pub struct AuthoritySetChanges { - // WIP(JON): reconsider this choice of container - // (block_number, set_id) - authority_set_changes: Vec<(N, u64)>, + authority_set_changes: Vec<(u64, N)>, // (set_id, block_number) } impl AuthoritySetChanges { @@ -706,20 +703,17 @@ impl AuthoritySetChanges { } } - pub(crate) fn append(&mut self, block_number: N, set_id: u64) { - println!("JON: AuthoritySetChanges::append()"); - // dbg!(&number); - // self.authority_set_changes.insert(block_number, set_id); - self.authority_set_changes.push((block_number, set_id)); + pub(crate) fn append(&mut self, set_id: u64, block_number: N) { + self.authority_set_changes.push((set_id, block_number)); } - pub(crate) fn get_set_id(&self, block_number: N) -> Option<(N, u64)> { - // self.authority_set_changes.range(block_number..).next() - // WIP(JON): just happy unwrap + pub(crate) fn get_set_id(&self, block_number: N) -> Option<(u64, N)> { let idx = self.authority_set_changes - .binary_search_by_key(&block_number, |(b, _n)| b.clone()) + .binary_search_by_key(&block_number, |(_set_id, n)| n.clone()) .unwrap_or_else(|b| b); if idx < self.authority_set_changes.len() { + // WIP(JON) remove assert + assert!(self.authority_set_changes[idx].clone().1 > block_number); Some(self.authority_set_changes[idx].clone()) } else { None diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 92a25ba899a98..65ce9fde064e3 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -208,12 +208,12 @@ impl FinalityProofProvider &self, block: NumberFor, ) -> Result>, ClientError> { - let (last_block_for_set, set_id) = match self.shared_authority_set + let (set_id, last_block_for_set) = match self.shared_authority_set .as_ref() .map(SharedAuthoritySet::authority_set_changes) .and_then(|changes| changes.get_set_id(block)) { - Some((last_block_for_set, set_id)) => (last_block_for_set, set_id), + Some((set_id, last_block_for_set)) => (set_id, last_block_for_set), None => return Ok(None), }; From ec52b6f1032f87bef83d1d0aee015b971dfde04b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 12 Nov 2020 17:01:54 +0100 Subject: [PATCH 11/28] grandpa: update some of the doc comments --- client/finality-grandpa/rpc/src/lib.rs | 4 ++-- client/finality-grandpa/src/authorities.rs | 8 +++----- client/finality-grandpa/src/finality_proof.rs | 4 ++-- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 251eff8605220..44f04828f0acf 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -82,8 +82,8 @@ pub trait GrandpaApi { id: SubscriptionId ) -> jsonrpc_core::Result; - /// Prove finality for the given block number. - /// WIP(JON): expand this description + /// Prove finality for the given block number by returning the Justification for the last block + /// in the set. #[rpc(name = "grandpa_proveFinality")] fn prove_finality( &self, diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 2fb9e74920350..cca32363282aa 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -115,7 +115,7 @@ where N: Add + Ord + Clone + Debug, self.inner.read().clone() } - /// WIP(JON) expand this description + /// Clone the inner `AuthoritySetChanges`. pub fn authority_set_changes(&self) -> AuthoritySetChanges { self.inner.read().authority_set_changes.clone() } @@ -167,8 +167,8 @@ pub struct AuthoritySet { /// is lower than the last finalized block (as signaled in the forced /// change) must be applied beforehand. pending_forced_changes: Vec>, - // WIP(JON) expand this description - pub(crate) authority_set_changes: AuthoritySetChanges, + /// Track at which blocks the set id changes. This is useful when we need to prove finality. + authority_set_changes: AuthoritySetChanges, } impl Decode for AuthoritySet @@ -712,8 +712,6 @@ impl AuthoritySetChanges { .binary_search_by_key(&block_number, |(_set_id, n)| n.clone()) .unwrap_or_else(|b| b); if idx < self.authority_set_changes.len() { - // WIP(JON) remove assert - assert!(self.authority_set_changes[idx].clone().1 > block_number); Some(self.authority_set_changes[idx].clone()) } else { None diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 65ce9fde064e3..536cf236697d3 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -202,8 +202,8 @@ impl FinalityProofProvider NumberFor: BlockNumberOps, B: Backend + Send + Sync + 'static, { - /// Prove finality for the given block number. - /// WIP(JON): expand this description + /// 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, From efcd04b127c1a00e8fcd2946c87b7c967ade416e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 19 Nov 2020 16:59:25 +0100 Subject: [PATCH 12/28] grandpa: store authority set changes when applying forced changes --- client/finality-grandpa/src/authorities.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index cca32363282aa..defe2ab160a79 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -441,7 +441,7 @@ where /// forced change), then the forced change cannot be applied. An error will /// be returned in that case which will prevent block import. pub(crate) fn apply_forced_changes( - &self, + &mut self, best_hash: H, best_number: N, is_descendent_of: &F, @@ -503,10 +503,7 @@ where "block" => ?change.canon_height ); - // WIP(JON): We should store the set_id for this, but what do we use as - // finalized_number? Maybe none as it seems it wont be used for Justifications - // anyway? - //self.authority_set_changes.append(self.set_id, finalized_number.clone()); + self.authority_set_changes.append(self.set_id, median_last_finalized.clone()); new_set = Some(( median_last_finalized, From 62a724552887726467bf0df744aed53992500b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 20 Nov 2020 14:34:18 +0100 Subject: [PATCH 13/28] grandpa: simplify finality_proof.rs --- client/finality-grandpa/rpc/src/lib.rs | 31 +- client/finality-grandpa/src/finality_proof.rs | 783 +++--------------- client/finality-grandpa/src/lib.rs | 2 +- client/finality-grandpa/src/tests.rs | 46 +- 4 files changed, 132 insertions(+), 730 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 44f04828f0acf..c39f498f77203 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -196,7 +196,7 @@ mod tests { use sc_block_builder::BlockBuilder; use sc_finality_grandpa::{ report, AuthorityId, GrandpaJustificationSender, GrandpaJustification, - FinalityProofFragment, + FinalityProof, }; use sp_blockchain::HeaderBackend; use sp_consensus::RecordProof; @@ -215,7 +215,7 @@ mod tests { struct EmptyVoterState; struct TestFinalityProofProvider { - finality_proofs: Vec>, + finality_proof: Option>, } fn voters() -> HashSet { @@ -256,7 +256,14 @@ mod tests { &self, _block: NumberFor ) -> Result, sp_blockchain::Error> { - Ok(Some(EncodedFinalityProofs(self.finality_proofs.encode().into()))) + // Don't call this without setting the FinalityProof + Ok(Some(EncodedFinalityProofs( + self.finality_proof + .as_ref() + .expect("Don't call rpc_prove_finality without setting the FinalityProof") + .encode() + .into() + ))) } } @@ -298,12 +305,12 @@ mod tests { ) where VoterState: ReportVoterState + Send + Sync + 'static, { - setup_io_handler_with_finality_proofs(voter_state, Default::default()) + setup_io_handler_with_finality_proofs(voter_state, None) } fn setup_io_handler_with_finality_proofs( voter_state: VoterState, - finality_proofs: Vec>, + finality_proof: Option>, ) -> ( jsonrpc_core::MetaIoHandler, GrandpaJustificationSender, @@ -311,7 +318,7 @@ mod tests { VoterState: ReportVoterState + Send + Sync + 'static, { let (justification_sender, justification_stream) = GrandpaJustificationStream::channel(); - let finality_proof_provider = Arc::new(TestFinalityProofProvider { finality_proofs }); + let finality_proof_provider = Arc::new(TestFinalityProofProvider { finality_proof }); let handler = GrandpaRpcHandler::new( TestAuthoritySet, @@ -510,15 +517,14 @@ mod tests { #[test] fn prove_finality_with_test_finality_proof_provider() { - let finality_proofs = vec![FinalityProofFragment { + let finality_proof = FinalityProof { block: header(42).hash(), justification: create_justification().encode(), unknown_headers: vec![header(2)], - authorities_proof: None, - }]; + }; let (io, _) = setup_io_handler_with_finality_proofs( TestVoterState, - finality_proofs.clone(), + Some(finality_proof.clone()), ); let request = @@ -528,8 +534,7 @@ mod tests { let resp = io.handle_request_sync(request, meta); let mut resp: serde_json::Value = serde_json::from_str(&resp.unwrap()).unwrap(); let result: sp_core::Bytes = serde_json::from_value(resp["result"].take()).unwrap(); - let fragments: Vec> = - Decode::decode(&mut &result[..]).unwrap(); - assert_eq!(fragments, finality_proofs); + let fragments: FinalityProof

= Decode::decode(&mut &result[..]).unwrap(); + assert_eq!(fragments, finality_proof); } } diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 7e3d0d8a6a4fd..893f3d700cb3e 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -16,9 +16,6 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -// NOTE: should be removed with: https://github.com/paritytech/substrate/pull/7339 -#![allow(dead_code)] - //! GRANDPA block finality proof generation and check. //! //! Finality of block B is proved by providing: @@ -43,13 +40,9 @@ use std::sync::Arc; use log::trace; use sp_blockchain::{ - Backend as BlockchainBackend, Error as ClientError, HeaderBackend, Result as ClientResult, -}; -use sc_client_api::{ - backend::Backend, StorageProof, - light::{FetchChecker, RemoteReadRequest}, - StorageProvider, ProofProvider, + Backend as BlockchainBackend, Error as ClientError, Result as ClientResult, }; +use sc_client_api::{backend::Backend, StorageProvider, ProofProvider}; use parity_scale_codec::{Encode, Decode}; use finality_grandpa::BlockNumberOps; use sp_runtime::{ @@ -57,22 +50,15 @@ use sp_runtime::{ traits::{NumberFor, Block as BlockT, Header as HeaderT, One}, }; use sp_core::storage::StorageKey; -use sc_telemetry::{telemetry, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GRANDPA_AUTHORITIES_KEY}; use crate::justification::GrandpaJustification; use crate::VoterSet; use crate::SharedAuthoritySet; -/// Maximum number of fragments that we want to return in a single prove_finality call. -const MAX_FRAGMENTS_IN_PROOF: usize = 8; - -/// GRANDPA authority set related methods for the finality proof provider. pub trait AuthoritySetForFinalityProver: Send + Sync { /// Read GRANDPA_AUTHORITIES_KEY from storage at given block. fn authorities(&self, block: &BlockId) -> ClientResult; - /// Prove storage read of GRANDPA_AUTHORITIES_KEY at given block. - fn prove_authorities(&self, block: &BlockId) -> ClientResult; } /// Trait that combines `StorageProvider` and `ProofProvider` @@ -102,58 +88,7 @@ impl AuthoritySetForFinalityProver for Arc) -> ClientResult { - self.read_proof(block, &mut std::iter::once(GRANDPA_AUTHORITIES_KEY)) - } } - -/// GRANDPA authority set related methods for the finality proof checker. -pub trait AuthoritySetForFinalityChecker: Send + Sync { - /// Check storage read proof of GRANDPA_AUTHORITIES_KEY at given block. - fn check_authorities_proof( - &self, - hash: Block::Hash, - header: Block::Header, - proof: StorageProof, - ) -> ClientResult; -} - -/// FetchChecker-based implementation of AuthoritySetForFinalityChecker. -impl AuthoritySetForFinalityChecker for Arc> { - fn check_authorities_proof( - &self, - hash: Block::Hash, - header: Block::Header, - proof: StorageProof, - ) -> ClientResult { - let storage_key = GRANDPA_AUTHORITIES_KEY.to_vec(); - let request = RemoteReadRequest { - block: hash, - header, - keys: vec![storage_key.clone()], - retry_count: None, - }; - - self.check_read_proof(&request, proof) - .and_then(|results| { - let maybe_encoded = results.get(&storage_key) - .expect( - "storage_key is listed in the request keys; \ - check_read_proof must return a value for each requested key; - qed" - ); - maybe_encoded - .as_ref() - .and_then(|encoded| { - VersionedAuthorityList::decode(&mut encoded.as_slice()).ok() - }) - .map(|versioned| versioned.into()) - .ok_or(ClientError::InvalidAuthoritiesSet) - }) - } -} - /// Finality proof provider for serving network requests. pub struct FinalityProofProvider { backend: Arc, @@ -220,17 +155,6 @@ impl FinalityProofProvider None => return Ok(None), }; - // Convert from block numbers to hashes - let block = match self.backend.blockchain().hash(block)? { - Some(block) => block, - None => return Ok(None), - }; - - let last_block_for_set = match self.backend.blockchain().hash(last_block_for_set)? { - Some(block) => block, - None => return Ok(None), - }; - prove_finality::<_, _, GrandpaJustification>( &*self.backend.blockchain(), &*self.authority_provider, @@ -241,350 +165,131 @@ impl FinalityProofProvider } } -/// The effects of block finality. -#[derive(Debug, PartialEq)] -pub struct FinalityEffects { - /// The (ordered) set of headers that could be imported. - pub headers_to_import: Vec
, - /// The hash of the block that could be finalized. - pub block: Header::Hash, - /// The justification for the block. - pub justification: Vec, - /// New authorities set id that should be applied starting from block. - pub new_set_id: u64, - /// New authorities set that should be applied starting from block. - pub new_authorities: AuthorityList, -} - -/// Single fragment of proof-of-finality. -/// /// Finality for block B is proved by providing: /// 1) the justification for the descendant block F; /// 2) headers sub-chain (B; F] if B != F; -/// 3) proof of GRANDPA::authorities() if the set changes at block F. #[derive(Debug, PartialEq, Encode, Decode, Clone)] -pub struct FinalityProofFragment { +pub struct FinalityProof { /// The hash of block F for which justification is provided. pub block: Header::Hash, /// Justification of the block F. pub justification: Vec, - /// The set of headers in the range (U; F] that we believe are unknown to the caller. Ordered. + /// The set of headers in the range (B; F] that we believe are unknown to the caller. Ordered. pub unknown_headers: Vec
, - /// Optional proof of execution of GRANDPA::authorities() at the `block`. - pub authorities_proof: Option, -} - -/// Proof of finality is the ordered set of finality fragments, where: -/// - last fragment provides justification for the best possible block from the requested range; -/// - all other fragments provide justifications for GRANDPA authorities set changes within requested range. -type FinalityProof
= Vec>; - -/// Finality proof request data. -#[derive(Debug, Encode, Decode)] -enum FinalityProofRequest { - /// Original version of the request. - Original(OriginalFinalityProofRequest), -} - -/// Original version of finality proof request. -#[derive(Debug, Encode, Decode)] -struct OriginalFinalityProofRequest { - /// The authorities set id we are waiting proof from. - /// - /// The first justification in the proof must be signed by this authority set. - pub authorities_set_id: u64, - /// Hash of the last known finalized block. - pub last_finalized: H, } -/// Prepare proof-of-finality for the best possible block in the range: (begin; end]. -/// -/// It is assumed that the caller already have a proof-of-finality for the block 'begin'. -/// It is assumed that the caller already knows all blocks in the range (begin; end]. -/// -/// Returns None if there are no finalized blocks unknown to the caller. -pub(crate) fn prove_finality, J>( +fn prove_finality, J>( blockchain: &B, authorities_provider: &dyn AuthoritySetForFinalityProver, authorities_set_id: u64, - begin: Block::Hash, - end: Block::Hash, + block: NumberFor, + last_block_of_the_set: NumberFor, ) -> ::sp_blockchain::Result>> - where - J: ProvableJustification, +where + J: ProvableJustification, { - let begin_id = BlockId::Hash(begin); - let begin_number = blockchain.expect_block_number_from_id(&begin_id)?; - - // early-return if we sure that there are no blocks finalized AFTER begin block + // Early-return if we sure that there are no blocks finalized AFTER begin block let info = blockchain.info(); - if info.finalized_number <= begin_number { + if info.finalized_number <= block { trace!( target: "afg", "Requested finality proof for descendant of #{} while we only have finalized #{}. Returning empty proof.", - begin_number, + block, info.finalized_number, ); return Ok(None); } - // check if blocks range is valid. It is the caller responsibility to ensure - // that it only asks peers that know about whole blocks range - let end_number = blockchain.expect_block_number_from_id(&BlockId::Hash(end))?; - if begin_number + One::one() > end_number { - return Err(ClientError::Backend( - format!("Cannot generate finality proof for invalid range: {}..{}", begin_number, end_number), - )); - } - - // early-return if we sure that the block is NOT a part of canonical chain - let canonical_begin = blockchain.expect_block_hash_from_id(&BlockId::Number(begin_number))?; - if begin != canonical_begin { - return Err(ClientError::Backend( - format!("Cannot generate finality proof for non-canonical block: {}", begin), - )); - } - - // iterate justifications && try to prove finality - let mut fragment_index = 0; - let mut current_authorities = authorities_provider.authorities(&begin_id)?; - let mut current_number = begin_number + One::one(); - let mut finality_proof = Vec::new(); - let mut unknown_headers = Vec::new(); - let mut latest_proof_fragment = None; - let begin_authorities = current_authorities.clone(); - loop { - let current_id = BlockId::Number(current_number); - - // check if header is unknown to the caller - if current_number > end_number { - let unknown_header = blockchain.expect_header(current_id)?; - unknown_headers.push(unknown_header); - } - - if let Some(justification) = blockchain.justification(current_id)? { - // check if the current block enacts new GRANDPA authorities set - let new_authorities = authorities_provider.authorities(¤t_id)?; - let new_authorities_proof = if current_authorities != new_authorities { - current_authorities = new_authorities; - Some(authorities_provider.prove_authorities(¤t_id)?) - } else { - None - }; - - // prepare finality proof for the current block - let current = blockchain.expect_block_hash_from_id(&BlockId::Number(current_number))?; - let proof_fragment = FinalityProofFragment { - block: current, - justification, - unknown_headers: ::std::mem::take(&mut unknown_headers), - authorities_proof: new_authorities_proof, - }; - - // append justification to finality proof if required - let justifies_end_block = current_number >= end_number; - let justifies_authority_set_change = proof_fragment.authorities_proof.is_some(); - if justifies_end_block || justifies_authority_set_change { - // check if the proof is generated by the requested authority set - if finality_proof.is_empty() { - let justification_check_result = J::decode_and_verify( - &proof_fragment.justification, - authorities_set_id, - &begin_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.", - authorities_set_id, - ); - - return Ok(None); - } - } + let last_block_of_the_set_id = BlockId::Number(last_block_of_the_set); + let justification = if let Some(justification) = blockchain.justification(last_block_of_the_set_id)? { + justification + } else { + trace!( + target: "afg", + "No justification found when making finality proof for {}. Returning empty proof.", + block, + ); + return Ok(None); + }; - finality_proof.push(proof_fragment); - latest_proof_fragment = None; - } else { - latest_proof_fragment = Some(proof_fragment); - } + let last_block_of_the_set_hash = blockchain.expect_block_hash_from_id(&last_block_of_the_set_id)?; - // we don't need to provide more justifications - if justifies_end_block { + // Get all headers from the requested block until the last block of the set + let unknown_headers = { + let mut unknown_headers = Vec::new(); + let mut current_number = block + One::one(); + loop { + if current_number >= last_block_of_the_set { break; } - } - - // we can't provide more justifications - if current_number == info.finalized_number { - // append last justification - even if we can't generate finality proof for - // the end block, we try to generate it for the latest possible block - if let Some(latest_proof_fragment) = latest_proof_fragment.take() { - finality_proof.push(latest_proof_fragment); - fragment_index += 1; - if fragment_index == MAX_FRAGMENTS_IN_PROOF { - break; - } - } - break; + let current_id = BlockId::Number(current_number); + let unknown_header = blockchain.expect_header(current_id)?; + unknown_headers.push(unknown_header); + current_number += One::one(); } - - // else search for the next justification - current_number += One::one(); - } - - if finality_proof.is_empty() { - trace!( - target: "afg", - "No justifications found when making finality proof for {}. Returning empty proof.", - end, - ); - - Ok(None) - } else { + unknown_headers + }; + + let finality_proof = FinalityProof { + block: last_block_of_the_set_hash, + justification, + unknown_headers, + }; + + // Check if the proof is generated by the requested authority set + let block_id = BlockId::Number(block); + let block_authorities = authorities_provider.authorities(&block_id)?; + let justification_check_result = J::decode_and_verify( + &finality_proof.justification, + authorities_set_id, + &block_authorities, + ); + if justification_check_result.is_err() { trace!( target: "afg", - "Built finality proof for {} of {} fragments. Last fragment for {}.", - end, - finality_proof.len(), - finality_proof.last().expect("checked that !finality_proof.is_empty(); qed").block, + "Can not provide finality proof with requested set id #{}\ + (possible forced change?). Returning empty proof.", + authorities_set_id, ); - Ok(Some(finality_proof.encode())) + return Ok(None); } + + Ok(Some(finality_proof.encode())) } /// 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. -pub(crate) fn check_finality_proof( - blockchain: &B, +/// +/// 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: AuthorityList, - authorities_provider: &dyn AuthoritySetForFinalityChecker, remote_proof: Vec, -) -> ClientResult> - where - NumberFor: BlockNumberOps, - B: BlockchainBackend, - J: ProvableJustification, +) -> ClientResult> +where + J: ProvableJustification
, { - // decode finality proof - let proof = FinalityProof::::decode(&mut &remote_proof[..]) + let proof = FinalityProof::
::decode(&mut &remote_proof[..]) .map_err(|_| ClientError::BadJustification("failed to decode finality proof".into()))?; - // empty proof can't prove anything - if proof.is_empty() { - return Err(ClientError::BadJustification("empty proof of finality".into())); - } - - // iterate and verify proof fragments - let last_fragment_index = proof.len() - 1; - let mut authorities = AuthoritiesOrEffects::Authorities(current_set_id, current_authorities); - for (proof_fragment_index, proof_fragment) in proof.into_iter().enumerate() { - // check that proof is non-redundant. The proof still can be valid, but - // we do not want peer to spam us with redundant data - if proof_fragment_index != last_fragment_index { - let has_unknown_headers = !proof_fragment.unknown_headers.is_empty(); - let has_new_authorities = proof_fragment.authorities_proof.is_some(); - if has_unknown_headers || !has_new_authorities { - return Err(ClientError::BadJustification("redundant proof of finality".into())); - } - } - - authorities = check_finality_proof_fragment::<_, _, J>( - blockchain, - authorities, - authorities_provider, - proof_fragment)?; - } - - let effects = authorities.extract_effects().expect("at least one loop iteration is guaranteed - because proof is not empty;\ - check_finality_proof_fragment is called on every iteration;\ - check_finality_proof_fragment always returns FinalityEffects;\ - qed"); - - telemetry!(CONSENSUS_INFO; "afg.finality_proof_ok"; - "set_id" => ?effects.new_set_id, "finalized_header_hash" => ?effects.block); - - Ok(effects) -} - -/// Check finality proof for the single block. -fn check_finality_proof_fragment( - blockchain: &B, - authority_set: AuthoritiesOrEffects, - authorities_provider: &dyn AuthoritySetForFinalityChecker, - proof_fragment: FinalityProofFragment, -) -> ClientResult> - where - NumberFor: BlockNumberOps, - B: BlockchainBackend, - J: Decode + ProvableJustification, -{ - // verify justification using previous authorities set - let (mut current_set_id, mut current_authorities) = authority_set.extract_authorities(); - let justification: J = Decode::decode(&mut &proof_fragment.justification[..]) + let justification: J = Decode::decode(&mut &proof.justification[..]) .map_err(|_| ClientError::JustificationDecode)?; justification.verify(current_set_id, ¤t_authorities)?; - // and now verify new authorities proof (if provided) - if let Some(new_authorities_proof) = proof_fragment.authorities_proof { - // the proof is either generated using known header and it is safe to query header - // here, because its non-finality proves that it can't be pruned - // or it is generated using last unknown header (because it is the one who has - // justification => we only generate proofs for headers with justifications) - let header = match proof_fragment.unknown_headers.iter().rev().next().cloned() { - Some(header) => header, - None => blockchain.expect_header(BlockId::Hash(proof_fragment.block))?, - }; - current_authorities = authorities_provider.check_authorities_proof( - proof_fragment.block, - header, - new_authorities_proof, - )?; - - current_set_id += 1; - } - - Ok(AuthoritiesOrEffects::Effects(FinalityEffects { - headers_to_import: proof_fragment.unknown_headers, - block: proof_fragment.block, - justification: proof_fragment.justification, - new_set_id: current_set_id, - new_authorities: current_authorities, - })) -} - -/// Authorities set from initial authorities set or finality effects. -enum AuthoritiesOrEffects { - Authorities(u64, AuthorityList), - Effects(FinalityEffects
), -} - -impl AuthoritiesOrEffects
{ - pub fn extract_authorities(self) -> (u64, AuthorityList) { - match self { - AuthoritiesOrEffects::Authorities(set_id, authorities) => (set_id, authorities), - AuthoritiesOrEffects::Effects(effects) => (effects.new_set_id, effects.new_authorities), - } - } - - pub fn extract_effects(self) -> Option> { - match self { - AuthoritiesOrEffects::Authorities(_, _) => None, - AuthoritiesOrEffects::Effects(effects) => Some(effects), - } - } + use sc_telemetry::{telemetry, CONSENSUS_INFO}; + telemetry!(CONSENSUS_INFO; "afg.finality_proof_ok"; + "finalized_header_hash" => ?proof.block); + Ok(proof) } /// Justification used to prove block finality. -pub(crate) trait ProvableJustification: Encode + Decode { +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<()>; @@ -624,33 +329,12 @@ pub(crate) mod tests { pub(crate) type FinalityProof = super::FinalityProof
; - impl AuthoritySetForFinalityProver for (GetAuthorities, ProveAuthorities) + impl AuthoritySetForFinalityProver for GetAuthorities where GetAuthorities: Send + Sync + Fn(BlockId) -> ClientResult, - ProveAuthorities: Send + Sync + Fn(BlockId) -> ClientResult, { fn authorities(&self, block: &BlockId) -> ClientResult { - self.0(*block) - } - - fn prove_authorities(&self, block: &BlockId) -> ClientResult { - self.1(*block) - } - } - - pub(crate) struct ClosureAuthoritySetForFinalityChecker(pub Closure); - - impl AuthoritySetForFinalityChecker for ClosureAuthoritySetForFinalityChecker - where - Closure: Send + Sync + Fn(H256, Header, StorageProof) -> ClientResult, - { - fn check_authorities_proof( - &self, - hash: H256, - header: Header, - proof: StorageProof, - ) -> ClientResult { - self.0(hash, header, proof) + self(*block) } } @@ -675,26 +359,6 @@ pub(crate) mod tests { Header::new(number, H256::from_low_u64_be(0), H256::from_low_u64_be(0), parent_hash, Default::default()) } - fn side_header(number: u64) -> Header { - Header::new( - number, - H256::from_low_u64_be(0), - H256::from_low_u64_be(1), - header(number - 1).hash(), - Default::default(), - ) - } - - fn second_side_header(number: u64) -> Header { - Header::new( - number, - H256::from_low_u64_be(0), - H256::from_low_u64_be(1), - side_header(number - 1).hash(), - Default::default(), - ) - } - fn test_blockchain() -> InMemoryBlockchain { let blockchain = InMemoryBlockchain::::new(); blockchain.insert(header(0).hash(), header(0), Some(vec![0]), None, NewBlockState::Final).unwrap(); @@ -704,25 +368,6 @@ pub(crate) mod tests { blockchain } - #[test] - fn finality_prove_fails_with_invalid_range() { - let blockchain = test_blockchain(); - - // their last finalized is: 2 - // they request for proof-of-finality of: 2 - // => range is invalid - prove_finality::<_, _, TestJustification>( - &blockchain, - &( - |_| unreachable!("should return before calling GetAuthorities"), - |_| unreachable!("should return before calling ProveAuthorities"), - ), - 0, - header(2).hash(), - header(2).hash(), - ).unwrap_err(); - } - #[test] fn finality_proof_is_none_if_no_more_last_finalized_blocks() { let blockchain = test_blockchain(); @@ -733,42 +378,14 @@ pub(crate) mod tests { // => we can't provide any additional justifications let proof_of_4 = prove_finality::<_, _, TestJustification>( &blockchain, - &( - |_| unreachable!("should return before calling GetAuthorities"), - |_| unreachable!("should return before calling ProveAuthorities"), - ), + &|_| unreachable!("should return before calling GetAuthorities"), 0, - header(3).hash(), - header(4).hash(), + *header(3).number(), + *header(4).number(), ).unwrap(); assert_eq!(proof_of_4, None); } - #[test] - fn finality_proof_fails_for_non_canonical_block() { - let blockchain = test_blockchain(); - blockchain.insert(header(4).hash(), header(4), None, None, NewBlockState::Best).unwrap(); - blockchain.insert(side_header(4).hash(), side_header(4), None, None, NewBlockState::Best).unwrap(); - blockchain.insert(second_side_header(5).hash(), second_side_header(5), None, None, NewBlockState::Best) - .unwrap(); - blockchain.insert(header(5).hash(), header(5), Some(vec![5]), None, NewBlockState::Final).unwrap(); - - // chain is 1 -> 2 -> 3 -> 4 -> 5 - // \> 4' -> 5' - // and the best finalized is 5 - // => when requesting for (4'; 5'], error is returned - prove_finality::<_, _, TestJustification>( - &blockchain, - &( - |_| unreachable!("should return before calling GetAuthorities"), - |_| unreachable!("should return before calling ProveAuthorities"), - ), - 0, - side_header(4).hash(), - second_side_header(5).hash(), - ).unwrap_err(); - } - #[test] fn finality_proof_is_none_if_no_justification_known() { let blockchain = test_blockchain(); @@ -778,263 +395,88 @@ pub(crate) mod tests { // => we can't prove finality let proof_of_4 = prove_finality::<_, _, TestJustification>( &blockchain, - &( - |_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), - |_| unreachable!("authorities didn't change => ProveAuthorities won't be called"), - ), + &|_| unreachable!("should return before calling GetAuthorities"), 0, - header(3).hash(), - header(4).hash(), + *header(3).number(), + *header(4).number(), ).unwrap(); assert_eq!(proof_of_4, None); } #[test] - fn finality_proof_works_without_authorities_change() { + fn finality_proof_works_for_basic_setup() { let blockchain = test_blockchain(); let authorities = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; let just4 = TestJustification((0, authorities.clone()), vec![4]).encode(); let just5 = TestJustification((0, authorities.clone()), vec![5]).encode(); - blockchain.insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final).unwrap(); - blockchain.insert(header(5).hash(), header(5), Some(just5.clone()), None, NewBlockState::Final).unwrap(); + blockchain.insert( + header(4).hash(), header(4), Some(just4), None, NewBlockState::Final + ).unwrap(); + blockchain.insert( + header(5).hash(), header(5), Some(just5.clone()), None, NewBlockState::Final + ).unwrap(); // blocks 4 && 5 are finalized with justification - // => since authorities are the same, we only need justification for 5 let proof_of_5: FinalityProof = Decode::decode(&mut &prove_finality::<_, _, TestJustification>( &blockchain, - &( - |_| Ok(authorities.clone()), - |_| unreachable!("should return before calling ProveAuthorities"), - ), + &|_| Ok(authorities.clone()), 0, - header(3).hash(), - header(5).hash(), + *header(3).number(), + *header(5).number(), ).unwrap().unwrap()[..]).unwrap(); - assert_eq!(proof_of_5, vec![FinalityProofFragment { + assert_eq!(proof_of_5, FinalityProof { block: header(5).hash(), justification: just5, - unknown_headers: Vec::new(), - authorities_proof: None, - }]); - } - - #[test] - fn finality_proof_finalized_earlier_block_if_no_justification_for_target_is_known() { - let blockchain = test_blockchain(); - blockchain.insert(header(4).hash(), header(4), Some(vec![4]), None, NewBlockState::Final).unwrap(); - blockchain.insert(header(5).hash(), header(5), None, None, NewBlockState::Final).unwrap(); - - // block 4 is finalized with justification + we request for finality of 5 - // => we can't prove finality of 5, but providing finality for 4 is still useful for requester - let proof_of_5: FinalityProof = Decode::decode(&mut &prove_finality::<_, _, TestJustification>( - &blockchain, - &( - |_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), - |_| unreachable!("should return before calling ProveAuthorities"), - ), - 0, - header(3).hash(), - header(5).hash(), - ).unwrap().unwrap()[..]).unwrap(); - assert_eq!(proof_of_5, vec![FinalityProofFragment { - block: header(4).hash(), - justification: vec![4], - unknown_headers: Vec::new(), - authorities_proof: None, - }]); - } - - #[test] - fn finality_proof_works_with_authorities_change() { - let blockchain = test_blockchain(); - let auth3 = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; - let auth5 = vec![(AuthorityId::from_slice(&[5u8; 32]), 1u64)]; - let auth7 = vec![(AuthorityId::from_slice(&[7u8; 32]), 1u64)]; - let just4 = TestJustification((0, auth3.clone()), vec![4]).encode(); - let just5 = TestJustification((0, auth3.clone()), vec![5]).encode(); - let just7 = TestJustification((1, auth5.clone()), vec![7]).encode(); - blockchain.insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final).unwrap(); - blockchain.insert(header(5).hash(), header(5), Some(just5.clone()), 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(); - - // when querying for finality of 6, we assume that the #3 is the last block known to the requester - // => since we only have justification for #7, we provide #7 - let proof_of_6: FinalityProof = Decode::decode(&mut &prove_finality::<_, _, TestJustification>( - &blockchain, - &( - |block_id| match block_id { - BlockId::Hash(h) if h == header(3).hash() => Ok(auth3.clone()), - BlockId::Number(4) => Ok(auth3.clone()), - BlockId::Number(5) => Ok(auth5.clone()), - BlockId::Number(7) => Ok(auth7.clone()), - _ => unreachable!("no other authorities should be fetched: {:?}", block_id), - }, - |block_id| match block_id { - BlockId::Number(5) => Ok(StorageProof::new(vec![vec![50]])), - BlockId::Number(7) => Ok(StorageProof::new(vec![vec![70]])), - _ => unreachable!("no other authorities should be proved: {:?}", block_id), - }, - ), - 0, - header(3).hash(), - header(6).hash(), - ).unwrap().unwrap()[..]).unwrap(); - // initial authorities set (which start acting from #0) is [3; 32] - assert_eq!(proof_of_6, vec![ - // new authorities set starts acting from #5 => we do not provide fragment for #4 - // first fragment provides justification for #5 && authorities set that starts acting from #5 - FinalityProofFragment { - block: header(5).hash(), - justification: just5, - unknown_headers: Vec::new(), - authorities_proof: Some(StorageProof::new(vec![vec![50]])), - }, - // last fragment provides justification for #7 && unknown#7 - FinalityProofFragment { - block: header(7).hash(), - justification: just7.clone(), - unknown_headers: vec![header(7)], - authorities_proof: Some(StorageProof::new(vec![vec![70]])), - }, - ]); + unknown_headers: vec![header(4)], + }); - // now let's verify finality proof + // Now let's verify finality proof let blockchain = test_blockchain(); blockchain.insert(header(4).hash(), header(4), None, 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(); - let effects = check_finality_proof::<_, _, TestJustification>( - &blockchain, + let proof = check_finality_proof::<_, TestJustification>( 0, - auth3, - &ClosureAuthoritySetForFinalityChecker( - |hash, _header, proof: StorageProof| match proof.clone().iter_nodes().next().map(|x| x[0]) { - Some(50) => Ok(auth5.clone()), - Some(70) => Ok(auth7.clone()), - _ => unreachable!("no other proofs should be checked: {}", hash), - } - ), - proof_of_6.encode(), + authorities.clone(), + proof_of_5.encode(), ).unwrap(); - assert_eq!(effects, FinalityEffects { - headers_to_import: vec![header(7)], - block: header(7).hash(), - justification: TestJustification((1, auth5.clone()), vec![7]).encode(), - new_set_id: 2, - new_authorities: auth7, - }); + assert_eq!(proof, proof_of_5); } #[test] fn finality_proof_check_fails_when_proof_decode_fails() { - let blockchain = test_blockchain(); - // when we can't decode proof from Vec - check_finality_proof::<_, _, TestJustification>( - &blockchain, + check_finality_proof::<_, TestJustification>( 1, vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], - &ClosureAuthoritySetForFinalityChecker(|_, _, _| unreachable!("returns before CheckAuthoritiesProof")), vec![42], ).unwrap_err(); } #[test] fn finality_proof_check_fails_when_proof_is_empty() { - let blockchain = test_blockchain(); - // when decoded proof has zero length - check_finality_proof::<_, _, TestJustification>( - &blockchain, + check_finality_proof::<_, TestJustification>( 1, vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], - &ClosureAuthoritySetForFinalityChecker(|_, _, _| unreachable!("returns before CheckAuthoritiesProof")), Vec::::new().encode(), ).unwrap_err(); } - #[test] - fn finality_proof_check_fails_when_intermediate_fragment_has_unknown_headers() { - let blockchain = test_blockchain(); - - // when intermediate (#0) fragment has non-empty unknown headers - let authorities = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; - check_finality_proof::<_, _, TestJustification>( - &blockchain, - 1, - authorities.clone(), - &ClosureAuthoritySetForFinalityChecker(|_, _, _| unreachable!("returns before CheckAuthoritiesProof")), - vec![FinalityProofFragment { - block: header(4).hash(), - justification: TestJustification((0, authorities.clone()), vec![7]).encode(), - unknown_headers: vec![header(4)], - authorities_proof: Some(StorageProof::new(vec![vec![42]])), - }, FinalityProofFragment { - block: header(5).hash(), - justification: TestJustification((0, authorities), vec![8]).encode(), - unknown_headers: vec![header(5)], - authorities_proof: None, - }].encode(), - ).unwrap_err(); - } - - #[test] - fn finality_proof_check_fails_when_intermediate_fragment_has_no_authorities_proof() { - let blockchain = test_blockchain(); - - // when intermediate (#0) fragment has empty authorities proof - let authorities = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; - check_finality_proof::<_, _, TestJustification>( - &blockchain, - 1, - authorities.clone(), - &ClosureAuthoritySetForFinalityChecker(|_, _, _| unreachable!("returns before CheckAuthoritiesProof")), - vec![FinalityProofFragment { - block: header(4).hash(), - justification: TestJustification((0, authorities.clone()), vec![7]).encode(), - unknown_headers: Vec::new(), - authorities_proof: None, - }, FinalityProofFragment { - block: header(5).hash(), - justification: TestJustification((0, authorities), vec![8]).encode(), - unknown_headers: vec![header(5)], - authorities_proof: None, - }].encode(), - ).unwrap_err(); - } - #[test] fn finality_proof_check_works() { - let blockchain = test_blockchain(); - let initial_authorities = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; - let next_authorities = vec![(AuthorityId::from_slice(&[4u8; 32]), 1u64)]; - let effects = check_finality_proof::<_, _, TestJustification>( - &blockchain, + let finality_proof = FinalityProof { + block: header(2).hash(), + justification: TestJustification((1, initial_authorities.clone()), vec![7]).encode(), + unknown_headers: Vec::new(), + }; + let proof = check_finality_proof::<_, TestJustification>( 1, initial_authorities.clone(), - &ClosureAuthoritySetForFinalityChecker(|_, _, _| Ok(next_authorities.clone())), - vec![FinalityProofFragment { - block: header(2).hash(), - justification: TestJustification((1, initial_authorities.clone()), vec![7]).encode(), - unknown_headers: Vec::new(), - authorities_proof: Some(StorageProof::new(vec![vec![42]])), - }, FinalityProofFragment { - block: header(4).hash(), - justification: TestJustification((2, next_authorities.clone()), vec![8]).encode(), - unknown_headers: vec![header(4)], - authorities_proof: None, - }].encode(), + finality_proof.encode(), ).unwrap(); - assert_eq!(effects, FinalityEffects { - headers_to_import: vec![header(4)], - block: header(4).hash(), - justification: TestJustification((2, next_authorities.clone()), vec![8]).encode(), - new_set_id: 2, - new_authorities: vec![(AuthorityId::from_slice(&[4u8; 32]), 1u64)], - }); + assert_eq!(proof, finality_proof); } #[test] @@ -1049,13 +491,10 @@ pub(crate) mod tests { let proof_of_4 = prove_finality::<_, _, TestJustification>( &blockchain, - &( - |_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), - |_| unreachable!("should return before calling ProveAuthorities"), - ), + &|_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), 0, - header(3).hash(), - header(4).hash(), + *header(3).number(), + *header(4).number(), ).unwrap(); assert!(proof_of_4.is_none()); } diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index c5f89717a64d7..066caeafc9091 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -122,7 +122,7 @@ mod until_imported; mod voting_rule; pub use authorities::{SharedAuthoritySet, AuthoritySet}; -pub use finality_proof::{FinalityProofFragment, FinalityProofProvider, StorageAndProofProvider}; +pub use finality_proof::{FinalityProof, FinalityProofProvider}; pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream}; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index ef8168e84f66c..e697c263184d5 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -32,25 +32,20 @@ use tokio::runtime::{Runtime, Handle}; use sp_keyring::Ed25519Keyring; use sc_client_api::backend::TransactionFor; use sp_blockchain::Result; -use sp_api::{ApiRef, StorageProof, ProvideRuntimeApi}; +use sp_api::{ApiRef, ProvideRuntimeApi}; use substrate_test_runtime_client::runtime::BlockNumber; use sp_consensus::{ BlockOrigin, ForkChoiceStrategy, ImportedAux, BlockImportParams, ImportResult, BlockImport, import_queue::BoxJustificationImport, }; use std::{collections::{HashMap, HashSet}, pin::Pin}; -use parity_scale_codec::Decode; -use sp_runtime::traits::{Block as BlockT, Header as HeaderT, HashFor}; +use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; use sp_runtime::generic::{BlockId, DigestItem}; use sp_core::H256; use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore}; use sp_finality_grandpa::{GRANDPA_ENGINE_ID, AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof}; -use sp_state_machine::{InMemoryBackend, prove_read, read_proof_check}; use authorities::AuthoritySet; -use finality_proof::{ - AuthoritySetForFinalityProver, AuthoritySetForFinalityChecker, -}; use sc_block_builder::BlockBuilderProvider; use sc_consensus::LongestChain; use sc_keystore::LocalKeystore; @@ -207,43 +202,6 @@ impl GenesisAuthoritySetProvider for TestApi { } } -impl AuthoritySetForFinalityProver for TestApi { - fn authorities(&self, _block: &BlockId) -> Result { - Ok(self.genesis_authorities.clone()) - } - - fn prove_authorities(&self, block: &BlockId) -> Result { - let authorities = self.authorities(block)?; - let backend = >>::from(vec![ - (None, vec![(b"authorities".to_vec(), Some(authorities.encode()))]) - ]); - let proof = prove_read(backend, vec![b"authorities"]) - .expect("failure proving read from in-memory storage backend"); - Ok(proof) - } -} - -impl AuthoritySetForFinalityChecker for TestApi { - fn check_authorities_proof( - &self, - _hash: ::Hash, - header: ::Header, - proof: StorageProof, - ) -> Result { - let results = read_proof_check::, _>( - *header.state_root(), proof, vec![b"authorities"] - ) - .expect("failure checking read proof for authorities"); - let encoded = results.get(&b"authorities"[..]) - .expect("returned map must contain all proof keys") - .as_ref() - .expect("authorities in proof is None"); - let authorities = Decode::decode(&mut &encoded[..]) - .expect("failure decoding authorities read from proof"); - Ok(authorities) - } -} - const TEST_GOSSIP_DURATION: Duration = Duration::from_millis(500); fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList { From 65c4243d44b17f4f959f3cc3f8d0ad074d8963f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 26 Nov 2020 11:53:52 +0100 Subject: [PATCH 14/28] grandpa: move checks and extend tests in finality_proof --- client/finality-grandpa/rpc/src/lib.rs | 1 - client/finality-grandpa/src/authorities.rs | 4 +- client/finality-grandpa/src/finality_proof.rs | 383 +++++++++++++----- 3 files changed, 282 insertions(+), 106 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index c39f498f77203..cd42787d353c3 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -256,7 +256,6 @@ mod tests { &self, _block: NumberFor ) -> Result, sp_blockchain::Error> { - // Don't call this without setting the FinalityProof Ok(Some(EncodedFinalityProofs( self.finality_proof .as_ref() diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index defe2ab160a79..bd629c26fd825 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -705,8 +705,8 @@ impl AuthoritySetChanges { } pub(crate) fn get_set_id(&self, block_number: N) -> Option<(u64, N)> { - let idx = self.authority_set_changes - .binary_search_by_key(&block_number, |(_set_id, n)| n.clone()) + let idx = self.authority_set_changes + .binary_search_by_key(&block_number, |(_set_id, n)| n.clone()) .unwrap_or_else(|b| b); if idx < self.authority_set_changes.len() { Some(self.authority_set_changes[idx].clone()) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 893f3d700cb3e..c53d825ae5b15 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -62,24 +62,26 @@ pub trait AuthoritySetForFinalityProver: Send + Sync { } /// Trait that combines `StorageProvider` and `ProofProvider` -pub trait StorageAndProofProvider: StorageProvider + ProofProvider + Send + Sync - where - Block: BlockT, - BE: Backend + Send + Sync, +pub trait StorageAndProofProvider: + StorageProvider + ProofProvider + Send + Sync +where + Block: BlockT, + BE: Backend + Send + Sync, {} /// Blanket implementation. impl StorageAndProofProvider for P - where - Block: BlockT, - BE: Backend + Send + Sync, - P: StorageProvider + ProofProvider + Send + Sync, +where + Block: BlockT, + BE: Backend + Send + Sync, + P: StorageProvider + ProofProvider + Send + Sync, {} /// Implementation of AuthoritySetForFinalityProver. -impl AuthoritySetForFinalityProver for Arc> - where - BE: Backend + Send + Sync + 'static, +impl AuthoritySetForFinalityProver + for Arc> +where + BE: Backend + Send + Sync + 'static, { fn authorities(&self, block: &BlockId) -> ClientResult { let storage_key = StorageKey(GRANDPA_AUTHORITIES_KEY.to_vec()); @@ -97,7 +99,8 @@ pub struct FinalityProofProvider { } impl FinalityProofProvider - where B: Backend + Send + Sync + 'static +where + B: Backend + Send + Sync + 'static, { /// Create new finality proof provider using: /// @@ -108,7 +111,8 @@ impl FinalityProofProvider authority_provider: P, shared_authority_set: Option>>, ) -> Self - where P: AuthoritySetForFinalityProver + 'static, + where + P: AuthoritySetForFinalityProver + 'static, { FinalityProofProvider { backend, @@ -135,18 +139,16 @@ impl FinalityProofProvider } impl FinalityProofProvider - where - Block: BlockT, - NumberFor: BlockNumberOps, - B: Backend + Send + Sync + 'static, +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, - ) -> Result>, ClientError> { - let (set_id, last_block_for_set) = match self.shared_authority_set + pub fn prove_finality(&self, block: NumberFor) -> Result>, ClientError> { + let (set_id, last_block_for_set) = match self + .shared_authority_set .as_ref() .map(SharedAuthoritySet::authority_set_changes) .and_then(|changes| changes.get_set_id(block)) @@ -197,34 +199,61 @@ where block, info.finalized_number, ); - return Ok(None); } let last_block_of_the_set_id = BlockId::Number(last_block_of_the_set); - let justification = if let Some(justification) = blockchain.justification(last_block_of_the_set_id)? { - justification - } else { + let last_block_of_the_set_hash = + blockchain.expect_block_hash_from_id(&last_block_of_the_set_id)?; + + let justification = + if let Some(justification) = blockchain.justification(last_block_of_the_set_id)? { + justification + } else { + trace!( + target: "afg", + "No justification found when making finality proof for {}. Returning empty proof.", + block, + ); + return Ok(None); + }; + + let block_id = BlockId::Number(block); + let block_authorities = authorities_provider.authorities(&block_id)?; + + // Check if the justification is generated by the requested authority set + let justification_check_result = + J::decode_and_verify(&justification, authorities_set_id, &block_authorities); + if justification_check_result.is_err() { trace!( target: "afg", - "No justification found when making finality proof for {}. Returning empty proof.", - block, + "Can not provide finality proof with requested set id #{}\ + (possible forced change?). Returning empty proof.", + authorities_set_id, ); return Ok(None); - }; - - let last_block_of_the_set_hash = blockchain.expect_block_hash_from_id(&last_block_of_the_set_id)?; + } // Get all headers from the requested block until the last block of the set let unknown_headers = { let mut unknown_headers = Vec::new(); let mut current_number = block + One::one(); + let max_loop = current_number + 1000000.into(); // WIP(JON) loop { - if current_number >= last_block_of_the_set { + if current_number >= last_block_of_the_set || current_number >= max_loop { break; } let current_id = BlockId::Number(current_number); + if block_authorities != authorities_provider.authorities(¤t_id)? { + trace!( + target: "afg", + "Encountered new authorities when collecting unknown headers. \ + Returning empty proof" + ); + return Ok(None); + } + let unknown_header = blockchain.expect_header(current_id)?; unknown_headers.push(unknown_header); current_number += One::one(); @@ -237,26 +266,6 @@ where justification, unknown_headers, }; - - // Check if the proof is generated by the requested authority set - let block_id = BlockId::Number(block); - let block_authorities = authorities_provider.authorities(&block_id)?; - let justification_check_result = J::decode_and_verify( - &finality_proof.justification, - authorities_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.", - authorities_set_id, - ); - - return Ok(None); - } - Ok(Some(finality_proof.encode())) } @@ -299,16 +308,16 @@ trait ProvableJustification: Encode + Decode { set_id: u64, authorities: &[(AuthorityId, u64)], ) -> ClientResult { - let justification = Self::decode(&mut &**justification) - .map_err(|_| ClientError::JustificationDecode)?; + let justification = + Self::decode(&mut &**justification).map_err(|_| ClientError::JustificationDecode)?; justification.verify(set_id, authorities)?; Ok(justification) } } impl ProvableJustification for GrandpaJustification - where - NumberFor: BlockNumberOps, +where + NumberFor: BlockNumberOps, { fn verify(&self, set_id: u64, authorities: &[(AuthorityId, u64)]) -> ClientResult<()> { let authorities = VoterSet::new(authorities.iter().cloned()).ok_or( @@ -321,17 +330,17 @@ impl ProvableJustification for GrandpaJustificatio #[cfg(test)] pub(crate) mod tests { + use super::*; use substrate_test_runtime_client::runtime::{Block, Header, H256}; use sc_client_api::NewBlockState; use sc_client_api::in_mem::Blockchain as InMemoryBlockchain; - use super::*; use sp_core::crypto::Public; pub(crate) type FinalityProof = super::FinalityProof
; impl AuthoritySetForFinalityProver for GetAuthorities - where - GetAuthorities: Send + Sync + Fn(BlockId) -> ClientResult, + where + GetAuthorities: Send + Sync + Fn(BlockId) -> ClientResult, { fn authorities(&self, block: &BlockId) -> ClientResult { self(*block) @@ -356,22 +365,38 @@ pub(crate) mod tests { 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()) + Header::new( + number, + H256::from_low_u64_be(0), + H256::from_low_u64_be(0), + parent_hash, + Default::default(), + ) } fn test_blockchain() -> InMemoryBlockchain { 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(); + 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(); blockchain } #[test] fn finality_proof_is_none_if_no_more_last_finalized_blocks() { let blockchain = test_blockchain(); - blockchain.insert(header(4).hash(), header(4), None, None, NewBlockState::Best).unwrap(); + blockchain + .insert(header(4).hash(), header(4), None, None, NewBlockState::Best) + .unwrap(); // our last finalized is: 3 // their last finalized is: 3 @@ -382,14 +407,17 @@ pub(crate) mod tests { 0, *header(3).number(), *header(4).number(), - ).unwrap(); + ) + .unwrap(); assert_eq!(proof_of_4, None); } #[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(); + blockchain + .insert(header(4).hash(), header(4), None, None, NewBlockState::Final) + .unwrap(); // block 4 is finalized without justification // => we can't prove finality @@ -399,7 +427,8 @@ pub(crate) mod tests { 0, *header(3).number(), *header(4).number(), - ).unwrap(); + ) + .unwrap(); assert_eq!(proof_of_4, None); } @@ -409,36 +438,49 @@ pub(crate) mod tests { let authorities = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; let just4 = TestJustification((0, authorities.clone()), vec![4]).encode(); let just5 = TestJustification((0, authorities.clone()), vec![5]).encode(); - blockchain.insert( - header(4).hash(), header(4), Some(just4), None, NewBlockState::Final - ).unwrap(); - blockchain.insert( - header(5).hash(), header(5), Some(just5.clone()), None, NewBlockState::Final - ).unwrap(); - - // blocks 4 && 5 are finalized with justification - let proof_of_5: FinalityProof = Decode::decode(&mut &prove_finality::<_, _, TestJustification>( - &blockchain, - &|_| Ok(authorities.clone()), - 0, - *header(3).number(), - *header(5).number(), - ).unwrap().unwrap()[..]).unwrap(); - assert_eq!(proof_of_5, FinalityProof { - block: header(5).hash(), - justification: just5, - unknown_headers: vec![header(4)], - }); + blockchain + .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) + .unwrap(); + blockchain + .insert(header(5).hash(), header(5), Some(just5.clone()), None, NewBlockState::Final) + .unwrap(); + + // Blocks 4 && 5 are finalized with justification + let proof_of_5: FinalityProof = Decode::decode( + &mut &prove_finality::<_, _, TestJustification>( + &blockchain, + &|_| Ok(authorities.clone()), + 0, + *header(3).number(), + *header(5).number(), + ) + .unwrap() + .unwrap()[..], + ) + .unwrap(); + assert_eq!( + proof_of_5, + FinalityProof { + block: header(5).hash(), + justification: just5, + unknown_headers: vec![header(4)], + } + ); // Now let's verify finality proof let blockchain = test_blockchain(); - blockchain.insert(header(4).hash(), header(4), None, None, NewBlockState::Final).unwrap(); - blockchain.insert(header(5).hash(), header(5), None, None, NewBlockState::Final).unwrap(); + blockchain + .insert(header(4).hash(), header(4), None, None, NewBlockState::Final) + .unwrap(); + blockchain + .insert(header(5).hash(), header(5), None, None, NewBlockState::Final) + .unwrap(); let proof = check_finality_proof::<_, TestJustification>( 0, authorities.clone(), proof_of_5.encode(), - ).unwrap(); + ) + .unwrap(); assert_eq!(proof, proof_of_5); } @@ -450,7 +492,8 @@ pub(crate) mod tests { 1, vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], vec![42], - ).unwrap_err(); + ) + .unwrap_err(); } #[test] @@ -460,7 +503,8 @@ pub(crate) mod tests { 1, vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], Vec::::new().encode(), - ).unwrap_err(); + ) + .unwrap_err(); } #[test] @@ -475,27 +519,160 @@ pub(crate) mod tests { 1, initial_authorities.clone(), finality_proof.encode(), - ).unwrap(); + ) + .unwrap(); assert_eq!(proof, finality_proof); } #[test] - fn finality_proof_is_none_if_first_justification_is_generated_by_unknown_set() { - // this is the case for forced change: set_id has been forcibly increased on full node - // and light node missed that - // => justification verification will fail on light node anyways, so we do not return - // finality proof at all + 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 just4 = TestJustification((0, vec![(AuthorityId::from_slice(&[42u8; 32]), 1u64)]), vec![4]).encode(); - blockchain.insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final).unwrap(); + let just4 = TestJustification( + (0, vec![(AuthorityId::from_slice(&[42u8; 32]), 1u64)]), + vec![4], + ) + .encode(); + blockchain + .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) + .unwrap(); - let proof_of_4 = prove_finality::<_, _, TestJustification>( + let proof_of_3 = prove_finality::<_, _, TestJustification>( &blockchain, &|_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), 0, *header(3).number(), *header(4).number(), - ).unwrap(); + ) + .unwrap(); + assert!(proof_of_3.is_none()); + } + + #[test] + fn finality_proof_is_none_if_authority_set_id_is_incorrect() { + let blockchain = test_blockchain(); + let just4 = TestJustification( + (0, vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), + vec![4], + ) + .encode(); + blockchain + .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) + .unwrap(); + + // 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(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), + 1, + *header(3).number(), + *header(4).number(), + ) + .unwrap(); + assert!(proof_of_3.is_none()); + } + + #[test] + fn finality_proof_is_none_for_next_set_id_with_same_authorities() { + let blockchain = test_blockchain(); + let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; + let just5 = TestJustification((0, auth.clone()), vec![5]).encode(); + let just6 = TestJustification((1, auth.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(); + + // Try to prove finality from the next set_id, but which has the same authorities. + let proof_of_4 = prove_finality::<_, _, TestJustification>( + &blockchain, + &|_| Ok(auth.clone()), + 1, + *header(4).number(), + *header(6).number(), + ) + .unwrap(); + // WIP(JON): Currently this test fails, as the different set_id isn't detected. + assert!(proof_of_4.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 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), + }, + 1, + *header(4).number(), + *header(6).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(); + + // Try to prove finality using a later set_id than the block we're trying to prove, but + // that has the same authorities. + 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()), + BlockId::Number(7) => Ok(auth1.clone()), + _ => unimplemented!("No other authorities should be proved: {:?}", block_id), + }, + 2, + *header(4).number(), + *header(7).number(), + ) + .unwrap(); assert!(proof_of_4.is_none()); } } From 282d9f64c4ba85471cc8c4e292ca8c69b86ed51d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 7 Dec 2020 10:10:19 +0100 Subject: [PATCH 15/28] grandpa: address first set of review comments --- client/finality-grandpa/rpc/src/finality.rs | 10 +++--- client/finality-grandpa/rpc/src/lib.rs | 20 +++++------ client/finality-grandpa/src/authorities.rs | 39 +++++++++++---------- client/finality-grandpa/src/environment.rs | 1 + 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/client/finality-grandpa/rpc/src/finality.rs b/client/finality-grandpa/rpc/src/finality.rs index 01006ed5f3a31..640b108f5aef8 100644 --- a/client/finality-grandpa/rpc/src/finality.rs +++ b/client/finality-grandpa/rpc/src/finality.rs @@ -22,14 +22,16 @@ use sc_finality_grandpa::FinalityProofProvider; use sp_runtime::traits::{Block as BlockT, NumberFor}; #[derive(Serialize, Deserialize)] -pub struct EncodedFinalityProofs(pub sp_core::Bytes); +pub struct EncodedFinalityProof(pub sp_core::Bytes); /// Local trait mainly to allow mocking in tests. pub trait RpcFinalityProofProvider { + /// Prove finality for the given block number by returning a Justification for the last block of + /// the authority set. fn rpc_prove_finality( &self, block: NumberFor, - ) -> Result, sp_blockchain::Error>; + ) -> Result, sp_blockchain::Error>; } impl RpcFinalityProofProvider for FinalityProofProvider @@ -41,8 +43,8 @@ where fn rpc_prove_finality( &self, block: NumberFor, - ) -> Result, sp_blockchain::Error> { + ) -> Result, sp_blockchain::Error> { self.prove_finality(block) - .map(|x| x.map(|y| EncodedFinalityProofs(y.into()))) + .map(|x| x.map(|y| EncodedFinalityProof(y.into()))) } } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index cd42787d353c3..36d3d5f49deed 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -39,7 +39,7 @@ mod report; use sc_finality_grandpa::GrandpaJustificationStream; use sp_runtime::traits::{Block as BlockT, NumberFor}; -use finality::{EncodedFinalityProofs, RpcFinalityProofProvider}; +use finality::{EncodedFinalityProof, RpcFinalityProofProvider}; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; use notification::JustificationNotification; @@ -48,7 +48,7 @@ type FutureResult = /// Provides RPC methods for interacting with GRANDPA. #[rpc] -pub trait GrandpaApi { +pub trait GrandpaApi { /// RPC Metadata type Metadata; @@ -83,12 +83,12 @@ pub trait GrandpaApi { ) -> jsonrpc_core::Result; /// Prove finality for the given block number by returning the Justification for the last block - /// in the set. + /// in the set and all the intermediary headers to link them together. #[rpc(name = "grandpa_proveFinality")] fn prove_finality( &self, - block: N, - ) -> FutureResult>; + block: Number, + ) -> FutureResult>; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. @@ -171,7 +171,7 @@ where fn prove_finality( &self, block: NumberFor, - ) -> FutureResult> { + ) -> FutureResult> { let result = self.finality_proof_provider.rpc_prove_finality(block); let future = async move { result }.boxed(); Box::new( @@ -255,8 +255,8 @@ mod tests { fn rpc_prove_finality( &self, _block: NumberFor - ) -> Result, sp_blockchain::Error> { - Ok(Some(EncodedFinalityProofs( + ) -> Result, sp_blockchain::Error> { + Ok(Some(EncodedFinalityProof( self.finality_proof .as_ref() .expect("Don't call rpc_prove_finality without setting the FinalityProof") @@ -533,7 +533,7 @@ mod tests { let resp = io.handle_request_sync(request, meta); let mut resp: serde_json::Value = serde_json::from_str(&resp.unwrap()).unwrap(); let result: sp_core::Bytes = serde_json::from_value(resp["result"].take()).unwrap(); - let fragments: FinalityProof
= Decode::decode(&mut &result[..]).unwrap(); - assert_eq!(fragments, finality_proof); + let finality_proof_rpc: FinalityProof
= Decode::decode(&mut &result[..]).unwrap(); + assert_eq!(finality_proof_rpc, finality_proof); } } diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index bd629c26fd825..60587a99be0b8 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -167,7 +167,9 @@ pub struct AuthoritySet { /// is lower than the last finalized block (as signaled in the forced /// change) must be applied beforehand. pending_forced_changes: Vec>, - /// Track at which blocks the set id changes. This is useful when we need to prove finality. + /// Track at which blocks the set id changed. This is useful when we need to prove finality for a + /// given block since we can figure out what set the block belongs to and when the set + /// started/ended. authority_set_changes: AuthoritySetChanges, } @@ -441,7 +443,7 @@ where /// forced change), then the forced change cannot be applied. An error will /// be returned in that case which will prevent block import. pub(crate) fn apply_forced_changes( - &mut self, + &self, best_hash: H, best_number: N, is_descendent_of: &F, @@ -503,7 +505,8 @@ where "block" => ?change.canon_height ); - self.authority_set_changes.append(self.set_id, median_last_finalized.clone()); + let mut authority_set_changes = self.authority_set_changes.clone(); + authority_set_changes.append(self.set_id, median_last_finalized.clone()); new_set = Some(( median_last_finalized, @@ -512,7 +515,7 @@ where set_id: self.set_id + 1, pending_standard_changes: ForkTree::new(), // new set, new changes. pending_forced_changes: Vec::new(), - authority_set_changes: AuthoritySetChanges::empty(), + authority_set_changes, }, )); @@ -686,34 +689,34 @@ impl + Clone> PendingChange { } } -// Tracks authority set changes. We store the block numbers for the first block of each authority -// set. +// Tracks historical authority set changes. We store the block numbers for the first block of each +// authority set, once they have been finalalized. #[derive(Debug, Encode, Decode, Clone, PartialEq)] pub struct AuthoritySetChanges { - authority_set_changes: Vec<(u64, N)>, // (set_id, block_number) + authority_set_changes: Vec<(u64, N)>, // (set_id, block_number) } impl AuthoritySetChanges { - pub(crate) fn empty() -> Self { - Self { - authority_set_changes: Default::default(), - } - } + pub(crate) fn empty() -> Self { + Self { + authority_set_changes: Default::default(), + } + } - pub(crate) fn append(&mut self, set_id: u64, block_number: N) { - self.authority_set_changes.push((set_id, block_number)); - } + pub(crate) fn append(&mut self, set_id: u64, block_number: N) { + self.authority_set_changes.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) -> Option<(u64, N)> { let idx = self.authority_set_changes - .binary_search_by_key(&block_number, |(_set_id, n)| n.clone()) + .binary_search_by_key(&block_number, |(_, n)| n.clone()) .unwrap_or_else(|b| b); if idx < self.authority_set_changes.len() { Some(self.authority_set_changes[idx].clone()) } else { None } - } + } } #[cfg(test)] diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 78039932316aa..790be2a221788 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1,4 +1,5 @@ // This file is part of Substrate. + // Copyright (C) 2018-2020 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 From a5644a97d750dfca95cc69409a4f70534c539187 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 7 Dec 2020 11:19:27 +0100 Subject: [PATCH 16/28] grandpa: check that set changes have well-defined start --- client/finality-grandpa/src/authorities.rs | 43 +++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 60587a99be0b8..ae0ddfc05868a 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -712,7 +712,20 @@ impl AuthoritySetChanges { .binary_search_by_key(&block_number, |(_, n)| n.clone()) .unwrap_or_else(|b| b); if idx < self.authority_set_changes.len() { - Some(self.authority_set_changes[idx].clone()) + let (set_id, block_number) = self.authority_set_changes[idx].clone(); + // To make sure we have the right set we need to check that the one before it also exists. + if idx > 0 { + let (prev_set_id, _) = self.authority_set_changes[idx - 1usize]; + if set_id != prev_set_id + 1u64 { + // Without the preceding set_id we don't have a well-defined start. + return None; + } + } 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; + } + Some((set_id, block_number)) } else { None } @@ -1618,4 +1631,32 @@ mod tests { "D" ); } + + #[test] + fn authority_set_changes_for_complete_data() { + let mut authority_set_changes = AuthoritySetChanges::empty(); + authority_set_changes.append(0, 41); + 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); + } + + #[test] + fn authority_set_changes_for_incomplete_data() { + let mut authority_set_changes = AuthoritySetChanges::empty(); + authority_set_changes.append(2, 41); + 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); + } } From 25a854a72e6523dd2ecbee524c99c85708347f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 7 Dec 2020 12:51:44 +0100 Subject: [PATCH 17/28] grandpa: rework prove_finality and assocated tests --- client/finality-grandpa/src/authorities.rs | 4 +- client/finality-grandpa/src/finality_proof.rs | 420 +++++++++--------- 2 files changed, 223 insertions(+), 201 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index ae0ddfc05868a..ba90f8c0ca6c0 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -721,8 +721,8 @@ impl AuthoritySetChanges { return None; } } 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. + // 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; } Some((set_id, block_number)) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index c53d825ae5b15..033d7c39bf3a8 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -55,6 +55,9 @@ use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GR use crate::justification::GrandpaJustification; use crate::VoterSet; use crate::SharedAuthoritySet; +use crate::authorities::AuthoritySetChanges; + +const MAX_UNKNOWN_HEADERS: usize = 100_000; pub trait AuthoritySetForFinalityProver: Send + Sync { /// Read GRANDPA_AUTHORITIES_KEY from storage at given block. @@ -147,22 +150,21 @@ 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>, ClientError> { - let (set_id, last_block_for_set) = match self + let authority_set_changes = if let Some(changes) = self .shared_authority_set .as_ref() .map(SharedAuthoritySet::authority_set_changes) - .and_then(|changes| changes.get_set_id(block)) { - Some((set_id, last_block_for_set)) => (set_id, last_block_for_set), - None => return Ok(None), + changes + } else { + return Ok(None); }; prove_finality::<_, _, GrandpaJustification>( &*self.backend.blockchain(), &*self.authority_provider, - set_id, + authority_set_changes, block, - last_block_for_set, ) } } @@ -180,14 +182,15 @@ pub struct FinalityProof { pub unknown_headers: Vec
, } -fn prove_finality, J>( +fn prove_finality( blockchain: &B, authorities_provider: &dyn AuthoritySetForFinalityProver, - authorities_set_id: u64, + authority_set_changes: AuthoritySetChanges>, block: NumberFor, - last_block_of_the_set: NumberFor, ) -> ::sp_blockchain::Result>> where + Block: BlockT, + B: BlockchainBackend, J: ProvableJustification, { // Early-return if we sure that there are no blocks finalized AFTER begin block @@ -202,12 +205,18 @@ where return Ok(None); } - let last_block_of_the_set_id = BlockId::Number(last_block_of_the_set); - let last_block_of_the_set_hash = - blockchain.expect_block_hash_from_id(&last_block_of_the_set_id)?; + // 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) { + id + } else { + return Ok(None); + }; + // 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_of_the_set_id)? { + if let Some(justification) = blockchain.justification(last_block_for_set_id)? { justification } else { trace!( @@ -218,34 +227,30 @@ where return Ok(None); }; - let block_id = BlockId::Number(block); - let block_authorities = authorities_provider.authorities(&block_id)?; // 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, authorities_set_id, &block_authorities); + 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.", - authorities_set_id, + set_id, ); return Ok(None); } - // Get all headers from the requested block until the last block of the set + // Collect all headers from the requested block until the last block of the set let unknown_headers = { - let mut unknown_headers = Vec::new(); - let mut current_number = block + One::one(); - let max_loop = current_number + 1000000.into(); // WIP(JON) + let mut headers = Vec::new(); + let mut current = block + One::one(); loop { - if current_number >= last_block_of_the_set || current_number >= max_loop { + if current >= last_block_for_set || headers.len() >= MAX_UNKNOWN_HEADERS { break; } - - let current_id = BlockId::Number(current_number); - if block_authorities != authorities_provider.authorities(¤t_id)? { + if block_authorities != authorities_provider.authorities(&BlockId::Number(current))? { trace!( target: "afg", "Encountered new authorities when collecting unknown headers. \ @@ -253,20 +258,20 @@ where ); return Ok(None); } - - let unknown_header = blockchain.expect_header(current_id)?; - unknown_headers.push(unknown_header); - current_number += One::one(); + headers.push(blockchain.expect_header(BlockId::Number(current))?); + current += One::one(); } - unknown_headers + headers }; - let finality_proof = FinalityProof { - block: last_block_of_the_set_hash, - justification, - unknown_headers, - }; - Ok(Some(finality_proof.encode())) + Ok(Some( + FinalityProof { + block: blockchain.expect_block_hash_from_id(&last_block_for_set_id)?, + justification, + unknown_headers, + } + .encode(), + )) } /// Check GRANDPA proof-of-finality for the given block. @@ -335,6 +340,7 @@ pub(crate) mod tests { use sc_client_api::NewBlockState; use sc_client_api::in_mem::Blockchain as InMemoryBlockchain; use sp_core::crypto::Public; + use crate::authorities::AuthoritySetChanges; pub(crate) type FinalityProof = super::FinalityProof
; @@ -395,37 +401,20 @@ pub(crate) mod tests { fn finality_proof_is_none_if_no_more_last_finalized_blocks() { let blockchain = test_blockchain(); blockchain - .insert(header(4).hash(), header(4), None, None, NewBlockState::Best) + .insert(header(4).hash(), header(4), Some(vec![1]), None, NewBlockState::Best) .unwrap(); - - // our last finalized is: 3 - // their last finalized is: 3 - // => we can't provide any additional justifications - let proof_of_4 = prove_finality::<_, _, TestJustification>( - &blockchain, - &|_| unreachable!("should return before calling GetAuthorities"), - 0, - *header(3).number(), - *header(4).number(), - ) - .unwrap(); - assert_eq!(proof_of_4, None); - } - - #[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) + .insert(header(5).hash(), header(5), Some(vec![2]), None, NewBlockState::Best) .unwrap(); - // block 4 is finalized without justification - // => we can't prove finality + 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, - &|_| unreachable!("should return before calling GetAuthorities"), - 0, - *header(3).number(), + &|_| unreachable!("Should return before calling GetAuthorities"), + authority_set_changes, *header(4).number(), ) .unwrap(); @@ -433,95 +422,25 @@ pub(crate) mod tests { } #[test] - fn finality_proof_works_for_basic_setup() { - let blockchain = test_blockchain(); - let authorities = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let just4 = TestJustification((0, authorities.clone()), vec![4]).encode(); - let just5 = TestJustification((0, authorities.clone()), vec![5]).encode(); - blockchain - .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) - .unwrap(); - blockchain - .insert(header(5).hash(), header(5), Some(just5.clone()), None, NewBlockState::Final) - .unwrap(); - - // Blocks 4 && 5 are finalized with justification - let proof_of_5: FinalityProof = Decode::decode( - &mut &prove_finality::<_, _, TestJustification>( - &blockchain, - &|_| Ok(authorities.clone()), - 0, - *header(3).number(), - *header(5).number(), - ) - .unwrap() - .unwrap()[..], - ) - .unwrap(); - assert_eq!( - proof_of_5, - FinalityProof { - block: header(5).hash(), - justification: just5, - unknown_headers: vec![header(4)], - } - ); - - // Now let's verify finality proof + 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(); - blockchain - .insert(header(5).hash(), header(5), None, None, NewBlockState::Final) - .unwrap(); - let proof = check_finality_proof::<_, TestJustification>( - 0, - authorities.clone(), - proof_of_5.encode(), - ) - .unwrap(); - assert_eq!(proof, proof_of_5); - } + let mut authority_set_changes = AuthoritySetChanges::empty(); + authority_set_changes.append(0, 4); - #[test] - fn finality_proof_check_fails_when_proof_decode_fails() { - // when we can't decode proof from Vec - check_finality_proof::<_, TestJustification>( - 1, - vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], - vec![42], - ) - .unwrap_err(); - } - - #[test] - fn finality_proof_check_fails_when_proof_is_empty() { - // when decoded proof has zero length - check_finality_proof::<_, TestJustification>( - 1, - vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], - Vec::::new().encode(), - ) - .unwrap_err(); - } - - #[test] - fn finality_proof_check_works() { - let initial_authorities = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; - let finality_proof = FinalityProof { - block: header(2).hash(), - justification: TestJustification((1, initial_authorities.clone()), vec![7]).encode(), - unknown_headers: Vec::new(), - }; - let proof = check_finality_proof::<_, TestJustification>( - 1, - initial_authorities.clone(), - finality_proof.encode(), + // Block 4 is finalized without justification + // => 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(), ) .unwrap(); - assert_eq!(proof, finality_proof); + assert_eq!(proof_of_3, None); } #[test] @@ -529,21 +448,20 @@ pub(crate) mod tests { // 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 just4 = TestJustification( - (0, vec![(AuthorityId::from_slice(&[42u8; 32]), 1u64)]), - vec![4], - ) - .encode(); + 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)]), - 0, + authority_set_changes, *header(3).number(), - *header(4).number(), ) .unwrap(); assert!(proof_of_3.is_none()); @@ -552,34 +470,35 @@ pub(crate) mod tests { #[test] fn finality_proof_is_none_if_authority_set_id_is_incorrect() { let blockchain = test_blockchain(); - let just4 = TestJustification( - (0, vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), - vec![4], - ) - .encode(); + 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(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]), - 1, + &|_| Ok(auth.clone()), + authority_set_changes, *header(3).number(), - *header(4).number(), ) .unwrap(); assert!(proof_of_3.is_none()); } #[test] - fn finality_proof_is_none_for_next_set_id_with_same_authorities() { + fn finality_proof_is_none_for_next_set_id_with_new_the_authority_set() { let blockchain = test_blockchain(); - let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]; - let just5 = TestJustification((0, auth.clone()), vec![5]).encode(); - let just6 = TestJustification((1, auth.clone()), vec![6]).encode(); + 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(); @@ -590,26 +509,32 @@ pub(crate) mod tests { .insert(header(6).hash(), header(6), Some(just6), None, NewBlockState::Final) .unwrap(); - // Try to prove finality from the next set_id, but which has the same authorities. + 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, - &|_| Ok(auth.clone()), - 1, + &|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(), - *header(6).number(), ) .unwrap(); - // WIP(JON): Currently this test fails, as the different set_id isn't detected. assert!(proof_of_4.is_none()); } #[test] - fn finality_proof_is_none_for_next_set_id_with_new_the_authority_set() { + 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(); @@ -619,60 +544,157 @@ pub(crate) mod tests { 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(); - 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), - }, + // 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 + check_finality_proof::<_, TestJustification>( 1, - *header(4).number(), - *header(6).number(), + vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], + vec![42], + ) + .unwrap_err(); + } + + #[test] + fn finality_proof_check_fails_when_proof_is_empty() { + // When decoded proof has zero length + check_finality_proof::<_, TestJustification>( + 1, + vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)], + Vec::::new().encode(), + ) + .unwrap_err(); + } + + #[test] + fn finality_proof_check_works() { + let auth = vec![(AuthorityId::from_slice(&[3u8; 32]), 1u64)]; + let finality_proof = FinalityProof { + block: header(2).hash(), + justification: TestJustification((1, auth.clone()), vec![7]).encode(), + unknown_headers: Vec::new(), + }; + let proof = check_finality_proof::<_, TestJustification>( + 1, + auth.clone(), + finality_proof.encode(), ) .unwrap(); - assert!(proof_of_4.is_none()); + assert_eq!(proof, finality_proof); } #[test] - fn finality_proof_is_none_if_the_authority_set_changes_and_changes_back() { + fn finality_proof_using_authority_set_changes_is_none_with_undefined_start() { 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(); + 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), None, None, NewBlockState::Final) + .insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final) .unwrap(); blockchain - .insert(header(5).hash(), header(5), Some(just5), None, NewBlockState::Final) + .insert(header(5).hash(), header(5), None, None, NewBlockState::Final) .unwrap(); blockchain - .insert(header(6).hash(), header(6), Some(just6), None, NewBlockState::Final) + .insert(header(6).hash(), header(6), None, None, NewBlockState::Final) .unwrap(); blockchain - .insert(header(7).hash(), header(7), Some(just7), None, NewBlockState::Final) + .insert(header(7).hash(), header(7), Some(just7.clone()), None, NewBlockState::Final) .unwrap(); - // Try to prove finality using a later set_id than the block we're trying to prove, but - // that has the same authorities. - let proof_of_4 = prove_finality::<_, _, TestJustification>( + // 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); + + let proof_of_5 = 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()), - BlockId::Number(7) => Ok(auth1.clone()), + BlockId::Number(5) => Ok(auth.clone()), + BlockId::Number(6) => Ok(auth.clone()), _ => unimplemented!("No other authorities should be proved: {:?}", block_id), }, - 2, - *header(4).number(), - *header(7).number(), + authority_set_changes, + *header(5).number(), ) .unwrap(); - assert!(proof_of_4.is_none()); + assert!(proof_of_5.is_none()); + } + + #[test] + fn finality_proof_using_authority_set_changes_works() { + 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 mut authority_set_changes = AuthoritySetChanges::empty(); + authority_set_changes.append(0, 4); + authority_set_changes.append(1, 7); + + 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(), + ) + .unwrap() + .unwrap()[..], + ) + .unwrap(); + assert_eq!( + proof_of_5, + FinalityProof { + block: header(7).hash(), + justification: just7, + unknown_headers: vec![header(6)], + } + ); } } From 79d1840e2c3222891d1119673d9f8489e1990745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 8 Dec 2020 22:44:54 +0100 Subject: [PATCH 18/28] grandpa: make AuthoritySetChanges tuple struct --- client/finality-grandpa/src/authorities.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index ba90f8c0ca6c0..9262a8bb4ce8c 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -692,30 +692,26 @@ impl + Clone> PendingChange { // Tracks historical authority set changes. We store the block numbers for the first block of each // authority set, once they have been finalalized. #[derive(Debug, Encode, Decode, Clone, PartialEq)] -pub struct AuthoritySetChanges { - authority_set_changes: Vec<(u64, N)>, // (set_id, block_number) -} +pub struct AuthoritySetChanges(pub Vec<(u64, N)>); impl AuthoritySetChanges { pub(crate) fn empty() -> Self { - Self { - authority_set_changes: Default::default(), - } + Self(Default::default()) } pub(crate) fn append(&mut self, set_id: u64, block_number: N) { - self.authority_set_changes.push((set_id, block_number)); + self.0.push((set_id, block_number)); } pub(crate) fn get_set_id(&self, block_number: N) -> Option<(u64, N)> { - let idx = self.authority_set_changes + let idx = self.0 .binary_search_by_key(&block_number, |(_, n)| n.clone()) .unwrap_or_else(|b| b); - if idx < self.authority_set_changes.len() { - let (set_id, block_number) = self.authority_set_changes[idx].clone(); + if idx < self.0.len() { + let (set_id, block_number) = self.0[idx].clone(); // To make sure we have the right set we need to check that the one before it also exists. if idx > 0 { - let (prev_set_id, _) = self.authority_set_changes[idx - 1usize]; + 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; From ef993a43e8bb63a5c781730177a4b3bf5c7a855e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 8 Dec 2020 23:59:48 +0100 Subject: [PATCH 19/28] grandpa: add assertions for tracking auth set changes --- client/finality-grandpa/src/authorities.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 9262a8bb4ce8c..effb943917bb0 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -919,6 +919,7 @@ mod tests { authorities.pending_changes().collect::>(), vec![&change_a], ); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges::empty()); // finalizing "hash_d" will enact the change signaled at "hash_a" let status = authorities.apply_standard_changes( @@ -937,6 +938,7 @@ mod tests { assert_eq!(authorities.current_authorities, set_a); assert_eq!(authorities.set_id, 1); assert_eq!(authorities.pending_changes().count(), 0); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)])); } #[test] @@ -989,6 +991,7 @@ mod tests { authorities.apply_standard_changes("hash_d", 40, &is_descendent_of, false), Err(Error::ForkTree(fork_tree::Error::UnfinalizedAncestor)) )); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges::empty()); let status = authorities.apply_standard_changes( "hash_b", @@ -1002,6 +1005,7 @@ mod tests { assert_eq!(authorities.current_authorities, set_a); assert_eq!(authorities.set_id, 1); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)])); // after finalizing `change_a` it should be possible to finalize `change_c` let status = authorities.apply_standard_changes( @@ -1016,6 +1020,7 @@ mod tests { assert_eq!(authorities.current_authorities, set_c); assert_eq!(authorities.set_id, 2); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15), (1, 40)])); } #[test] @@ -1176,7 +1181,7 @@ mod tests { set_id: 1, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), - authority_set_changes: AuthoritySetChanges::empty(), + authority_set_changes: AuthoritySetChanges(vec![(0, 42)]), }, ) ); @@ -1284,22 +1289,26 @@ mod tests { authorities.apply_forced_changes("hash_d45", 45, &static_is_descendent_of(true), false), Err(Error::ForcedAuthoritySetChangeDependencyUnsatisfied(15)) )); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges::empty()); // we apply the first pending standard change at #15 authorities .apply_standard_changes("hash_a15", 15, &static_is_descendent_of(true), false) .unwrap(); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)])); // but the forced change still depends on the next standard change assert!(matches!( authorities.apply_forced_changes("hash_d", 45, &static_is_descendent_of(true), false), Err(Error::ForcedAuthoritySetChangeDependencyUnsatisfied(20)) )); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)])); // we apply the pending standard change at #20 authorities .apply_standard_changes("hash_b", 20, &static_is_descendent_of(true), false) .unwrap(); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15), (1, 20)])); // afterwards the forced change at #45 can already be applied since it signals // that finality stalled at #31, and the next pending standard change is effective @@ -1316,10 +1325,11 @@ mod tests { set_id: 3, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), - authority_set_changes: AuthoritySetChanges::empty(), + authority_set_changes: AuthoritySetChanges(vec![(0, 15), (1, 20), (2, 31)]), } ), ); + assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15), (1, 20)])); } #[test] From abeb9eca59f8df5f4b603a48e591bdcd308b33fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 9 Dec 2020 00:46:32 +0100 Subject: [PATCH 20/28] grandpa: remove StorageAndProofProvider trait --- client/finality-grandpa/src/finality_proof.rs | 33 ++++++------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 033d7c39bf3a8..355b118abe21e 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -42,7 +42,7 @@ use log::trace; use sp_blockchain::{ Backend as BlockchainBackend, Error as ClientError, Result as ClientResult, }; -use sc_client_api::{backend::Backend, StorageProvider, ProofProvider}; +use sc_client_api::{backend::Backend, StorageProvider}; use parity_scale_codec::{Encode, Decode}; use finality_grandpa::BlockNumberOps; use sp_runtime::{ @@ -64,25 +64,9 @@ pub trait AuthoritySetForFinalityProver: Send + Sync { fn authorities(&self, block: &BlockId) -> ClientResult; } -/// Trait that combines `StorageProvider` and `ProofProvider` -pub trait StorageAndProofProvider: - StorageProvider + ProofProvider + Send + Sync -where - Block: BlockT, - BE: Backend + Send + Sync, -{} - -/// Blanket implementation. -impl StorageAndProofProvider for P -where - Block: BlockT, - BE: Backend + Send + Sync, - P: StorageProvider + ProofProvider + Send + Sync, -{} - /// Implementation of AuthoritySetForFinalityProver. impl AuthoritySetForFinalityProver - for Arc> + for Arc + Send + Sync> where BE: Backend + Send + Sync + 'static, { @@ -94,9 +78,10 @@ where .ok_or(ClientError::InvalidAuthoritiesSet) } } + /// Finality proof provider for serving network requests. -pub struct FinalityProofProvider { - backend: Arc, +pub struct FinalityProofProvider { + backend: Arc, authority_provider: Arc>, shared_authority_set: Option>>, } @@ -109,6 +94,7 @@ 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

( backend: Arc, authority_provider: P, @@ -127,15 +113,16 @@ where /// Create new finality proof provider for the service using: /// /// - backend for accessing blockchain data; - /// - storage_and_proof_provider, which is generally a client. + /// - storage_provider, which is generally a client. + /// - shared_authority_set for accessing authority set data pub fn new_for_service( backend: Arc, - storage_and_proof_provider: Arc>, + storage_provider: Arc + Send + Sync>, shared_authority_set: Option>>, ) -> Arc { Arc::new(Self::new( backend, - storage_and_proof_provider, + storage_provider, shared_authority_set, )) } From b14d97c7ee28f1f3ce50fc39203969783b744f1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 9 Dec 2020 16:06:44 +0100 Subject: [PATCH 21/28] grandpa: return more informative results for unexpected input to RPC --- client/finality-grandpa/src/finality_proof.rs | 14 ++++++++++---- primitives/blockchain/src/error.rs | 4 ++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 355b118abe21e..27e58d64e6ef4 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -183,13 +183,13 @@ where // Early-return if we sure that there are no blocks finalized AFTER begin block let info = blockchain.info(); if info.finalized_number <= block { - trace!( - target: "afg", + let err = format!( "Requested finality proof for descendant of #{} while we only have finalized #{}. Returning empty proof.", block, info.finalized_number, ); - return Ok(None); + trace!(target: "afg", "{}", &err); + return Err(ClientError::ProveFinalityRpc(err)); } // Get set_id the block belongs to, and the last block of the set which should contain a @@ -197,7 +197,13 @@ where let (set_id, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) { id } else { - return Ok(None); + let err = format!( + "AuthoritySetChanges does not cover the requested block #{}. \ + Maybe the subscription API is more appropriate.", + block, + ); + trace!(target: "afg", "{}", &err); + return Err(ClientError::ProveFinalityRpc(err)); }; // Get the Justification stored at the last block of the set diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index c7180a61b00ca..01fc62dc0ef82 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -99,6 +99,10 @@ pub enum Error { #[error("bad justification for header: {0}")] BadJustification(String), + // #[error("No AuthoritySetChanges entry for set id.")] + #[error("{0}")] + ProveFinalityRpc(String), + #[error("This method is not currently available when running in light client mode")] NotAvailableOnLightClient, From f2c3b236d26c423eaa4bd49fa97cf72b6143c4ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 9 Dec 2020 16:09:49 +0100 Subject: [PATCH 22/28] grandpa: tiny tweak to error msg --- client/finality-grandpa/src/finality_proof.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 27e58d64e6ef4..38d285e86185f 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -184,7 +184,7 @@ where let info = blockchain.info(); if info.finalized_number <= block { let err = format!( - "Requested finality proof for descendant of #{} while we only have finalized #{}. Returning empty proof.", + "Requested finality proof for descendant of #{} while we only have finalized #{}.", block, info.finalized_number, ); From 30341932354acca520e7a07643333c31dfdaf652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 9 Dec 2020 16:57:27 +0100 Subject: [PATCH 23/28] grandpa: fix tests --- client/finality-grandpa/src/finality_proof.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 38d285e86185f..55948e0157613 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -409,9 +409,8 @@ pub(crate) mod tests { &|_| unreachable!("Should return before calling GetAuthorities"), authority_set_changes, *header(4).number(), - ) - .unwrap(); - assert_eq!(proof_of_4, None); + ); + assert!(matches!(proof_of_4, Err(ClientError::ProveFinalityRpc(_)))); } #[test] @@ -638,9 +637,8 @@ pub(crate) mod tests { }, authority_set_changes, *header(5).number(), - ) - .unwrap(); - assert!(proof_of_5.is_none()); + ); + assert!(matches!(proof_of_5, Err(ClientError::ProveFinalityRpc(..)))); } #[test] From c8efdbbe7d482c6672effd7b933e14def3d79682 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 12 Jan 2021 10:33:36 +0100 Subject: [PATCH 24/28] grandpa: add error specific to finality_proof --- client/finality-grandpa/rpc/src/error.rs | 2 +- client/finality-grandpa/rpc/src/finality.rs | 4 +-- client/finality-grandpa/rpc/src/lib.rs | 2 +- client/finality-grandpa/src/finality_proof.rs | 28 +++++++++++++++---- client/finality-grandpa/src/lib.rs | 2 +- primitives/blockchain/src/error.rs | 4 --- 6 files changed, 27 insertions(+), 15 deletions(-) diff --git a/client/finality-grandpa/rpc/src/error.rs b/client/finality-grandpa/rpc/src/error.rs index 6122db03f8805..c812b78f3fd8e 100644 --- a/client/finality-grandpa/rpc/src/error.rs +++ b/client/finality-grandpa/rpc/src/error.rs @@ -30,7 +30,7 @@ pub enum Error { VoterStateReportsUnreasonablyLargeNumbers, /// GRANDPA prove finality failed. #[display(fmt = "GRANDPA prove finality rpc failed: {}", _0)] - ProveFinalityFailed(sp_blockchain::Error), + ProveFinalityFailed(sc_finality_grandpa::FinalityProofError), } /// The error codes returned by jsonrpc. diff --git a/client/finality-grandpa/rpc/src/finality.rs b/client/finality-grandpa/rpc/src/finality.rs index 83a32a6340ca9..cfd8f68e5ce60 100644 --- a/client/finality-grandpa/rpc/src/finality.rs +++ b/client/finality-grandpa/rpc/src/finality.rs @@ -31,7 +31,7 @@ pub trait RpcFinalityProofProvider { fn rpc_prove_finality( &self, block: NumberFor, - ) -> Result, sp_blockchain::Error>; + ) -> Result, sc_finality_grandpa::FinalityProofError>; } impl RpcFinalityProofProvider for FinalityProofProvider @@ -43,7 +43,7 @@ where fn rpc_prove_finality( &self, block: NumberFor, - ) -> Result, sp_blockchain::Error> { + ) -> Result, sc_finality_grandpa::FinalityProofError> { self.prove_finality(block) .map(|x| x.map(|y| EncodedFinalityProof(y.into()))) } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index bef20de35530d..204bea4c18e2c 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -255,7 +255,7 @@ mod tests { fn rpc_prove_finality( &self, _block: NumberFor - ) -> Result, sp_blockchain::Error> { + ) -> Result, sc_finality_grandpa::FinalityProofError> { Ok(Some(EncodedFinalityProof( self.finality_proof .as_ref() diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 7e662ab16571e..ccb6429a2c3c7 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -136,7 +136,9 @@ 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>, ClientError> { + pub fn prove_finality(&self, block: NumberFor) + -> Result>, FinalityProofError> + { let authority_set_changes = if let Some(changes) = self .shared_authority_set .as_ref() @@ -169,12 +171,26 @@ pub struct FinalityProof { pub unknown_headers: Vec

, } +/// Errors occurring when trying to prove finality +#[derive(Debug, derive_more::Display, derive_more::From)] +pub enum FinalityProofError { + /// The requested block has not yet been finalized. + #[display(fmt = "Block not yet finalized")] + BlockNotYetFinalized, + /// The requested block is not covered by authority set changes. Likely this means the block is + /// in the latest authority set, and the subscription API is more appropriate. + #[display(fmt = "Block not covered by authority set changes")] + BlockNotInAuthoritySetChanges, + /// Errors originating from the client. + Client(sp_blockchain::Error), +} + fn prove_finality( blockchain: &B, authorities_provider: &dyn AuthoritySetForFinalityProver, authority_set_changes: AuthoritySetChanges>, block: NumberFor, -) -> ::sp_blockchain::Result>> +) -> Result>, FinalityProofError> where Block: BlockT, B: BlockchainBackend, @@ -189,7 +205,7 @@ where info.finalized_number, ); trace!(target: "afg", "{}", &err); - return Err(ClientError::ProveFinalityRpc(err)); + return Err(FinalityProofError::BlockNotYetFinalized); } // Get set_id the block belongs to, and the last block of the set which should contain a @@ -203,7 +219,7 @@ where block, ); trace!(target: "afg", "{}", &err); - return Err(ClientError::ProveFinalityRpc(err)); + return Err(FinalityProofError::BlockNotInAuthoritySetChanges); }; // Get the Justification stored at the last block of the set @@ -410,7 +426,7 @@ pub(crate) mod tests { authority_set_changes, *header(4).number(), ); - assert!(matches!(proof_of_4, Err(ClientError::ProveFinalityRpc(_)))); + assert!(matches!(proof_of_4, Err(FinalityProofError::BlockNotYetFinalized))); } #[test] @@ -638,7 +654,7 @@ pub(crate) mod tests { authority_set_changes, *header(5).number(), ); - assert!(matches!(proof_of_5, Err(ClientError::ProveFinalityRpc(..)))); + assert!(matches!(proof_of_5, Err(FinalityProofError::BlockNotInAuthoritySetChanges))); } #[test] diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 96e1e7bdc2f80..934b119a5ef4e 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -122,7 +122,7 @@ mod until_imported; mod voting_rule; pub use authorities::{SharedAuthoritySet, AuthoritySet}; -pub use finality_proof::{FinalityProof, FinalityProofProvider}; +pub use finality_proof::{FinalityProof, FinalityProofProvider, FinalityProofError}; pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream}; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index a4cc51b517440..6ed5fe1b335f3 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -99,10 +99,6 @@ pub enum Error { #[error("bad justification for header: {0}")] BadJustification(String), - // #[error("No AuthoritySetChanges entry for set id.")] - #[error("{0}")] - ProveFinalityRpc(String), - #[error("This method is not currently available when running in light client mode")] NotAvailableOnLightClient, From ceee16aabeb08dcfad693fc8e00acfecd242e3ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 15 Jan 2021 22:54:54 +0100 Subject: [PATCH 25/28] grandpa: fix review comments --- client/finality-grandpa/src/authorities.rs | 2 +- client/finality-grandpa/src/finality_proof.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 307cd33d89006..c7ea653e58ddb 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -690,7 +690,7 @@ impl + Clone> PendingChange { } // Tracks historical authority set changes. We store the block numbers for the first block of each -// authority set, once they have been finalalized. +// authority set, once they have been finalized. #[derive(Debug, Encode, Decode, Clone, PartialEq)] pub struct AuthoritySetChanges(pub Vec<(u64, N)>); diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index ccb6429a2c3c7..bf60a3093e049 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -213,12 +213,12 @@ where let (set_id, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) { id } else { - let err = format!( + trace!( + target: "afg", "AuthoritySetChanges does not cover the requested block #{}. \ Maybe the subscription API is more appropriate.", block, ); - trace!(target: "afg", "{}", &err); return Err(FinalityProofError::BlockNotInAuthoritySetChanges); }; @@ -263,7 +263,7 @@ where trace!( target: "afg", "Encountered new authorities when collecting unknown headers. \ - Returning empty proof" + Returning empty proof", ); return Ok(None); } From aa6282e8112b06bc251134938994a79d057de24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 20 Jan 2021 22:25:16 +0100 Subject: [PATCH 26/28] grandpa: proper migration to new AuthoritySet --- client/finality-grandpa/src/authorities.rs | 43 +----- client/finality-grandpa/src/aux_schema.rs | 154 ++++++++++++++++++++- 2 files changed, 153 insertions(+), 44 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index c7ea653e58ddb..067f6dfc1ae65 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -21,7 +21,7 @@ use fork_tree::ForkTree; use parking_lot::RwLock; use finality_grandpa::voter_set::VoterSet; -use parity_scale_codec::{Encode, Decode, Input}; +use parity_scale_codec::{Encode, Decode}; use log::debug; use sc_telemetry::{telemetry, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList}; @@ -137,18 +137,8 @@ pub(crate) struct Status { pub(crate) new_set_block: Option<(H, N)>, } -// Same as `AuthoritySet`, but without the last field `authority_set_changes`. Only used during -// decoding. -#[derive(Debug, Clone, Encode, Decode, PartialEq)] -struct LegacyAuthoritySet { - current_authorities: AuthorityList, - set_id: u64, - pending_standard_changes: ForkTree>, - pending_forced_changes: Vec>, -} - /// A set of authorities. -#[derive(Debug, Clone, Encode, PartialEq)] +#[derive(Debug, Clone, Encode, Decode, PartialEq)] pub struct AuthoritySet { /// The current active authorities. pub(crate) current_authorities: AuthorityList, @@ -173,35 +163,6 @@ pub struct AuthoritySet { authority_set_changes: AuthoritySetChanges, } -impl Decode for AuthoritySet -where - H: Decode, - N: Decode + Clone + Ord -{ - fn decode(value: &mut I) -> Result { - let legacy = LegacyAuthoritySet::decode(value)?; - let authority_set_changes = match >::decode(value) { - Ok(v) => v, - Err(_) => AuthoritySetChanges::empty(), - }; - - let LegacyAuthoritySet { - current_authorities, - set_id, - pending_standard_changes, - pending_forced_changes, - } = legacy; - - Ok(AuthoritySet { - current_authorities, - set_id, - pending_standard_changes, - pending_forced_changes, - authority_set_changes, - }) - } -} - impl AuthoritySet where H: PartialEq, diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index c87db41d79137..25c3027945c33 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -41,7 +41,7 @@ const SET_STATE_KEY: &[u8] = b"grandpa_completed_round"; const CONCLUDED_ROUNDS: &[u8] = b"grandpa_concluded_rounds"; const AUTHORITY_SET_KEY: &[u8] = b"grandpa_voters"; -const CURRENT_VERSION: u32 = 2; +const CURRENT_VERSION: u32 = 3; /// The voter set state. #[derive(Debug, Clone, Encode, Decode)] @@ -110,6 +110,31 @@ where H: Clone + Debug + PartialEq, } } +impl Into> for V2AuthoritySet +where + H: Clone + Debug + PartialEq, + N: Clone + Debug + Ord, +{ + fn into(self) -> AuthoritySet { + AuthoritySet::new( + self.current_authorities, + self.set_id, + self.pending_standard_changes, + self.pending_forced_changes, + AuthoritySetChanges::empty(), + ) + .expect("current_authorities is non-empty and weights are non-zero; qed.") + } +} + +#[derive(Debug, Clone, Encode, Decode, PartialEq)] +struct V2AuthoritySet { + current_authorities: AuthorityList, + set_id: u64, + pending_standard_changes: ForkTree>, + pending_forced_changes: Vec>, +} + pub(crate) fn load_decode(backend: &B, key: &[u8]) -> ClientResult> { match backend.get_aux(key)? { None => Ok(None), @@ -260,6 +285,52 @@ fn migrate_from_version1( Ok(None) } +fn migrate_from_version2( + backend: &B, + genesis_round: &G, +) -> ClientResult>, + VoterSetState, +)>> +where + B: AuxStore, + G: Fn() -> RoundState>, +{ + CURRENT_VERSION.using_encoded(|s| + backend.insert_aux(&[(VERSION_KEY, s)], &[]) + )?; + + if let Some(old_set) = load_decode::<_, V2AuthoritySet>>( + backend, + AUTHORITY_SET_KEY, + )? { + let new_set: AuthoritySet> = old_set.into(); + backend.insert_aux(&[(AUTHORITY_SET_KEY, new_set.encode().as_slice())], &[])?; + + let set_state = match load_decode::<_, VoterSetState>( + backend, + SET_STATE_KEY, + )? { + Some(state) => state, + None => { + let state = genesis_round(); + let base = state.prevote_ghost + .expect("state is for completed round; completed rounds must have a prevote ghost; qed."); + + VoterSetState::live( + new_set.set_id, + &new_set, + base, + ) + } + }; + + return Ok(Some((new_set, set_state))); + } + + Ok(None) +} + /// Load or initialize persistent data from backend. pub(crate) fn load_persistent( backend: &B, @@ -294,6 +365,14 @@ pub(crate) fn load_persistent( } }, Some(2) => { + if let Some((new_set, set_state)) = migrate_from_version2::(backend, &make_genesis_round)? { + return Ok(PersistentData { + authority_set: new_set.into(), + set_state: set_state.into(), + }); + } + } + Some(3) => { if let Some(set) = load_decode::<_, AuthoritySet>>( backend, AUTHORITY_SET_KEY, @@ -477,7 +556,7 @@ mod test { assert_eq!( load_decode::<_, u32>(&client, VERSION_KEY).unwrap(), - Some(2), + Some(3), ); let PersistentData { authority_set, set_state, .. } = load_persistent::( @@ -569,7 +648,7 @@ mod test { assert_eq!( load_decode::<_, u32>(&client, VERSION_KEY).unwrap(), - Some(2), + Some(3), ); let PersistentData { authority_set, set_state, .. } = load_persistent::( @@ -611,6 +690,75 @@ mod test { ); } + #[test] + fn load_decode_from_v2_migrates_data_format() { + let client = substrate_test_runtime_client::new(); + + let authorities = vec![(AuthorityId::default(), 100)]; + let set_id = 3; + + { + let authority_set = V2AuthoritySet:: { + current_authorities: authorities.clone(), + set_id, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }; + + let genesis_state = (H256::random(), 32); + let voter_set_state: VoterSetState = VoterSetState::live( + set_id, + &authority_set.clone().into(), // Note the conversion! + genesis_state + ); + + client.insert_aux( + &[ + (AUTHORITY_SET_KEY, authority_set.encode().as_slice()), + (SET_STATE_KEY, voter_set_state.encode().as_slice()), + (VERSION_KEY, 2u32.encode().as_slice()), + ], + &[], + ).unwrap(); + } + + assert_eq!( + load_decode::<_, u32>(&client, VERSION_KEY).unwrap(), + Some(2), + ); + + // should perform the migration + load_persistent::( + &client, + H256::random(), + 0, + || unreachable!(), + ).unwrap(); + + assert_eq!( + load_decode::<_, u32>(&client, VERSION_KEY).unwrap(), + Some(3), + ); + + let PersistentData { authority_set, set_state, .. } = load_persistent::( + &client, + H256::random(), + 0, + || unreachable!(), + ).unwrap(); + + assert_eq!( + *authority_set.inner().read(), + AuthoritySet::new( + authorities.clone(), + set_id, + ForkTree::new(), + Vec::new(), + AuthoritySetChanges::empty(), + ).unwrap(), + ); + } + #[test] fn write_read_concluded_rounds() { let client = substrate_test_runtime_client::new(); From 6d90ce31fc8360d066b673151cd0bcbd98818e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 21 Jan 2021 13:04:32 +0100 Subject: [PATCH 27/28] grandpa: fix long lines --- client/finality-grandpa/src/aux_schema.rs | 127 ++++++++++++++-------- 1 file changed, 81 insertions(+), 46 deletions(-) diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 25c3027945c33..a5092334b99f5 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -71,8 +71,9 @@ struct V0AuthoritySet { } impl Into> for V0AuthoritySet -where H: Clone + Debug + PartialEq, - N: Clone + Debug + Ord, +where + H: Clone + Debug + PartialEq, + N: Clone + Debug + Ord, { fn into(self) -> AuthoritySet { let mut pending_standard_changes = ForkTree::new(); @@ -135,13 +136,14 @@ struct V2AuthoritySet { pending_forced_changes: Vec>, } -pub(crate) fn load_decode(backend: &B, key: &[u8]) -> ClientResult> { +pub(crate) fn load_decode( + backend: &B, + key: &[u8] +) -> ClientResult> { match backend.get_aux(key)? { None => Ok(None), Some(t) => T::decode(&mut &t[..]) - .map_err( - |e| ClientError::Backend(format!("GRANDPA DB is corrupted: {}", e.what())), - ) + .map_err(|e| ClientError::Backend(format!("GRANDPA DB is corrupted: {}", e.what()))) .map(Some) } } @@ -155,11 +157,15 @@ pub(crate) struct PersistentData { fn migrate_from_version0( backend: &B, genesis_round: &G, -) -> ClientResult>, - VoterSetState, -)>> where B: AuxStore, - G: Fn() -> RoundState>, +) -> ClientResult< + Option<( + AuthoritySet>, + VoterSetState, + )>, +> +where + B: AuxStore, + G: Fn() -> RoundState>, { CURRENT_VERSION.using_encoded(|s| backend.insert_aux(&[(VERSION_KEY, s)], &[]) @@ -172,18 +178,20 @@ fn migrate_from_version0( let new_set: AuthoritySet> = old_set.into(); backend.insert_aux(&[(AUTHORITY_SET_KEY, new_set.encode().as_slice())], &[])?; - let (last_round_number, last_round_state) = match load_decode::<_, V0VoterSetState>>( - backend, - SET_STATE_KEY, - )? { + let (last_round_number, last_round_state) = match load_decode::< + _, + V0VoterSetState>, + >(backend, SET_STATE_KEY)? + { Some((number, state)) => (number, state), None => (0, genesis_round()), }; let set_id = new_set.set_id; - let base = last_round_state.prevote_ghost - .expect("state is for completed round; completed rounds must have a prevote ghost; qed."); + let base = last_round_state.prevote_ghost.expect( + "state is for completed round; completed rounds must have a prevote ghost; qed." + ); let mut current_rounds = CurrentRounds::new(); current_rounds.insert(last_round_number + 1, HasVoted::No); @@ -213,11 +221,15 @@ fn migrate_from_version0( fn migrate_from_version1( backend: &B, genesis_round: &G, -) -> ClientResult>, - VoterSetState, -)>> where B: AuxStore, - G: Fn() -> RoundState>, +) -> ClientResult< + Option<( + AuthoritySet>, + VoterSetState, + )>, +> +where + B: AuxStore, + G: Fn() -> RoundState>, { CURRENT_VERSION.using_encoded(|s| backend.insert_aux(&[(VERSION_KEY, s)], &[]) @@ -288,10 +300,12 @@ fn migrate_from_version1( fn migrate_from_version2( backend: &B, genesis_round: &G, -) -> ClientResult>, - VoterSetState, -)>> +) -> ClientResult< + Option<( + AuthoritySet>, + VoterSetState, + )>, +> where B: AuxStore, G: Fn() -> RoundState>, @@ -337,11 +351,10 @@ pub(crate) fn load_persistent( genesis_hash: Block::Hash, genesis_number: NumberFor, genesis_authorities: G, -) - -> ClientResult> - where - B: AuxStore, - G: FnOnce() -> ClientResult, +) -> ClientResult> +where + B: AuxStore, + G: FnOnce() -> ClientResult, { let version: Option = load_decode(backend, VERSION_KEY)?; @@ -349,7 +362,9 @@ pub(crate) fn load_persistent( match version { None => { - if let Some((new_set, set_state)) = migrate_from_version0::(backend, &make_genesis_round)? { + if let Some((new_set, set_state)) = + migrate_from_version0::(backend, &make_genesis_round)? + { return Ok(PersistentData { authority_set: new_set.into(), set_state: set_state.into(), @@ -357,7 +372,9 @@ pub(crate) fn load_persistent( } }, Some(1) => { - if let Some((new_set, set_state)) = migrate_from_version1::(backend, &make_genesis_round)? { + if let Some((new_set, set_state)) = + migrate_from_version1::(backend, &make_genesis_round)? + { return Ok(PersistentData { authority_set: new_set.into(), set_state: set_state.into(), @@ -365,7 +382,9 @@ pub(crate) fn load_persistent( } }, Some(2) => { - if let Some((new_set, set_state)) = migrate_from_version2::(backend, &make_genesis_round)? { + if let Some((new_set, set_state)) = + migrate_from_version2::(backend, &make_genesis_round)? + { return Ok(PersistentData { authority_set: new_set.into(), set_state: set_state.into(), @@ -446,7 +465,8 @@ pub(crate) fn update_authority_set( set: &AuthoritySet>, new_set: Option<&NewAuthoritySet>>, write_aux: F -) -> R where +) -> R +where F: FnOnce(&[(&'static [u8], &[u8])]) -> R, { // write new authority set state to disk. @@ -496,8 +516,9 @@ pub(crate) fn write_concluded_round( } #[cfg(test)] -pub(crate) fn load_authorities(backend: &B) - -> Option> { +pub(crate) fn load_authorities( + backend: &B +) -> Option> { load_decode::<_, AuthoritySet>(backend, AUTHORITY_SET_KEY) .expect("backend error") } @@ -559,7 +580,11 @@ mod test { Some(3), ); - let PersistentData { authority_set, set_state, .. } = load_persistent::( + let PersistentData { + authority_set, + set_state, + .. + } = load_persistent::( &client, H256::random(), 0, @@ -651,7 +676,11 @@ mod test { Some(3), ); - let PersistentData { authority_set, set_state, .. } = load_persistent::( + let PersistentData { + authority_set, + set_state, + .. + } = load_persistent::( &client, H256::random(), 0, @@ -706,11 +735,12 @@ mod test { }; let genesis_state = (H256::random(), 32); - let voter_set_state: VoterSetState = VoterSetState::live( - set_id, - &authority_set.clone().into(), // Note the conversion! - genesis_state - ); + let voter_set_state: VoterSetState = + VoterSetState::live( + set_id, + &authority_set.clone().into(), // Note the conversion! + genesis_state + ); client.insert_aux( &[ @@ -740,7 +770,10 @@ mod test { Some(3), ); - let PersistentData { authority_set, set_state, .. } = load_persistent::( + let PersistentData { + authority_set, + .. + } = load_persistent::( &client, H256::random(), 0, @@ -779,7 +812,9 @@ mod test { round_number.using_encoded(|n| key.extend(n)); assert_eq!( - load_decode::<_, CompletedRound::>(&client, &key).unwrap(), + load_decode::<_, CompletedRound::>( + &client, &key + ).unwrap(), Some(completed_round), ); } From a8878a15f993769ef31c9e33efea49b21803c79f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 21 Jan 2021 22:34:07 +0100 Subject: [PATCH 28/28] grandpa: fix unused warning after merge --- client/finality-grandpa/src/finality_proof.rs | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 7488ccdbfcae9..73726146af4e0 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -202,24 +202,6 @@ pub(crate) struct AuthoritySetProofFragment { /// - last fragment match target block. type AuthoritySetProof
= Vec>; -/// Finality proof request data. -#[derive(Debug, Encode, Decode)] -enum FinalityProofRequest { - /// Original version of the request. - Original(OriginalFinalityProofRequest), -} - -/// Original version of finality proof request. -#[derive(Debug, Encode, Decode)] -struct OriginalFinalityProofRequest { - /// The authorities set id we are waiting proof from. - /// - /// The first justification in the proof must be signed by this authority set. - pub authorities_set_id: u64, - /// Hash of the last known finalized block. - pub last_finalized: H, -} - fn prove_finality( blockchain: &B, authorities_provider: &dyn AuthoritySetForFinalityProver, @@ -448,6 +430,7 @@ fn get_warp_sync_proof_fragment>( /// Check GRANDPA authority change sequence to assert finality of a target block. /// /// Returns the header of the target block. +#[allow(unused)] pub(crate) fn check_warp_sync_proof( current_set_id: u64, current_authorities: AuthorityList,