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

Remove exception throwing from System.Runtime.GetInvocationCounter #1930

Closed
ixje opened this issue Sep 11, 2020 · 6 comments · Fixed by #1948
Closed

Remove exception throwing from System.Runtime.GetInvocationCounter #1930

ixje opened this issue Sep 11, 2020 · 6 comments · Fixed by #1948
Labels
discussion Initial issue state - proposed but not yet accepted

Comments

@ixje
Copy link
Contributor

ixje commented Sep 11, 2020

Summary or problem description
We throw an error if the invocation counter is basically 0. It seems excessive to halt the contract execution for just asking for a counter value.

protected internal int GetInvocationCounter()
{
if (!invocationCounter.TryGetValue(CurrentScriptHash, out var counter))
throw new InvalidOperationException();
return counter;
}

Do you have any solution you want to propose?
remove the exception and return 0 instead

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Other: smart contracts / interop
@ixje ixje added the discussion Initial issue state - proposed but not yet accepted label Sep 11, 2020
@roman-khimov
Copy link
Contributor

I guess it's more for the case when there is no CurrentScriptHash in the invocationCounter dictionary, which technically could only happen because of ApplicationEngine bug. And this case should be handled somehow, either by returning zero to the script (but it's not supposed to ever get zero for this call) or by halting the execution in some way (and that's what we have).

@shargon
Copy link
Member

shargon commented Sep 11, 2020

exactly it should never happen because we add it during the call

@ixje
Copy link
Contributor Author

ixje commented Sep 12, 2020

It is only added during a System.Contract.Call, but the entry script could arguably call System.Runtime.GetInvocationCounter on itself if it expects something like contract A.func1 call -> contract B.func1 call -> A.func2. Anyway, we'll see if that ever happens :)

@ixje ixje closed this as completed Sep 12, 2020
@roman-khimov
Copy link
Contributor

only happen because of ApplicationEngine bug

the entry script

Hmmm, maybe that's one of them, it's "called" implicitly, but still it's being run so it should be accounted for by invocationCounter. Especially for cases like verification with #1800 where a real contract is being loaded and it might use GetInvocationCounter for its purposes (which is not exactly expected from a simple transaction's entry script).

@ixje
Copy link
Contributor Author

ixje commented Sep 21, 2020

@roman-khimov should I re-open it?

@roman-khimov
Copy link
Contributor

I think so. At least in my interpretation of this API the entry script (whatever that is) counts too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants