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

p2p: define DiscReason as uint8 (#24507) #1886

Merged
merged 1 commit into from
May 11, 2022

Conversation

piersy
Copy link
Contributor

@piersy piersy commented May 9, 2022

Cherry picking this pr from upstream.

All other implementations store disconnect reasons as a single byte,
so go-ethereum should do it too.

All other implementations store disconnect reasons as a single byte,
so go-ethereum should do it too.
@piersy piersy requested a review from a team as a code owner May 9, 2022 17:14
@piersy piersy requested review from hbandura and nategraf and removed request for a team May 9, 2022 17:14
@piersy
Copy link
Contributor Author

piersy commented May 9, 2022

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 69ae2cc

coverage: 51.1% of statements across all listed packages
coverage:  61.6% of statements in consensus/istanbul
coverage:  41.7% of statements in consensus/istanbul/announce
coverage:  54.9% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  71.2% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.4% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random
CommentID: 79e7b93e93

Copy link
Contributor

@nategraf nategraf left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this @piersy

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #1886 (69ae2cc) into master (da90e93) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
+ Coverage   54.35%   54.41%   +0.06%     
==========================================
  Files         673      673              
  Lines       88954    88954              
==========================================
+ Hits        48349    48406      +57     
+ Misses      36953    36894      -59     
- Partials     3652     3654       +2     
Impacted Files Coverage Δ
p2p/peer_error.go 58.62% <ø> (ø)
p2p/peer.go 69.34% <100.00%> (-0.73%) ⬇️
miner/worker.go 65.72% <0.00%> (-2.35%) ⬇️
consensus/istanbul/core/handler.go 81.88% <0.00%> (-1.58%) ⬇️
les/vflux/client/serverpool.go 77.49% <0.00%> (-1.48%) ⬇️
les/utils/limiter.go 91.26% <0.00%> (-1.46%) ⬇️
core/rawdb/chain_iterator.go 61.84% <0.00%> (-1.16%) ⬇️
les/odr.go 89.79% <0.00%> (-1.03%) ⬇️
core/tx_pool.go 71.24% <0.00%> (+0.21%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da90e93...69ae2cc. Read the comment docs.

@piersy piersy merged commit 9a89ff8 into master May 11, 2022
@piersy piersy deleted the piersy/cherry_pick_disconnect_log_vuln branch May 11, 2022 09:42
piersy added a commit that referenced this pull request May 11, 2022
Cherry picking upstream pr

All other implementations store disconnect reasons as a single byte,
so go-ethereum should do it too.

Co-authored-by: Felix Lange <fjl@twurst.com>
piersy added a commit that referenced this pull request May 12, 2022
Cherry picking upstream pr

All other implementations store disconnect reasons as a single byte,
so go-ethereum should do it too.

Co-authored-by: Felix Lange <fjl@twurst.com>
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