p2p/simulations: remove simulations module #30250
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lightclient
changed the title
fjl
reviewed
Aug 1, 2024
p2p/rlpx/rlpx_test.go
Outdated
| @@ -383,12 +382,8 @@ func BenchmarkHandshakeRead(b *testing.B) { | |||
| } | |||
|
|
|||
| func BenchmarkThroughput(b *testing.B) { | |||
| pipe1, pipe2, err := pipes.TCPPipe() | |||
There was a problem hiding this comment.
It's useful to have TCPPipe because it buffers the input to some extent, unlike net.Pipe which is implemented purely in Go and does not buffer.
There was a problem hiding this comment.
Okay I see. I added back the TCPPipe implementation.
lightclient
force-pushed
the
p2p-rm-simulations
branch
from
8767c0e
to
558b5b8
Compare
lightclient
force-pushed
the
p2p-rm-simulations
branch
from
558b5b8
to
a50fc3b
Compare
islishude
reviewed
Aug 12, 2024
| "github.com/ethereum/go-ethereum/p2p/simulations/adapters" | ||
| "github.com/ethereum/go-ethereum/rpc" | ||
| "github.com/gorilla/websocket" | ||
| "github.com/julienschmidt/httprouter" |
There was a problem hiding this comment.
Need to run go mod tidy to disregard this package in the go.mod
rjl493456442
pushed a commit
to rjl493456442/go-ethereum
that referenced
this pull request
Aug 13, 2024
Looking at the history of these packages over the past several years, there haven't been any meaningful contributions or usages: https://github.com/ethereum/go-ethereum/commits/master/p2p/simulations?before=de6d5976794a9ed3b626d4eba57bf7f0806fb970+35 Almost all of the commits are part of larger refactors or low-hanging-fruit contributions. Seems like it's not providing much value and taking up team + contributor time.
13 tasks
RaghavSood
pushed a commit
to NixOS/nixpkgs
that referenced
this pull request
Aug 15, 2024
Release notes: https://github.com/ethereum/go-ethereum/releases/tag/v1.14.8 Note that cmd/p2psim was removed in ethereum/go-ethereum#30250
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposing we remove the module
p2p/simulationsfrom geth. This was previously suggested in 2019 by the swarm team, but at that point @fjl was happy keeping it in geth. Over the past five years we've developed the suite of tests incmd/devp2p. I can't say for certain, but it appears this has reduced our reliance onp2p/simulations?Looking at the history of the module over the past several years, there haven't been any meaningful contributions or usages: https://github.com/ethereum/go-ethereum/commits/master/p2p/simulations?before=de6d5976794a9ed3b626d4eba57bf7f0806fb970+35
Almost all of the commits to the module are part of larger refactors or low-hanging-fruit contributions. Seems like it's not providing much value and taking up team + contributor time.