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

fix Storage.Find #200

Merged
merged 6 commits into from
Mar 22, 2018
Merged

fix Storage.Find #200

merged 6 commits into from
Mar 22, 2018

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented Mar 20, 2018

fix #199

@erikzhang
Copy link
Member Author

@RavenXce Can you review this?

@RavenXce
Copy link
Contributor

RavenXce commented Mar 20, 2018

I don't think this will work. As mentioned in the edit, the additional size prefix in WriteVarBytes during serialization causes iteration to not work unless the full key length is known.

By constructing the StorageKey with only the prefix, the calculated key size will never match the actual keys which will always have a longer length.

E.g. searching for prefix of 9901:
Storage.Put(..., new byte[3] { 0x99, 0x01, 0x02 }) is stored to: ...[scripthash]03990102

However previously,
Storage.Find(..., new byte[2] { 0x99, 0x01 }) constructs a prefix: ...[scripthash]9901 (does not match due to missing 03)

And with this change,
Storage.Find(..., new byte[2] { 0x99, 0x01 }) constructs a prefix: ...[scripthash]029901 (does not match due to 02 instead of 03)


So I managed to make it work only because all the keys I'm iterating over have a same known length, and so I can prepend the full key size as bytes directly.

Test code:
https://github.com/ConjurTech/switcheo/blob/master/switcheo/BrokerContract.cs#L367

@erikzhang
Copy link
Member Author

erikzhang commented Mar 20, 2018

You are right, it won't work..

This reverts commit d82d01a.
@erikzhang
Copy link
Member Author

We can group key data and add a flag to each group that indicates whether it is the last group.

@erikzhang erikzhang changed the title fix #199 fix Storage.Find Mar 20, 2018
@erikzhang
Copy link
Member Author

@RavenXce Will this work? Please review again.

Copy link
Contributor

@RavenXce RavenXce left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I have not tested it on our testnet yet. I will test it out very soon.

ms.Write(prefix, index, remain);
prefix_key = context.ScriptHash.ToArray().Concat(ms.ToArray()).ToArray();
}
StorageIterator iterator = new StorageIterator(Storages.Find(prefix_key).Where(p => p.Key.Key.Take(prefix.Length).SequenceEqual(prefix)).GetEnumerator());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Where part necessary? Since the prefix does not write any additional padding now

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's necessary. Think of this, if the key is 0x112233, the padded key is 0x11223300000000000000000000000000. Then we find with the prefix 0x11223300. This should find nothing. But if we don't have the Where part, it will match the padded key and return wrong results.

@erikzhang erikzhang merged commit 6d5cf63 into master Mar 22, 2018
@erikzhang erikzhang deleted the erik-storage-find-patch branch March 22, 2018 10:07
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
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.

StorageIterator does not work
2 participants