Skip to content
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

Merged
merged 9 commits into from
Sep 5, 2022

Conversation

austinabell
Copy link
Contributor

Let me know if this follows the correct procedure or anything else I can do :)

@render
Copy link

render bot commented Oct 1, 2021

@austinabell austinabell marked this pull request as ready for review October 1, 2021 01:11

For example, if there are three ratios, `a`, `b`, `c`:
```
v = remaining_gas / (a + b + c)
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

@austinabell austinabell Oct 8, 2021

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?

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?
Copy link
Contributor

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

Copy link
Contributor

@EgorKulikov EgorKulikov left a 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

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a 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.

@austinabell
Copy link
Contributor Author

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 d9fb020 (#264) is adequate, with using the exploratory PR for a more in-depth understanding of what the API would roughly look like.

@bowenwang1996
Copy link
Collaborator

Yeah the thing is I'd like to make sure this is indeed what developers want. cc @DiscRiskandBisque

@matklad
Copy link
Contributor

matklad commented Jan 14, 2022

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 min(remaining_gas, SOME_CONSTANT) gas). In this sense, it's tempting to say that this should be implemented in the contract itself.

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 adjust_attached_gas host function which takes a vector of receit indices and gas costs, and adjust attached gas for previously created function call actions. The idea would be that calling such a host function would be the last thing that the contract does, such that used_gas host function returns a result which is close enough to the real used_gas after the fact. But I see that even that wouldn't be too trivial.

How sure are we that fixed_gas + ratio-of-remaining is enough to cover, say, 95% of use-cases? What about the cases where someone calls a contract with unreasonably high amount of gas? Are we OK with just splitting that according to the ratios, without some kind of "wait, that's way too much gas you are attaching here" sanity check?

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?

@austinabell
Copy link
Contributor Author

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 min(remaining_gas, SOME_CONSTANT) gas). In this sense, it's tempting to say that this should be implemented in the contract itself.

I mean, implementing this in the contract itself would be different since it would be the remaining gas when calculating prepaid_gas - used_gas rather than gas at the end of the transaction, but yeah roughly this pattern could be done at the contract level if necessary.

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 adjust_attached_gas host function which takes a vector of receit indices and gas costs, and adjust attached gas for previously created function call actions. The idea would be that calling such a host function would be the last thing that the contract does, such that used_gas host function returns a result which is close enough to the real used_gas after the fact. But I see that even that wouldn't be too trivial.

The issue is just that we would need a protocol change for adjusting attached gas, but also to add a new promise_batch_action_function_call to return the receipt index as well as maybe promise_create and promise_then because those also schedule function calls. Also, keeping around some data (or more likely a stateful function) to adjust each receipt at the end will have overhead, and if this is done, the gas calculated at the end will be invalidated slightly by the cost to adjust gas :)

How sure are we that fixed_gas + ratio-of-remaining is enough to cover, say, 95% of use-cases? What about the cases where someone calls a contract with unreasonably high amount of gas? Are we OK with just splitting that according to the ratios, without some kind of "wait, that's way too much gas you are attaching here" sanity check?

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.

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?

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?

@matklad
Copy link
Contributor

matklad commented Jan 21, 2022

Do you have any thoughts on what would be the best direction?

Filed the tracking issue here: near/nearcore#6150

0264-promise_gas_ratio.md Outdated Show resolved Hide resolved
@austinabell austinabell requested a review from a team as a code owner May 26, 2022 20:27
@mfornet
Copy link
Member

mfornet commented May 28, 2022

@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.

@austinabell
Copy link
Contributor Author

austinabell commented May 28, 2022

@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

@matklad
Copy link
Contributor

matklad commented Jun 30, 2022

@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?

@frol frol added WG-contract-standards Contract Standards Work Group should be accountable WG-protocol Protocol Standards Work Group should be accountable T-runtime About NEAR Protocol Runtime (actions, Wasm, fees accounting) and removed WG-contract-standards Contract Standards Work Group should be accountable labels Sep 5, 2022
Copy link
Collaborator

@frol frol left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-runtime About NEAR Protocol Runtime (actions, Wasm, fees accounting) WG-protocol Protocol Standards Work Group should be accountable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants