Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

[9177] finality_target returns a hash instead of an option #9192

Closed
tau3 wants to merge 6 commits into from
Closed

[9177] finality_target returns a hash instead of an option #9192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e034fad
make finality_target return target hash in case of absence
tau3 Jun 23, 2021
b69cf5d
revert changes affected by formatting
tau3 Jun 23, 2021
7b3cb50
refactor changed function; fix unit test
tau3 Jun 23, 2021
4f9c10d
fixes after code review
tau3 Aug 10, 2021
c175353
making linter happy
tau3 Aug 10, 2021
26f853e
Merge remote-tracking branch 'upstream/master' into feature/9177/fina…
tau3 Aug 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
make finality_target return target hash in case of absence
  • Loading branch information
@tau3
tau3 committed Jun 23, 2021
commit e034fad05034357d51e180cd9d425a561341294f
27 changes: 16 additions & 11 deletions client/consensus/common/src/longest_chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,23 @@ use sp_runtime::{
/// where 'longest' is defined as the highest number of blocks
pub struct LongestChain<B, Block> {
backend: Arc<B>,
_phantom: PhantomData<Block>
_phantom: PhantomData<Block>,
}

impl<B, Block> Clone for LongestChain<B, Block> {
fn clone(&self) -> Self {
let backend = self.backend.clone();
LongestChain {
backend,
_phantom: Default::default()
_phantom: Default::default(),
}
}
}

impl<B, Block> LongestChain<B, Block>
where
B: backend::Backend<Block>,
Block: BlockT,
where
B: backend::Backend<Block>,
Block: BlockT,
Copy link
Contributor

@andresilva andresilva Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop these formatting changes.

{
/// Instantiate a new LongestChain for Backend B
pub fn new(backend: Arc<B>) -> Self {
Expand Down Expand Up @@ -77,9 +77,9 @@ where

#[async_trait::async_trait]
impl<B, Block> SelectChain<Block> for LongestChain<B, Block>
where
B: backend::Backend<Block>,
Block: BlockT,
where
B: backend::Backend<Block>,
Block: BlockT,
Copy link
Contributor

@andresilva andresilva Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these as well.

{
async fn leaves(&self) -> Result<Vec<<Block as BlockT>::Hash>, ConsensusError> {
LongestChain::leaves(self).map_err(|e| ConsensusError::ChainLookup(e.to_string()).into())
Expand All @@ -94,11 +94,16 @@ where
&self,
target_hash: Block::Hash,
maybe_max_number: Option<NumberFor<Block>>,
) -> Result<Option<Block::Hash>, ConsensusError> {
) -> Result<Block::Hash, ConsensusError> {
let import_lock = self.backend.get_import_lock();
self.backend
let x = self.backend
.blockchain()
.best_containing(target_hash, maybe_max_number, import_lock)
.map_err(|e| ConsensusError::ChainLookup(e.to_string()).into())
.map_err(|e| ConsensusError::ChainLookup(e.to_string()).into());
match x {
Err(e) => Err(e),
Ok(Some(h)) => Ok(h),
Ok(None) => Ok(target_hash),
}
}
}
6 changes: 1 addition & 5 deletions client/finality-grandpa/src/environment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,7 +1173,7 @@ where
debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit);

let result = match select_chain.finality_target(block, None).await {
Ok(Some(best_hash)) => {
Ok(best_hash) => {
let best_header = client
.header(BlockId::Hash(best_hash))?
.expect("Header known to exist after `finality_target` call; qed");
Copy link
Contributor

@andresilva andresilva Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may no longer be true, there is a possibility that SelectChain::finality_target just returned the passed value without checking the DB at all (in practice it shouldn't happen as we can only have previously finalized something that already existed in the database). Either way we should not call expect here and instead handle the case where we get None from this call. Something like this should work:

let best_header = match client.header(BlockId::Hash(best_hash))? {
    Some(header) => header,
    None => {
        debug!(target: "afg", "Couldn't find target block from `finality_target`: {:?}", best_hash);
        return Ok(None);
    },
};

Expand Down Expand Up @@ -1231,10 +1231,6 @@ where
})
.or_else(|| Some((target_header.hash(), *target_header.number())))
}
Ok(None) => {
debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block);
None
}
Err(e) => {
debug!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e);
None
Expand Down
4 changes: 2 additions & 2 deletions client/finality-grandpa/src/import.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ where
)
.await
} else {
Ok(Some(pending_change.canon_hash))
Ok(pending_change.canon_hash)
};

if let Ok(Some(hash)) = effective_block_hash {
if let Ok(hash) = effective_block_hash {
if let Ok(Some(header)) = self.inner.header(BlockId::Hash(hash)) {
if *header.number() == pending_change.effective_number() {
out.push((header.hash(), *header.number()));
Expand Down
Loading