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

core, eth, trie, light: clean up trie interface #26388

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Dec 28, 2022

This PR cleans up/improves some trie interfaces:

  • TryGetAccount, TryUpdateAccount, TryDeleteAccount accepts common.Address as the parameters, which is more intuitive to callers

@rjl493456442
Copy link
Member Author

Let's hold-on this PR for a while.

I am not sure the change for TryGetNode is really needed.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, only minor suggestion to name them ByHash instead of WithHash, since it refers to whether we look it up by hash.

@@ -523,7 +523,7 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket, s
if snap == nil {
// We don't have the requested state snapshotted yet (or it is stale),
// but can look up the account via the trie instead.
account, err := accTrie.TryGetAccountWithPreHashedKey(pathset[0])
account, err := accTrie.TryGetAccountWithHash(common.BytesToHash(pathset[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
account, err := accTrie.TryGetAccountWithHash(common.BytesToHash(pathset[0]))
account, err := accTrie.TryGetAccountByHash(common.BytesToHash(pathset[0]))

?

@@ -417,7 +417,7 @@ func ServiceGetStorageRangesQuery(chain *core.BlockChain, req *GetStorageRangesP
if err != nil {
return nil, nil
}
acc, err := accTrie.TryGetAccountWithPreHashedKey(account[:])
acc, err := accTrie.TryGetAccountWithHash(account)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
acc, err := accTrie.TryGetAccountWithHash(account)
acc, err := accTrie.TryGetAccountByHash(account)

?

@holiman holiman added this to the 1.11.0 milestone Dec 30, 2022
@karalabe karalabe merged commit 6c149fd into ethereum:master Jan 3, 2023
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
* all: cleanup trie interface

* eth, trie: address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants