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

Contract state problem #2181

Closed
AnnaShaleva opened this issue Dec 25, 2020 · 7 comments · Fixed by #2254
Closed

Contract state problem #2181

AnnaShaleva opened this issue Dec 25, 2020 · 7 comments · Fixed by #2254
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@AnnaShaleva
Copy link
Member

Summary or problem description
Since #766 contract's manifest is stored along with other data in ContractState. We use JSON marshalling for manifest serialisation, but JSON specification does not guarantee that the order of the key-value pairs is strictly preserved during JSONisation. So there are two problems connected with that:

Problem 1: Regular contracts deployment/updates. In ContractMenedgment's Deploy method we perform the following steps:

  • Step 1: unmarshal the provided manifest from JSON
  • Step 2: validate the manifest
  • Step 3: serialise it back to JSON to put it into the storage

So there's a chance that Neo nodes (C# and Go) will have different ContractStates after Step 3 due to ambiguous manifest JSONisation. This problem affects storage dumps and stateroots.

Problem 2: Native contracts deployment. We don't have binary manifests for native contracts, so during native contracts deployment we just marshall native manifests (here) to put them into the storage. As a result, we've faced with the state difference for the genesis block during Neo-Go node testing.

Do you have any solution you want to propose?
The first problem can be solved by preserving the original (binary) manifest while storing ContractState. Then all Neo nodes will have the same manifests and the same ContractStates.

The second problem is a bit more complicated. We don't have binary manifest representation for native contracts, so we can't synchronize it between different nodes at the current moment. That's why it might be a good idea to replace the current manifest serialisation scheme by the new binary format where the order of fields is strictly defined. In this case, we can use binary format to store ContractStates and leave the JSONised manifest for RPC.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • SmartContract
@AnnaShaleva AnnaShaleva added the Discussion Initial issue state - proposed but not yet accepted label Dec 25, 2020
@roman-khimov
Copy link
Contributor

It also is a side-effect of #2119, we need some stable binary format for manifests now.

@erikzhang
Copy link
Member

public IDictionary<string, JObject> Properties { get; } = new OrderedDictionary<string, JObject>();

We use OrderedDictionary in JObject, which ensures the order.

@roman-khimov
Copy link
Contributor

That's an implementation detail that is not easy to reproduce, ordinary JSON libraries following JSON specification just don't provide this guarantee.

@roman-khimov
Copy link
Contributor

We might also solve it by storing manifest in the NEF itself (of course, in non-JSON format), it's auto-generated anyway and NEF is not really usable without it. And I think it's important to fix it before preview5 (#2171).

@roman-khimov
Copy link
Contributor

Just to reiterate, it's a problem of {"a":1,"b":2} vs {"b":2,"a":1}, as per JSON specification these are identical documents (An object is an unordered set of name/value pairs), even though the strings are different.

@erikzhang
Copy link
Member

NEO follows the writing order of JSON text.

@roman-khimov
Copy link
Contributor

It basically means that we're inventing some NEO-specific version of JSON which is not really productive. If JSON doesn't provide some characteristics we need it just doesn't fit here and we need to use something different. JSON is good for data exchange between different systems so let's use it this way, we can have manifest parameter in JSON format for contract deployment/update (any toolchain can easily generate it), but we shouldn't store it in the DB this way if we care about state reproducibility.

Any other implementation will have the same problem, no one is writing project-specific JSON libraries, there are lots of them already available following the same basic standard. And they can have lots of variations in their output (hi, #2050 and let's say I have some \b\u001B in the name field, no one is checking it anyway) while still being absolutely conformant and producing identical result if it is to be interpreted as JSON. But when we need binary stable format for the data (and that's when it becomes a part of ContractState) we should use exactly that, stable binary serialization scheme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants