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

Compressed BigInteger format (NeoVM 3) #173

Merged
merged 7 commits into from Jun 15, 2019

Conversation

6 participants
@igormcoelho
Copy link
Contributor

commented Jun 13, 2019

This is a simpler reimplementation of #171

The idea is that BigInteger can already be represented as both 0x0000, 0x00 or 0x'' (empty bytearray). So let's adopt the most compressed version.
The gains are huge, because we can use it to automatically Delete entries on NEP-5, as soon as they become zero: neo-project/neo#825

Another gain is a clear different between opcode 0x00, previously called PUSH0, but it usually pushed an empty array instead of number zero (Integer zero didn't have explicitly an empty bytearray as "standard" hex encoding).
Now, opcode 0x00 will be called PUSHBYTES0, which is a nice name, that implies that an empty byte array is pushed to stack.
Position 0x50 was "not used" (officially), but in fact it was implictly used as "PUSH0", pushing the actual Integer zero to the stack, so now it can have its name.

@igormcoelho igormcoelho requested review from erikzhang, shargon, lightszero and vncoelho and removed request for shargon Jun 13, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 13, 2019

Codecov Report

Merging #173 into master will increase coverage by 0.73%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage    61.8%   62.53%   +0.73%     
==========================================
  Files          16       16              
  Lines        1411     1412       +1     
==========================================
+ Hits          872      883      +11     
+ Misses        539      529      -10
Impacted Files Coverage Δ
src/neo-vm/Types/Integer.cs 68.75% <100%> (+1%) ⬆️
src/neo-vm/ExecutionEngine.cs 57.28% <0%> (+1.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1282c92...db35233. Read the comment docs.

igormcoelho added some commits Jun 13, 2019

@vncoelho

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

I will try to revise, brother, but I still do not dominate this part as you, @erikzhang @shargon, @lightszero

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

This is a very simple, yet powerful change brother. Nothing will really change, arithmetics will still give same results for all existing cases. Yet, this will allow us to have a clear definition of Neo Integers, stored in a very efficient format, and to implement automated garbage collection for NEP-5, etc.
This is also necessary for another very nice change that I'm working on neo PR 820 ;)

@vncoelho
Copy link
Member

left a comment

That is true, I just took some minutes to check.
It is more readble and the push bytes0 different than push0 was a nice change for making it more clear.

@vncoelho

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Could it have a more clear title? ehwuahah
Or more descriptive?

@vncoelho

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@igormcoelho, maybe we could a tag like port-to-devpack, right?

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Thanks Vitor, I tried to make the title the simplest as possible hahaha well, as long as @lightszero is well aware of this, we're good on the compiler ;)

@erikzhang

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I don't think we need 0x50.

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

0x50 is only necessary to force push StackItem Integer 0. There's no other direct way to push Integer 0 into the stack (but you calculate like PUSH1 DEC). I see that Integer 0 is equivalent to an empty byte array, but it's still different stack item types. In my opinion, PUSHBYTES0 could be kept for byte array computation (such as CATs), and PUSH0 for int computation.

The other point is that 0x50 can only be used for this... since it's after 4f and before 51 to 60, if you try to put any other opcode in this position, it will complicate many other direct calculations. So the option is either letting it unused or use like this.I prefer to formally activate it... but if you think it's a bad idea, we can leave it as it was (no 0x50).

[EDIT:] I retracted from this position, as this opcode is not strictly necessary, and could over complicate some existing tools, already prepared and well-tested for neo2 opcodes.

PUSH0 = 0x00,
PUSHF = PUSH0,
PUSHBYTES0 = 0x00,
PUSHF = PUSHBYTES0,

This comment has been minimized.

Copy link
@shargon

shargon Jun 14, 2019

Member

We can remove PUSHF

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Jun 14, 2019

Author Contributor

I agree, if it's never used... but I think we need to discuss that on another PR, because it could be used somewhere else (just like PUSHT on witness verifications, an although the same as PUSH1, it would requiring changing many tools just because of the name). I'm reverting all changes not directly connected to the fact that BigInteger 0 needs to be a byte array, proposed here.

@igormcoelho igormcoelho dismissed stale reviews from shargon and vncoelho via cec3884 Jun 14, 2019

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@erikzhang I thought about it, and changing PUSH0 will impact many existing tools, so you're right. PUSH0 is not necessary together with a "PUSHBYTES0", both are the same (in practice), so I removed PUSHBYTES0 and put PUSH0 back to its position.

@igormcoelho

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@shargon @vncoelho @lightszero @erikzhang what do you think now? Change is quite direct now.. do we want VM Integer(0) to be 0x00 in hex or 0x''?

@igormcoelho igormcoelho requested review from shargon and vncoelho Jun 14, 2019

@shargon

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

if we use 0x, when a balance is 0 will be automatically deleted, so for me is good 0x

@lightszero

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

agree

/// An empty array of bytes is pushed onto the stack.
/// An empty array of bytes is pushed onto the stack.
/// This is equivalent to pushing Integer zero to the stack.
/// This is equivalent to pushing Boolean false to the stack.

This comment has been minimized.

Copy link
@shargon

shargon Jun 15, 2019

Member

Bool will be 0x or 0x00? Now is better 0x

@shargon shargon merged commit b4382aa into neo-project:master Jun 15, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@igormcoelho igormcoelho deleted the igormcoelho:feat_3x_std_integer2 branch Jun 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.