Improve gas checks in Randomness precompile #2051
Conversation
nanocryk
added
A0-pleasereview
librelois
added
breaking
| .checked_add(request_gas_limit_with_overhead) | ||
| .ok_or(revert("overflow when computed max amount of refunded gas"))?; | ||
|
|
||
| let total_refunded_gas: U256 = total_refunded_gas.into(); | ||
| let (base_fee, _) = <Runtime as pallet_evm::Config>::FeeCalculator::min_gas_price(); |
There was a problem hiding this comment.
in theory we should include the second parameter (it is the amount of weight for calling min_gas_price() itself). we currently use a value of 0 for this, though.
Right, I think this is how it was supposed to work. I think the fulfiller should be refunded >= entire txn cost, probably slightly more so there is an incentive for doing it. I'm not sure the changes in this PR are sufficient to calculate exactly this amount. One way to do that would be to benchmark a no-op fulfill request to get an idea of how much gas it costs.
Yeah, as above -- it should be |
|
Another thing to consider is the risk of submitting a fulfill request txn but being too late (that is, someone beat you to it). Then you will get no refund and you'll lose some money in fees. It's not easy to quantify this... |
There was a problem hiding this comment.
I think this is good, but there might be room for more discussion around the design.
One improvement overall would be to have some documentation somewhere that covers all the different overhead values that we calculate and how they relate to the overall txn cost, refunds, risk, etc.
| // Since we cannot compute `prepare_and_finish_fulfillment_cost` now (we don't | ||
| // know the number of words), we compute the cost for the maximum allowed number of | ||
| // words. | ||
| let max_prepare_and_finish_fulfillment_cost = | ||
| prepare_and_finish_fulfillment_gas_cost::<Runtime>( | ||
| <Runtime as pallet_randomness::Config>::MaxRandomWords::get(), | ||
| ); | ||
|
|
||
| if handle.remaining_gas() < max_prepare_and_finish_fulfillment_cost { | ||
| return Err(revert(alloc::format!( | ||
| "provided gas must be at least {max_prepare_and_finish_fulfillment_cost}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
So now the fulfill_request caller must overestimate the gas_limit just in case request.num_words = MaxWords:::get()?
Will the UI automatically do this or do we need to document is it somewhere
There was a problem hiding this comment.
Yes, it must overestimate because it can be this value.
If it is under it will revert, thus the gas estimator will increase the gas limit automatically.
notlesh
added
D1-audited👍
* fix gas checks + more tests * fix clippy warning * PR feedback + refund tx cost Co-authored-by: librelois <c@elo.tf>
What does it do?
Improve gas checks in the precompile.
What important points reviewers should know?
Now the request gas_limit is how much gas will be available inside the the target
rawFulfillRandomWordsfunction, without any overhead costs. This simplifies the computations, and is backward compatible with current design (since it will be less that what it is required now).The fulfiller however must now call the precompile with enough gas for the request gas_limit plus the overhead. They will be refunded the actually used amount, overhead included. They are also refunded the cost of the transaction itself.
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?