-
Notifications
You must be signed in to change notification settings - Fork 112
p2p/simulation: Test snapshot correctness and minimal benchmark #1038
Changes from 1 commit
89245fb
1e0244d
f44da20
5752241
de133fc
8386370
9532734
f32748a
fb82f70
b503e42
76c6758
834bafd
b5ee891
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ package simulations | |
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"testing" | ||
"time" | ||
|
@@ -28,19 +29,28 @@ import ( | |
"github.com/ethereum/go-ethereum/p2p/simulations/adapters" | ||
) | ||
|
||
// Tests that a created snapshot with a minimal service only contains the expected connections | ||
// and that a network when loaded with this snapshot only contains those same connections | ||
func TestSnapshot(t *testing.T) { | ||
protoCMap := make(map[enode.ID]map[enode.ID]chan struct{}) | ||
adapter := adapters.NewSimAdapter(adapters.Services{ | ||
"noopwoop": func(ctx *adapters.ServiceContext) (node.Service, error) { | ||
svc := NewNoopService() | ||
svc := NewNoopService(false) | ||
protoCMap[ctx.Config.ID] = svc.C | ||
return svc, nil | ||
}, | ||
}) | ||
network := NewNetwork(adapter, &NetworkConfig{ | ||
DefaultService: "noopwoop", | ||
}) | ||
defer network.Shutdown() | ||
|
||
// \todo consider making a member of network, set to true threadsafe when shutdown | ||
runningOne := true | ||
defer func() { | ||
if runningOne { | ||
network.Shutdown() | ||
} | ||
}() | ||
|
||
nodeCount := 20 | ||
ids := make([]enode.ID, nodeCount) | ||
|
@@ -72,6 +82,7 @@ func TestSnapshot(t *testing.T) { | |
ctx, cancel := context.WithTimeout(context.TODO(), time.Second) | ||
defer cancel() | ||
|
||
checkIds := make(map[enode.ID][]enode.ID) | ||
connEventCount := nodeCount | ||
acud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
OUTER: | ||
for { | ||
|
@@ -80,6 +91,11 @@ OUTER: | |
t.Fatal(ctx.Err()) | ||
case ev := <-evC: | ||
if ev.Type == EventTypeConn && !ev.Control { | ||
if !ev.Conn.Up { | ||
t.Fatalf("unexpected disconnect: %v -> %v", ev.Conn.One, ev.Conn.Other) | ||
} | ||
checkIds[ev.Conn.One] = append(checkIds[ev.Conn.One], ev.Conn.Other) | ||
checkIds[ev.Conn.Other] = append(checkIds[ev.Conn.Other], ev.Conn.One) | ||
connEventCount-- | ||
log.Debug("ev", "count", connEventCount) | ||
if connEventCount == 0 { | ||
|
@@ -89,14 +105,106 @@ OUTER: | |
} | ||
} | ||
|
||
snap, err := network.Snapshot() | ||
acud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
j, err := json.Marshal(snap) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
log.Debug("snapshot taken", "nodes", len(snap.Nodes), "conns", len(snap.Conns), "json", string(j)) | ||
|
||
// verify that the snap element numbers check out | ||
if len(checkIds) != len(snap.Conns) || len(checkIds) != len(snap.Nodes) { | ||
t.Fatalf("snapshot wrong node,conn counts %d,%d != %d", len(snap.Nodes), len(snap.Conns), len(checkIds)) | ||
} | ||
|
||
// wait for all protocols to signal to close down | ||
for nodid, peers := range protoCMap { | ||
for peerid, peerC := range peers { | ||
log.Debug("getting ", "node", nodid, "peer", peerid) | ||
<-peerC | ||
} | ||
} | ||
runningOne = false | ||
sub.Unsubscribe() | ||
network.Shutdown() | ||
|
||
// check that we have all expected connections in snapshot | ||
for nodid, nodConns := range checkIds { | ||
for _, nodConn := range nodConns { | ||
var match bool | ||
for _, snapConn := range snap.Conns { | ||
if snapConn.One == nodid && snapConn.Other == nodConn { | ||
match = true | ||
break | ||
} else if snapConn.Other == nodid && snapConn.One == nodConn { | ||
match = true | ||
break | ||
} | ||
} | ||
if !match { | ||
t.Fatalf("snapshot missing conn %v -> %v", nodid, nodConn) | ||
} | ||
} | ||
} | ||
log.Info("snapshot checked") | ||
|
||
// PART II | ||
// load snapshot and verify that exactly same connections are formed | ||
protoCMap = make(map[enode.ID]map[enode.ID]chan struct{}) | ||
adapter = adapters.NewSimAdapter(adapters.Services{ | ||
"noopwoop": func(ctx *adapters.ServiceContext) (node.Service, error) { | ||
svc := NewNoopService(false) | ||
protoCMap[ctx.Config.ID] = svc.C | ||
return svc, nil | ||
}, | ||
}) | ||
network = NewNetwork(adapter, &NetworkConfig{ | ||
DefaultService: "noopwoop", | ||
}) | ||
defer func() { | ||
network.Shutdown() | ||
}() | ||
|
||
evC = make(chan *Event) | ||
sub = network.Events().Subscribe(evC) | ||
defer sub.Unsubscribe() | ||
|
||
go func() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, let's mark this PR pending yours. That makes sense anyhow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
err = network.Load(snap) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
}() | ||
|
||
ctx, cancel = context.WithTimeout(context.TODO(), time.Second*3) | ||
defer cancel() | ||
|
||
connEventCount = nodeCount | ||
OUTER_TWO: | ||
for { | ||
select { | ||
case <-ctx.Done(): | ||
t.Fatal(ctx.Err()) | ||
case ev := <-evC: | ||
if ev.Type == EventTypeConn && !ev.Control { | ||
if !ev.Conn.Up { | ||
t.Fatalf("unexpected disconnect: %v -> %v", ev.Conn.One, ev.Conn.Other) | ||
} | ||
log.Debug("conn", "on", ev.Conn.One, "other", ev.Conn.Other) | ||
checkIds[ev.Conn.One] = append(checkIds[ev.Conn.One], ev.Conn.Other) | ||
checkIds[ev.Conn.Other] = append(checkIds[ev.Conn.Other], ev.Conn.One) | ||
connEventCount-- | ||
log.Debug("ev", "count", connEventCount) | ||
if connEventCount == 0 { | ||
break OUTER_TWO | ||
} | ||
} | ||
} | ||
} | ||
|
||
t.Logf("ok") | ||
} | ||
|
||
// TestNetworkSimulation creates a multi-node simulation network with each node | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use
sync.Once
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I'm not sure it's clearer in this case. It would if both shutdowns were the same.