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

InvokeTwice #746

Closed
wants to merge 9 commits into from
Closed

Conversation

Ashuaidehao
Copy link
Contributor

Relate #743

@shargon
Copy link
Member

shargon commented Jul 26, 2022

Won't this negate the use of GasLeft? Some projects might use the GasLeft method to either execute or not execute code, or simply to iterate.

@Ashuaidehao
Copy link
Contributor Author

Won't this negate the use of GasLeft? Some projects might use the GasLeft method to either execute or not execute code, or simply to iterate.

It depends. It may happens even without this changes.

@erikzhang erikzhang mentioned this pull request Jul 27, 2022
@roman-khimov
Copy link
Contributor

If both invocations end up the same way, then why do we do the second one? If the result of two invocations is different, then which is the correct one and what should be returned to the user?

BTW, network fee can probably be roughly estimated (not precisely, but still).

@erikzhang
Copy link
Member

If the result of two invocations is different, then which is the correct one and what should be returned to the user?

The second one is the correct one.

@roman-khimov
Copy link
Contributor

If we suppose different execution results, then the second run can easily end up in FAULT state just because of insufficient GAS (gas: Engine.GasConsumed from the first invocation).

@erikzhang
Copy link
Member

If we suppose different execution results, then the second run can easily end up in FAULT state just because of insufficient GAS

Why?

@roman-khimov
Copy link
Contributor

If contract's logic depends on fees it might take some longer branch on the second execution, so the GAS amount from the first execution won't be enough. I'd suggest using gas: gas for the second one, but even that is not perfect of course. If Engine.GasConsumed will be different, then we don't know what's going to happen with the contract in a transaction with this new SystemFee.

I don't see any perfect solution to this, setting some semi-random value or using statistical data to set fees for executions is wrong as well. And code can always check Runtime.GasLeft to see that it has more (or less) GAS there than there is in the SystemFee.

@erikzhang
Copy link
Member

erikzhang commented Aug 3, 2022

If contract's logic depends on fees it might take some longer branch on the second execution

The idea is that if a contract's logic depends on fees, it checks if sys_fee is 0 to determine if it's currently a test execution.

@roman-khimov
Copy link
Contributor

While that's the most likely scenario, it's not the only one that can happen. And zero vs non-zero fee can already lead to any kind of change between two invocations, including those that will require more GAS to finish successfully.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks go to me, I just need to test.

{
tx.SystemFee = Engine.GasConsumed;
tx.NetworkFee = 1_00000000;
Engine = ApplicationEngine.Run(script, Snapshot, container: tx, settings: system.Settings, gas: Engine.GasConsumed, diagnostic: diagnostic);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what if it fails?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it fails it will be a good change, because before it was HALT.
The flow looks like to be the same, @roman-khimov, nothing will change from the original flow, it just add a more precise gas limit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it fails it will be a good change, because before it was HALT.

But what's good about it? Estimation failed? How do I send a transaction then?

I know these things are not likely to happen, but contracts checking fees shouldn't exist as well and yet they do.

it just add a more precise gas limit

For normal contracts it changes nothing (except for additional execution overhead), for strange contracts that care about fee values it can lead to any result --- some will work, some will fail.

@Jim8y Jim8y added the Need Active Pr will be closed after one week if no new activity. label Feb 12, 2024
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be merged.

Can you verify @superboyiii
@shargon, you have approved in the past, are you in agreement still?

@shargon
Copy link
Member

shargon commented Mar 18, 2024

I think that the project should use GasLeft, it's a very specific case that can be solved in this specific smart contract

@vncoelho
Copy link
Member

vncoelho commented Mar 18, 2024

So, @shargon, you mean make another change that set GasLeft as default?

@shargon
Copy link
Member

shargon commented Mar 18, 2024

So, @shargon, you mean make another change that set GasLeft as default?

As I understand it's a problem that can be solved by the smart contract

@vncoelho
Copy link
Member

I think the problem here is more related to the simulation when we invoke RPC, no?

@Jim8y
Copy link
Contributor

Jim8y commented May 11, 2024

close as not planed and author agree to close

@Jim8y Jim8y closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Active Pr will be closed after one week if no new activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants