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

Fix genesis block sync #917

Merged
merged 6 commits into from
Dec 9, 2022

Conversation

shunsukew
Copy link
Contributor

@shunsukew shunsukew commented Nov 23, 2022

This PR fixes genesis block not accessible issue (#78) which blockchains started even before EthereumRuntimeRPCApi runtime API version 4 (current version) still have.

Because some blockchains have runtime API version lower than 4 at genesis block, has_api conditional becomes false for those blockchains. (fn has_api returns false if version mismatch). That means, ethereum genesis block cannot be properly mapped. (For example, our chains have runtime API version 1 at genesis block). Regardless of the runtime version blockchains have at genesis block, it should be properly synced.

@shunsukew shunsukew requested a review from sorpaas as a code owner November 23, 2022 11:53
@shunsukew
Copy link
Contributor Author

shunsukew commented Nov 30, 2022

@sorpaas @tgmichel @koushiro
Changes should be small, could you kindly give me feedback about this?
Thank you in advance

Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, but I think this is not really right. As I recall there were indeed some changes to current_block in version 2.

Please add an extra branch:

  • If API version is equal or smaller than 2, call current_block_before_version_2.
  • Otherwise, call current_block.

@shunsukew
Copy link
Contributor Author

shunsukew commented Dec 5, 2022

@sorpaas
Thank you so much for reviewing! (For some reason, I didn't notice notification and saw the review now.)

I see, I didn't know the background about changes to current_block. please let me do it.

@shunsukew
Copy link
Contributor Author

shunsukew commented Dec 6, 2022

@sorpaas
In other places, current_block_before_version_2 is called when API version is smaller than 2. If it is equal to 2, current_block is called.
For example, here
https://github.com/paritytech/frontier/blob/3173b07a166a8930b4cb1e69d34f2ba52ef4e9f3/client/rpc/src/eth/execute.rs#L126

Judging from EthereumRuntimeRPCApi definition, I think current_block should be called if version is 2 .
https://github.com/paritytech/frontier/blob/3173b07a166a8930b4cb1e69d34f2ba52ef4e9f3/primitives/rpc/src/lib.rs#L126

@shunsukew shunsukew requested a review from sorpaas December 6, 2022 11:11
@sorpaas
Copy link
Member

sorpaas commented Dec 8, 2022

Hmm looks like Github bugged out and it refused to run CI for the last commit. I also couldn't find any button to retry the deployment. Would you mind to push a new empty commit?

@shunsukew
Copy link
Contributor Author

Sure, pushed now! and it seems CI got triggered properly this time

@sorpaas sorpaas merged commit e9c7743 into polkadot-evm:master Dec 9, 2022
abhijeetbhagat pushed a commit to web3labs/frontier that referenced this pull request Jan 11, 2023
* fix mapping-sync sync_genesis_block

* fix lint

* use legacy current_block before api version v2

* empty commit
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.

2 participants