Revert some checks for 2.x #140

shargon · 2019-04-22T18:04:50Z

Version 2.10.1 have differences with the storages, so i think that we should revert all of these new verifications that not exists previously on 2.9.x

TODO: We should test if the problem is solved with these patch

codecov-io · 2019-04-22T18:07:48Z

Codecov Report

Merging #140 into master-2.x will decrease coverage by 0.14%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##           master-2.x     #140      +/-   ##
==============================================
- Coverage       80.54%   80.39%   -0.15%     
==============================================
  Files              45       45              
  Lines            4580     4545      -35     
==============================================
- Hits             3689     3654      -35     
  Misses            891      891

Impacted Files	Coverage Δ
src/neo-vm/ExecutionEngine.cs	`99.56% <100%> (-0.02%)`	⬇️

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 fb5d6c9...4ba0cde. Read the comment docs.

vncoelho · 2019-04-22T18:22:35Z

@shargon and @jsolman, give me some couple of days.
I am currently checking the master2.x against 2.9.5 for testnet.

vncoelho · 2019-04-22T20:01:28Z

Current branch 2.x still has problems, I will check this one now.

vncoelho · 2019-04-22T22:40:31Z

In addition, @shargon, we are also going to include comparison of application logs.

erikzhang · 2019-04-23T10:45:33Z

neo-project/neo-modules#89

@vncoelho

RavenXce · 2019-04-23T14:07:36Z

May I know which storages (mainnet/testnet) are being checked for consistency, and how?

Because storage isolation and interop name hashing will likely cause the mainnet storages to not be exactly the same when re-synced from beginning.

For example, we ran some functions on mainnet assuming that the name hashing feature is enabled before it actually was. This means that a full re-resync will never be able to get back the same storage state (previously fail, now pass), unless some sort of start-block flag is for these features. The same goes for NEP-8 op-codes.

We abandoned addresses and contracts that used these features before they were actually running on consensus nodes, to minimize actual application issues.

igormcoelho · 2019-04-23T14:33:15Z

@RavenXce you are correct in this point, but we are testing over the time where these features were included to the chain, so these past events will actually pass, like you explained. So, the changes we are looking for now are much more subtle, related to very recent updates on vm limits checking and similar stuff.
The final solution will come in short term with the adoption of Merkle Patricia to handle storage and application hashes, so that after this point, hashes will rule the definitive state of these side storages. We are already discussing this here: neo-project/neo#528

NeoResearch "official" database regarding storage is this one: https://github.com/neoresearch/neo-storage-audit
This one has never changed since we started creating the audits.

RavenXce · 2019-04-23T17:34:36Z

@igormcoelho okay sure, saw this issue and just wanted to make sure that you guys aren't wasting time looking at the wrong things. Thanks for making sure the vm changes are non-breaking! 👍

vncoelho

Shargon, this surely allows some failed storage persistence to happen again.
I did not investigate if that would be right or not, since it is a revert that covers from 2.10.0 to 2.10.1.

Historically, even if we compare different versions, such as 2.9.4 to 2.9.10, there might differences somewhere.

I am still afraid this is reverting more than necessary

vncoelho · 2019-04-23T19:44:32Z

Here is the first storage diff on Testnet:
This patch:

Client 2.9.4 (the same in 2.10.0):

This e615bf5#diff-76ae2659301a783b97ae4c99999464cc produce a difference in the storage

shargon · 2019-04-26T12:25:33Z

After a lot of hours of research, we found that PICKITEM previously doesn't accept integers neither byteArrays, we introduce this feature on shargon@e615bf5#diff-76ae2659301a783b97ae4c99999464ccR1269

This produce a difference with the block (testnet) {{ "block": 460271, "size": 1, "storage": [ { "state": "Added", "key": "26b16223ae5418cd0d01b8f20ae51022fa13c0e3696e666c6174696f6e5261746500000003", "value": "00010200" } ] }} because the TX (0x836487ae75e331fa53ccd4bc2a1674fbbd4d732143d2fe43648ffa753e66a1ea) in 2.9.4 cause a FAULT, without storage changes, and with 2.10.1 we store a new value.

Previously the code was like this: https://github.com/shargon/neo-vm/blob/663e9c8583e9919adbdb88896e42711e8715a69e/src/neo-vm/ExecutionEngine.cs#L1253

Thanks @vncoelho for your amazing work :)

shargon · 2019-04-26T13:02:59Z

There are more differences

2.10.1 [FAULT]

{{
  "block": 1568312,
  "size": 2,
  "storage": [
    {
      "state": "Changed",
      "key": "1bc123aa6ac42f73075ffda3da7c396263d5f5d723c73353261ed22a9a8766d0b1bbd68d0072a80c3f0000000000000000000000000c",
      "value": "0005009699000200"
    },
    {
      "state": "Added",
      "key": "1bc123aa6ac42f73075ffda3da7c396263d5f5d71cbca158f62258e7f79004c275e25ea700369fca9d0000000000000000000000000c",
      "value": "000400ca9a3b00"
    }
  ]
}}

2.9.4

{{
  "block": 1568312,
  "size": 7,
  "storage": [
    {
      "state": "Changed",
      "key": "218399fea119811f82f8a80a0ce0dd90a65f569d6b5eee56b5e5919c8e15706a9d48b79300719f2dda4a9091137e2026ef04feeeed0081899a37cded1e59000000000000000008",
      "value": "0005f04c3ba40b00"
    },
    {
      "state": "Changed",
      "key": "218399fea119811f82f8a80a0ce0dd90a65f569d6b5eee56b5e5919c8e15706a9d48b79300719f2dda9083b425bf81e44316cc729a005477d34e85535115000000000000000008",
      "value": "0005001a71180200"
    },
    {
      "state": "Changed",
      "key": "218399fea119811f82f8a80a0ce0dd90a65f569d1a1af8a9f46ebcaa47f3b2f967dd7bf60006d893169083b425bf81e44316cc729a005477d34e85535115000000000000000008",
      "value": "0005000c77420300"
    },
    {
      "state": "Changed",
      "key": "218399fea119811f82f8a80a0ce0dd90a65f569d6b5eee56b5e5919c8e15706a9d48b79300719f2ddae72d286979ee6cb1b7e65dfd00dfb2e384100b8d148e7758de42e4168b0071792c600000000000000000000000000c",
      "value": "000460774c7700"
    },
    {
      "state": "Added",
      "key": "218399fea119811f82f8a80a0ce0dd90a65f569d4a9091137e2026ef04feeeed81899a3700cded1e59fc00000000000000000000000b",
      "value": "0010401f000000000000d00700000000000000"
    },
    {
      "state": "Changed",
      "key": "1bc123aa6ac42f73075ffda3da7c396263d5f5d723c73353261ed22a9a8766d0b1bbd68d0072a80c3f0000000000000000000000000c",
      "value": "0005009699000200"
    },
    {
      "state": "Added",
      "key": "1bc123aa6ac42f73075ffda3da7c396263d5f5d71cbca158f62258e7f79004c275e25ea700369fca9d0000000000000000000000000c",
      "value": "000400ca9a3b00"
    }
  ]
}}

shargon · 2019-04-27T08:34:13Z

The difference for block 1568312 is SUBSTR

and before was like:

neo-vm/src/neo-vm/ExecutionEngine.cs

Lines 296 to 312 in 43b357d

    
           case OpCode.SUBSTR: 
        
               { 
        
                   int count = (int)context.EvaluationStack.Pop().GetBigInteger(); 
        
                   if (count < 0) 
        
                   { 
        
                       State |= VMState.FAULT; 
        
                       return; 
        
                   } 
        
                   int index = (int)context.EvaluationStack.Pop().GetBigInteger(); 
        
                   if (index < 0) 
        
                   { 
        
                       State |= VMState.FAULT; 
        
                       return; 
        
                   } 
        
                   byte[] x = context.EvaluationStack.Pop().GetByteArray(); 
        
                   context.EvaluationStack.Push(x.Skip(index).Take(count).ToArray()); 
        
               }

shargon · 2019-04-27T10:07:26Z

Whole testnet without issues now! @vncoelho could you confirm?

vncoelho · 2019-04-27T19:24:15Z

Great job, @shargon's brother.
It was surely and arduous task. Thanks for hunting it with care.
I may need 2 days but I will soon confirm it.

superboyiii · 2019-04-28T02:43:21Z

Amazing work! I'll test it, too. @shargon

erikzhang · 2019-04-29T03:04:30Z

@vncoelho @superboyiii Did you finish your tests?

vncoelho · 2019-04-29T13:04:05Z

Still under evaluation. Some more couple of hours and it will be finished.

vncoelho · 2019-04-29T13:20:10Z

Compared to 2.9.4 still differs:

In 2.9.4 Testnet we have 1Changed + 3 Added

{"block":759289,"size":4,"storage":[{"state":"Changed","key":"855b8152706e8beb3e3aa4188160a2262a04c85062616c616e63654f700000000000000007","value":"000410460afa00"}
{"state":"Added","key":"855b8152706e8beb3e3aa4188160a2262a04c85070ba03f52af8153d0cc1c781477b8c2600ce05b89c0000000000000000000000000c","value":"0002102700"}
{"state":"Added","key":"855b8152706e8beb3e3aa4188160a2262a04c850746f74616c537570706c79000000000005","value":"0002102700"}
{"state":"Added","key":"855b8152706e8beb3e3aa4188160a2262a04c850747275650000000000000000000000000c","value":"00047472756500"}]}

While in this current branch we also have this storage ( 1Changed + 3 Added + 1 Added):

{"state":"Added","key":"855b8152706e8beb3e3aa4188160a2262a04c85064656661756c7400000000000000000009","value":"001544656661756c7420726573756c742068657265212100"}]}

vncoelho · 2019-04-29T14:37:38Z

Compared to 2.10.0 it looks like to be exactly the same. Great job @shargon.
Some more couple of hours and let me double check and I am going to approve.
About 2.9.4 we check later.

shargon · 2019-04-29T15:40:15Z

could you confirm the 2.9.4 differences?

vncoelho · 2019-04-29T15:47:04Z

I posted it above, @shargon.

vncoelho

Compared to 2.10.0 it is exactly the same.
A temporally .zip with the storages for testnet can be download here: https://www.dropbox.com/s/44w5o0k9gpp7ybv/Storage_2.10.2.zip?dl=0
1GB but only 89MB when zipped! O.o

vncoelho · 2019-04-29T15:56:48Z

Let's continue the tests on PR138.

vncoelho · 2019-04-29T17:18:51Z

@jsolman, this current branch may solve any compatibility problem that were experienced in early versions of 2.10.1.
However, we are still going to research further in order to comprehend key historical changes, as a matter of report. But, surely, nothing critical.

jsolman · 2019-04-30T05:50:47Z

I finished syncing MainNet and the results for balances look good after these changes.

superboyiii · 2019-04-30T05:52:10Z

The same to mine.

Revert some checks for 2.x

f674fe7

shargon requested review from jsolman and erikzhang April 22, 2019 18:04

vncoelho previously approved these changes Apr 23, 2019

View reviewed changes

Update ExecutionEngine.cs

4828712

This e615bf5#diff-76ae2659301a783b97ae4c99999464cc produce a difference in the storage

Fix test

73c68ea

shargon added 2 commits April 27, 2019 10:35

SUBSTR revert

5b54a1a

Fix test

4ba0cde

shargon mentioned this pull request Apr 28, 2019

Fixes GetEntryScriptHash and GetCallingScriptHash (2.x) #138

Merged

vncoelho approved these changes Apr 29, 2019

View reviewed changes

vncoelho merged commit 0d13635 into neo-project:master-2.x Apr 29, 2019

