-
Notifications
You must be signed in to change notification settings - Fork 104
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
backport upstream hashdb locking changes #291
Conversation
trie/triedb/hashdb/database.go
Outdated
@@ -183,7 +178,7 @@ func (db *Database) insert(hash common.Hash, node []byte) { | |||
db.dirtiesSize += common.StorageSize(common.HashLength + len(node)) | |||
} | |||
|
|||
// Node retrieves an encoded cached trie node from memory. If it cannot be found | |||
// node retrieves an encoded cached trie node from memory. If it cannot be found | |||
// cached, the method queries the persistent database for the content. | |||
func (db *Database) Node(hash common.Hash) ([]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.
In this one you did not make the function private. Is that on purpose? Why?
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.
we need it to be public for trie.Database.Node
method that was removed in upstream, but we use it in RecordingKV.Get
.
Either way, good catch, as I should:
- either add new method
hashdb.Database.Node
for exposinghashdb.Database.node
- or probably better: I should refactor
RecordingKV
to not usetriedb.Database.Node
and usetriedb.Reader
there instead
Taking into account that pathdb
doesn't support Node
method, seems best to go ahead with second option and use triedb.Reader
, so we will be one more step closer to supporting pathdb
and also minimizing the diff. What do you think?
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'm really curious if / how well we can implement RecordingKV based off pathdb. I don't know the details.
I assume it's not trivial, so I'd say for this PR let's have a Node function exporting node, and leave pathdb support for a separate PR.
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.
makes sense, I've added the Node method, thanks!
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
backports: ethereum/go-ethereum#28542