search

aeternity / aepp-sdk-js

JavaScript SDK for the æternity blockchain
http://docs.aeternity.com/aepp-sdk-js/
ISC License
120 stars 59 forks source link

Optional automatic gas consumption estimation #1286

Closed nikita-fuchs closed 2 years ago

nikita-fuchs commented 3 years ago

Current issue: We used to have a high gas limit for stateful calls. Now we lowered it. The consequence of this is that many calls done in aestudio will fail for out of gas, inconveniently requiring people to poke around with the gas amount and misleading them into thinking their code might be the fault.

Possible fix: We provide a tx option that

this value shall be then taken as gas amount for the statefull call (including contract deployment). If there is a gas amount provided though, that value should be taken and override the auto-estimation option for quick testing (open for other suggestions here).

marc0olo commented 3 years ago

@nikita-fuchs your problem is specifically addressing dry-run calls, right?

if so we should generally analyze what's the default behavior of the SDK at the moment if a non-stateful entrypoint (or a staticCall) is being executed. it might already be the case that the dry-run is using a much higher gas-limit.

I assume that the out of gas issue that we have for dry-runs now is because the SDK is using the new protected dry-run endpoint of the hosted node which is limited by default. to overcome this, many applications explicitely use the workaround to use the unlimited debug dry-run endpoint.

this can be achieved with this workaround:

client.api.protectedDryRunTxs = client.api.dryRunTxs;

@nikita-fuchs is AEstudio using this workaround right now?

marc0olo commented 3 years ago

when speaking about gas estimation behavior for stateful and non-stateful contract calls then I see multiple options here:

General

Non-stateful, static calls

Stateful calls

what do the others think here?

nikita-fuchs commented 3 years ago

@nikita-fuchs is AEstudio using this workaround right now?

No it's using the default dryrun still.

marc0olo commented 3 years ago

No it's using the default dryrun still.

maybe you can change that along with the SDK update ;-) ... would be a quick win here as long as we haven't decided how to proceed here

kenodressel commented 3 years ago

do NOT call dry-run for gas-estimation by default

I disagree here because this means there will be a lot of errors where tx will run out of gas and users will not really know what to do. Obviously this comes with the performance pain and we might want to issue a warning based on this but I think by default the SDK should be easy to use and not necessarily the fastest thing in the world.

marc0olo commented 3 years ago

I am fine with doing it the other way round and having gas-estimation via dry-run active by default. but it should be possible to deactivate this behavior.

but that's kind of a perspective thing. personally I still prefer to make the automated gas calculation optional.

marc0olo commented 2 years ago

for automatic gas consumption estimation we should also handle errors accordingly. see discussion here:

davidyuk commented 2 years ago

My proposal:

So, by default sdk would work the best as possible (but a bit slower), and the developer would be able to speedup it by providing gas limits by himself.

marc0olo commented 2 years ago

I like that proposal regarding handling gas limit and I think @kenodressel will like it, too :D but I think we should define a default offset (e.g. *1,5) to the gas limit returned by the dry-run call. the offset should also be configurable via options.

also please don't forget about the possibility to choose between debug and protected dry-run endpoint via config. by default it should be the protected dry-run but it should be possible to configure usage of debug endpoint via options.

assuming I want to handle the gas usage by myself. how do I currently have to perform a dry-run for a specific contract call? maybe we could provide dry-run methods to the contract instance to get dry-run responses independently.

what do you think?

edit

nikita-fuchs commented 2 years ago

I disagree here because this means there will be a lot of errors where tx will run out of gas and users will not really know what to do.

Since when are you guys so about the dev experience - might this have something to do with all the work on superhero ? 😜😜 anyways, a default dryrun is a totally smooth idea, no doubt.. I'm just worried about the load on the nodes here. Plus it will only catch most error cases anyways, it could happen that some other TX slips in before the one you dry-ran, changes logic of the contract causing your TX to "correctly fail".

marc0olo commented 2 years ago

@nikita-fuchs yeah that was also the reason I didn't wanna have this as default. but I think we can start this way and if we identify load issues we need to change the behavior and update the devs accordingly. shouldn't be a big deal

davidyuk commented 2 years ago

define a default offset (e.g. *1,5) to the gas limit returned by the dry-run call

The amount of gas needed can change dramatically depending on the nearly mined transactions, like:

if (flag) return // I don't know Sophia well yet
do a lot of work

if dry-run calculate costs on state when flag is false, and on-chain flag would be true then factor as 1.5 may not help us. I don't think that we can somehow accurately calculate this factor, but this factor would increase the call price that wallets will show at some step. So the user may just refuse to sign a 1.5x expensive call. Reverting this default would be an insidious breaking change.

the offset should also be configurable via options

Looks like an extra complication of the interface, if the developer cares about this he can set gas prices explicitly for each call.

the possibility to choose between debug and protected dry-run endpoint via config

The intention of the public (protected) dry-run endpoint was to simplify SDK, not to complicate it. In my opinion, production apps should use public (protected) dry-run only (developer needs to ensure that dry-run limits fit his app needs). At work, the developer may use public nodes, but if their's dry-run limit doesn't fit his needs then he needs to run his own. Currently, aeternity-hosted nodes expose debug endpoints that is handy to use, but it shouldn't affect the SDK.

how do I currently have to perform a dry-run for a specific contract call?

you can pass callStatic: true in options or call get method like in: https://github.com/aeternity/aepp-sdk-js/blob/409ce5e4adb2bb0e57c92e1ee892dd6cb3d366c8/test/integration/contract-aci.js#L222-L223

I'm just worried about the load on the nodes here.

I assume that we can freely use the public (protected) dry-run endpoint

it will only catch most error cases anyways, it could happen that some other TX slips in before the one you dry-ran, changes logic of the contract causing your TX to "correctly fail".

The developer should be able to define gas limits by himself to avoid wrong gas estimation in cases like this

marc0olo commented 2 years ago

if dry-run calculate costs on state when flag is false, and on-chain flag would be true then factor as 1.5 may not help us. I don't think that we can somehow accurately calculate this factor, but this factor would increase the call price that wallets will show at some step. So the user may just refuse to sign a 1.5x expensive call. Reverting this default would be an insidious breaking change.

if we don't introduce an offset the risk of the tx running out of gas is significant higher. the gas limit is independent of the gas price. with the offset we just ensure that in case it takes more gas than the dry-run calculated it is still more likely to be mined.

apart of that we already had a significant breaking change in using a gas limit default of 25_000 which is one of the reasons this issue now exists to improve the overall behavior.

the user that might want to refuse what you mentioned here cannot do anything in this case because the tx is immediately broadcasted? or is there any user-prompt in that usecase we are talking here? just wondering. but anyway, we are discussing gas limit and not the gas price. we just want to ensure the tx doesn't run out of gas.

Looks like an extra complication of the interface, if the developer cares about this he can set gas prices explicitly for each call.

if I want to be as safe as possible and make sure the tx passes in every case (e.g. the case you mentioned above) I could adjust this offset and set e.g. to *3 instead of *1.5

The intention of the public (protected) dry-run endpoint was to simplify SDK, not to complicate it. In my opinion, production apps should use public (protected) dry-run only (developer needs to ensure that dry-run limits fit his app needs). At work, the developer may use public nodes, but if their's dry-run limit doesn't fit his needs then he needs to run his own. Currently, aeternity-hosted nodes expose debug endpoints that is handy to use, but it shouldn't affect the SDK.

you can ask @thepiwo how happy he is with this change. I agree that if you run your own node you would be able to increase the limit of the protected public dry-run endpoint. but some developers still prefer to stick to the unlimited debug endpoint by default. being able to provide that as an option when initializing the SDK would be good I think. of course we should keep the protected dry-run endpoint as default.

@thepiwo your opinion on that?

I assume that we can freely use the public (protected) dry-run endpoint

sure, no problem with that. but we would call it for every transaction compared to now where we have a default gas limit of 25_000.

thepiwo commented 2 years ago

I agree with @davidyuk here, we should drop the default limit and default to dry-running to estimate gas usage, then apply a factor, e.g. +20% as a margin for on-chain execution. If the developer wants to speed things up, gas should be provided with the call.

I think in terms of load on the node we can use the endpoint as much as we want, the node operator is free to disable or limit the endpoint. For this limitations we need to communicate this well to the aepp developer, that no one can rely on the dry-running service to be available or allow for the necessary limits. I don't know the overall plan for the ecosystem here, as with the current state the only way to provide a stable and reliable aepp is to host your own node, as otherwise dry-running can be taken away or be limited at any time.

marc0olo commented 2 years ago

@thepiwo what's your take on dry-run calls for non-stateful entrypoints where we already have applications that we know exceed the limit of the public protected dry-run endpoint. should the SDK provide the possibility to configure this for debug-endpoint or would you expect this to be configured on the node side on the public endpoint then?

thepiwo commented 2 years ago

