Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

populate parent hash for indexers #1044

Merged

Conversation

nbaztec
Copy link
Contributor

@nbaztec nbaztec commented Apr 19, 2023

What
Populates the parent hash to enable indexers in the rpc response.

Note
We have been maintaining this functionality in our custom fork for a long time know, and would like to discuss around it if it makes sense for upstream as well.

/cc @tgmichel

@nbaztec nbaztec requested a review from sorpaas as a code owner April 19, 2023 17:14
@sorpaas
Copy link
Member

sorpaas commented Apr 19, 2023

It's a bug if currently parent hash is not populated other than genesis block.

@sorpaas
Copy link
Member

sorpaas commented Apr 19, 2023

Some basic testing didn't reproduce the issue for me. Appreciate it that you can submit a bug report issue with more details of exactly which situations you are not getting the parent hashes.

This PR is definitely not the right way though.

@tgmichel
Copy link
Contributor

@sorpaas this is an old patch we need to have in our fork to solve the bug that was introduced in #570 (context explained there). So is a patch to fix, at most, a single block.

Basically means that, in the case the retrieved on-chain parent_hash is H256::default() - which was the case as described in #570 - we calculate it on the fly to build the rpc response.

We hope to have this upstream, and not to have to cherrypick it forever on our side.

@sorpaas
Copy link
Member

sorpaas commented Apr 20, 2023

Please hard code those problematic blocks instead:

  • Create a config option forced_parent_hashes to be a mapping of block hash to parent block hash.
  • And then just check in the function to force set parent hash if the block hash matches.

@nbaztec
Copy link
Contributor Author

nbaztec commented Apr 20, 2023

  • forced_parent_hashes

Should we use the newly added EthConfig trait for this? Something like:

pub trait EthConfig<B: BlockT, C>: Send + Sync + 'static {
	type EstimateGasAdapter: EstimateGasAdapter + Send + Sync;
	type RuntimeStorageOverride: RuntimeStorageOverride<B, C>;

       // new
       type ForcedParentHashProvider: Get<HashMap<B::Hash, B::Hash>>
}

@nbaztec
Copy link
Contributor Author

nbaztec commented Apr 25, 2023

  • forced_parent_hashes

Should we use the newly added EthConfig trait for this? Something like:

pub trait EthConfig<B: BlockT, C>: Send + Sync + 'static {
	type EstimateGasAdapter: EstimateGasAdapter + Send + Sync;
	type RuntimeStorageOverride: RuntimeStorageOverride<B, C>;

       // new
       type ForcedParentHashProvider: Get<HashMap<B::Hash, B::Hash>>
}

@sorpaas ?

@sorpaas
Copy link
Member

sorpaas commented Apr 25, 2023

Should we use the newly added EthConfig trait for this?

This is up to you. Either a type parameter in EthConfig trait or as a config parameter when initializing RPC. Check which works best for your use case.

One potential issue making this a type parameter is that it can't be customized "dynamically". This can happen, for example, when you need to start multiple chains based on the same type parameters.

@nbaztec
Copy link
Contributor Author

nbaztec commented Apr 27, 2023

@sorpaas your suggestion made good sense since multiple chains based on same start code, are now able to pass in their own block hashes seamlessly.

Copy link
Collaborator

@boundless-forest boundless-forest left a comment

Choose a reason for hiding this comment

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

This solution LGTM.

Another question is that can we use the db cmd to fix the ethereum block hash storage associated with the broken block hash mapping? Seems another doable way.

@sorpaas sorpaas merged commit 893f924 into polkadot-evm:master May 5, 2023
@sorpaas
Copy link
Member

sorpaas commented May 5, 2023

Another question is that can we use the db cmd to fix the ethereum block hash storage associated with the broken block hash mapping? Seems another doable way.

The wrong hash is in the runtime storage, so it stays permanently in the blockchain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants