-
Notifications
You must be signed in to change notification settings - Fork 20k
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
params, trie: add verkle fork management + upgrade go-verkle #27464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have a verkle
package inside trie
? So trie/verkle
and, at some point, move old stuff into trie/mpt
?
In principle, yes, however the whole point of this PR is to make a horrible rebase easier. Doing this would have the opposite effect. Here's what I suggest: I'll remove the verkle files for now, and I'll do the move to its own subdir after the rebase. |
trie/utils/verkle.go
Outdated
return PointToHash(ret, subIndex) | ||
} | ||
|
||
func GetTreeKeyAccountLeaf(address []byte, leaf byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add function description.
trie/utils/verkle.go
Outdated
return point | ||
} | ||
|
||
func (pc *PointCache) GetTreeKeyVersionCached(addr []byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's duplicated with API *WithEvaluatedAddress
. We can always cache evaluated address in point cache and combine it with *WithEvaluatedAddress
APIs.
So we dont' need to define API like GetTreeKeyVersionCached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, as it saves on the allocation of a big.Int
and is called quite often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forget that, that was nonsense, the big.Int
is created no matter what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks, otherwise lgtm
trie/utils/verkle.go
Outdated
// Basic account leaf suffixes, see: | ||
// https://notes.ethereum.org/@vbuterin/verkle_tree_eip#Tree-embedding | ||
const ( | ||
VersionLeafKey = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to expose these constants?
trie/utils/verkle.go
Outdated
|
||
var ( | ||
zero = uint256.NewInt(0) | ||
HeaderStorageOffset = uint256.NewInt(64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to expose these variables?
trie/utils/verkle.go
Outdated
return GetTreeKey(address, zero, VersionLeafKey) | ||
} | ||
|
||
// Get does the, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? This function description is duplicated.
trie/utils/verkle.go
Outdated
} | ||
|
||
func GetTreeKeyStorageSlot(address []byte, storageKey *uint256.Int) []byte { | ||
pos := storageKey.Clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps pos := new(uint256.Int)
? We either initialize the pos via pos.Add(HeaderStorageOffset, storageKey)
or pos.Add(MainStorageOffset, storageKey)
. Make no sense to do clone first.
trie/utils/verkle.go
Outdated
} | ||
|
||
func pointToHash(evaluated *verkle.Point, suffix byte) []byte { | ||
// The output of Byte() is big engian for banderwagon. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big endian
trie/utils/verkle.go
Outdated
verkle.FromLEBytes(&poly[1], address[:16]) | ||
verkle.FromLEBytes(&poly[2], address[16:]) | ||
|
||
// tree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
trie/utils/verkle.go
Outdated
} | ||
|
||
func GetTreeKeyCodeChunk(address []byte, chunk *uint256.Int) []byte { | ||
chunkOffset := new(uint256.Int).Add(CodeOffset, chunk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will prefer this style, but yeah, feel free to ignore
var (
subIndex byte
chunkOffset = new(uint256.Int).Add(CodeOffset, chunk)
treeIndex = new(uint256.Int).Div(chunkOffset, VerkleNodeWidth)
subIndexMod = new(uint256.Int).Mod(chunkOffset, VerkleNodeWidth)
)
trie/utils/verkle.go
Outdated
// GetTreeKeyCodeChunkWithEvaluatedAddress does the same thing as GetTreeKeyCodeChunk, | ||
// but uses the cached point corresponding to the address part of the polynomial. | ||
func GetTreeKeyCodeChunkWithEvaluatedAddress(addressPoint *verkle.Point, chunk *uint256.Int) []byte { | ||
chunkOffset := new(uint256.Int).Add(CodeOffset, chunk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will prefer this style, feel free to ignore
var (
subIndex byte
chunkOffset = new(uint256.Int).Add(CodeOffset, chunk)
treeIndex = new(uint256.Int).Div(chunkOffset, VerkleNodeWidth)
subIndexMod = new(uint256.Int).Mod(chunkOffset, VerkleNodeWidth)
trie/utils/verkle.go
Outdated
func pointToHash(evaluated *verkle.Point, suffix byte) []byte { | ||
// The output of Byte() is big engian for banderwagon. This | ||
// introduces an imbalance in the tree, because hashes are | ||
// elements of a 253-bit field. This means more than half the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something irrelevant, just want to confirm.
The Byte() return 353-bit hash, so the 3 zero bits are in the beginning of the bytes, or at the end?
I guess it's at the beginning, so you want to covert to little endian?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to merge the PR, because all the stuffs are only used by verkle.
But I agree with Martin, it makes more sense to have trie/verkle
and trie/mpt
packages later on, to not mix everything in a single directory.
That makes sense, but what doesn't make sense, I just realized, is trying to integrate this last file if it's not used anywhere at the moment. I have taken notes of all the review comments and will add them to the next PR when these files are needed. |
PR SGTM, we've agreed to also add the --override.verkle flag as it might come in hangy to test different scenarios without screwing too much around with different genesis jsons and reinits. |
…m#27464) * params, trie: add verkle fork management + upgrade go-verkle * remove the two verkle files * core, eth, params: add missing function * Gary's feedback * remove trie/utils/verkle.go * add verkle block override --------- Co-authored-by: Gary Rong <garyrong0905@gmail.com>
…thereum#27464)" This reverts commit 59c73c0.
…thereum#27464)" This reverts commit 59c73c0.
In order to simplify the verkle code rebase and reduce the code diff, this PR:
IsVerkle
rule and function