From a9a55d1dcf3809ce37a0fcd9b68b61375fb5d3b6 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 1 Apr 2021 17:54:16 +0200 Subject: [PATCH 1/3] Another tweak to GrandPa warp sync --- .../finality-grandpa-warp-sync/src/proof.rs | 52 +++++++++---------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/client/finality-grandpa-warp-sync/src/proof.rs b/client/finality-grandpa-warp-sync/src/proof.rs index 6b7002555d39a..c0517ffebf4a9 100644 --- a/client/finality-grandpa-warp-sync/src/proof.rs +++ b/client/finality-grandpa-warp-sync/src/proof.rs @@ -24,7 +24,7 @@ use sp_blockchain::{Backend as BlockchainBackend, HeaderBackend}; use sp_finality_grandpa::{AuthorityList, SetId, GRANDPA_ENGINE_ID}; use sp_runtime::{ generic::BlockId, - traits::{Block as BlockT, NumberFor, One}, + traits::{Block as BlockT, Header as HeaderT, NumberFor, One}, }; use crate::HandleRequestError; @@ -43,21 +43,11 @@ pub struct AuthoritySetChangeProof { pub justification: GrandpaJustification, } -/// Represents the current state of the warp sync, namely whether it is considered -/// finished, i.e. we have proved everything up until the latest authority set, or not. -/// When the warp sync is finished we might optionally provide a justification for the -/// latest finalized block, which should be checked against the latest authority set. -#[derive(Debug, Decode, Encode)] -pub enum WarpSyncFinished { - No, - Yes(Option>), -} - /// An accumulated proof of multiple authority set changes. #[derive(Decode, Encode)] pub struct WarpSyncProof { proofs: Vec>, - is_finished: WarpSyncFinished, + is_finished: bool, } impl WarpSyncProof { @@ -137,9 +127,9 @@ impl WarpSyncProof { } let is_finished = if proof_limit_reached { - WarpSyncFinished::No + false } else { - let latest = + let latest_justification = sc_finality_grandpa::best_justification(backend)?.filter(|justification| { // the existing best justification must be for a block higher than the // last authority set change. if we didn't prove any authority set @@ -153,7 +143,17 @@ impl WarpSyncProof { justification.target().0 >= limit }); - WarpSyncFinished::Yes(latest) + if let Some(latest_justification) = latest_justification { + let header = blockchain.header(BlockId::Hash(latest_justification.target().1))? + .expect("header hash corresponds to a justification in db; must exist in db as well; qed."); + + proofs.push(AuthoritySetChangeProof { + header, + justification: latest_justification, + }) + } + + true }; Ok(WarpSyncProof { @@ -181,20 +181,16 @@ impl WarpSyncProof { .verify(current_set_id, ¤t_authorities) .map_err(|err| HandleRequestError::InvalidProof(err.to_string()))?; - let scheduled_change = find_scheduled_change::(&proof.header).ok_or( - HandleRequestError::InvalidProof( - "Header is missing authority set change digest".to_string(), - ), - )?; - - current_authorities = scheduled_change.next_authorities; - current_set_id += 1; - } + if proof.justification.target().1 != proof.header.hash() { + return Err(HandleRequestError::InvalidProof( + "mismatch between header and justification".to_owned() + )); + } - if let WarpSyncFinished::Yes(Some(ref justification)) = self.is_finished { - justification - .verify(current_set_id, ¤t_authorities) - .map_err(|err| HandleRequestError::InvalidProof(err.to_string()))?; + if let Some(scheduled_change) = find_scheduled_change::(&proof.header) { + current_authorities = scheduled_change.next_authorities; + current_set_id += 1; + } } Ok((current_set_id, current_authorities)) From 85a43bb054d0f3d04eda0737312db2c3f15b06af Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 1 Apr 2021 18:57:01 +0200 Subject: [PATCH 2/3] Rename to WarpSyncFragment --- client/finality-grandpa-warp-sync/src/lib.rs | 2 +- client/finality-grandpa-warp-sync/src/proof.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/client/finality-grandpa-warp-sync/src/lib.rs b/client/finality-grandpa-warp-sync/src/lib.rs index 52e18e38909c4..807397f05780b 100644 --- a/client/finality-grandpa-warp-sync/src/lib.rs +++ b/client/finality-grandpa-warp-sync/src/lib.rs @@ -31,7 +31,7 @@ use sc_finality_grandpa::SharedAuthoritySet; mod proof; -pub use proof::{AuthoritySetChangeProof, WarpSyncProof}; +pub use proof::{WarpSyncFragment, WarpSyncProof}; /// Generates the appropriate [`RequestResponseConfig`] for a given chain configuration. pub fn request_response_config_for_chain + 'static>( diff --git a/client/finality-grandpa-warp-sync/src/proof.rs b/client/finality-grandpa-warp-sync/src/proof.rs index c0517ffebf4a9..5be1b955b2014 100644 --- a/client/finality-grandpa-warp-sync/src/proof.rs +++ b/client/finality-grandpa-warp-sync/src/proof.rs @@ -34,7 +34,7 @@ const MAX_CHANGES_PER_WARP_SYNC_PROOF: usize = 256; /// A proof of an authority set change. #[derive(Decode, Encode)] -pub struct AuthoritySetChangeProof { +pub struct WarpSyncFragment { /// The last block that the given authority set finalized. This block should contain a digest /// signaling an authority set change from which we can fetch the next authority set. pub header: Block::Header, @@ -46,7 +46,7 @@ pub struct AuthoritySetChangeProof { /// An accumulated proof of multiple authority set changes. #[derive(Decode, Encode)] pub struct WarpSyncProof { - proofs: Vec>, + proofs: Vec>, is_finished: bool, } @@ -120,7 +120,7 @@ impl WarpSyncProof { let justification = GrandpaJustification::::decode(&mut &justification[..])?; - proofs.push(AuthoritySetChangeProof { + proofs.push(WarpSyncFragment { header: header.clone(), justification, }); @@ -147,7 +147,7 @@ impl WarpSyncProof { let header = blockchain.header(BlockId::Hash(latest_justification.target().1))? .expect("header hash corresponds to a justification in db; must exist in db as well; qed."); - proofs.push(AuthoritySetChangeProof { + proofs.push(WarpSyncFragment { header, justification: latest_justification, }) From 3ef840f79ca9e655a10cba4cb067021e93f7f45b Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 2 Apr 2021 10:58:44 +0200 Subject: [PATCH 3/3] Ensure proof is minimal --- client/finality-grandpa-warp-sync/src/proof.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/client/finality-grandpa-warp-sync/src/proof.rs b/client/finality-grandpa-warp-sync/src/proof.rs index 5be1b955b2014..08effcf1c24b3 100644 --- a/client/finality-grandpa-warp-sync/src/proof.rs +++ b/client/finality-grandpa-warp-sync/src/proof.rs @@ -175,7 +175,7 @@ impl WarpSyncProof { let mut current_set_id = set_id; let mut current_authorities = authorities; - for proof in &self.proofs { + for (fragment_num, proof) in self.proofs.iter().enumerate() { proof .justification .verify(current_set_id, ¤t_authorities) @@ -190,6 +190,12 @@ impl WarpSyncProof { if let Some(scheduled_change) = find_scheduled_change::(&proof.header) { current_authorities = scheduled_change.next_authorities; current_set_id += 1; + } else if fragment_num != self.proofs.len() - 1 { + // Only the last fragment of the proof is allowed to be missing the authority + // set change. + return Err(HandleRequestError::InvalidProof( + "Header is missing authority set change digest".to_string(), + )); } }