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

zktrie part2: add zktrie; allow switch trie type by config; #113

Merged
merged 25 commits into from
Jun 27, 2022

Conversation

noel2004
Copy link
Member

@noel2004 noel2004 commented Jun 8, 2022

This PR re-organize the original #101 :

  • Induce zktrie implement on top of part1 (zktrie part1: change storage proof from per step to per block  #102, which has been merged)

  • Significant refactoring:

    • Inducing unify hashing scheme for leaf value. So there is need to store vHash in node. And it is compatible with the original one
    • Update the hashing for Leaf Node to H(Leaf) = H(H(1, NodeKey), vHash)
    • Separate the key hashing scheme with the zktrie layer, like the trie/secure_trie designation.
    • Simplifiy the zktrie code

trie/database.go Outdated Show resolved Hide resolved
lispc
lispc previously approved these changes Jun 9, 2022
lispc
lispc previously approved these changes Jun 9, 2022
core/state/statedb.go Outdated Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
core/types/zktrie/util.go Outdated Show resolved Hide resolved
core/types/zktrie/util.go Show resolved Hide resolved
core/genesis.go Show resolved Hide resolved
core/types/zktrie/hash.go Outdated Show resolved Hide resolved
core/types/zktrie/hash.go Outdated Show resolved Hide resolved
core/vm/logger.go Show resolved Hide resolved
icemelon
icemelon previously approved these changes Jun 21, 2022
Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

Just one minor comment. otherwise LGTM

icemelon
icemelon previously approved these changes Jun 23, 2022
trie/zk_trie_node.go Outdated Show resolved Hide resolved
0xmountaintop
0xmountaintop previously approved these changes Jun 23, 2022
Copy link

@0xmountaintop 0xmountaintop left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a nitpick

@mask-pp
Copy link

mask-pp commented Jun 24, 2022

It's necessary to update readme so we can use it easily.

@lispc
Copy link

lispc commented Jun 24, 2022

@noel2004 @HAOYUatHZ @mask-pp

do you think it is necessary to add another "zktrie genesis json"? Or add "zktrie: false/true" in old genesis.json, whenever we need to switch on/off zktrie, just edit this bool flag?

mask-pp
mask-pp previously approved these changes Jun 24, 2022
"period": 15,
"epoch": 30000
},
"zktrie": true
Copy link

Choose a reason for hiding this comment

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

Is it possible to use the same genesis.json file, and choose zktrie module by cmd params i.g: 'geth --zktrie' ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cmd params may be not feasible because the data being persisted in db is complete different for zktrie. Run geth with zktrie on data saved without this flag just ruin the whole db and vice visa. It has to be a flag in chain's config.

Copy link

@mask-pp mask-pp Jun 24, 2022

Choose a reason for hiding this comment

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

i.g: ./build/bin/geth init --zktrie,init is subcmd and can add the flag at here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is ok only being applied in init sub command. But the genesis object in init is mandatory, as the result it may be a confliction if we have specified zktrie flag both in genesis object and cmd flag and we need a rule for respecting which one.

I found the original geth cmd do not handle such an issue by put all possible flags only avaliable in the genesis object. So should we really need such a flag for cmd (even for the possible risk of causing some confusion)?

@mask-pp
Copy link

mask-pp commented Jun 24, 2022

The comm

@noel2004 @HAOYUatHZ @mask-pp

do you think it is necessary to add another "zktrie genesis json"? Or add "zktrie: false/true" in old genesis.json, whenever we need to switch on/off zktrie, just edit this bool flag?

I think so it's very necessary.

@mask-pp mask-pp self-requested a review June 24, 2022 02:03
@mask-pp mask-pp self-requested a review June 24, 2022 02:39
@noel2004 noel2004 dismissed stale reviews from mask-pp and 0xmountaintop via 80f4832 June 24, 2022 03:14
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