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

chore(net): support multiple eth protocol versions. #1152

Merged
merged 2 commits into from Feb 4, 2023

Conversation

jinsankim
Copy link
Contributor

@jinsankim jinsankim commented Feb 3, 2023

Closes #1101

Motivation

we're announcing support for eth66/67 by default in Hello but only check for a single version in the eth Status handshake

Direction

we add a list of versions as a separate parameter to the handshake function

Alternative idea to direction

  • overwrite SharedCapability::Eth.version to Status.version

Note

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty

pending @Rjected

Comment on lines +821 to +822
// Before trying status handshake, set up the version to shared_capability
let status = Status { version: p2p_stream.shared_capability().version(), ..status };
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks correct.

@mattsse mattsse added C-bug An unexpected or incorrect behavior A-networking Related to networking in general labels Feb 3, 2023
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

please add a test for this, agree with the fix, testing the PR now

@jinsankim
Copy link
Contributor Author

please add a test for this, agree with the fix, testing the PR now

follow it up w/ b4d6880

@codecov-commenter
Copy link

Codecov Report

Merging #1152 (b4d6880) into main (a55e7fd) will increase coverage by 0.51%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1152      +/-   ##
==========================================
+ Coverage   74.75%   75.27%   +0.51%     
==========================================
  Files         333      335       +2     
  Lines       35442    36373     +931     
==========================================
+ Hits        26495    27380     +885     
- Misses       8947     8993      +46     
Flag Coverage Δ
unit-tests 75.27% <100.00%> (+0.51%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/net/network/src/session/mod.rs 68.75% <ø> (+0.06%) ⬆️
crates/net/network/src/test_utils/testnet.rs 88.20% <100.00%> (+0.92%) ⬆️
crates/transaction-pool/src/test_utils/mock.rs 51.77% <0.00%> (-6.74%) ⬇️
crates/executor/src/revm_wrap.rs 38.21% <0.00%> (-1.79%) ⬇️
crates/net/network/src/session/active.rs 85.90% <0.00%> (-1.00%) ⬇️
crates/primitives/src/transaction/mod.rs 89.29% <0.00%> (-0.54%) ⬇️
crates/transaction-pool/src/pool/txpool.rs 58.82% <0.00%> (-0.51%) ⬇️
crates/stages/src/stages/bodies.rs 92.68% <0.00%> (-0.45%) ⬇️
crates/stages/src/stages/execution.rs 92.04% <0.00%> (-0.14%) ⬇️
crates/executor/src/executor.rs 93.76% <0.00%> (-0.13%) ⬇️
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jinsankim jinsankim requested review from gakonst and removed request for Rjected February 4, 2023 13:43
@gakonst gakonst merged commit 786a0d3 into paradigmxyz:main Feb 4, 2023
@jinsankim jinsankim deleted the support-multiple-protocol branch February 5, 2023 02:05
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 6, 2023
ensi321 pushed a commit to ensi321/reth that referenced this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple all eth protocol versions announced in HelloMessage
4 participants