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: eth api: current_block can be a BlockV0 #1269

Closed
wants to merge 1 commit into from

Conversation

librelois
Copy link
Collaborator

@librelois librelois commented Feb 8, 2022

What does it do?

If the call to runtime API EthereumRuntimeRPCApi _current_block fail in v2, just redo the API call in v1.

This is because current_block actually contains the information of the previous block (perhaps the name should be changed to make it clearer).

What important points reviewers should know?

Ideally we should use the previous block to ask for the version of the runtime API, but in this part of the code we don't have easy access to the hash of the previous block, so it's easier to just recall the runtime API in v1 in case of failure.

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

* Get parent hash from BlockHash storage mapping

* Revert "Fix EIP-2929 cold slot costs in estimation mode"

This reverts commit 06d2429.
@librelois librelois added the B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes label Feb 8, 2022
@crystalin
Copy link
Collaborator

I think it is ok for debug, but I'm never a big fan of those logics: in case of error we have to try again with the api in version 1

@tgmichel
Copy link
Contributor

tgmichel commented Feb 9, 2022

I'm working in paritytech/substrate#1270 with an alternative. The benefit of it is we rely on the :ethereum_schema storage value, which holds the actual pallet_ethereum storage version and is updated on_runtime_upgrade, instead of the misleading runtime version metadata. The override handles the decoding accordingly, is faster and imo is a better suited solution for future updates.

@librelois
Copy link
Collaborator Author

I kept thinking about it this morning, and actually we can' t just call a previous version of the runtime API, it will still panic because substrate will load the code that is in the post-storage.
So in the case of the last block of the old runtime, the runtime API will be executed with the code of the new runtime, which will therefore fail to decode a BlockV0.

It reminds me of this issue: paritytech/polkadot-sdk#64

So, I see 2 solutions:

  1. Use a substitute runtime specifically for 1 block
  2. Don't use the runtime API anymore and read the storage directly as @tgmichel suggests

The solution 2. seems to me simpler and more robust in the long run.

@librelois librelois closed this Feb 9, 2022
@librelois librelois deleted the elois-fix-curremnt-block-version branch February 9, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants