Conversation
serban300
left a comment
serban300
left a comment
There was a problem hiding this comment.
Looks good ! I just left a few comments. I would be happy to address them if they make sense.
| let para_hash_at_source = match self.on_chain_parachain_header(at_block, para_id).await? { | ||
| Some(parachain_header) => { | ||
| let mut parachain_head = ParaHashAtSource::Some(parachain_header.hash()); | ||
| para_header_number_at_source = Some(*parachain_header.number()); |
There was a problem hiding this comment.
Nit: para_hash_at_source and parachain_head seem to be almost the same variable. I think we can delete one of them.
There was a problem hiding this comment.
Yeah, but they have different scopes. Feel free to change the code if you think your version is better, though :)
| .client | ||
| .storage_value::<ParaHead>(imported_para_head_key, Some(at_block.1)) | ||
| .await? | ||
| .and_then(|h| match HeaderOf::<P::SourceParachain>::decode(&mut &h.0[..]) { |
There was a problem hiding this comment.
Nit: Would it make sense to add a helper function similar to storage_value for double-encoded storage values instead of doing the decoding here ?
There was a problem hiding this comment.
I don't mind - feel free to add :)
| head_hash: ParaHash, | ||
| ) -> StorageKey { | ||
| bp_runtime::storage_double_map_final_key::<Blake2_128Concat, Twox64Concat>( | ||
| bp_runtime::storage_double_map_final_key::<Blake2_128Concat, Blake2_128Concat>( |
There was a problem hiding this comment.
I'm not very familiar with storage values, but considering these fixes, I was wondering if we could use a safer approach here. I'm thinking of the following issues:
- As seen here, we can use different hashers for the storage key vs the
StorageDoubleMapthat is defined in the pallet. This seems to be covered by unit tests however. - We can use different types when we decode the value. For example here the correct ones would be
ParaId,ParaHash, but we might by mistake use something else when decoding. Not sure if this is covered by unit tests.
So I was wondering if we could use a safer approach to cover these issues. Maybe define a new structure to hold all these hashers and types and use if both when defining the StorageDoubleMap and when computing the key and reading the value. A scaled-down version of StorageDoubleMap maybe.
There was a problem hiding this comment.
Yep, that's a good idea. I'm not sure, though that you'll able to do that - all storage delarations are handled by procedural macro and it may happen that you can't specify e,g, MyMap::Hasher there. But you can try to experiment of course :)
| let imported_para_header = self | ||
| .client | ||
| .storage_value::<ParaHead>(imported_para_head_key, Some(at_block.1)) | ||
| .await? |
There was a problem hiding this comment.
Sorry, one more thing. await? returns the error directly, if any, while if HeaderOf::<P::SourceParachain>::decode() returns an error, we also log a message. Is this desired? Or should we log a message in both error cases ?
There was a problem hiding this comment.
If you're going to make a new method for double-encoded storage values, then let's log it here, where we know what the storage value actually mean and where we may log the meaningful error.
Ideally (imo) we'll need to migrate to some crate that would allow us to propagate error with context and log it once at the upper level (inside the relay loop). But that's not a part of your follow-up PR of course :)
There was a problem hiding this comment.
Sure, I will log the error here, but I was wondering if we should log an error both when storage_value() returns an Err and when decode returns an Err. Or should we keep the await? and log an error only when decode() returns an Err ?
The idea with having a new method for double-encoded storage value is not going very well so far. I think I might have to give it up.
There was a problem hiding this comment.
Sure, I will log the error here, but I was wondering if we should log an error both when storage_value() returns an Err and when decode returns an Err. Or should we keep the await? and log an error only when decode() returns an Err ?
Imo more detailing logging = better :) The fact that logging occurs at the one branch only is probably the outcome of some bug that I've met. In the end the error is logged at the top level anyway, but this additional trace adds some context. So I'd add the second logging call here.
|
The comments have been addressed in #1521 . Marking this PR as reviewed. |
* parachain loop metrics * some fixes * mini refactoring * add tests
* parachain loop metrics * some fixes * mini refactoring * add tests
2 participants
closes #1481
Two metrics are provided: best parachain head numbers at the source relay chain and at the target chain.