Skip to content
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

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Conversation

magicxyyz
Copy link
Contributor

@magicxyyz magicxyyz commented Feb 20, 2024

@cla-bot cla-bot bot added the s CLA signed label Feb 20, 2024
@magicxyyz magicxyyz changed the title backport upstream hashdb locking changes (#28542) backport upstream hashdb locking changes Feb 20, 2024
@magicxyyz magicxyyz marked this pull request as ready for review February 27, 2024 16:02
@@ -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) {
Copy link
Contributor

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?

Copy link
Contributor Author

@magicxyyz magicxyyz Feb 28, 2024

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 exposing hashdb.Database.node
  • or probably better: I should refactor RecordingKV to not use triedb.Database.Node and use triedb.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?

Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tsahee tsahee merged commit a104baf into master Mar 7, 2024
3 checks passed
@tsahee tsahee deleted the backport-hashdb-locking branch March 7, 2024 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants