Cache code size/hash in storage by nanocryk · Pull Request #893 · polkadot-evm/frontier

nanocryk · 2022-11-03T13:14:25Z

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 · 2022-11-28T13:19:33Z

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

crystalin · 2023-03-26T18:27:59Z

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

…-metadata

sorpaas

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.

nanocryk · 2023-03-27T10:09:27Z

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.

librelois · 2023-03-30T19:21:40Z

+
+	/// 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 {


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

librelois · 2023-03-30T19:23:30Z

+		use sha3::Digest;
+
+		let size = code.len() as u64;
+		let hash = H256::from_slice(sha3::Keccak256::digest(&code).as_slice());


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.

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.

librelois · 2023-03-30T19:25:41Z

+			return meta;
+		}
+
+		let code = <AccountCodes<T>>::get(&address);


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.

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.

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.

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.

Polkadot-Forum · 2023-03-30T20:20:15Z

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 · 2023-04-01T10:01:26Z

Please update the evm pin.

librelois · 2023-04-03T16:37:50Z

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

sorpaas · 2023-04-03T16:57:04Z

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 · 2023-04-03T18:19:27Z

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

koushiro · 2023-04-04T02:34:19Z

 	pub type AccountCodes<T: Config> = StorageMap<_, Blake2_128Concat, H160, Vec<u8>, ValueQuery>;

+	#[pallet::storage]
+	#[pallet::getter(fn account_codes_metadata)]


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

nanocryk · 2023-04-04T14:27:06Z

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 · 2023-05-21T15:46:47Z

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!

…-metadata

cache code size/hash in storage

59e36b4

nanocryk requested a review from sorpaas as a code owner November 3, 2022 13:14

nanocryk mentioned this pull request Nov 7, 2022

Support for Weight V2 (with storage size) #881

Closed

nanocryk added 2 commits March 27, 2023 10:48

Merge remote-tracking branch 'origin/master' into jeremy-account-code…

ef356ce

…-metadata

Merge remote-tracking branch 'origin/master' into jeremy-account-code…

99ff55c

…-metadata

sorpaas requested changes Mar 27, 2023

View reviewed changes

sorpaas reviewed Mar 27, 2023

View reviewed changes

Comment thread frame/evm/src/lib.rs Outdated

use in-memory code

0dfb2e0

sorpaas reviewed Mar 27, 2023

View reviewed changes

Comment thread frame/evm/src/lib.rs Outdated

don't clone metadata

0b860ef

sorpaas reviewed Mar 27, 2023

View reviewed changes

Comment thread frame/evm/src/lib.rs

sorpaas approved these changes Mar 27, 2023

View reviewed changes

librelois reviewed Mar 30, 2023

View reviewed changes

nanocryk added 3 commits April 3, 2023 11:42

bump evm

02b8485

don't cache empty code metadata + tests

40f96da

clippy

b6ea1c1

librelois approved these changes Apr 3, 2023

View reviewed changes

koushiro reviewed Apr 4, 2023

View reviewed changes

remove deprecated getter attribute

2c33399

tgmichel mentioned this pull request Apr 13, 2023

EVM + Weight v2 support #1039

Merged

Merge remote-tracking branch 'origin/master' into jeremy-account-code…

23a1a8e

…-metadata

koushiro reviewed May 25, 2023

View reviewed changes

Comment thread frame/evm/src/lib.rs Outdated

koushiro reviewed May 25, 2023

View reviewed changes

Comment thread frame/evm/src/lib.rs Outdated

remove dep on sha3 crate

6b3cb91

koushiro reviewed May 25, 2023

View reviewed changes

Comment thread frame/evm/src/lib.rs Outdated

feedback

ffb58e5

sorpaas approved these changes May 31, 2023

View reviewed changes

sorpaas merged commit a81265d into polkadot-evm:master May 31, 2023

dmitrylavrenov mentioned this pull request Oct 16, 2023

Bump to polkadot-sdk v1.1.0 as a base for substrate related deps humanode-network/humanode#803

Closed

37 tasks

dmitrylavrenov mentioned this pull request Nov 28, 2023

Tracking issue to bump substrate related deps to polkadot-v0.9.40 humanode-network/humanode#839

Closed

46 tasks

Conversation

nanocryk commented Nov 3, 2022

Uh oh!

nanocryk commented Nov 28, 2022

Uh oh!

crystalin commented Mar 26, 2023

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nanocryk commented Mar 27, 2023

Uh oh!

Uh oh!

Uh oh!

librelois Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

librelois Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

nanocryk Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

librelois Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

sorpaas Apr 1, 2023

Choose a reason for hiding this comment

Uh oh!

librelois Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanocryk Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

librelois Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

Polkadot-Forum commented Mar 30, 2023

Uh oh!

sorpaas commented Apr 1, 2023

Uh oh!

librelois commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sorpaas commented Apr 3, 2023

Uh oh!

koushiro commented Apr 3, 2023

Uh oh!

koushiro Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanocryk commented Apr 4, 2023

Uh oh!

sorpaas commented May 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

librelois Apr 1, 2023 •

edited

Loading

librelois commented Apr 3, 2023 •

edited

Loading

koushiro Apr 4, 2023 •

edited

Loading