we can't expect the node to expose the debug endpoints, so I guess those should just fail if the gas limit of the node is exceeded and aepp devs will have to run their own nodes.

As I said when this got discussed and introduced first, this change is not compatible with the way our ecosystem works right now. An aepp dev can't set the node fully even if he wanted, as the node is users choice. But a user may not use a node that has dry-running enabled at all, there is no fix for that, only bad workarounds. E.g. the aepp dev can include a secondary client used only for dry-runs that connect to a node under his control, but then the user has to fully trust that.

marc0olo commented 2 years ago

sounds good to me. we just need to make a clear statement here and provide transparency about the changes and the situation how to deal with this.

still want to second that we need that offset (+20-50%) for the dry-run call estimation on stateful calls. personally I would like to have this offset configurable by the aepp dev.

mradkov commented 2 years ago

I'm not a fan of offsetting .. better to "correctly fail" IMO.

marc0olo commented 2 years ago

I'm not a fan of offsetting .. better to "correctly fail" IMO.

why is this "correctly failing" - I would argue even more that this should be up to the user to decide (see #1328). how is it currently working from user perspective if I use an aepp that uses the sdk, do I have the choice to define certain parameters on my own? is an aepp dev capable of providing the user this kind of choice (gas price & gas limit) in a convenient way right now? actually I am not sure about this. cc @kenodressel @thepiwo @davidyuk @CedrikNikita @nikita-fuchs

I mean of course - if there is a loop or sth. like that where a call always fails then it seems to be bad to have an offset. but in any other case the dev/user would probably be angry if the tx runs out of gas which could have been avoided with the offset.

mradkov commented 2 years ago

As a user I wouldn’t want to pay an arbitrary random multiplier more for something that can be precisely calculated.

hanssv commented 2 years ago

You wouldn't pay arbitrarily more - you always pay for the gas used.

marc0olo commented 2 years ago

As a user I wouldn’t want to pay an arbitrary random multiplier more for something that can be precisely calculated.

this is why I think #1328 should also be addressed if not possible yet. ideally we can provide the decision to a user.

as Hans says only the used gas is being payed. with the default multiplier for gas limit we just ensure that the tx doesn't fail in case the mempool is full with competing transactions to be mined. I mean yeah this is currently very unlikely, but hopefully this will become a "problem" for us in the foreseeable future.

mradkov commented 2 years ago

we just ensure that the tx doesn't fail in case the mempool is full with competing transactions to be mined

What is the problem with failing it and giving the option to the user to provide bigger gas limit manually (e.g. via the wallet)?

hanssv commented 2 years ago

So, first, the reason for lowering the default gas limit is because currently (this is to be addressed in the next hard-fork) the gas limit of microblocks is computed using ContractCreate/Call gas limit and not gas used. So over-estimating the gas limit hinder efficient block packing - this isn't too much of a problem since there is left-over space in most microblocks. But overall it is good for the chain if the gas limit is a decent approximation.

So, when you write a contract, and deploy it to be used in an aepp I would expect the developer to be able to (after some testing) set a reasonable gas limit - most contracts/entrypoints have a fairly stable cost (lists grow, state grow, large numbers may affect it a bit) at least for well written contracts. Still, for the lazy developer, having the option to dry-run it and multiply by 1.X is a plausible option (with a little bit of extra overhead).

In the best of worlds I'd expect this to be configurable - either a fixed provided limit, a dry-run + factor or even the max/min/average of the two. As for default I think dry-run and a factor of 1.25 or so would be the best.

If you write a contract where gas usage fluctuate - you probably know, or notice during testing, and then you have to pick a large enough value. After all you just pay for the gas used, but if you run out of gas, you pay all your (insufficient) gas for nothing!

hanssv commented 2 years ago

we just ensure that the tx doesn't fail in case the mempool is full with competing transactions to be mined

What is the problem with failing it and giving the option to the user to provide bigger gas limit manually (e.g. via the wallet)?

You would spend your insufficient gas for nothing - you are (naturally) charged all the gas when failing with out of gas

marc0olo commented 2 years ago

@mradkov I am totally open to provide the user an option, see #1328. we can also don't have an offset as default. but I personally as developer would like to be able to have such offset and ideally also be able to configure it. to avoid spending gas for nothing I would argue it's best to have an offset by default

marc0olo commented 2 years ago

So, first, the reason for lowering the default gas limit is because currently (this is to be addressed in the next hard-fork) the gas limit of microblocks is computed using ContractCreate/Call gas limit and not gas used.

looking forward to the upcoming hardfork =)

marc0olo commented 2 years ago

yeah 🥳