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

Fix allowing of selecting protocol versions after merge of 1.10.x #373

Conversation

ziogaschr
Copy link
Member

@ziogaschr ziogaschr commented May 10, 2021

Keep in mind that Eth protocol version 63 has been dropped.
EDIT(meowsbits) (And 62 was dropped a long time ago.)

Ref #365

@ziogaschr ziogaschr requested a review from meowsbits May 10, 2021 13:28
@@ -51,6 +51,10 @@ var (
syncChallengeTimeout = 15 * time.Second // Time allowance for a node to reply to the sync progress challenge
)

// DefaultProtocolVersions are the supported versions of the `eth` protocol (first
// is primary).
var DefaultProtocolVersions = []uint{eth.ETH66, eth.ETH65, eth.ETH64}
Copy link
Member Author

Choose a reason for hiding this comment

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

This had to be set in the eth package as the protocol has nested packages logic for (eth|snap) protocols.

@@ -543,7 +543,7 @@ func (s *Ethereum) BloomIndexer() *core.ChainIndexer { return s.bloomIndexer }
// Protocols returns all the currently configured
// network protocols to start.
func (s *Ethereum) Protocols() []p2p.Protocol {
protos := eth.MakeProtocols((*ethHandler)(s.handler), s.networkID, s.ethDialCandidates)
protos := eth.MakeProtocols((*ethHandler)(s.handler), s.networkID, s.config.ProtocolVersions, s.ethDialCandidates)
if s.config.SnapshotCache > 0 {
protos = append(protos, snap.MakeProtocols((*snapHandler)(s.handler), s.snapDialCandidates)...)
Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment we only have a flag for the eth protocols (66|65|64) and not for the snap protocols. I am not sure if we prefer handling the snap protocol version now or leave them out, as it's just one at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Noted, thanks for noticing this. SGTM to leave as-is (no flag), then think about adding one when there's more than one to choose from.

@meowsbits
Copy link
Member

So the EthProtocols flag is core-geth specific. (IIRC we added it as a kind of just-in-case option for security cases like Ethereum Classic's Phoenix upgrade where a subset of nodes (OpenEthereum, I think) failed to support 65 properly and nodes supporting (only) lower versions were important in "bridging" the network.)

This patch fixes that flag to be effectual again, and to support the 66,65,64 set.

Just making sure I understand the motivation and context here.

@ziogaschr
Copy link
Member Author

Yes, you recall correctly. This has been added on core-geth only for this exact reason.

@meowsbits
Copy link
Member

Note that I've created #375 in response to the failing tests here; it's just an impatient timeout issue.

@ziogaschr ziogaschr merged commit b9a4ddd into merge/foundation-release/1.10.1-resolved May 11, 2021
@ziogaschr ziogaschr deleted the merge/foundation-release/1.10.1-resolved-protocol-versions branch May 11, 2021 11:31
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.

2 participants