-
Notifications
You must be signed in to change notification settings - Fork 146
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
Utilization of unspent gas for promise function calls #264
Conversation
Your Render PR Server URL is https://nomicon-pr-264.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c5b5vh46fj39uid34ju0. |
2d2594e
to
c669aaa
Compare
0264-promise_gas_ratio.md
Outdated
|
||
For example, if there are three ratios, `a`, `b`, `c`: | ||
``` | ||
v = remaining_gas / (a + b + c) |
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 you specify how it works in terms of integer division? Do we always loose remainder of remaining gas by modulo (a + b + c + ...)? I think it should be ok for reasonable values of ratios, just want to make sure
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.
Also I guess we should specify that sum is calculated in u128 to avoid overflow problems
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 mention a few lines above that it's floor division, but let me make it clear here also. As for u128 for the sum, good point I didn't include that information previously.
Edit: I do agree but not certain about ratios summing to greater than u64 max. Should this not just error if this case is hit? There is no point in going past the u64 mark anyway since gas is a u64 and we are doing floored division. Should we maybe consider shifting gas values left as u128 before calculations and then shifting right after all operations are done to handle large ratio values better?
0264-promise_gas_ratio.md
Outdated
What needs to be addressed before this gets merged: | ||
- How much refactoring exactly is needed to handle this pattern? | ||
- Can we keep a queue of receipt and action indices with their respective ratios and update their gas values after the current method is executed? Is there a cleaner way to handle this while keeping order? | ||
- Do we want to attach the gas lost due to precision on division to any function? |
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'd say we just burn it. This is such a small amount of gas (for reasonable values of ratios) that doing anything else would just consume more gas than the amount that is being saved
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.
Looks good overall, thanks
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.
Thanks for the detailed writeup! It would be great if you could also include a discussion on how this would impact developer experience and what corresponding changes are needed on the sdk level. The host function itself feels confusing -- especially when you can specify both an absolute amount and a ratio. If we want to go with this route, I think more work on the sdk level is certainly needed.
Good point, I hadn't gone into detail about this. Hopefully the explanation with |
Yeah the thing is I'd like to make sure this is indeed what developers want. cc @DiscRiskandBisque |
This sounds reasonable. I worry a bit that this is a rigid API -- I can imagine various slightly fancier rules, which are not expressible with this (Eg, split only But I also see how that's not really possible, as we don't know the true amount of remaining gas until after the contract finishes. I think we can in theory do something to make that easier. For example, we can have How sure are we that If we are somewhat sure that this is a sufficient API, it seems to just do this. It's slightly awkward to implement, but not too much. So even in the worst case that we'll figure out that this idea somehow isn't enough, this won't create a too big maintainance burden on us. What are the next steps here? Are we ready to start an implementation of this under a feature flag? |
I mean, implementing this in the contract itself would be different since it would be the remaining gas when calculating
The issue is just that we would need a protocol change for adjusting attached gas, but also to add a new
Well, the gas would be refunded based on the unused amount at the end, is it an issue that the gas is held for however many blocks it takes to fully execute? This proposal might not handle all use-cases, but if we can think of classes of use cases that don't fit, I'd love to include it in this NEP.
Hopefully not. And yes, I agree with your sentiments the next steps would be this I believe. This is something I think I will take on this quarter unless something has changed. Do you have any thoughts on what would be the best direction? |
Filed the tracking issue here: near/nearcore#6150 |
@austinabell it is not completely clear what happens when there are promises that uses absolute and relative amount of gas. I'm assuming what happens is that first all absolute amount is subtracted, and the relative amount is computed for the remaining gas. This could be made more clear by putting an example that uses both absolute and relative amount of gas. With regards to defaults on the SDK, IMO it makes more sense to use 0 for relative amount of gas when the absolute amount of gas is specified, otherwise use 1 as suggested. |
What happens if both are specified is that the static amount will be scheduled and deducted and then at the end unused gas based on the weight is distributed. As for the SDK where this is used, it's a bit opinionated but feels like a strange side effect that if someone specifies a static amount of gas it would remove the weight iff it has not already been set. Of course, anyone can create whatever semantics they want on top of this host function, so doesn't affect this protocol change that was stabilized. Edit: can see some cases in the unit tests here https://github.com/near/nearcore/blob/39ab20bc8f3832e9f66a098d600ce9b2998e1cd7/runtime/near-vm-logic/src/tests/gas_counter.rs#L159-L203 |
@bowenwang1996 given that this is stabilized, it seems like we should merge this! Whose the primary owner of the neps repo at the moment, who should be pinged? |
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.
Given that the implementation is already stabilized, I believe we MUST merge this NEP, so I am approving it as NEPs moderator
Let me know if this follows the correct procedure or anything else I can do :)