Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-evm: "native" Substrate runner #7260

Closed
wants to merge 25 commits into from
Closed

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Oct 4, 2020

Currently, pallet-evm uses a runner which handles call stack and state reversion / commitment logic by itself. The advantage of this runner is that we ensure good compatibility, as the runner can be tested with Ethereum test suites. However, the disadvantage is that anything that interacts with EVM will have to take this specific state reversion logic into considerations and handle them.

This PR implements an additional "native" runner for pallet-evm, which uses Substrate's native state reversion / commitment logic. The runner is abstracted into a Runner trait in the module trait definition. One can set this to pallet_evm::runner::stack::Runner<Self> to continue the current behavior, and pallet_evm::runner::native::Runner<Self> to use the new native Substrate runner.

@sorpaas sorpaas added A3-in_progress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Oct 4, 2020
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

What are the reasons to use the native runner over stack runner? Better interoperability with other pallets?

The name native runner make me think it is using natively compiled evm interpreter, the evm interpreter running inside wasm.

&target,
transfer.value.low_u128().unique_saturated_into(),
ExistenceRequirement::AllowDeath,
).map_err(|_| ExitError::OutOfGas)
Copy link
Contributor

Choose a reason for hiding this comment

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

why OutOfGas?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the transfer fails we need to early return an error. In EVM it doesn't matter much what error we return here. The usual choice for a blanket error is OutOfGas, so here it is. We can also change this to ExitError::Other(_) and return a string here.

@sorpaas
Copy link
Member Author

sorpaas commented Oct 5, 2020

What are the reasons to use the native runner over stack runner? Better interoperability with other pallets?

Yeah. It makes precompile implementations much easier, because now it does not need to care about the custom call-stack revert/commit logic in the "stack" runner. Stack runner maintains the state changes itself, while native runner does not -- it just directly commit all changes to Substrate state and relies on Substrate's transaction commit / revert logic to handle reversions.

@sorpaas sorpaas marked this pull request as ready for review October 11, 2020 02:47
@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 11, 2020
@xlc
Copy link
Contributor

xlc commented Oct 11, 2020

Should keep this in mind. I think we will need some benchmarking to understand the performance difference between runners.

This PR introduces a minimal viable product that hits the described complexity profile. There is much opportunity for optimization left on the table. As our current runtimes are not using storage transactions there is no regression expected for those.

If you start nesting this things deeply you can expect O(dirty_keys) for every transaction commit or rollback. This can be optimized in the future to coalesce multiple consecutive committed transactions. That said, adding one transaction per Dispatchable should be totally fine.

#6269 (comment)

@crystalin
Copy link
Contributor

Will this have an impact on block production (because of using a (possibly?) shared runner) when the EVM is overloaded/stuck/panic ?

@sorpaas
Copy link
Member Author

sorpaas commented Oct 13, 2020

@crystalin There's no shared runner, actually. The difference between native and stack runner is how or whether they use some Substrate features (and give in some Ethereum consistency, particularly gas costs). So I don't think this impacts anything in block production.

value: U256,
gas_limit: u32,
gas_price: U256,
nonce: Option<U256>,

Choose a reason for hiding this comment

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

@sorpaas I think we need apply_state argument so we can use it for eth_call and eth_estimateGas

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't, actually!

What eth_call and eth_estimateGas do in Frontier is to spawn a new runtime instance based on a particular block, run functions, and retrieve the results. The runtime instance is then discarded, hence the state changes as well.

Previously we had apply_state because it brings slight performance improvements -- state applications happen at the end of the block, and eth_call and eth_estimateGas do not need them, so with the flag setting to false we can early return. Right now, however, it is really hard to support this apply state flag in native runner, because we instantly apply changes as the EVM runs. So for a generic interface we had to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @sorpaas, I have many puzzles about apply_state usage here. Especially about these comments:

Previously we had apply_state because it brings slight performance improvements -- state applications happen at the end of the block, and eth_call and eth_estimateGas do not need them, so with the flag setting to false we can early return.

The following is what I understand about this:

fn execute_evm<F, R>(
	......
	if apply_state {
		backend.apply(values, logs_data, true);
	}
}

In evm-pallet, all execute_call, execute_create finally use execute_evm to start evm StackExecutor running
and get result. At the end of execute_evm, if apply_state is true, we will change the AccountCodes, AccountStorages and so on. If apply_state is false, we skip the evm-store values modifactions and return early.

While we use eth_call or eth_estimateGas, we set the apply_state flag with false to avoid modification storage stored in the pallet. This makes sense to me.

Also, I have noticed this comment:

The runtime instance is then discarded, hence the state changes as well.`

I am confused about state changes here. Could someone do a more clear explanation about this?

Thanks.

value: U256,
gas_limit: u32,
gas_price: U256,
nonce: Option<U256>,

Choose a reason for hiding this comment

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

same here

Allowing those basic struct to be used in other sp contexts
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants