Fixes #946 by erikzhang · Pull Request #950 · neo-project/neo

erikzhang · 2019-07-24T06:08:13Z

Close #947
Close #946

codecov-io · 2019-07-24T06:16:16Z

Codecov Report

Merging #950 into master will increase coverage by 0.14%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master     #950      +/-   ##
==========================================
+ Coverage   52.59%   52.74%   +0.14%     
==========================================
  Files         179      180       +1     
  Lines       12707    12749      +42     
==========================================
+ Hits         6683     6724      +41     
- Misses       6024     6025       +1

Impacted Files	Coverage Δ
neo/SmartContract/InteropService.NEO.cs	`25.73% <0%> (ø)`	⬆️
neo/IO/Helper.cs	`100% <100%> (ø)`	⬆️
neo/IO/Caching/DataCache.cs	`100% <100%> (ø)`	⬆️
neo/IO/ByteArrayComparer.cs	`90% <90%> (ø)`

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 9e2faf5...ca51799. Read the comment docs.

shargon · 2019-07-24T10:13:06Z

Stranger things

shargon · 2019-07-24T10:15:10Z

Very stranger things ...

shargon · 2019-07-24T10:21:36Z

Grouping was playing with us 😆

neo/IO/Caching/DataCache.cs

erikzhang · 2019-07-24T12:15:50Z

You can't remove grouping. Because it is stored with grouping and sorted with grouping in leveldb.

shargon · 2019-07-25T07:22:53Z

The last fix was wrong.

This will produce that contract destroy or other calls fail, we need to call Find without the grouping.

neo/neo/SmartContract/InteropService.cs

Line 540 in abda789

foreach (var pair in engine.Snapshot.Storages.Find(hash.ToArray()))

In fact, we should remove the grouping, is very confuse.

erikzhang · 2019-07-25T07:27:58Z

Can you point out where it is wrong?

shargon · 2019-07-25T07:30:19Z

I think that we can remove the grouping like this

We know that this stream is fixed and is for the key, we could know the length.

erikzhang · 2019-07-25T07:32:46Z

The BaseStream may be a NetworkStream, and we can't get the Length from it.

shargon · 2019-07-25T07:33:05Z

Is wrong because you need to pass the arguments in the unit test with the group

ac85174#diff-a3f0e31debacd97d5d593e04923f7f99R53

But in other calls within the code, it was called without grouping.

neo/neo/SmartContract/InteropService.cs

Line 540 in abda789

foreach (var pair in engine.Snapshot.Storages.Find(hash.ToArray()))

shargon · 2019-07-25T07:34:25Z

The BaseStream may be a NetworkStream, and we can't get the Length from it.

I know it, but only in this case always will come from a storage (file).

erikzhang · 2019-07-25T07:34:46Z

But in other calls within the code, it was called without grouping.

Only StorageKey.Key is grouped. StorageKey.ScriptHash is not grouped. So it's okay.

erikzhang · 2019-07-25T07:36:38Z

The only case that use Storage.ScriptHash in Find() is this:

neo/neo/SmartContract/InteropService.NEO.cs

Lines 341 to 369 in abda789

    
           private static bool Storage_Find(ApplicationEngine engine) 
        
           { 
        
               if (engine.CurrentContext.EvaluationStack.Pop() is InteropInterface _interface) 
        
               { 
        
                   StorageContext context = _interface.GetInterface<StorageContext>(); 
        
                   if (!CheckStorageContext(engine, context)) return false; 
        
                   byte[] prefix = engine.CurrentContext.EvaluationStack.Pop().GetByteArray(); 
        
                   byte[] prefix_key; 
        
                   using (MemoryStream ms = new MemoryStream()) 
        
                   { 
        
                       int index = 0; 
        
                       int remain = prefix.Length; 
        
                       while (remain >= 16) 
        
                       { 
        
                           ms.Write(prefix, index, 16); 
        
                           ms.WriteByte(0); 
        
                           index += 16; 
        
                           remain -= 16; 
        
                       } 
        
                       if (remain > 0) 
        
                           ms.Write(prefix, index, remain); 
        
                       prefix_key = context.ScriptHash.ToArray().Concat(ms.ToArray()).ToArray(); 
        
                   } 
        
                   StorageIterator iterator = engine.AddDisposable(new StorageIterator(engine.Snapshot.Storages.Find(prefix_key).Where(p => p.Key.Key.Take(prefix.Length).SequenceEqual(prefix)).GetEnumerator())); 
        
                   engine.CurrentContext.EvaluationStack.Push(StackItem.FromInterface(iterator)); 
        
                   return true; 
        
               } 
        
               return false; 
        
           }

And it has processed the group correctly.

shargon · 2019-07-25T07:53:57Z

Only StorageKey.Key is grouped. StorageKey.ScriptHash is not grouped. So it's okay.

I understand you, but is very weird, if i use the class, and i insert 0x00, and i need to find it with new byte[21], i need to know how works the storage layer, and this is wrong.

erikzhang · 2019-07-25T07:59:32Z

I think you misunderstood. The first 20 bytes is the script hash. The last byte is the prefix.

Your original unit test missed scripthash, I just added it for you.

shargon · 2019-07-25T08:05:21Z

Now I catch you 😆 , i was thinking in the key, but Find include the scriptHash and the key, so you are right!

vncoelho

@erikzhang, it looks great to me.

@shargon, I am trying to complete understand these UTs.

Can we take 1-2 more days to check it out? I want to talk with @igormcoelho about this sorting strategy.

igormcoelho · 2019-07-25T14:15:54Z

neo/IO/ByteArrayComparer.cs

+            return x.Length.CompareTo(y.Length);
+        }
+    }
+}


@rodoufu can you review this?

igormcoelho · 2019-07-25T14:29:01Z

