Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

network/simulation: Basic bootnode support + expanded enode.ID exclusion #1799

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chadsr
Copy link

@chadsr chadsr commented Sep 23, 2019

Note: This PR is dependent on this go-ethereum PR.

  • Adds basic bootnode support to simulations, using the new NodeConfig.BootNode flag. (Lightnode support was omitted since it will soon be deprecated in Swarm)
  • Expands stop/start random node functions to support the exclusion of certain nodes (So for instance, bootnodes can be excluded)
  • Minor comment cleanup

@acud
Copy link
Member

acud commented Sep 24, 2019

@chadsr I'm not sure the PR to the main ethereum repo is necessary.
Everything in p2p/simulations is under our maintenance.

If anything, we should just PR into our master the pulling out of the vendor directory of the p2p/simulations directory, then PR to ethereum/go-ethereum a removal of the p2p/simulations directory (since they're not using it).

Please when you move stuff out of vendor use git mv and make sure you remove the appropriate entries in modules.txt.

Thanks!

@chadsr
Copy link
Author

chadsr commented Sep 24, 2019

@acud Makes sense. I'm happy to prepare said PRs and see if that's the general consensus. I'll then just rebase this PR to the migration.

Thanks for the feedback!

@chadsr
Copy link
Author

chadsr commented Sep 25, 2019

@acud After the input from fjl at go-ethereum, I figured this PR is still the best path to go?

@chadsr chadsr force-pushed the simulation-bootnode-lightnode-support branch from 26abce5 to 2655c10 Compare September 27, 2019 12:58
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

few minor comments.
also: travis is red

network/simulation/node.go Outdated Show resolved Hide resolved
network/simulation/node.go Outdated Show resolved Hide resolved
network/simulation/node_test.go Outdated Show resolved Hide resolved
network/simulation/node_test.go Outdated Show resolved Hide resolved
network/simulation/node_test.go Outdated Show resolved Hide resolved
network/simulation/node_test.go Show resolved Hide resolved
@chadsr chadsr force-pushed the simulation-bootnode-lightnode-support branch from 2655c10 to f82120d Compare October 17, 2019 14:57
@chadsr
Copy link
Author

chadsr commented Oct 17, 2019

Also in regards to the failing build, this is still reliant on a go-ethereum PR which was just merged into master.
It's not in the release yet, so this PR should not be merged until go-ethereum has released and the vendored version here has been updated.

@acud
Copy link
Member

acud commented Oct 19, 2019

Thanks for taking care of this PR @chadsr.
Approved pending vendoring of the necessary dependencies from go-ethereum on their next release.

@acud acud requested review from janos and nolash October 19, 2019 08:15
@janos
Copy link
Member

janos commented Oct 21, 2019

There are build failures:

# github.com/ethersphere/swarm/network/simulation [github.com/ethersphere/swarm/network/simulation.test]
network/simulation/node.go:93: o.Properties undefined (type *adapters.NodeConfig has no field or method Properties)
network/simulation/node.go:132: conf.Properties undefined (type *adapters.NodeConfig has no field or method Properties)
network/simulation/simulation.go:123: ctx.Config.Properties undefined (type *adapters.NodeConfig has no field or method Properties)
network/simulation/node_test.go:237:27: sim.Net.GetNodeIDsByProperty undefined (type *simulations.Network has no field or method GetNodeIDsByProperty)
FAIL	github.com/ethersphere/swarm/network/simulation [build failed]

@chadsr
Copy link
Author

chadsr commented Oct 21, 2019

@janos This is due to what I mentioned above about depending on a new go-ethereum release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants