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

Jump to bottom

eth: don't wait for snap registration if we're not running snap #22272

Merged
merged 3 commits into from
Feb 5, 2021
Merged

eth: don't wait for snap registration if we're not running snap #22272

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3277dbf
eth: don't wait for snap registration if we're not running snap
holiman Feb 5, 2021
ecada57
eth, p2p: better logic surrounding protocol capabilities
holiman Feb 5, 2021
34241b5
eth: simplify protocol logic
holiman Feb 5, 2021
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
19 changes: 11 additions & 8 deletions eth/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,13 @@ func newHandler(config *handlerConfig) (*handler, error) {
func (h *handler) runEthPeer(peer *eth.Peer, handler eth.Handler) error {
// If the peer has a `snap` extension, wait for it to connect so we can have
// a uniform initialization/teardown mechanism
snap, err := h.peers.waitSnapExtension(peer)
if err != nil {
peer.Log().Error("Snapshot extension barrier failed", "err", err)
return err
var snapPeer *snap.Peer
if peer.RunningCap(snap.ProtocolName, snap.ProtocolVersions) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is not necessary here. The original code is fine, will return nil if snap is not enabled. No need to check it here too.

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 right -- I added this first, and later changed the inner one. and didn't consider that the inner change made this moot

var err error
if snapPeer, err = h.peers.waitSnapExtension(peer); err != nil {
peer.Log().Error("Snapshot extension barrier failed", "err", err)
return err
}
}
// TODO(karalabe): Not sure why this is needed
if !h.chainSync.handlePeerEvent(peer) {
Expand All @@ -261,7 +264,7 @@ func (h *handler) runEthPeer(peer *eth.Peer, handler eth.Handler) error {
}
reject := false // reserved peer slots
if atomic.LoadUint32(&h.snapSync) == 1 {
if snap == nil {
if snapPeer == nil {
// If we are running snap-sync, we want to reserve roughly half the peer
// slots for peers supporting the snap protocol.
// The logic here is; we only allow up to 5 more non-snap peers than snap-peers.
Expand All @@ -279,7 +282,7 @@ func (h *handler) runEthPeer(peer *eth.Peer, handler eth.Handler) error {
peer.Log().Debug("Ethereum peer connected", "name", peer.Name())

// Register the peer locally
if err := h.peers.registerPeer(peer, snap); err != nil {
if err := h.peers.registerPeer(peer, snapPeer); err != nil {
peer.Log().Error("Ethereum peer registration failed", "err", err)
return err
}
Expand All @@ -294,8 +297,8 @@ func (h *handler) runEthPeer(peer *eth.Peer, handler eth.Handler) error {
peer.Log().Error("Failed to register peer in eth syncer", "err", err)
return err
}
if snap != nil {
if err := h.downloader.SnapSyncer.Register(snap); err != nil {
if snapPeer != nil {
if err := h.downloader.SnapSyncer.Register(snapPeer); err != nil {
peer.Log().Error("Failed to register peer in snap syncer", "err", err)
return err
}
Expand Down
4 changes: 2 additions & 2 deletions eth/peerset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func newPeerSet() *peerSet {
func (ps *peerSet) registerSnapExtension(peer *snap.Peer) error {
// Reject the peer if it advertises `snap` without `eth` as `snap` is only a
// satellite protocol meaningful with the chain selection of `eth`
if !peer.SupportsCap(eth.ProtocolName, eth.ProtocolVersions) {
if !peer.RunningCap(eth.ProtocolName, eth.ProtocolVersions) {
return errSnapWithoutEth
}
// Ensure nobody can double connect
Expand Down Expand Up @@ -101,7 +101,7 @@ func (ps *peerSet) registerSnapExtension(peer *snap.Peer) error {
// by the peerset.
func (ps *peerSet) waitSnapExtension(peer *eth.Peer) (*snap.Peer, error) {
// If the peer does not support a compatible `snap`, don't wait
if !peer.SupportsCap(snap.ProtocolName, snap.ProtocolVersions) {
if !peer.RunningCap(snap.ProtocolName, snap.ProtocolVersions) {
return nil, nil
}
// Ensure nobody can double connect
Expand Down
17 changes: 8 additions & 9 deletions p2p/peer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,14 @@ func (p *Peer) Caps() []Cap {
return p.rw.caps
}

// SupportsCap returns true if the peer supports any of the enumerated versions
// of a specific protocol.
func (p *Peer) SupportsCap(protocol string, versions []uint) bool {
for _, cap := range p.rw.caps {
if cap.Name == protocol {
for _, ver := range versions {
if cap.Version == ver {
return true
}
// RunningCap returns true if the peer is actively connected using any of the
// enumerated versions of a specific protocol, meaning that at least one of the
// versions is supported by both this node and the peer p.
func (p *Peer) RunningCap(protocol string, versions []uint) bool {
if proto, ok := p.running[protocol]; ok {
for _, ver := range versions {
if proto.Version == ver {
return true
}
}
}
Expand Down