Check finalized block in blockchain::Blockchain #8410
Conversation
sorpaas
added
A0-pleasereview 🤓
|
In the short term we probably need to keep |
ethcore/src/blockchain/blockchain.rs
Outdated
| // but nothing in between. | ||
| let finalized_block = route.blocks.iter() | ||
| .map(|route_hash| self.block_details(&route_hash).unwrap_or_else(|| panic!("Invalid block hash in tree route: {:?}", route_hash))) | ||
| .enumerate() |
There was a problem hiding this comment.
why not just take(route.index)?
ethcore/src/blockchain/blockchain.rs
Outdated
| // If anything on the tree route is finalized, refuse to reorg. The common ancestor can be finalized, | ||
| // but nothing in between. | ||
| let finalized_block = route.blocks.iter() | ||
| .map(|route_hash| self.block_details(&route_hash).unwrap_or_else(|| panic!("Invalid block hash in tree route: {:?}", route_hash))) |
There was a problem hiding this comment.
Wouldn't it be better to include finalized already in tree_route? Seems that we query block details there anyway.
ethcore/src/blockchain/blockchain.rs
Outdated
| @@ -979,6 +995,34 @@ impl BlockChain { | |||
| } | |||
| } | |||
|
|
|||
| /// Mark a block to be considered finalized. Panic if the hash does not exist. | |||
| pub fn mark_finalized(&self, batch: &mut DBTransaction, block_hash: H256) { | |||
| let mut block_details = self.block_details(&block_hash).unwrap_or_else(|| panic!("Invalid block hash: {:?}", block_hash)); | |||
There was a problem hiding this comment.
Maybe better to return Result instead of panicking?
There was a problem hiding this comment.
agree, or at least prove it won't happen
There was a problem hiding this comment.
The rest of BlockChain struct is also using panic for pub fns when a block hash is not found. I think for our case is that we're going to use mark_finalized here for results returned from Engine, and it will only call this function for already known block hashes. Otherwise, it indicates a logic error in our code. So I think it would be fine to use panic given we indicated the panic condition in the documentation.
There was a problem hiding this comment.
@sorpaas I'm pretty sure we use .expect(PROOF) form everywhere, and public APIs return Option<.> if some particular block/header/details might not be available.
Seems that only add_child panics in case it's called with invalid block, the other ones calling panic are not public, and I guess could easily use expect instead.
ethcore/src/blockchain/blockchain.rs
Outdated
| } | ||
|
|
||
| /// Update metadata detail for an existing block. | ||
| pub fn update_metadatas(&self, batch: &mut DBTransaction, block_hash: H256, metadatas: HashMap<Bytes, Bytes>) { |
There was a problem hiding this comment.
Wouldn't it be better to have metadata in some more structured way? Seems that HashMap<Bytes, Bytes> is even worse than HashMap<String, String> (the latter is at least human readable)
There was a problem hiding this comment.
Maybe HashMap<String, Bytes>? For many cases (for example, to store Casper score info U256), the value itself is readable anyway.
Another issue for the key is if we just want to prefix something recognizable with something non-readable. In that case, converting from prefix string to Bytes is easy, but probably not the other way around.
There was a problem hiding this comment.
also: metadata is already plural
There was a problem hiding this comment.
What I mean is maybe to return T: TryFrom<HashMap<Bytes, Bytes>> or sth (if we want to be super generic), so that the caller can decide on the expected type of metadata, otherwise we might be passing a lot of weakly-typed structures in the codebase and imho would be best to avoid that.
Alternatively maybe an enum with possible types of stored metadata if we are ok with having less generic solution and a single place where all kinds of metadata is defined.
ethcore/src/blockchain/blockchain.rs
Outdated
| } | ||
|
|
||
| /// Prepares extras block detail update. | ||
| fn prepare_details_update(&self, batch: &mut DBTransaction, block_hash: H256, block_details: BlockDetails) { |
There was a problem hiding this comment.
Does it just prepare the update or write it to cache? Seems there is already prepare_block_details_update method in this struct and it returns the HashMap, might be a bit misleading to have that one here as well.
There was a problem hiding this comment.
That was meant to be a stripped version of prepare_update, but yeah looks confusing. I'll change the name.
|
I'm going to move this to #8401 because some of the changes are related and I might create unnecessary merge conflicts. I think I got a design now, and will explain it soon. |
5chdn
added
A8-looksgood 🦄
finalizedandmetadatasin a backward compatible way -- if finalized is false and metadata is empty, it is stored as the old 4 value RLP. Otherwise, it's stored as the extended 6 value RLP.Blockchain::mark_finalizedandBlockchain::update_metadatato update finalized value and metadata for any block (but it's not currently used anywhere).Blockchain::block_infoto refuse re-org if it goes pass a finalized block.