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 bug parent hash on BlockV2 upgrade #570

Merged
merged 3 commits into from
Feb 23, 2022

Conversation

tgmichel
Copy link
Contributor

@tgmichel tgmichel commented Feb 10, 2022

This bug can be reproduced after storage upgrade from BlockV0 to BlockV2, and only if the preceding block contained transactions. In that case the current_block_hash function will try to decode BlockV2, failing to do so because the block contains TransactionV0s and returning None, defaulting to H256::default(), rendering the parent hash useless and of course messing the block hashing of the subsequent blocks.

This PR proposes just use the already existing BlockHash storage and query it using the previous block number.

This is a pretty nasty bug and should merged in asap @sorpaas . Also, do you have any suggestion on how to test this type of situation in frontier?

@sorpaas
Copy link
Member

sorpaas commented Feb 15, 2022

This is a pretty nasty bug and should merged in asap

The current PR changes the meaning of parent hash though. For the majority of Ethereum dapps, it just shouldn't matter at all, and we can set the parent hash to even H256::default() without any issues. However, for anything that does matter, this break things -- the parent hash is supposed to be used to be able to fetch the parent Ethereum block (encoded by the Ethereum block hash, not Substrate block hash).

I'd prefer you change this PR to a runtime migration (decode BlockV0 and write BlockV2 for CurrentBlock on runtime upgrade).

Also, do you have any suggestion on how to test this type of situation in frontier?

This bug could have been detected if Substrate wasn't silently failing on those should-be-panic cases. It's essentially a migration bug. With above we could have prevented it together with try-runtime (which should print the fail-to-decode error). The primitive for the "defensive infallible" are already merged in paritytech/substrate#10626, but it still has to be integrated in frame's decoding code path.

@tgmichel
Copy link
Contributor Author

I agree that runtime migration is preferable. @sorpaas for visibility, as we talked:

However, for anything that does matter, this break things -- the parent hash is supposed to be used to be able to fetch the parent Ethereum block (encoded by the Ethereum block hash, not Substrate block hash)

That's not the case, as BlockHash is Ethereum block hash, not the Substrate one.

@tgmichel
Copy link
Contributor Author

@sorpaas added the migration functions. I still think reading BlockHash storage is an improvement over performing the keccak on-the-fly so I still include that change, but please let me know.

@sorpaas
Copy link
Member

sorpaas commented Feb 23, 2022

Please pull master.

# Conflicts:
#	frame/ethereum/src/lib.rs
@sorpaas sorpaas merged commit fbf7da4 into polkadot-evm:master Feb 23, 2022
jordy25519 pushed a commit to cennznet/frontier that referenced this pull request Apr 6, 2022
* Fix bug parent hash on `BlockV2` upgrade

* BlockV0 -> BlockV2 migration utility functions
@tgmichel tgmichel deleted the tgm-parent-hash branch June 24, 2022 07:39
abhijeetbhagat pushed a commit to web3labs/frontier that referenced this pull request Jan 11, 2023
* Fix bug parent hash on `BlockV2` upgrade

* BlockV0 -> BlockV2 migration utility functions
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