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

Cache code size/hash in storage #893

Merged

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented Nov 3, 2022

Depends on rust-ethereum/evm#140

Cache the code size/hash of an account in storage to avoid reading the full code to compute those values. Aims to reduce storage proof size.

@nanocryk
Copy link
Contributor Author

Should I wait for an evm crate release or use git dependency? @sorpaas

@crystalin
Copy link
Contributor

@nanocryk can you merge master
@sorpaas can we get it reviewed and merged also ?

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.

We shouldn't use lazy evaluation here. The security property is hard to guarantee. It's unsafe especially just around pre-/post-runtime upgrade time.

Please generate the code metadata on code creation instead. You can keep the current fallback code path just in case and/or use it in manual migrations.

frame/evm/src/lib.rs Outdated Show resolved Hide resolved
@nanocryk
Copy link
Contributor Author

We shouldn't use lazy evaluation here. The security property is hard to guarantee. It's unsafe especially just around pre-/post-runtime upgrade time.

Please generate the code metadata on code creation instead. You can keep the current fallback code path just in case and/or use it in manual migrations.

Metadata is inserted in create_account which is "code creation". The fallback is necessary for code being deployed before this change.

frame/evm/src/lib.rs Outdated Show resolved Hide resolved

/// Get the account metadata (hash and size) from storage if it exists,
/// or compute it from code and store it if it doesn't exist.
pub fn account_code_metadata(address: H160) -> CodeMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should return an option to express the case where the account not have any code.

use sha3::Digest;

let size = code.len() as u64;
let hash = H256::from_slice(sha3::Keccak256::digest(&code).as_slice());
Copy link
Contributor

Choose a reason for hiding this comment

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

If the account not have any code, we will compute a hash of an empty bytes vector at runtime, I wonder if it's the expected behavior.
If it's expected it's till unoptimized, it would be better to check if code.is_empty() and return an hardcoded constant in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the metadata should not even be computed and stored for an empty code, because it could be an address that later gets code deployed.

return meta;
}

let code = <AccountCodes<T>>::get(&address);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me really unsafe, and not compatible with Weight v2, to force the reading of the code if the metadata are not set.

It would be better to return None and create a dedicated call for migration.

Copy link
Member

Choose a reason for hiding this comment

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

In this case we need to patch evm so that Handler::code_size / Handler::code_hash can return error. It's relatively trivial.

However, my view is that this is optional. If a chain is upgraded, then they should be recommended to "touch" all contracts upon upgrade (probably through a multi-call to ensure no other code are executed in between). If a chain starts with the new runtime, then the None path will never happen.

Copy link
Contributor

@librelois librelois Apr 1, 2023

Choose a reason for hiding this comment

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

my view is that this is optional. If a chain is upgraded, then they should be recommended to "touch" all contracts upon upgrade (probably through a multi-call to ensure no other code are executed in between)

This is not possible for parachains, it will make a too big storage proof size. For decentralized parachains, metadata must be computed offchain and injected onchain by a referendum via governance, which requires the code to support hybrid state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that data should be inserted via governance.

However in the meantime knowing the code hash and size is necessary and require to read the full code. Once it is computed it is better to store it than to have to compute it again the next time. Thus it seems better to not return an Option and keep the unhappy path, which will never be reached once all contracts have been migrated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/frontier-support-for-evm-weight-v2/2470/1

@sorpaas
Copy link
Member

sorpaas commented Apr 1, 2023

Please update the evm pin.

@librelois
Copy link
Contributor

librelois commented Apr 3, 2023

LGTM and CI is green, @sorpaas can you merge it?

@sorpaas
Copy link
Member

sorpaas commented Apr 3, 2023

Sorry. Let me publish a new release (not yet, will ping you) and then change the evm version to that before merge. Otherwise things will be difficult to manage.

@koushiro
Copy link
Collaborator

koushiro commented Apr 3, 2023

Sorry. Let me publish a new release (not yet, will ping you) and then change the evm version to that before merge. Otherwise things will be difficult to manage.

@sorpaas before release a new version of evm, maybe we could release a new version of ethereum firstly?
rust-ethereum/ethereum#50

@@ -506,6 +507,11 @@ pub mod pallet {
#[pallet::getter(fn account_codes)]
pub type AccountCodes<T: Config> = StorageMap<_, Blake2_128Concat, H160, Vec<u8>, ValueQuery>;

#[pallet::storage]
#[pallet::getter(fn account_codes_metadata)]
Copy link
Collaborator

@koushiro koushiro Apr 4, 2023

Choose a reason for hiding this comment

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

better not to add pallet::getter syntax for new storages, because it will be deprecated by upstream.

@nanocryk
Copy link
Contributor Author

nanocryk commented Apr 4, 2023

Sorry. Let me publish a new release (not yet, will ping you) and then change the evm version to that before merge. Otherwise things will be difficult to manage.

We'll have to merge PR #1032 first then.

@sorpaas
Copy link
Member

sorpaas commented May 21, 2023

I'm dealing with rust-ethereum/evm#162 and rust-ethereum/evm#163
Once they are merged I'll make a new evm release. After that I'll definitely merge this first.

Sorry about all the delays!

frame/evm/src/lib.rs Outdated Show resolved Hide resolved
frame/evm/src/lib.rs Outdated Show resolved Hide resolved
frame/evm/src/lib.rs Outdated Show resolved Hide resolved
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.

6 participants