From 6a2bb65fe225c434041de6c3325734962991cee1 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 23 Oct 2020 05:49:42 +0300 Subject: [PATCH] Fix BestFinalized method (#7619) * turn tests to use testing vector * add regression test case * complete tests --- beacon-chain/p2p/peers/status.go | 17 ++- beacon-chain/p2p/peers/status_test.go | 182 +++++++++++++++++++------- 2 files changed, 143 insertions(+), 56 deletions(-) diff --git a/beacon-chain/p2p/peers/status.go b/beacon-chain/p2p/peers/status.go index 2a85859c8..ac13c0e9c 100644 --- a/beacon-chain/p2p/peers/status.go +++ b/beacon-chain/p2p/peers/status.go @@ -426,11 +426,12 @@ func (p *Status) Prune() { } } -// BestFinalized returns the highest finalized epoch equal to or higher than ours that is agreed upon by the majority of peers. -// This method may not return the absolute highest finalized, but the finalized epoch in which most peers can serve blocks. -// Ideally, all peers would be reporting the same finalized epoch but some may be behind due to their own latency, or because of -// their finalized epoch at the time we queried them. -// Returns the best finalized root, epoch number, and list of peers that are at or beyond that epoch. +// BestFinalized returns the highest finalized epoch equal to or higher than ours that is agreed +// upon by the majority of peers. This method may not return the absolute highest finalized, but +// the finalized epoch in which most peers can serve blocks (plurality voting). +// Ideally, all peers would be reporting the same finalized epoch but some may be behind due to their +// own latency, or because of their finalized epoch at the time we queried them. +// Returns epoch number and list of peers that are at or beyond that epoch. func (p *Status) BestFinalized(maxPeers int, ourFinalizedEpoch uint64) (uint64, []peer.ID) { connected := p.Connected() finalizedEpochVotes := make(map[uint64]uint64) @@ -459,8 +460,10 @@ func (p *Status) BestFinalized(maxPeers int, ourFinalizedEpoch uint64) (uint64, // Sort PIDs by finalized epoch, in decreasing order. sort.Slice(potentialPIDs, func(i, j int) bool { - return pidEpoch[potentialPIDs[i]] > pidEpoch[potentialPIDs[j]] && - pidHead[potentialPIDs[i]] > pidHead[potentialPIDs[j]] + if pidEpoch[potentialPIDs[i]] == pidEpoch[potentialPIDs[j]] { + return pidHead[potentialPIDs[i]] > pidHead[potentialPIDs[j]] + } + return pidEpoch[potentialPIDs[i]] > pidEpoch[potentialPIDs[j]] }) // Trim potential peers to those on or after target epoch. diff --git a/beacon-chain/p2p/peers/status_test.go b/beacon-chain/p2p/peers/status_test.go index 4c24b8466..2301d477c 100644 --- a/beacon-chain/p2p/peers/status_test.go +++ b/beacon-chain/p2p/peers/status_test.go @@ -536,58 +536,142 @@ func TestTrimmedOrderedPeers(t *testing.T) { assert.Equal(t, pid1, pids[2], "Incorrect third peer") } -func TestBestPeer(t *testing.T) { - maxBadResponses := 2 - expectedFinEpoch := uint64(4) - expectedRoot := [32]byte{'t', 'e', 's', 't'} - junkRoot := [32]byte{'j', 'u', 'n', 'k'} - p := peers.NewStatus(context.Background(), &peers.StatusConfig{ - PeerLimit: 30, - ScorerParams: &scorers.Config{ - BadResponsesScorerConfig: &scorers.BadResponsesScorerConfig{ - Threshold: maxBadResponses, +func TestStatus_BestPeer(t *testing.T) { + type peerConfig struct { + headSlot uint64 + finalizedEpoch uint64 + } + tests := []struct { + name string + peers []*peerConfig + limitPeers int + ourFinalizedEpoch uint64 + targetEpoch uint64 + // targetEpochSupport denotes how many peers support returned epoch. + targetEpochSupport int + }{ + { + name: "head slot matches finalized epoch", + peers: []*peerConfig{ + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 3 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 3 * params.BeaconConfig().SlotsPerEpoch}, }, + limitPeers: 15, + targetEpoch: 4, + targetEpochSupport: 4, }, - }) + { + // Peers are compared using their finalized epoch, head should not affect peer selection. + // Test case below is a regression case: to ensure that only epoch is used indeed. + // (Function sorts peers, and on equal head slot, produced incorrect results). + name: "head slots equal for peers with different finalized epochs", + peers: []*peerConfig{ + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 4 * params.BeaconConfig().SlotsPerEpoch}, + }, + limitPeers: 15, + targetEpoch: 4, + targetEpochSupport: 4, + }, + { + name: "head slot significantly ahead of finalized epoch (long period of non-finality)", + peers: []*peerConfig{ + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + }, + limitPeers: 15, + targetEpoch: 4, + targetEpochSupport: 4, + }, + { + name: "ignore lower epoch peers", + peers: []*peerConfig{ + {finalizedEpoch: 4, headSlot: 41 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 43 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 44 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 45 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 46 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + }, + ourFinalizedEpoch: 5, + limitPeers: 15, + targetEpoch: 6, + targetEpochSupport: 1, + }, + { + name: "combine peers from several epochs starting from epoch higher than ours", + peers: []*peerConfig{ + {finalizedEpoch: 4, headSlot: 41 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 43 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 44 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 45 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 46 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 7, headSlot: 7 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 8, headSlot: 8 * params.BeaconConfig().SlotsPerEpoch}, + }, + ourFinalizedEpoch: 5, + limitPeers: 15, + targetEpoch: 6, + targetEpochSupport: 5, + }, + { + name: "limit number of returned peers", + peers: []*peerConfig{ + {finalizedEpoch: 4, headSlot: 41 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 42 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 43 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 44 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 4, headSlot: 45 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 3, headSlot: 46 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 6, headSlot: 6 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 7, headSlot: 7 * params.BeaconConfig().SlotsPerEpoch}, + {finalizedEpoch: 8, headSlot: 8 * params.BeaconConfig().SlotsPerEpoch}, + }, + ourFinalizedEpoch: 5, + limitPeers: 4, + targetEpoch: 6, + targetEpochSupport: 4, + }, + } - // Peer 1 - pid1 := addPeer(t, p, peers.PeerConnected) - p.SetChainState(pid1, &pb.Status{ - FinalizedEpoch: expectedFinEpoch, - FinalizedRoot: expectedRoot[:], - }) - // Peer 2 - pid2 := addPeer(t, p, peers.PeerConnected) - p.SetChainState(pid2, &pb.Status{ - FinalizedEpoch: expectedFinEpoch, - FinalizedRoot: expectedRoot[:], - }) - // Peer 3 - pid3 := addPeer(t, p, peers.PeerConnected) - p.SetChainState(pid3, &pb.Status{ - FinalizedEpoch: 3, - FinalizedRoot: junkRoot[:], - }) - // Peer 4 - pid4 := addPeer(t, p, peers.PeerConnected) - p.SetChainState(pid4, &pb.Status{ - FinalizedEpoch: expectedFinEpoch, - FinalizedRoot: expectedRoot[:], - }) - // Peer 5 - pid5 := addPeer(t, p, peers.PeerConnected) - p.SetChainState(pid5, &pb.Status{ - FinalizedEpoch: expectedFinEpoch, - FinalizedRoot: expectedRoot[:], - }) - // Peer 6 - pid6 := addPeer(t, p, peers.PeerConnected) - p.SetChainState(pid6, &pb.Status{ - FinalizedEpoch: 3, - FinalizedRoot: junkRoot[:], - }) - retEpoch, _ := p.BestFinalized(15, 0) - assert.Equal(t, expectedFinEpoch, retEpoch, "Incorrect Finalized epoch retrieved") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := peers.NewStatus(context.Background(), &peers.StatusConfig{ + PeerLimit: 30, + ScorerParams: &scorers.Config{ + BadResponsesScorerConfig: &scorers.BadResponsesScorerConfig{Threshold: 2}, + }, + }) + for _, peerConfig := range tt.peers { + p.SetChainState(addPeer(t, p, peers.PeerConnected), &pb.Status{ + FinalizedEpoch: peerConfig.finalizedEpoch, + HeadSlot: peerConfig.headSlot, + }) + } + epoch, pids := p.BestFinalized(tt.limitPeers, tt.ourFinalizedEpoch) + assert.Equal(t, tt.targetEpoch, epoch, "Unexpected epoch retrieved") + assert.Equal(t, tt.targetEpochSupport, len(pids), "Unexpected number of peers supporting retrieved epoch") + }) + } } func TestBestFinalized_returnsMaxValue(t *testing.T) {