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 redirectiton #1391

Closed
wants to merge 19 commits into from
Closed

Contract redirectiton #1391

wants to merge 19 commits into from

Conversation

doubiliu
Copy link
Contributor

@doubiliu doubiliu commented Jan 2, 2020

Contract redirectiton.Draft

Copy link
Contributor

@eryeer eryeer left a comment

Choose a reason for hiding this comment

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

Great work guy! Congratulations.

src/neo/SmartContract/InteropService.Contract.cs Outdated Show resolved Hide resolved
src/neo/SmartContract/InteropService.Contract.cs Outdated Show resolved Hide resolved
src/neo/Ledger/ContractState.cs Outdated Show resolved Hide resolved
src/neo/SmartContract/InteropService.Storage.cs Outdated Show resolved Hide resolved
eryeer
eryeer previously approved these changes Jan 2, 2020
Tommo-L
Tommo-L previously approved these changes Jan 6, 2020
@doubiliu doubiliu marked this pull request as ready for review January 6, 2020 08:24
eryeer
eryeer previously approved these changes Jan 6, 2020
@vncoelho vncoelho dismissed stale reviews from eryeer and Tommo-L via 0224e69 January 6, 2020 20:32
@@ -4,5 +4,6 @@ internal class StorageContext
{
public UInt160 ScriptHash;
public bool IsReadOnly;
public UInt160 PreDataKey;
Copy link
Member

Choose a reason for hiding this comment

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

Any other idea for the name of this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is preFix well? @vncoelho

Copy link
Contributor

@Tommo-L Tommo-L Jan 7, 2020

Choose a reason for hiding this comment

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

How about renaming the ScriptHash to ContractHash, PreDataKey to NameSpace, Preifx, or StorageKeyPrefix ?

@erikzhang
Copy link
Member

I rethought it a bit. I don't think we should use script hash as a storage key anymore. Maybe we should use GUID from the beginning. This eliminates the need for redirection.

@doubiliu
Copy link
Contributor Author

doubiliu commented Jan 7, 2020

@erikzhang Hi,大佬. Can you describe the logic for generating the GUID?If you use a random number, it means that when you send the contract TX, you have to add this field to the transaction or contract properties, and you have to do the check.

@erikzhang
Copy link
Member

The GUID is a local value. It is used in local database, and is not on chain.

We can create a GUID when deploying a contract. Each contract has a unique (but not global) GUID, and then the contract's storage is prefixed with that GUID.

@eryeer
Copy link
Contributor

eryeer commented Jan 7, 2020

Does it mean that each node generates its own Guid when creating a contract? If so how to ensure that the Guid generated by each node is the same? Otherwise, the state of each Storage will be different, and the state root will be different too.

@doubiliu
Copy link
Contributor Author

doubiliu commented Jan 7, 2020

I agree with @eryeer , we need to maintain the consistency of each node. If we cannot guarantee that the GUID of each node is consistent, then the influence of GUID needs to be removed in the calculation of stateroot or some other places. @erikzhang

@erikzhang
Copy link
Member

There are many ways to create deterministic GUIDs.

@doubiliu
Copy link
Contributor Author

doubiliu commented Jan 7, 2020

But if we use a deterministic GUID, it is not much different from scripthash. @erikzhang

@erikzhang
Copy link
Member

Here is an example: #1400

@erikzhang
Copy link
Member

But if we use a deterministic GUID, it is not much different from scripthash.

The main difference is that with the GUID you don't need to add redirection records, and the contract can be upgraded any times without recording additional information to prevent old contracts from being redeployed. Because if an old contract is deployed, a new GUID is created.

@doubiliu
Copy link
Contributor Author

doubiliu commented Jan 7, 2020

@erikzhang

            internal static Guid GetDeterministicGuid(Transaction tx, uint nonce)
            {
                return new Guid(Concat(tx.Hash.ToArray(), BitConverter.GetBytes(nonce)).Sha256().AsSpan(0, 16));
            }

Maybe it is not safe here.We can attack the storage area of ​​a contract by means of hash collision.

@erikzhang
Copy link
Member

@doubiliu Please discuss at #1400

@doubiliu
Copy link
Contributor Author

doubiliu commented Jan 8, 2020

OK.Got it.

@doubiliu doubiliu closed this Jan 20, 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.

6 participants