-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
core, trie: new trie #1405
core, trie: new trie #1405
Conversation
) | ||
|
||
func ParanoiaCheck(t1 *Trie, backend Backend) (bool, *Trie) { | ||
t2 := New(nil, backend) | ||
const lruSize = 200 |
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.
What unit is this, number of nodes, leaf values?
A comment above with the unit and motivation behind the chosen size would be useful.
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.
Number of DB entries.
e0ccd13
to
289130a
Compare
Preliminary performance report:
|
// | ||
// If root is the zero hash or the sha3 hash of an empty string, the | ||
// trie is initially empty. Otherwise, New will panics if db is nil or | ||
// root does not exist in the database. Accessing the trie loads nodes |
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.
will panic [typo]
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 will return an error in the final version of this PR.
is ethdb.Database put/get meant to be concurrently used? cos AFAIU LDBDatabase implementation is threadsafe due to leveldb and batch calls being safe but MemDatabase is not, nor is its batch. |
|
Yes the trie is clearly documented not to be. But ethdb.Database is independent and ldbd is actually perfectly functional concurrently right? Either way it's worth a comment |
f23bda1
to
43cc17b
Compare
I've rebased this and added my initial stab at the merkle proof functions. Some comments on those below. My plan is to move parts of this (rlp changes in #1778, ethdb changes after #1779) into develop and wait for the other pending core changes to be merged before bringing in the trie. This also leaves more time for reviewing. About the merkle proof code: @ebuchman did an implementation for the old trie in #1624, but I couldn't port these directly because the API wasn't quite right. For the light client, the initial use of these proofs is that we have the state root hash and wish to retrieve the proof and value for a key in one request. The API in #1624 expected the value to be passed as an input to @zsfelfoldi Please comment on the API. The two functions of interest are
We'll probably find some bugs in the proof stuff and |
The prove/verify API is perfect for me, I'll probably add support for my skipLevels improvement (omitting the highest levels from the proof if we already have them) later if there's a consensus about adding it to the eth/64 protocol in the first place. I agree about the value being a result of verify, not an input parameter (although it can be read from the proof itself but it's illogical to pass as an input). |
You can also leave the debugging of proofs to me. Do you think there's a chance that the new trie can be merged into develop in the next few days? I need it but I'm already rebased on top of Peter's #1779 and I really wouldn't like to go any further away from develop :) |
Nice work! I agree the Value doesn't belong in the API, I was using it for sanity checking while testing and forgot to remove (it only just checks that the value passed in and the value at the bottom of the proof are the same). Also wasn't sure about encoding so just tried something simple. But looks like you have a shorter implementation anyways so great! I'll take a look at some testing. |
@zsfelfoldi I've rebased this on develop (including the ethdb move). If you rebase the light client work on it, your PR will no longer be cluttered with them. |
* Disable gas price minimum updates * Round robin transfers
This PR contains a rewrite of package trie with the following goals: