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

Record the storage proof_size with hostfunction #1490

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

boundless-forest
Copy link
Collaborator

Implementation of #1483,

This PR is based on rust-ethereum/evm#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
Copy link
Contributor

librelois commented Aug 12, 2024

@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

@boundless-forest
Copy link
Collaborator Author

@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

Copy link
Contributor

@librelois librelois left a comment

Choose a reason for hiding this comment

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

LGTM

return Ok(());
};

let mut record_account_codes_proof_size =
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not remove theses proof size check, 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, if we are going to add those hooks back, seems there is no need to introduce the host function. The rational behind this pr is trying to avoid using the evm hook and keep the evm clean.

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 the host function is still useful to refund extra proof size, because the "computed proof size" overestimate a bit (we have to overestimate to account for the worst case scenario)

Copy link
Contributor

@librelois librelois left a comment

Choose a reason for hiding this comment

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

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

@boundless-forest
Copy link
Collaborator Author

boundless-forest commented Sep 10, 2024

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
Copy link
Contributor

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.

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