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.

Sign up for GitHub

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

eth/protocols/snap: serve snap requests when possible #25644

Merged
merged 3 commits into from
Oct 3, 2022
Merged

eth/protocols/snap: serve snap requests when possible #25644

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b60dec9
eth/protocols/snap: serve snap requests when possible
holiman Aug 31, 2022
aa224a6
eth/protocols/snap: use trie to serve trie healing requests if snapsh…
holiman Sep 29, 2022
1286fbd
eth/protocols/snap, cmd/devp2p: fix trie-based heal-responses, increa…
holiman Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions eth/protocols/snap/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ import (

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/light"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/metrics"
"github.com/ethereum/go-ethereum/p2p"
"github.com/ethereum/go-ethereum/p2p/enode"
"github.com/ethereum/go-ethereum/p2p/enr"
"github.com/ethereum/go-ethereum/rlp"
"github.com/ethereum/go-ethereum/trie"
)

Expand Down Expand Up @@ -493,14 +495,8 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket, s
// We don't have the requested state available, bail out
return nil, nil
}
// The 'snap' might be nil, in which case we cannot serve storage slots.
snap := chain.Snapshots().Snapshot(req.Root)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to move this line of code to before line 525.

if snap == nil {
// We don't have the requested state snapshotted yet, bail out.
// In reality we could still serve using the account and storage
// tries only, but let's protect the node a bit while it's doing
// snapshot generation.
return nil, nil
}
// Retrieve trie nodes until the packet size limit is reached
var (
nodes [][]byte
Expand All @@ -524,13 +520,30 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket, s
bytes += uint64(len(blob))

default:
var stRoot common.Hash
// Storage slots requested, open the storage trie and retrieve from there
account, err := snap.Account(common.BytesToHash(pathset[0]))
loads++ // always account database reads, even for failures
if err != nil || account == nil {
break
if snap == nil {
var account types.StateAccount
// We don't have the requested state snapshotted yet (or it is stale).
// We can look up the account from the account trie instead.
blob, resolved, err := accTrie.TryGetNode(pathset[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjl493456442 lgty?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's not correct.

pathset[0] represents the hash of account address. TryGetNode accepts the COMPACT-format node path, but the pathset[0] is actually the raw trie node path.

we should use this code instead

account, err := accTrie.TryGetAccountWithPreHashedKey(pathset[0])
if err != nil || account == nil {
	break
}

Copy link
Member

Choose a reason for hiding this comment

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

Another issue is the counter for resolved nodes. Previously it's 1 for a single database lookup(snapshot), but it's not the case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I went with TryGetNode in this case, is because it's what is used at line 514 -- which I think is totally equivalent "path-wise". So now I'm confused...

Another issue is the counter for resolved nodes.

What's the issue? We account for it, just like we do on other lookups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I get it, you mean if TryGetAccountWithPreHashedKey is used.

loads += resolved // always account database reads, even for failures
if err != nil {
break
}
if err = rlp.DecodeBytes(blob, &account); err != nil {
break
}
stRoot = account.Root
} else {
account, err := snap.Account(common.BytesToHash(pathset[0]))
loads++ // always account database reads, even for failures
if err != nil || account == nil {
break
}
stRoot = common.BytesToHash(account.Root)
}
id := trie.StorageTrieID(req.Root, common.BytesToHash(pathset[0]), common.BytesToHash(account.Root))
id := trie.StorageTrieID(req.Root, common.BytesToHash(pathset[0]), stRoot)
stTrie, err := trie.NewStateTrie(id, triedb)
loads++ // always account database reads, even for failures
if err != nil {
Expand Down