-
Notifications
You must be signed in to change notification settings - Fork 14
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
[PoC] Snapshot to bintrie #12
Conversation
Yes, please rebase this monster :)
|
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.
A few comments (yeah I know it's work in progress and you haven't optimized yet, but I was eager to take a peek)
trie/binary.go
Outdated
data[0] = byte(len(payload[0])) | ||
copy(data[1:], payload[0]) | ||
data[len(payload[0])+1] = byte(len(payload[1])) | ||
copy(data[2+len(payload[0]):], payload[1]) | ||
data[len(payload[0])+len(payload[1])+2] = byte(len(payload[2])) | ||
copy(data[2+len(payload[0])+len(payload[1]):], payload[2]) |
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.
Where is this format defined?
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 a pure flight of fancy for the moment, I wanted to see what it would take to convert the trie
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 changed the format to be closer to what the hex prefix does.
trie/binary.go
Outdated
return err | ||
} | ||
|
||
func (t *BinaryTrie) insert(depth int, key, value []byte) error { |
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.
Random thought: I wonder if an iterative insert would be faster than a recursive one. I wouldn't mind giving it a go, once we have some benchmark-tests for it
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.
sure 👍
03c51b1
to
2d92754
Compare
Co-Authored-By: Martin Holst Swende <martin@swende.se>
Co-Authored-By: Martin Holst Swende <martin@swende.se>
Do you want me to merge this or want to tick those boxes first? |
It would be better to tick the boxes first. It shouldn't make it to mainnet until I fixed the OOM that just happened on mon06 😁 |
After using node extensions:
That's roughly 30 minutes for the account trie conversion, which is a nice improvement. |
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.
LGTM, but I havent' really dived into the binary trie implementation, to be honest.
return fmt.Errorf("Could not create iterator for root %x: %v", root, err) | ||
} | ||
log.Info("Generating binary trie", "root", root) | ||
generatedRoot := snapshot.GenerateBinaryTree(ctx.GlobalString(utils.DataDirFlag.Name), it) |
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.
Does that resolve itself correctly? Like, what if --goerli
is specified, is the GlobalString (utils.DataDir...
really correct?
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.
While you're at it above, doing log.Info
, maybe add
dbPath := ctx.GlobalString(utils.DataDirFlag.Name)
log.Info("Generating binary trie", "root", root, "database", dbPath)
generatedRoot := snapshot.GenerateBinaryTree(dbPath, it)
func GenerateBinaryTree(path string, it AccountIterator) common.Hash { | ||
db, err := rawdb.NewLevelDBDatabase(path+"/bintrie", 128, 1024, "") | ||
if err != nil { | ||
panic(fmt.Sprintf("error opening bintrie db, err=%v", err)) |
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.
Whoa, please don't . You have a live iterator on the original trie db, please back out carefully and close it nicely
defer wg.Done() | ||
for kv := range btrie.CommitCh { | ||
nodeCount++ | ||
db.Put(kv.Key, kv.Value) |
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.
Might go a bit faster if you use db batches.. probably a whole lot faster, actually, since the data you put in there is pretty tiny
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.
Unless you depend on the commit being done synchronously. If you don't require it to be synchronous, you could do two things:
- Do batching here,
- Add a buffer to the
CommitCh
. Sometimes it will lag behind, but other times the db insert may be fast (when it only puts it into batch), so a reasonably sized buffer may be good here.
For some reason the correct value can't be retreived from the DB, this will be investigated later.
Verkle trees present a promising approach. Closing. |
|
Configures the genesis and network parameters of the protodanksharding devnet.
Quick and Dirty prototype to build a binary trie from the snapshot, aimed at producing initial data for [1]. It doesn't do any caching, it doesn't do any parallelism, doesn't try to save memory and it stores branches very inefficiently.
@holiman it's also based on a pre-rebase version of
trie_gen
, I'll rebase if it helps/makes sense.Refs
TODO
Running
This adds a
bintrie
subcommand togeth
that takes the current snapshot and performs the conversion based on that, so conversion can be started with: