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

[neox] persistence #1692

Closed
wants to merge 61 commits into from

Conversation

ZhangTao1596
Copy link
Contributor

@ZhangTao1596 ZhangTao1596 commented Jun 10, 2020

mpt integration (Step 2/4)

  • Add LocalStateRoot in persistence to store every local root hash when block persisted completed.
  • Add ValidatorsStateRoot to store the latest confirmed state root with cn signatures.
  • Use MPT in Storages

We use the latest StateRoot from cn to verify local state no matter historical state roots are.

@ZhangTao1596 ZhangTao1596 reopened this Jun 10, 2020
@roman-khimov
Copy link
Contributor

I don't think this one takes into account the discussion in #1526 and #1383 and follows the design we currently have in neox-2.x with completely detached StateRoot. While this design might be appropriate for 2.x because of backwards compatibility requirements, it's certainly not optimal for 3.0 because as we have recently seen in the testnet, having block sync detached from state sync is dangerous and better be avoided.

Putting StateRoot in the block header also simplifies the design a lot, we just won't need any additional messages and won't have the situation when there is a state, but it's 'unconfirmed'.

You can also look at it from a bit different perspective --- this network is synchronized by blocks and it shouldn't require anything additional for correct operation, while a separate StateRoot entities distributed with different messages create an additional synchronization mechanism that at the same time somehow affects the primary block-bound synchronization. This leads to complex interaction between these two mechanism and creates additional failure modes that again can be avoided with a simple move of StateRoot info into the block header.

@ZhangTao1596
Copy link
Contributor Author

@roman-khimov We should also consider upgrade and rollback.
Agree what we have learnt in testnet. I think we can continue improving it. Anyway it's only our preview1.
On neo3, we can use the latest state root instead sync one by one. Remove the state height requirement for consensus node. So that we can avoid the influence to block consensus.
I agree put state root into header is simple and clean. But we need virtual execution environments to run old blocks. We need a specific solution to this before we put state root into header.

@roman-khimov
Copy link
Contributor

We should also consider upgrade and rollback.

We have #1287 and #2987 proposals that can handle that in a well-defined predictable manner.

Agree what we have learnt in testnet. I think we can continue improving it. Anyway it's only our preview1.

Well, that's kinda what testnet and previews are made for --- to see if and how things break with more realistic usage scenarios. But at the same time I think we should immediately take into account the experience we're getting from these tests and previews. And that experience shows some problems with the design we currently have.

We need a specific solution to this before we put state root into header.

That's probably where I would disagree, we can make an implementation of header-tied StateRoot first and deal with versioning/upgrade handling later, there is nothing preventing that. At the same time it could also be done the other way around starting with versioning, of course. But any of these two routes saves us some time on implementing/debugging/reviewing/reworking StateRoot messages and block-state synchronization issues.

@ZhangTao1596
Copy link
Contributor Author

@erikzhang Can you have a look?

@ZhangTao1596
Copy link
Contributor Author

ZhangTao1596 commented Jun 12, 2020

Update the consensus first?

I will commit consensus change in another pr soon.

snapshot.Storages.Commit();
var root = snapshot.LocalStateRoot.GetAndChange(snapshot.Height, () => new HashIndexState());
root.Index = snapshot.Height;
root.Hash = ((MPTDataCache<StorageKey, StorageItem>)snapshot.Storages).Root.Hash;
Copy link
Member

Choose a reason for hiding this comment

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

If we require a MPTDataCache maybe we should modify https://github.com/neo-project/neo/pull/1692/files#diff-de4093105173aee33135b364a5a08edaR17 to be a MPTDataCache

Copy link
Contributor

Choose a reason for hiding this comment

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

But we can't convert DataClone to MPTCache after CreateSnapshot method.

public DataCache<TKey, TValue> CreateSnapshot()
{
return new CloneCache<TKey, TValue>(this);
}

@shargon
Copy link
Member

shargon commented Jun 20, 2020

Could you merge this neox changes in other branch different than master ? we need the specification and the agreement of #1526 first , so it will be faster if we join the changes, but not merge it into master until these decisions.

@ZhangTao1596
Copy link
Contributor Author

@shargon Can you create neox branch?

@shargon
Copy link
Member

shargon commented Jun 22, 2020

@KickSeason done https://github.com/neo-project/neo/tree/neox

@ZhangTao1596
Copy link
Contributor Author

Close this to change to neox.

This was referenced Jun 22, 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.

None yet

5 participants