Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 10 additions & 8 deletions client/consensus/pow/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
//! clients.

use std::sync::Arc;
use std::any::Any;
use std::borrow::Cow;
use std::thread;
use std::collections::HashMap;
use std::marker::PhantomData;
Expand Down Expand Up @@ -63,8 +65,6 @@ pub enum Error<B: BlockT> {
InvalidSeal,
#[display(fmt = "PoW validation error: invalid difficulty")]
InvalidDifficulty,
#[display(fmt = "PoW block import expects an intermediate, but not found one")]
NoIntermediate,
#[display(fmt = "Rejecting block too far in future")]
TooFarInFuture,
#[display(fmt = "Fetching best header failed using select chain: {:?}", _0)]
Expand Down Expand Up @@ -257,6 +257,7 @@ impl<B, I, C, S, Algorithm> BlockImport<B> for PowBlockImport<B, I, C, S, Algori
C: ProvideRuntimeApi<B> + Send + Sync + HeaderBackend<B> + AuxStore + ProvideCache<B> + BlockOf,
C::Api: BlockBuilderApi<B, Error = sp_blockchain::Error>,
Algorithm: PowAlgorithm<B>,
Algorithm::Difficulty: 'static,
{
type Error = ConsensusError;
type Transaction = sp_api::TransactionFor<C, B>;
Expand Down Expand Up @@ -312,10 +313,9 @@ impl<B, I, C, S, Algorithm> BlockImport<B> for PowBlockImport<B, I, C, S, Algori
_ => return Err(Error::<B>::HeaderUnsealed(block.header.hash()).into()),
};

let intermediate = PowIntermediate::<B, Algorithm::Difficulty>::decode(
&mut &block.intermediates.remove(INTERMEDIATE_KEY)
.ok_or(Error::<B>::NoIntermediate)?[..]
).map_err(|_| Error::<B>::NoIntermediate)?;
let intermediate = block.take_intermediate::<PowIntermediate::<B, Algorithm::Difficulty>>(
INTERMEDIATE_KEY
)?;

let difficulty = match intermediate.difficulty {
Some(difficulty) => difficulty,
Expand Down Expand Up @@ -392,6 +392,7 @@ impl<B: BlockT, Algorithm> PowVerifier<B, Algorithm> {

impl<B: BlockT, Algorithm> Verifier<B> for PowVerifier<B, Algorithm> where
Algorithm: PowAlgorithm<B> + Send + Sync,
Algorithm::Difficulty: 'static,
{
fn verify(
&mut self,
Expand All @@ -418,7 +419,7 @@ impl<B: BlockT, Algorithm> Verifier<B> for PowVerifier<B, Algorithm> where
justification,
intermediates: {
let mut ret = HashMap::new();
ret.insert(INTERMEDIATE_KEY.to_vec(), intermediate.encode());
ret.insert(Cow::from(INTERMEDIATE_KEY), Box::new(intermediate) as Box<dyn Any>);
ret
},
auxiliary: vec![],
Expand Down Expand Up @@ -553,6 +554,7 @@ fn mine_loop<B: BlockT, C, Algorithm, E, SO, S, CAW>(
) -> Result<(), Error<B>> where
C: HeaderBackend<B> + AuxStore + ProvideRuntimeApi<B>,
Algorithm: PowAlgorithm<B>,
Algorithm::Difficulty: 'static,
E: Environment<B>,
E::Proposer: Proposer<B, Transaction = sp_api::TransactionFor<C, B>>,
E::Error: std::fmt::Debug,
Expand Down Expand Up @@ -659,7 +661,7 @@ fn mine_loop<B: BlockT, C, Algorithm, E, SO, S, CAW>(
storage_changes: Some(proposal.storage_changes),
intermediates: {
let mut ret = HashMap::new();
ret.insert(INTERMEDIATE_KEY.to_vec(), intermediate.encode());
ret.insert(Cow::from(INTERMEDIATE_KEY), Box::new(intermediate) as Box<dyn Any>);
ret
},
finalized: false,
Expand Down
34 changes: 33 additions & 1 deletion primitives/consensus/common/src/block_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ use serde::{Serialize, Deserialize};
use std::borrow::Cow;
use std::collections::HashMap;
use std::sync::Arc;
use std::any::Any;

use crate::Error;
use crate::import_queue::{Verifier, CacheKeyId};

/// Block import result.
Expand Down Expand Up @@ -144,7 +146,7 @@ pub struct BlockImportParams<Block: BlockT, Transaction> {
/// Intermediate values that are interpreted by block importers. Each block importer,
/// upon handling a value, removes it from the intermediate list. The final block importer
/// rejects block import if there are still intermediate values that remain unhandled.
pub intermediates: HashMap<Vec<u8>, Vec<u8>>,
pub intermediates: HashMap<Cow<'static, [u8]>, Box<dyn Any>>,
/// Auxiliary consensus data produced by the block.
/// Contains a list of key-value pairs. If values are `None`, the keys
/// will be deleted.
Expand Down Expand Up @@ -223,6 +225,36 @@ impl<Block: BlockT, Transaction> BlockImportParams<Block, Transaction> {
import_existing: self.import_existing,
}
}

/// Take interemdiate by given key, and remove it from the processing list.
pub fn take_intermediate<T: 'static>(&mut self, key: &[u8]) -> Result<Box<T>, Error> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for not being clear.

What I actually wanted was this Result<Option<Box<T>>, Error>.

The Err would just be returned when the downcast fails. I think this makes it easier if at some point we need to debug this, as there is a clear distinction between not-found and wrong-type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bkchr The error type for downcast is Box<dyn Any>. I think the intention of the API is that:

  • If downcast succeeds, return the downcasted type in Ok.
  • If downcast fails, return the original value in Err.

So there's really no additional errors to be returned/debugged.

In take_intermediate, we mostly want to do the same thing both when the key is not found, and when the downcast fails -- stop block importing and return error, so I'd suggest we have the return type to be either Option<Box<T>> or Result<Box<T>, ConsensusError>. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And indeed we can distinguish not-found/wrong-type in ConsensusError -- ConsensusError::NoIntermediate, ConsensusError::IntermediateWrongType.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I know that downcast doesn't return an error. However in the take_intermediate case, the value will be removed and is lost.

The problem I see with Any is that &T and T give you a different TypeId and down-casting will fail: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=872b02f7968b9528140d3ec8ed7628be

And I think this is such a subtile difference, that it easily could be done wrong. Making it easier for the user to differentiate between not-found and wrong-type will really help with these kind of errors, especially when you are new to Rust.

That both should make the import fail, was nothing I wanted to question here :)

if self.intermediates.contains_key(key) {
self.intermediates.remove(key)
.ok_or(Error::NoIntermediate)
.and_then(|value| {
value.downcast::<T>()
.map_err(|_| Error::InvalidIntermediate)
})
} else {
Err(Error::NoIntermediate)
}
}

/// Get a reference to a given intermediate.
pub fn intermediate<T: 'static>(&self, key: &[u8]) -> Result<&T, Error> {
self.intermediates.get(key)
.ok_or(Error::NoIntermediate)?
.downcast_ref::<T>()
.ok_or(Error::InvalidIntermediate)
}

/// Get a mutable reference to a given intermediate.
pub fn intermediate_mut<T: 'static>(&mut self, key: &[u8]) -> Result<&mut T, Error> {
self.intermediates.get_mut(key)
.ok_or(Error::NoIntermediate)?
.downcast_mut::<T>()
.ok_or(Error::InvalidIntermediate)
}
}

/// Block import trait.
Expand Down
6 changes: 6 additions & 0 deletions primitives/consensus/common/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ pub enum Error {
/// I/O terminated unexpectedly
#[display(fmt="I/O terminated unexpectedly.")]
IoTerminated,
/// Intermediate missing.
#[display(fmt="Missing intermediate.")]
NoIntermediate,
/// Intermediate is of wrong type.
#[display(fmt="Invalid intermediate.")]
InvalidIntermediate,
/// Unable to schedule wakeup.
#[display(fmt="Timer error: {}", _0)]
FaultyTimer(std::io::Error),
Expand Down