-
Notifications
You must be signed in to change notification settings - Fork 374
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
External cost recorder #170
External cost recorder #170
Conversation
@sorpaas depends on rust-blockchain/evm-tests#24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the with-substrate
feature. It's really unnecessary and I can't think of good reason why we wouldn't want to keep this enabled by default.
LGTM otherwise.
with-substrate
feature@@ -50,7 +49,7 @@ pub enum Apply<I> { | |||
} | |||
|
|||
/// EVM backend. | |||
#[auto_impl::auto_impl(&, Arc, Box)] | |||
//#[auto_impl::auto_impl(&, Arc, Box)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we still implement this for &mut
and Box
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I'm fine keeping this as is for now.
A refactoring we can do is to move record_external_operation
into a separate trait Record
, and then add yet another type parameter to StackExecutor
R: Record
. Then we can uncomment this again.
But the changes are already huge so we can do that in a separate PR.
Please create a companion PR in the |
* update PrecompileHandle ref: rust-ethereum/evm#122 * update fee calculation ref: rust-ethereum/evm#132 * add code_size/code_hash fn in StackState trait ref: rust-ethereum/evm#140 * update evm call stack ref: rust-ethereum/evm#136 * update evm call stack ref: rust-ethereum/evm#155 * add shanghai eips 3651, 3855, 3860 ref: rust-ethereum/evm#152 * update is_precompile ref: rust-ethereum/evm#157 * fix eip-3860 ref: rust-ethereum/evm#160 * update runtime config ref: rust-ethereum/evm#161 * add eip-4399 ref: rust-ethereum/evm#162 * fix eip-2618 ref: rust-ethereum/evm#163 * fix nonce back to U256 ref: rust-ethereum/evm#166 * remove exit_substate in create functions ref: rust-ethereum/evm#168 * record external cost ref: rust-ethereum/evm#170 * add record_external_operation ref: rust-ethereum/evm#171 * add storage_growth ref: rust-ethereum/evm#173 * update evm * switch to shanghai hardfork * update ecrecover ref: polkadot-evm/frontier#964 (#2696)
This PR proposes adding 3 new methods to
StackState
trait (and 2 additional counterparts inPrecompileHandle
) behind awith-substrate
feature flag. This change is forced by additional resource restrictions in parachains and leaves the native gasometer logic and code totally unchanged.With this new interface implementors can have a capacity for any required metric in the
Backend
. This way, prior to executing anyOpcode
(inpre_validate
), there is an attempt to record this new external cost(s), and much like in the case of the gasometer, it will proceed with the Opcode execution or fail (OutOfGas
) if the capacity for the external metric is exhausted.As a whole, this PR is proposed in favor of alternative designs that would support this type of functionality, like for example being generic over the
Gas
type in the native gasometer. Those proposals were preliminarily discarded for being too intrusive for the evm code, given this is not an standard feature and can be solved in theBackend
.The methods introduced in
StackState
are:record_external_dynamic_opcode_cost
: meant to be called prior to executing an Opcode (pre_validate
). Calculates the amount of external resources needed for the given Opcode, and records the cost.record_external_cost
: meant to be called fromrecord_external_dynamic_opcode_cost
implementation and fromPrecompileHandle
implementor. Records a cost (deducts an amount from the resource pool).refund_external_cost
: meant to be called fromrecord_external_dynamic_opcode_cost
implementation and fromPrecompileHandle
implementor. Refunds a cost (sums an amount from the resource pool).The methods introduced in
PrecompileHandle
are:record_external_cost
: meant to interface withStackState
.refund_external_cost
: meant to interface withStackState
.In addition to the feature-specific additions, in this PR there are some additional changes included that are needed to properly fail the gasometer in case any of this external resources are exhausted.