p2p/simulation: Test snapshot correctness and minimal benchmark #1038

nolash · 2018-12-02T11:55:10Z

Pending merge of ethereum/go-ethereum#18220

This PR is part one of four PRs that adds test and benchmark to ensure that the snapshot connection content is correct upon creation, and that the correct connections are made after snapshot is loaded. All actual connections should be contained in snapshot, and all connections and only those connections should be in the network after load.

Part one creates the correctness test and adds a vanilla benchmark function.
Part two will move connection methods from swarm/network/simulation to p2p/simulations
Part three will add a snapshot generation binary leveraging the different configurations provided by part two.
Part four will add exhaustive benchmarks with different snapshots generated from part three.

This PR also moved the NoopService from the swarm/network/simulation package, since this minimal service is also useful for tests involving p2p/simulations directly.

~~It also adds a "test" tag to the travis script, to enable build of symbols across packages needed for testing only.~~ (linter script will apparently not easily work with build tags)

janos · 2018-12-03T14:35:33Z

p2p/simulations/test.go

+// but implements node.Service interface.
+type NoopService struct {
+	useChannel bool
+	C          map[enode.ID]chan struct{}


C should not be exported. It is used only internally.

Actually, it's used in network_test.go. And the idea is that this should be available outside of the package, which means it would need to be exported. But you mean that as long as that's not the case it should be private?

Yes, when there is a need for it to be exported, then it should be exported, not before. Or if you want to provide the functionality from start, It would be good to have comments or examples how it should be used, as in network_test.go it is injected as the exported field.

I would even say that maybe useChannel is not needed, as NewNoopService can accept the map and set it to the field c so the Run function can check if the c map is nil instead to check if useChannel is true. This way, c can remain unexported with the functionality is available outside.

janos · 2018-12-03T14:50:24Z

swarm/network/simulation/simulation_test.go

+}
+
+type noopService struct {
+	simulations.NoopService


While reducing code duplication, which is good, this approach also changes the previous noopService to have protocols, even more not to initialize a map used in the protocols run function. While this is working now, changes to the simulations.NoopService Run function may result in an unexpected failure in simulation package. This is creating more coupled code at the benefit of reducing a few lines in simulations package. I do not think that that benefit is great enough for this coupling, but I do not insist for changes, just awareness.

How about a bool in the constructor that omits the protocol altogether, or is that too convoluted?

Alternately, chaining methods to add protocol or not, api or not...?

I think that any more options would just add to the complexity. I would like to avoid any more complexity. This is fine now, until it does not extended to be more convoluted.

janos · 2018-12-03T14:57:32Z

p2p/simulations/network_test.go

+
+	// load the snapshot
+	// spawn separate thread to avoid deadlock in the event listeners
+	go func() {


We should signal to the main goroutine when the Load has returned to check if all connections are created before or after. They all should be created before when the ethereum/go-ethereum#18220 is merged.

Cool, let's mark this PR pending yours. That makes sense anyhow.

Actually since the subscription is before, this is ok and even should be in a goroutine because we want to handle all the events as they happen, no?

Yes, this should be in a goroutine, but this goroutine should signal the main goroutine when Load function returns, to check if that happens after all expected connection events are received. Or there are connections from the snapshot that happened after the Load function has returned, which should be an error.

I understand what you mean. But given that we know exactly how many connections to expect, and we audit exactly those connections, and check at the end of the function for one second of "silence," isn't that already guaranteed?

(the latter would have to be checked in any case?)

The main purpose for ethereum/go-ethereum#18220 is that connections are all established before the Load function returns. This test currently do not provide such validation, as Load function is in the goroutine that can finish at any time and connection checking is in another goroutine that can finish at any time. Connections are checked but they can be established after the Load function return and this test does not check for that as the time when Load function returns is not known or such event is not signaled to the main goroutine.

p2p/simulations/network_test.go

zelig · 2018-12-07T05:50:58Z

@nolash gofmt please https://travis-ci.org/ethersphere/go-ethereum/jobs/462927624#L502

nolash · 2018-12-10T14:35:44Z

p2p/simulations/network_test.go

+		}
+
+		// check that we have all expected connections in the network
+		for _, snapConn := range snap.Conns {


@janos since this is now a separate goroutine, can we be sure all of this will be executed before the test ends? The main routine may finish first. Or do tests in all cases join all threads before finishing?

Great catch @nolash. No implicit joining happens. I updated the code.

nolash · 2018-12-10T19:21:48Z

LGTM. Need one more review, I guess. Maybe even two, since we're now both complicit, @janos

janos · 2018-12-10T19:24:28Z

Thanks @nolash. I agree. Since I also edited this PR, my approval should not count, but it LGTM.

zelig

LGTM, rebase and submit to upstream

params/config.go

acud

@nolash generally LGTM

acud · 2018-12-11T12:42:46Z

p2p/simulations/network_test.go

+	})
+	// \todo consider making a member of network, set to true threadsafe when shutdown
+	runningOne := true
+	defer func() {


use sync.Once instead?

well, I'm not sure it's clearer in this case. It would if both shutdowns were the same.

p2p/simulations/network_test.go

nolash · 2018-12-11T19:31:02Z

submitted upstream ethereum/go-ethereum#18287

This refactor was done as the second part of our current network testing efforts; as outlined in #1038. Co-authored-by: Elad Nachmias <theman@elad.im>

zelig · 2018-12-18T14:25:28Z

merged as ethereum/go-ethereum#18287

nolash added in progress network testing simulations labels Dec 2, 2018

nolash self-assigned this Dec 2, 2018

nolash requested review from lmars and nonsense as code owners December 2, 2018 11:55

nolash removed the request for review from lmars December 2, 2018 12:11

nolash force-pushed the sim-minimal-snapshot branch from a4aa391 to c3ca10e Compare December 3, 2018 13:58

nolash requested a review from janos December 3, 2018 14:00

nolash added ready for review and removed in progress labels Dec 3, 2018

nolash removed the request for review from nonsense December 3, 2018 14:18

zelig added the stability label Dec 3, 2018

zelig added this to the 2018 Christmas Edition milestone Dec 3, 2018

janos reviewed Dec 3, 2018

View reviewed changes

nolash added pending-other-merge and removed ready for review labels Dec 3, 2018

nolash added the in progress label Dec 10, 2018

nolash force-pushed the sim-minimal-snapshot branch from 8c4e8d7 to 9dd40d2 Compare December 10, 2018 10:39

nolash removed the pending-other-merge label Dec 10, 2018

nolash requested a review from holisticode December 10, 2018 13:13

nolash added ready for review and removed in progress labels Dec 10, 2018

nolash requested a review from acud December 10, 2018 13:13

nolash commented Dec 10, 2018

View reviewed changes

zelig approved these changes Dec 11, 2018

View reviewed changes

params/config.go Outdated Show resolved Hide resolved

nolash force-pushed the sim-minimal-snapshot branch from 61ca22e to 6fb6980 Compare December 11, 2018 11:39

acud approved these changes Dec 11, 2018

View reviewed changes

zelig added submitted to ethereum/go-ethereum and removed ready for review labels Dec 12, 2018

This was referenced Dec 13, 2018

p2p/simulation: move connection methods from swarm/network/simulation ethereum/go-ethereum#18305

Closed

p2p/simulation: move connection methods from swarm/network/simulation #1057

Closed

nolash and others added 13 commits December 18, 2018 10:16

p2p/simulation: WIP minimal snapshot test

89245fb

p2p/simulation: Add snapshot create, load and verify to snapshot test

1e0244d

build: add test tag for tests

f44da20

p2p/simulations, build: Revert travis change, build test sym always

5752241

p2p/simulations: Add comments, timeout check on additional events

de133fc

p2p/simulation: Add benchmark template for minimal peer protocol init

8386370

p2p/simulations: Remove unused code

9532734

p2p/simulation: Correct timer reset

f32748a

p2p/simulations: Put snapshot check events in buffer and call blocking

fb82f70

p2p/simulations: TestSnapshot fail if Load function returns early

b503e42

p2p/simulations: TestSnapshot wait for all connections before returning

76c6758

p2p/simulation: Revert to before wait for snap load (5e75594)

834bafd

p2p/simulations: add "conns after load" subtest to TestSnapshot

b5ee891

nolash force-pushed the sim-minimal-snapshot branch from 1478373 to b5ee891 Compare December 18, 2018 09:20

zelig closed this Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p/simulation: Test snapshot correctness and minimal benchmark #1038

p2p/simulation: Test snapshot correctness and minimal benchmark #1038

nolash commented Dec 2, 2018 •

edited

Loading

janos Dec 3, 2018

nolash Dec 3, 2018

janos Dec 3, 2018

janos Dec 3, 2018

nolash Dec 3, 2018 •

edited

Loading

janos Dec 3, 2018

janos Dec 3, 2018

nolash Dec 3, 2018

nolash Dec 3, 2018

janos Dec 3, 2018

nolash Dec 3, 2018 •

edited

Loading

janos Dec 3, 2018

zelig commented Dec 7, 2018

nolash Dec 10, 2018 •

edited

Loading

janos Dec 10, 2018

nolash commented Dec 10, 2018

janos commented Dec 10, 2018

zelig left a comment

acud left a comment

acud Dec 11, 2018

nolash Dec 11, 2018

nolash commented Dec 11, 2018

zelig commented Dec 18, 2018

p2p/simulation: Test snapshot correctness and minimal benchmark #1038

p2p/simulation: Test snapshot correctness and minimal benchmark #1038

Conversation

nolash commented Dec 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig commented Dec 7, 2018

nolash Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash commented Dec 10, 2018

janos commented Dec 10, 2018

zelig left a comment

Choose a reason for hiding this comment

acud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash commented Dec 11, 2018

zelig commented Dec 18, 2018

nolash commented Dec 2, 2018 •

edited

Loading

nolash Dec 3, 2018 •

edited

Loading

nolash Dec 3, 2018 •

edited

Loading

nolash Dec 10, 2018 •

edited

Loading