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 redirection record #694

Closed
erikzhang opened this issue Apr 8, 2019 · 16 comments
Closed

Contract redirection record #694

erikzhang opened this issue Apr 8, 2019 · 16 comments
Labels
Design Issue state - Feature accepted but the solution requires a design before being implemented Ledger Module - The ledger is our 'database', this is used to tag changes about how we store information Migration Type - These issues require knowledge in both Neo 2 and Neo 3

Comments

@erikzhang
Copy link
Member

Currently, when a smart contract is upgraded, all stored data is migrated to the new contract. If there is a large amount of data in the contract, the workload of the migration will be very large and may cause a DoS attack.

So I proposed a redirection mechanism. Instead of migrating data directly during each contract upgrade, a redirection record is created. When the contract reads the data, it can find the original contract hash based on the redirection record, thus accessing the correct data.

The downside to this is that the old contract cannot be redeployed in the future. But I don't think this will have any effect. Unless a hash collision occurs, this probability is negligible.

@erikzhang erikzhang added the Discussion Initial issue state - proposed but not yet accepted label Apr 8, 2019
@erikzhang erikzhang added this to the NEO 3.0 milestone Apr 8, 2019
@shargon
Copy link
Member

shargon commented Apr 8, 2019

Maybe we can iterate all the keys and move these keys to the new one, is faster than Get, Remove and Put.

@igormcoelho
Copy link
Contributor

I agree Erik, for the current data format.. however, on MPT, the scripthash is just a header, so migration would happen immediately, independent of how many keys are there. So, I think its better to adopt MPT asap.

@erikzhang
Copy link
Member Author

however, on MPT, the scripthash is just a header, so migration would happen immediately, independent of how many keys are there. So, I think its better to adopt MPT asap.

How does MPT help migration happen immediately?

@igormcoelho
Copy link
Contributor

On MPT, storage keys are spread through all the tree (or trie to be precise), so when a change is made on the leaf (value change for example), the root node changes. However, when a change is made on root, no change needs to be propagated down the tree. Our current proposal creates a MPT for every contract, so a scripthash change will just affect the MPT contract root node, a constant-time change O(1). To make things more solid, we can also propose a global MPT node that stores the scripthashes of the existing contracts (with storage capabilities), but even this change would only require a maximum of 20 steps in a near impossible scenario, so it will also be O(1).
On general terms, problem is fully solved with MPT. However, MPT is not good for parallel write access, and this is one thing we may want to think carefully.
Currently, there's no good/perfect way to allow parallelism on storage, but this will become even harder with MPT. One natural parallelism that will keep working without locks, as explained on whitepaper, is by executing different contracts in parallel (without dynamic invoke). But when executing operations on th same contract, I don't see many options instead of a lock mechanism. I think the trace proposal (#602) can help in this sense, but it is only a performance feature that we can think about for the future, and does not impact in current design.

To finish, even if storage is not affected by migration, perhaps it's nice to leave a proxy from the old contract to the new one, in order to make things easier for users when migrating contracts. In this sense, I fully agree with this proposal.

@igormcoelho
Copy link
Contributor

@rodoufu we need to prepare a tutorial on MPT for the whole community understand our proposal. We also need to make sure that issue with "other implementations" is effectively resolved.

@rodoufu
Copy link

rodoufu commented Apr 9, 2019

@igormcoelho, when you say tutorial, what are you thinking about?

@igormcoelho
Copy link
Contributor

@igormcoelho, when you say tutorial, what are you thinking about?

Some slides, or a medium post... explaining step by step how the proposal will be. The simpler the better :)

@igormcoelho
Copy link
Contributor

@erikzhang This contract redirection is an amazing idea, just to enforce it here... I always want this feature!!

In the case of NEP-5 "frameworks" and contract inheritance this would be very useful, as people may migrate the base, and it will still be accessible: #772

@shargon
Copy link
Member

shargon commented Jul 17, 2019

Is possible to use leveldb in parallel? is thread safe?

@erikzhang
Copy link
Member Author

