-
Notifications
You must be signed in to change notification settings - Fork 153
backport upstream hashdb locking changes #291
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
Conversation
trie/triedb/hashdb/database.go
Outdated
| // 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.Nodefor exposinghashdb.Database.node - or probably better: I should refactor
RecordingKVto not usetriedb.Database.Nodeand usetriedb.Readerthere 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!
tsahee
left a comment
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