From 3e608712c96ed501a63c1dfd0b8d4919d3614ca5 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Wed, 3 Feb 2021 16:45:47 +0100 Subject: [PATCH 1/2] Fix some problems with prove_warp_sync --- client/finality-grandpa-warp-sync/src/lib.rs | 6 +-- client/finality-grandpa/src/finality_proof.rs | 49 ++++++++++--------- 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/client/finality-grandpa-warp-sync/src/lib.rs b/client/finality-grandpa-warp-sync/src/lib.rs index cae28173f09ee..f7ce59b1c168f 100644 --- a/client/finality-grandpa-warp-sync/src/lib.rs +++ b/client/finality-grandpa-warp-sync/src/lib.rs @@ -86,9 +86,9 @@ struct Request { const WARP_SYNC_FRAGMENTS_LIMIT: usize = 100; /// Number of item with justification in warp sync cache. -/// This should be customizable, setting a low number -/// until then. -const WARP_SYNC_CACHE_SIZE: usize = 20; +/// This should be customizable, but setting it to the max number of fragments +/// we return seems like a good idea until then. +const WARP_SYNC_CACHE_SIZE: usize = WARP_SYNC_FRAGMENTS_LIMIT; /// Handler for incoming grandpa warp sync requests from a remote peer. pub struct GrandpaWarpSyncRequestHandler { diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index e17045349d452..55dceb4cfaf41 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -277,9 +277,9 @@ pub fn prove_warp_sync>( // This operation is a costy and only for the delay corner case. while index > Zero::zero() { index = index - One::one(); - if let Some((fragement, apply_block)) = get_warp_sync_proof_fragment(blockchain, index, &mut cache)? { + if let Some((fragment, apply_block)) = get_warp_sync_proof_fragment(blockchain, index, &mut cache)? { if last_apply.map(|next| &next > header.number()).unwrap_or(false) { - result.push(fragement); + result.push(fragment); last_apply = Some(apply_block); } else { break; @@ -289,7 +289,7 @@ pub fn prove_warp_sync>( let mut index = *header.number(); while index <= end_number { - if max_fragment_limit.map(|limit| result.len() <= limit).unwrap_or(false) { + if max_fragment_limit.map(|limit| result.len() >= limit).unwrap_or(false) { break; } @@ -305,7 +305,9 @@ pub fn prove_warp_sync>( index = index + One::one(); } - if result.last().as_ref().map(|head| head.header.number()) != Some(&end_number) { + let at_limit = max_fragment_limit.map(|limit| result.len() >= limit).unwrap_or(false); + + if !at_limit && result.last().as_ref().map(|head| head.header.number()) != Some(&end_number) { let header = blockchain.expect_header(end)?; if let Some(justification) = blockchain.justification(BlockId::Number(end_number.clone()))? { result.push(AuthoritySetProofFragment { @@ -328,7 +330,7 @@ fn get_warp_sync_proof_fragment>( ) -> sp_blockchain::Result, NumberFor)>> { if let Some(cache) = cache.as_mut() { if let Some(result) = cache.get_item(index) { - return Ok(result.clone()); + return Ok(result); } } @@ -541,11 +543,11 @@ impl BlockJustification for GrandpaJustification { + header_has_proof_fragment: std::collections::HashMap, cache: linked_hash_map::LinkedHashMap< Header::Number, - Option<(AuthoritySetProofFragment
, Header::Number)>, + (AuthoritySetProofFragment
, Header::Number), >, - headers_with_justification: usize, limit: usize, } @@ -553,8 +555,8 @@ impl WarpSyncFragmentCache
{ /// Instantiate a new cache for the warp sync prover. pub fn new(size: usize) -> Self { WarpSyncFragmentCache { + header_has_proof_fragment: Default::default(), cache: Default::default(), - headers_with_justification: 0, limit: size, } } @@ -564,31 +566,32 @@ impl WarpSyncFragmentCache
{ at: Header::Number, item: Option<(AuthoritySetProofFragment
, Header::Number)>, ) { - if self.cache.len() == self.limit { - self.pop_one(); - } - if item.is_some() { - // we do not check previous value as cached value is always supposed to - // be queried before calling 'new_item'. - self.headers_with_justification += 1; + self.header_has_proof_fragment.insert(at, item.is_some()); + + if let Some(item) = item { + if self.cache.len() == self.limit { + self.pop_one(); + } + + self.cache.insert(at, item); } - self.cache.insert(at, item); } fn pop_one(&mut self) { - while let Some(v) = self.cache.pop_front() { - if v.1.is_some() { - self.headers_with_justification -= 1; - break; - } + if let Some((header_number, _)) = self.cache.pop_front() { + self.header_has_proof_fragment.remove(&header_number); } } fn get_item( &mut self, block: Header::Number, - ) -> Option<&mut Option<(AuthoritySetProofFragment
, Header::Number)>> { - self.cache.get_refresh(&block) + ) -> Option, Header::Number)>> { + match self.header_has_proof_fragment.get(&block) { + Some(true) => Some(self.cache.get_refresh(&block).cloned()), + Some(false) => Some(None), + None => None + } } } From 38afb53f9a14a2aaf65ff47347fcbabcc4dae6eb Mon Sep 17 00:00:00 2001 From: Ashley Date: Thu, 4 Feb 2021 12:02:18 +0100 Subject: [PATCH 2/2] Update client/finality-grandpa/src/finality_proof.rs Co-authored-by: cheme --- client/finality-grandpa/src/finality_proof.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 55dceb4cfaf41..e1e424472ff98 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -307,6 +307,7 @@ pub fn prove_warp_sync>( let at_limit = max_fragment_limit.map(|limit| result.len() >= limit).unwrap_or(false); + // add last finalized block if reached and not already included. if !at_limit && result.last().as_ref().map(|head| head.header.number()) != Some(&end_number) { let header = blockchain.expect_header(end)?; if let Some(justification) = blockchain.justification(BlockId::Number(end_number.clone()))? {