Is possible to use leveldb in parallel? is thread safe?

Yes.

@vncoelho
Copy link
Member

vncoelho commented Jul 17, 2019

@erikzhang and @igormcoelho, it is another topic, however, a little bit related, what about a native class that contracts inherits if they are willing to use migrate?

@igormcoelho
Copy link
Contributor

igormcoelho commented Jul 18, 2019

In fact, it's a connected topic brother... very well remembered, it's fundamental to the concept of redirection here. Perhaps, users should be able to disable redirection when invoking the appcall (Call and CallWithRedirection). This way, you can have "raw" call (doesnt trusting migration), and "fluid call", that trusts contract and its migration. These models may interoperate for a "safe" inheritance model (native contract is in fact a contract with continuous migration... every release is a new migration, transparent to the user).

@igormcoelho
Copy link
Contributor

In the case of massive migration, it's better to solve this with MPT storages. A single storage key changes the whole storage location @rodoufu (whole storage is moved in O(1) constant time)

The proposal by Erik is in fact much deeper (in my opinion), it means that you could invoke some contract, and it "automatically" invoke another one (migrated from the first one).
The point there is:

  • Do we want/need this feature?
  • How to implement it?

To implement it, my opinion is to have two different types of calls, one accepting direction, and another that won't accept. There are situations where you must be sure that scripthash matches exactly.

Some examples of needed discussion:

  • Withdraw funds from contractA on verification. This will invoke contractA... but wait, contractA now is contractB (after migration), but which funds will be taken back, contractA, or contractB? What happens if I put funds on contractA, after migration to contractB? Should it migrate from contractA to B? Will it allow another future deploy on contractA after migration to contractB? Will a contract actually invoke the next one (by redirection), or only a transparent redirection (I favor this option)?

@igormcoelho
Copy link
Contributor

igormcoelho commented Jul 18, 2019

My opinion in what should happen after redirection (we can discuss over the bullet points):

  1. Funds (native) should be redirected after migration, and no future deploy allowed on the same key (this allows native assets to flow).
  2. Redirection should happen in a transparent manner (such as on verification), so that it's resolved directly after resolving script itself (in a single transparent step). Example: entryscript calling Call contractA will actually execute contractB, and contractB will see callingscript as the entryscript (not contractA redirected to it). Native NEP5 can implement this by calling some sort of System.Contract.MigratedTo, and re-moving the assets, on the fly (native can be done automatically), or even letting them be there, as future access will also provide valid access (unless verification rules changed after migration).
  3. Verification should work properly, using redirection, without noticing that redirection happened (this is natural due to the workings of Verification Trigger... a new contract will be called over the same trigger). This is important to be able to move any NEP5 asset (non-native), from past accounts to newer ones.
  4. Calls should have the ability to CallWithoutRedirection, so that it will fail if contract has migrated. This should be the default on final/sealed contracts (no migration allowed)

@lock9 lock9 added Design Issue state - Feature accepted but the solution requires a design before being implemented Ledger Module - The ledger is our 'database', this is used to tag changes about how we store information Migration Type - These issues require knowledge in both Neo 2 and Neo 3 and removed Discussion Initial issue state - proposed but not yet accepted potential enhancement labels Aug 12, 2019
@shargon
Copy link
Member

shargon commented Sep 12, 2019

In my opinion we need to know how many information could produce a big delay, because maybe is easier to solve it with multi threads.
We need some benchmarks.

@erikzhang
Copy link
Member Author

The bottleneck of storage migration lies in the hard disk IO, not the CPU. So multithreading does not solve the problem here.

@erikzhang erikzhang removed this from the NEO 3.0 milestone Jan 29, 2020
Thacryba pushed a commit to simplitech/neo that referenced this issue Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Issue state - Feature accepted but the solution requires a design before being implemented Ledger Module - The ledger is our 'database', this is used to tag changes about how we store information Migration Type - These issues require knowledge in both Neo 2 and Neo 3
Projects
None yet
Development

No branches or pull requests

6 participants