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.

Sign up for GitHub

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

Jump to bottom

Improve ApplicationEngine #2796

Merged
merged 2 commits into from
Aug 2, 2022
Merged

Improve ApplicationEngine #2796

merged 2 commits into from
Aug 2, 2022

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented Jul 24, 2022 •
edited
Loading

Some improvements from #2756

  1. Added CallingContext to ExecutionContextState. This fixes a potential issue in CalledByEntryCondition.
  2. Improved the return logic of dynamic calls.
  3. Disallowed notifications in entry script.

@erikzhang erikzhang mentioned this pull request Jul 24, 2022
Merged
@superboyiii superboyiii mentioned this pull request Jul 26, 2022
Closed
roman-khimov
roman-khimov reviewed Jul 27, 2022
View reviewed changes
src/Neo/SmartContract/ApplicationEngine.cs
{
if (context.EvaluationStack.Count == 0)
Push(StackItem.Null);
else if (context.EvaluationStack.Count > 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this duplicating https://github.com/neo-project/neo-vm/blob/f48de7e595ade7c72be6ccd5cce0ab99708f0afd/src/Neo.VM/ExecutionEngine.cs#L427 (rvcount normally is 0 or 1 in this case:

neo/src/Neo/SmartContract/ApplicationEngine.cs

Line 331 in 8e68c3f

rvcount: method.ReturnType == ContractParameterType.Void ? 0 : 1,
)? Can this ever happen without #2756?

Copy link
Member Author

Choose a reason for hiding this comment

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

rvcount: method.ReturnType == ContractParameterType.Void ? 0 : 1,

This is in LoadContract(). For a dynamic script, we have no way of knowing in advance how many values it will return. So set rvcount to -1.

https://github.com/neo-project/neo-vm/blob/master/src/Neo.VM/ExecutionEngine.cs#L1580

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I mean, this change probably can be a part of #2756, it doesn't seem to be useful without it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use it as a double check.

roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Jul 27, 2022
@roman-khimov
interop/runtime: disable notifications in dynamic scripts
e5c59f8 
That are only entry scripts today. See neo-project/neo#2796.
@roman-khimov roman-khimov mentioned this pull request Jul 27, 2022
Merged
@superboyiii
Copy link
Member

Compatibility test pass. No storage difference.

@erikzhang
Copy link
Member Author

Merge?

@superboyiii
Copy link
Member

Tested OK. I think we can merge this.

@erikzhang erikzhang merged commit 718fc9f into master Aug 2, 2022
@erikzhang erikzhang deleted the improve-applicationengine branch August 2, 2022 23:23
This was referenced Oct 2, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov left review comments

@shargon shargon shargon approved these changes

@superboyiii superboyiii superboyiii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@erikzhang @superboyiii @shargon @roman-khimov