shargon deleted the revert-some-checks branch April 30, 2019 07:38

shargon mentioned this pull request May 9, 2019

[2.10.1]invokefunction rpc execution failure neo-project/neo-node#348

Closed

superboyiii mentioned this pull request May 11, 2019

Storage return different value on neo-cli-2.10.2preview3 neo-project/neo#732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert some checks for 2.x #140

Revert some checks for 2.x #140

shargon commented Apr 22, 2019 •

edited

Loading

codecov-io commented Apr 22, 2019 •

edited

Loading

vncoelho commented Apr 22, 2019

vncoelho commented Apr 22, 2019

vncoelho commented Apr 22, 2019

erikzhang commented Apr 23, 2019

RavenXce commented Apr 23, 2019 •

edited

Loading

igormcoelho commented Apr 23, 2019 •

edited

Loading

RavenXce commented Apr 23, 2019 •

edited

Loading

vncoelho left a comment •

edited

Loading

vncoelho commented Apr 23, 2019

shargon commented Apr 26, 2019 •

edited

Loading

shargon commented Apr 26, 2019 •

edited

Loading

shargon commented Apr 27, 2019

shargon commented Apr 27, 2019

vncoelho commented Apr 27, 2019 •

edited

Loading

superboyiii commented Apr 28, 2019

erikzhang commented Apr 29, 2019

vncoelho commented Apr 29, 2019

vncoelho commented Apr 29, 2019 •

edited

Loading

vncoelho commented Apr 29, 2019

shargon commented Apr 29, 2019

vncoelho commented Apr 29, 2019

vncoelho left a comment

vncoelho commented Apr 29, 2019

vncoelho commented Apr 29, 2019 •

edited

Loading

jsolman commented Apr 30, 2019

superboyiii commented Apr 30, 2019

Revert some checks for 2.x #140

Revert some checks for 2.x #140

Conversation

shargon commented Apr 22, 2019 • edited Loading

codecov-io commented Apr 22, 2019 • edited Loading

Codecov Report

vncoelho commented Apr 22, 2019

vncoelho commented Apr 22, 2019

vncoelho commented Apr 22, 2019

erikzhang commented Apr 23, 2019

RavenXce commented Apr 23, 2019 • edited Loading

igormcoelho commented Apr 23, 2019 • edited Loading

RavenXce commented Apr 23, 2019 • edited Loading

vncoelho left a comment • edited Loading

Choose a reason for hiding this comment

vncoelho commented Apr 23, 2019

shargon commented Apr 26, 2019 • edited Loading

shargon commented Apr 26, 2019 • edited Loading

shargon commented Apr 27, 2019

shargon commented Apr 27, 2019

vncoelho commented Apr 27, 2019 • edited Loading

superboyiii commented Apr 28, 2019

erikzhang commented Apr 29, 2019

vncoelho commented Apr 29, 2019

vncoelho commented Apr 29, 2019 • edited Loading

vncoelho commented Apr 29, 2019

shargon commented Apr 29, 2019

vncoelho commented Apr 29, 2019

vncoelho left a comment

Choose a reason for hiding this comment

vncoelho commented Apr 29, 2019

vncoelho commented Apr 29, 2019 • edited Loading

jsolman commented Apr 30, 2019

superboyiii commented Apr 30, 2019

shargon commented Apr 22, 2019 •

edited

Loading

codecov-io commented Apr 22, 2019 •

edited

Loading

RavenXce commented Apr 23, 2019 •

edited

Loading

igormcoelho commented Apr 23, 2019 •

edited

Loading

RavenXce commented Apr 23, 2019 •

edited

Loading

vncoelho left a comment •

edited

Loading

shargon commented Apr 26, 2019 •

edited

Loading

shargon commented Apr 26, 2019 •

edited

Loading

vncoelho commented Apr 27, 2019 •

edited

Loading

vncoelho commented Apr 29, 2019 •

edited

Loading

vncoelho commented Apr 29, 2019 •

edited

Loading