search

polkadot-evm / frontier

Ethereum compatibility layer for Substrate.
Apache License 2.0
567 stars 480 forks source link

Record the storage proof_size with hostfunction #1490

Open boundless-forest opened 1 month ago

boundless-forest commented 1 month ago

Implementation of https://github.com/polkadot-evm/frontier/issues/1483,

This PR is based on https://github.com/rust-ethereum/evm/pull/292, which needs to be reviewed and merged first.

This change utilized a new host function in the polkadot-sdk instead of evm hooks. It requires the node to force-upgrade, which is a significant drawback. However, it greatly simplifies the storage proof size calculation and paves the way for a clean evm executor.

@sorpaas @librelois @koushiro Welcome to your feedback.

librelois commented 1 month ago

@boundless-forest I had exactly the same idea, we can also remove accounting for the transaction size (previously named "base cost" and now called "pre execution" in your PR), because now substrate compatibilizes well the size of extrinsics encoded in the pov, so currently we count the size of each transaction twice: https://github.com/paritytech/polkadot-sdk/pull/4765

boundless-forest commented 1 month ago

@boundless-forest I had exactly the same idea, we can also remove accounting for the transaction size (previously named "base cost" and now called "pre execution" in your PR), because now substrate compatibilizes well the size of extrinsics encoded in the pov, so currently we count the size of each transaction twice: paritytech/polkadot-sdk#4765

Done

boundless-forest commented 3 weeks ago

We should not remove proof size check per opcode, otherwise the evm execution can exceed proof size limit, and we can't revert storage reads, so we must keep all proof size checks even if we use the proof_size host function to reclaim the diff at the end

Nice catch! I came up with an idea that we can pre-run the transaction and check if the transaction execution triggers the proof size limit error in the validate_transaction_in_pool. If it exceeds, we should reject including that transaction in the block. Since this is done in the off-chain producer, we won't encounter several performance issues.

In fact, I'm curious why we don't record the storage read/write proof size in the SubstrateStackState implementation in the runner. The EVM executor interacts with storage through these methods, allowing us to calculate the proof size within them. Recording proof size based on the opcode complicates the calculation.

librelois commented 3 weeks ago

I came up with an idea that we can pre-run the transaction and check if the transaction execution triggers the proof size limit error in the validate_transaction_in_pool

Transaction validation is free of charge, as long as we're not sure we'll be able to charge fees, which means you can spam collators for free. Also, according to several profiling on moonbeam, we already spend far too much time on transaction validation, which is precisely what we're trying to optimize. For these two reasons, it is not feasible for us to pre-execute the transaction during the validation phase. Furthermore, the proof size obtained during validation may be lower than that obtained on-chain, as the storage does not have exactly the same state.

I'm curious why we don't record the storage read/write proof size in the SubstrateStackState implementation in the runner. The EVM executor interacts with storage through these methods, allowing us to calculate the proof size within them.

When I designed proof size metering with tgmichel we thought about using SubstrateStackState, and tgmichel even tried, but the implementation turned out to be more complex in the end, and hardly compatible with precompiles. Indeed, we have stateful precompiles, which consume proof size, and we need to count this consumption at the same level to ensure that the ethereum transaction doesn't produce more proof size than its gas limit allows.

boundless-forest commented 1 week ago

Update: I'm upgrading the EVM version to 1.0 along with changes in this PR. I will raise a PR later.