feat(runtime): Track block gas allowance in builtins #4345
Conversation
ekovalev
added
A0-pleasereview
ekovalev
force-pushed
the
ek-gas-allowance-in-builtins
branch
2 times, most recently
from
0085e4d to
f1257de
Compare
| @@ -93,6 +95,9 @@ pub enum BuiltinActorError { | |||
| /// Actor's inner error encoded as a String. | |||
| #[display(fmt = "Builtin execution resulted in error: {_0}")] | |||
| Custom(LimitedStr<'static>), | |||
| /// Occurs if a builtin actor execution does not fit in the current block. | |||
| #[display(fmt = "Block gas allowance exceeded")] | |||
There was a problem hiding this comment.
Im wondering If we could abstract it since it more about how system implemented rather than each particular builtin
| } | ||
|
|
||
| gas_spent += to_spend; | ||
| gas_left -= to_spend; |
There was a problem hiding this comment.
Safe due to a check above - if gas_left is less than to_spend we abort earlier
There was a problem hiding this comment.
But for the sake of universality we can use saturated arithmetics - no problem
| } | ||
|
|
||
| gas_spent += to_spend; | ||
| gas_left -= to_spend; |
breathx
added
A3-gotissues
|
discussed |
ekovalev
force-pushed
the
ek-gas-allowance-in-builtins
branch
from
f1257de to
4ab1f6e
Compare
ekovalev
added
A0-pleasereview
ekovalev
force-pushed
the
ek-gas-allowance-in-builtins
branch
from
4ab1f6e to
0029069
Compare
pallets/gear/src/builtin.rs
Outdated
| pub type HandleFn<C, E> = dyn Fn(&StoredDispatch, &mut C) -> Result<Payload, E>; | ||
|
|
||
| /// Builtin actor `max_gas` function signature. | ||
| // TODO: let the weight function take a complexity argument similar to extrinsics weight functions |
There was a problem hiding this comment.
Sounds like a new epic story so definitely needs an issue =)
pallets/gear-builtin/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// TODO: |
There was a problem hiding this comment.
It would be nice to have some description
| context: &mut BuiltinContext, | ||
| ) -> Result<Payload, BuiltinActorError>; | ||
|
|
||
| /// Returns the maximum gas that can be spent by the actor. |
There was a problem hiding this comment.
Does zero value get interpreted in special way?
There was a problem hiding this comment.
Does zero value get interpreted in special way?
Not really: it'll just always pass the "initial screening" in the BuiltinDispatcher, but checks inside the handle() still remain (every time the gas counter is charged). However, if the max_gas() reports an accurate estimation, the handle would be skipped entirely and message re-queued. In the latter case we are certain no gas is going to be wasted as opposed to the situation if we still have to requeue the message after all, but only know it half way down inside the handle.
Addresses #3752.
The initial implementation of builtin actors overlooked the need of keeping track of the block gas allowance. So far only individual message gas limit has been taken in consideration.
To address this, the way how gas usage is tracked in builtin actors has been modified. Specifically:
BuiltinActor::handlemethod signature has been change to return just aResult<Payload, BuilintActorErrorand not a tuple with actual gas burned; instead, gas usage is captured by theGasCounter(as a part of mutablecontextpassed as an input tohandle);BuiltinActor::max_gas()method has been added to the trait to allow builtin actors report the maximum possible amount of gas that can be burned during a message processing. Thehandlewould only be called if it fits in the current block in the worst possible case based on themax_gasoutput (similarly to how it is decided whether an extrinsic is added to a block based on its weight). It doesn't have to be 100% accurate, I guess - but better be not too far from real values.The refactoring has rendered the codebase more readable.
TODO:
max_gas()for every builtin actor with actual values - either through benchmarking or theoretically.