neo/IO/Caching/DataCache.cs

-                    if (pair.Value.State != TrackState.Deleted && (key_prefix == null || pair.Key.ToArray().Take(key_prefix.Length).SequenceEqual(key_prefix)))
-                        yield return new KeyValuePair<TKey, TValue>(pair.Key, pair.Value.Item);
+                cached = dictionary
+                    .Where(p => p.Value.State != TrackState.Deleted && (key_prefix == null || p.Key.ToArray().Take(key_prefix.Length).SequenceEqual(key_prefix)))


Looks good to me, must do it in a deserialized format, to avoid different storage formats affecting Find.

igormcoelho · 2019-07-25T14:30:04Z

neo/IO/Caching/DataCache.cs

+                    .OrderBy(p => p.KeyBytes, ByteArrayComparer.Default)
+                    .ToArray();
+            }
+            var uncached = FindInternal(key_prefix ?? new byte[0])


Is this FindInternal implementation-specific? This is a good moment to avoid specific decisions affecting the outcome of Find.

igormcoelho · 2019-07-25T14:31:31Z

neo/IO/Caching/DataCache.cs

+                    p.Key,
+                    p.Value
+                ));
+            using (var e1 = cached.GetEnumerator())


I think this "intercalation" strategy should be done in a separate function, so it's clear and also testable (perhaps useful in other situations too).

igormcoelho · 2019-07-25T14:42:38Z

Just to make sure, this sort is not considering Grouping anymore, just raw key, correct? Because grouping looks like an implementation-specific strategy, not a protocol level one.
I don't think Find should consider ScriptHash and other prefixes... it is already processed on a selected contract, so all scripthashes and prefixes are supposed to be the same. Although I don't think this should affect any result, since they are the same prefixes 😂

erikzhang · 2019-07-26T06:34:07Z

Just to make sure, this sort is not considering Grouping anymore, just raw key, correct?

I optimized it, and now our grouping algorithm will not affect the sorting.

shargon · 2019-07-26T08:19:31Z

neo/IO/Helper.cs

+                int count;
                do
                {
                    byte[] group = reader.ReadBytes(GroupingSizeInBytes);


We should reuse this buffer

byte[] group=new byte[GroupSizeInBytes]; do{ read.Read(group,0,GroupSizeInBytes);

read.Read(group,0,GroupSizeInBytes);

This may read data less than GroupSizeInBytes and use an extra loop, which is very troublesome.

Fixes #946

99059aa

erikzhang requested a review from shargon July 24, 2019 06:08

erikzhang mentioned this pull request Jul 24, 2019

Sort during find results #947

Closed

shargon added 2 commits July 24, 2019 11:50

Copy ut from #947 (#952)

ad67223

Added entries with wrong prefix

2dfac18

shargon added 2 commits July 24, 2019 12:18

Fix

8f689e1

Complete fix

8ac83c6

Update DataCache.cs

06349e3

shargon reviewed Jul 24, 2019

View reviewed changes

neo/IO/Caching/DataCache.cs Show resolved Hide resolved

Revert DataCache.cs

f2606e1

real fix

ac85174

Update DataCache.cs

50c51da

shargon previously approved these changes Jul 25, 2019

View reviewed changes

vncoelho reviewed Jul 25, 2019

View reviewed changes

igormcoelho reviewed Jul 25, 2019

View reviewed changes

Sort unrelated grouping

41ce918

erikzhang dismissed shargon’s stale review via 41ce918 July 26, 2019 06:32

Merge branch 'master' into fixes/find

12570f9

shargon reviewed Jul 26, 2019

View reviewed changes

shargon added 3 commits July 28, 2019 12:21

Merge branch 'master' into fixes/find

d2ccbaa

Merge branch 'master' into fixes/find

875407d

Fix unit tests

68ab63a

shargon approved these changes Jul 29, 2019

View reviewed changes

Merge branch 'master' into fixes/find

ca51799

erikzhang merged commit 547234a into master Jul 30, 2019

erikzhang deleted the fixes/find branch July 30, 2019 05:00

This was referenced Apr 20, 2020

State difference at mainnet's block 3474317 nspcc-dev/neo-go#822

Closed

Make search results deterministic and following C# implementation nspcc-dev/neo-go#909

Closed

Tommo-L pushed a commit to Tommo-L/neo that referenced this pull request Jun 22, 2020

Fixes neo-project#946 (neo-project#950)

1064e77

Conversation

erikzhang commented Jul 24, 2019 • edited by shargon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shargon commented Jul 24, 2019

Uh oh!

shargon commented Jul 24, 2019

Uh oh!

shargon commented Jul 24, 2019

Uh oh!

Uh oh!

erikzhang commented Jul 24, 2019

Uh oh!

shargon commented Jul 25, 2019

Uh oh!

erikzhang commented Jul 25, 2019

Uh oh!

shargon commented Jul 25, 2019

Uh oh!

erikzhang commented Jul 25, 2019

Uh oh!

shargon commented Jul 25, 2019

Uh oh!

shargon commented Jul 25, 2019

Uh oh!

erikzhang commented Jul 25, 2019

Uh oh!

erikzhang commented Jul 25, 2019

Uh oh!

shargon commented Jul 25, 2019

Uh oh!

erikzhang commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Jul 25, 2019

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

igormcoelho Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

igormcoelho Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

igormcoelho Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

igormcoelho Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

igormcoelho commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikzhang commented Jul 26, 2019

Uh oh!

shargon Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

erikzhang Jul 28, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erikzhang commented Jul 24, 2019 •

edited by shargon

Loading

codecov-io commented Jul 24, 2019 •

edited

Loading

erikzhang commented Jul 25, 2019 •

edited

Loading

igormcoelho commented Jul 25, 2019 •

edited

Loading