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

VM - Jump table #3120

Merged
merged 48 commits into from
Feb 23, 2024
Merged

VM - Jump table #3120

merged 48 commits into from
Feb 23, 2024

Conversation

shargon
Copy link
Member

@shargon shargon commented Feb 7, 2024

Close #2987

@shargon shargon requested a review from Jim8y February 7, 2024 11:42
@shargon shargon mentioned this pull request Feb 7, 2024
Closed
@shargon shargon marked this pull request as ready for review February 8, 2024 09:58
@shargon shargon marked this pull request as draft February 8, 2024 09:59
@shargon shargon marked this pull request as ready for review February 8, 2024 10:06
@shargon shargon requested a review from Jim8y February 13, 2024 09:21
@cschuchardt88
Copy link
Member

But #3120 (comment) is an issue.....

@shargon
Copy link
Member Author

shargon commented Feb 13, 2024

@superboyiii could you check if something change in storage please?

roman-khimov
roman-khimov reviewed Feb 13, 2024
View reviewed changes
Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

src/Neo.VM/JumpTable/JumpTable.Splice.cs
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public virtual void SubStr(ExecutionEngine engine, Instruction instruction)
{
int count = (int)engine.Pop().GetInteger();
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to the current logic better be in separate PRs.

Jim8y
Jim8y previously approved these changes Feb 13, 2024
View reviewed changes
@Jim8y
Copy link
Contributor

Jim8y commented Feb 13, 2024

@superboyiii is still in spring festival break, wont back to work until a few days later.

@shargon
Copy link
Member Author

shargon commented Feb 13, 2024

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

It must be exactly the same, only allow in ApplicationEngine to change the opcodes logic according to a fork.

@roman-khimov roman-khimov mentioned this pull request Feb 14, 2024
Open
@shargon shargon requested a review from superboyiii February 15, 2024 20:15
@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 16, 2024 •
edited
Loading

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

It must be exactly the same, only allow in ApplicationEngine to change the opcodes logic according to a fork.

Why not have a native contract store the opcodes, so the users can have access as well? So it lives on chain.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 16, 2024

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

It must be exactly the same, only allow in ApplicationEngine to change the opcodes logic according to a fork.

Why not have a native contract store the opcodes, so the users can have access as well? So it lives on chain.

Structure problem, opcode is part of the vm while vm has no reference to neo.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 16, 2024 •
edited
Loading

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

It must be exactly the same, only allow in ApplicationEngine to change the opcodes logic according to a fork.

Why not have a native contract store the opcodes, so the users can have access as well? So it lives on chain.

Structure problem, opcode is part of the vm while vm has no reference to neo.

ApplicationEngine does. Why can't we add something like ExecutionEngine.GetPrice(Opcode.PUSHT); for smart contracts.

@shargon
Copy link
Member Author

shargon commented Feb 16, 2024

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

It must be exactly the same, only allow in ApplicationEngine to change the opcodes logic according to a fork.

Why not have a native contract store the opcodes, so the users can have access as well? So it lives on chain.

Structure problem, opcode is part of the vm while vm has no reference to neo.

ApplicationEngine does. Why can't we add something like ExecutionEngine.GetPrice(Opcode.PUSHT); for smart contracts.

Please create a issue for this, what's the use case?

@cschuchardt88
Copy link
Member

IIUC this lays the foundation for switching opcode sets/implementations of them. In a compatible way that leaves the current logic as is by default. Do I understand correctly that in future we'll choose these tables based on hardforks?

It must be exactly the same, only allow in ApplicationEngine to change the opcodes logic according to a fork.

Why not have a native contract store the opcodes, so the users can have access as well? So it lives on chain.

Structure problem, opcode is part of the vm while vm has no reference to neo.

ApplicationEngine does. Why can't we add something like ExecutionEngine.GetPrice(Opcode.PUSHT); for smart contracts.

Please create a issue for this, what's the use case?

Ok, Just a thought, Why make it hard to find this information? Issue will be created..

@superboyiii
Copy link
Member

@shargon Tested, it's compatible!

@shargon shargon requested a review from roman-khimov February 19, 2024 10:14
vncoelho
vncoelho approved these changes Feb 22, 2024
View reviewed changes
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.

Very good change!

@shargon
Copy link
Member Author

shargon commented Feb 23, 2024

I want to check the coverage before merge

@shargon shargon merged commit 1fffb95 into master Feb 23, 2024
7 checks passed
@shargon shargon deleted the core-jumptable branch February 23, 2024 10:19
@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cschuchardt88 cschuchardt88 cschuchardt88 approved these changes

@vncoelho vncoelho vncoelho approved these changes

@Jim8y Jim8y Jim8y approved these changes

@superboyiii superboyiii Awaiting requested review from superboyiii

@roman-khimov roman-khimov Awaiting requested review from roman-khimov

Assignees
No one assigned
Labels
Blocker Issues that are blocking other issues. Check issues details to see what it is blocking. Ready to Merge Waiting for Review
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

vm: Versioning the VM
6 participants
@shargon @Jim8y @cschuchardt88 @superboyiii @vncoelho @roman-khimov