-
Notifications
You must be signed in to change notification settings - Fork 336
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
Improve gas checks in Randomness precompile #2051
Conversation
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Note: the idea is that fulfilling a request is free for the fulfiller if they call it correctly, right? In that case, the cost of the transaction itself should be refunded too, right?
That sounds right
* 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
rawFulfillRandomWords
function, 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?