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

Fixed some opcode price #2020

Merged
merged 11 commits into from
Nov 12, 2020
Merged

Conversation

Qiao-Jin
Copy link
Contributor

@Qiao-Jin Qiao-Jin commented Oct 26, 2020

Close #2004
Fixed some opcode prices which can be used for DOS attack.

@superboyiii superboyiii mentioned this pull request Nov 2, 2020
44 tasks
@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 6, 2020

Ping

@erikzhang
Copy link
Member

Wait 24 hours to see other people's opinions.

@roman-khimov
Copy link
Contributor

Can we postpone this and change all opcodes in a single PR? It does improve some things, but at the same time we have a lot of other clear proven mismatches between execution cost and opcode price (and opcodes also have a lot of inter-relations, so modifying some without touching another is a bit inconsistent, like not changing obviously overpriced PICKITEM or HASKEY here).

So maybe we're better changing them all once, of course with proper discussion for every change and actually starting with simpler things like pushes/stack management (that are also quite consistent across two VM implementations) and then moving on to more complex ones like things touched here. This change can also then be easily weighted against preview3 testnet transaction set (just as a reality check), doing that with full instruction set is easier.

As for particular changes:

  • while INITSLOT can be made 4×INITSSLOT (theoretically it's expected to be 2×, but practically worst-case scenarios are 4× and this is quite consistent for two VMs), I think INITSSLOT itself is a little off wrt the data we have (and if we're to change that maybe we can still use 2× for INITSLOT because typical values probably differ from expected ones). Also note that INITSSLOT is executed once per contract, while INITSLOT is executed once per call (see this data with 177 INITSLOTs vs 29 INITSSLOTs)
  • VALUES/APPEND/REVERSEITEMS become a little overpriced IMO, but that's a question of balance between typical and worst case scenarios. Typical REVERSEITEMS or APPEND usages are actually very cheap, so they shouldn't cost a fortune, but at the same time they shouldn't be cheap enough to cause any trouble, so we should apply some consistent ratio to worst cases for these.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 9, 2020

@roman-khimov Hey folk, ur tests are upon neo-go, which might be not enough as a proof due to code difference & difference between C# and Go.

@roman-khimov
Copy link
Contributor

#1875:

To really nail this fairness part we should use two full independent Neo 3 VM implementations, one neo-vm itself and another neo-go. They have a lot of similarities, but they also have a number of differences, so if we're to define a set of tests in VM code (like neo-vm unit tests) we can run them in both VMs and find some average values.

Here is the table based on neo-go testing results and considering numbers outlined in #2004 for neo-vm.

If I were to use neo-go only to build the table it would look quite different from what it looks like. Both implementations matter and both should be accounted for.

But even that is not the main point. The main point is that we have consistent reproducible (at least for neo-go, I don't know what code has tested neo-vm) results that reliably show things like (proven by two VM implementations):

  • underpiced PUSHINT* instructions
  • overpriced CALL*
  • undepriced XDROP
  • overpriced PICKITEM/HASKEY

Should we ignore these? I don't think so. I think that's where we can and should set prices very close to actual execution cost.

But of course then there are more complicated instructions. Things where the difference between typical and worst-case cost can be 100- or even 1000-fold. And where implementations differ. These can be additionally discussed, tested and wheighted. Setting all of them using worst-case only data for one implementation is wrong, but at the same time we probably can all agree that we can't use 1/100 of this worst-case cost too.

And,

This change can also then be easily weighted against preview3 testnet transaction set (just as a reality check), doing that with full instruction set is easier.

It's important to evaluate the effect of these changes using at least some transaction set we have.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 9, 2020

@roman-khimov I think u'd better re-test tests of neo-go in neo-vm C# and see what's the difference. In the result u listed in #1875 , there are some opcodes whose exhausted time period are totally different from possible cases in neo C# (i.e. DUP). I don't know how u write neo-vm in GO, but it's worthy to examine how much differences there can be between the 2 solutions, to make sure that we are talking about the same matter.

@Qiao-Jin
Copy link
Contributor Author

Ping

@erikzhang erikzhang merged commit 188b131 into neo-project:master Nov 12, 2020
roman-khimov added a commit to roman-khimov/neo that referenced this pull request Nov 12, 2020
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Nov 12, 2020
ShawnYun pushed a commit to ShawnYun/neo that referenced this pull request Jan 8, 2021
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redefine Opcode/Syscall price
5 participants