diff --git a/Makefile b/Makefile index 33030c02c..fff06f6c7 100644 --- a/Makefile +++ b/Makefile @@ -431,8 +431,8 @@ benchmark_sortition: ## Benchmark the Sortition library go test ${VERBOSE_TEST} -bench=. -run ^# ./consensus/leader_election/sortition .PHONY: benchmark_p2p_addrbook -benchmark_p2p_addrbook: ## Benchmark all P2P addr book related tests - go test ${VERBOSE_TEST} -bench=. -run BenchmarkAddrBook -count=1 ./p2p/... +benchmark_p2p_peerstore: ## Run P2P peerstore benchmarks + go test ${VERBOSE_TEST} -bench=. -run BenchmarkPeerstore -count=1 ./p2p/... ### Inspired by @goldinguy_ in this post: https://goldin.io/blog/stop-using-todo ### # TODO - General Purpose catch-all. diff --git a/p2p/CHANGELOG.md b/p2p/CHANGELOG.md index 31167fc0b..a107982c2 100644 --- a/p2p/CHANGELOG.md +++ b/p2p/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.0.0.43] - 2023-04-17 + +- Add test to exercise `sortedPeersView#Add()` and `#Remove()` +- Fix raintree add/remove index + ## [0.0.0.42] - 2023-04-17 - Added a test which asserts that transport encryption is required (i.e. unencrypted connections are refused) diff --git a/p2p/raintree/peers_manager_test.go b/p2p/raintree/peers_manager_test.go index 8e2273c6a..cd8fdc43c 100644 --- a/p2p/raintree/peers_manager_test.go +++ b/p2p/raintree/peers_manager_test.go @@ -125,7 +125,7 @@ func TestRainTree_Peerstore_HandleUpdate(t *testing.T) { func BenchmarkPeerstoreUpdates(b *testing.B) { ctrl := gomock.NewController(gomock.TestReporter(b)) - addr, err := cryptoPocket.GenerateAddress() + pubKey, err := cryptoPocket.GeneratePublicKey() require.NoError(b, err) testCases := []ExpectedRainTreeNetworkConfig{ @@ -146,16 +146,26 @@ func BenchmarkPeerstoreUpdates(b *testing.B) { n := testCase.numNodes b.Run(fmt.Sprintf("n=%d", n), func(b *testing.B) { pstore := getPeerstore(nil, n-1) - err := pstore.AddPeer(&typesP2P.NetworkPeer{Address: addr}) + err := pstore.AddPeer(&typesP2P.NetworkPeer{ + Address: pubKey.Address(), + PublicKey: pubKey, + ServiceURL: testLocalServiceURL, + }) require.NoError(b, err) mockBus := mockBus(ctrl) pstoreProviderMock := mockPeerstoreProvider(ctrl, pstore) currentHeightProviderMock := mockCurrentHeightProvider(ctrl, 0) + + libp2pPStore, err := pstoremem.NewPeerstore() + require.NoError(b, err) + hostMock := mocksP2P.NewMockHost(ctrl) + hostMock.EXPECT().Peerstore().Return(libp2pPStore).AnyTimes() + netCfg := RainTreeConfig{ Host: hostMock, - Addr: addr, + Addr: pubKey.Address(), PeerstoreProvider: pstoreProviderMock, CurrentHeightProvider: currentHeightProviderMock, } @@ -169,13 +179,17 @@ func BenchmarkPeerstoreUpdates(b *testing.B) { require.Equal(b, n, network.GetPeerstore().Size()) require.Equal(b, n, len(peersManagerStateView.GetAddrs())) - require.ElementsMatchf(b, pstore.GetPeerList(), peersManagerStateView.GetAddrs(), "peers don't match") + require.ElementsMatchf(b, pstore.GetPeerList(), peersManagerStateView.GetPeers(), "peers don't match") require.Equal(b, testCase.numExpectedLevels, int(actualMaxNumLevels)) for i := 0; i < numAddressesToBeAdded; i++ { - newAddr, err := cryptoPocket.GenerateAddress() + newPubKey, err := cryptoPocket.GeneratePublicKey() require.NoError(b, err) - err = rainTree.AddPeer(&typesP2P.NetworkPeer{Address: newAddr}) + err = rainTree.AddPeer(&typesP2P.NetworkPeer{ + Address: newPubKey.Address(), + PublicKey: newPubKey, + ServiceURL: testLocalServiceURL, + }) require.NoError(b, err) } diff --git a/p2p/types/peers_view.go b/p2p/types/peers_view.go index d595940d6..85f813d55 100644 --- a/p2p/types/peers_view.go +++ b/p2p/types/peers_view.go @@ -61,7 +61,8 @@ func (view *sortedPeersView) GetPeers() PeerList { // Searches from index 1 because index 0 is self by convention and the rest of // the slice is sorted. func (view *sortedPeersView) Add(peer Peer) { - i := sort.SearchStrings(view.sortedAddrs[1:], peer.GetAddress().String()) + i := view.getAddrIndex(peer.GetAddress()) + view.sortedAddrs = insertElementAtIndex(view.sortedAddrs, peer.GetAddress().String(), i) view.sortedPeers = insertElementAtIndex(view.sortedPeers, peer, i) } @@ -70,7 +71,7 @@ func (view *sortedPeersView) Add(peer Peer) { // Searches from index 1 because index 0 is self by convention and the rest of // the slice is sorted. func (view *sortedPeersView) Remove(addr crypto.Address) { - i := sort.SearchStrings(view.sortedAddrs[1:], addr.String()) + i := view.getAddrIndex(addr) if i == len(view.sortedAddrs) { logger.Global.Debug(). Str("pokt_addr", addr.String()). @@ -89,18 +90,27 @@ func (view *sortedPeersView) init(startAddr crypto.Address, pstore Peerstore) *s view.sortedAddrs[i] = peer.GetAddress().String() } + // sort sortedAddrs + view.sortAddrs(startAddr) + // Copying sortedPeers, preserving the sort order of sortedAddrs. for i := 0; i < len(view.sortedAddrs); i++ { view.sortedPeers[i] = pstore.GetPeerFromString(view.sortedAddrs[i]) } - view.sortAddrs(startAddr) return view } -// sortAddrs sorts addresses in `sortedAddrs` lexicographically but then `startAddr` -// is moved to be first in the list. This makes RainTree propagation easier to -// compute and interpret. +// sortAddrs sorts addresses in `sortedAddrs` lexicographically then shifts +// `startAddr` to the first index, moving any preceding values to the end +// of the list; effectively preserving the order by "wrapping around". +// +// self addr: "D", +// initial: "BACEDF", +// sorted: "ABCDEF", +// shifted: "DEFABC", +// +// See the relevant tests for more examples. func (view *sortedPeersView) sortAddrs(startAddr crypto.Address) { sort.Strings(view.sortedAddrs) @@ -113,3 +123,27 @@ func (view *sortedPeersView) sortAddrs(startAddr crypto.Address) { } view.sortedAddrs = append(view.sortedAddrs[i:len(view.sortedAddrs)], view.sortedAddrs[0:i]...) } + +// getAddrIndex returns the sortedAddrs index at which the given address is stored +// or at which to insert it if not present. +func (view *sortedPeersView) getAddrIndex(addr crypto.Address) int { + // sort.Search uses binary search to find the smallest index in `view.sortedAddrs` + // at which the given function returns `true`. + // (i.e.: where does the address order start to "wrap-around"?) + // (see: https://pkg.go.dev/sort#Search) + wrapIdx := sort.Search(len(view.sortedAddrs), func(visitIdx int) bool { + return view.sortedAddrs[visitIdx] < view.sortedAddrs[0] + }) + + frontAddrs := view.sortedAddrs[:wrapIdx] + backAddrs := view.sortedAddrs[wrapIdx:] + i := sort.SearchStrings(frontAddrs, addr.String()) + // index 0 means "before the self addr". In that case we need to consider + // backAddrs because, by definition, all of its values are "smaller" + // than self addr. + if i == 0 { + i = sort.SearchStrings(backAddrs, addr.String()) + i += len(frontAddrs) + } + return i +} diff --git a/p2p/types/peers_view_test.go b/p2p/types/peers_view_test.go index 8dd58ae0c..f49b1ef39 100644 --- a/p2p/types/peers_view_test.go +++ b/p2p/types/peers_view_test.go @@ -1,9 +1,177 @@ package types import ( + "strings" "testing" + + "github.com/stretchr/testify/require" + + cryptoPocket "github.com/pokt-network/pocket/shared/crypto" ) +func TestSortedPeersView_Add_Remove(t *testing.T) { + testCases := []struct { + name string + selfAddr string + addAddrs string + removeAddrs string + expectedAddrs string + }{ + { + name: "lowest self address", + selfAddr: "A", + addAddrs: "BC", + expectedAddrs: "ABC", + }, + { + name: "highest self address", + selfAddr: "C", + addAddrs: "AB", + expectedAddrs: "CAB", + }, + { + name: "penultimate self address", + selfAddr: "W", + addAddrs: "DYZEHGI", + removeAddrs: "YE", + expectedAddrs: "WZDGHI", + }, + { + name: "middle self address", + selfAddr: "S", + addAddrs: "DTUEVGH", + removeAddrs: "E", + expectedAddrs: "STUVDGH", + }, + { + name: "discontiguous resulting addresses", + selfAddr: "O", + addAddrs: "DTURSEVGH", + removeAddrs: "EDU", + expectedAddrs: "ORSTVGH", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + selfAddr := cryptoPocket.Address(testCase.selfAddr) + selfPeer := &NetworkPeer{Address: selfAddr} + + pstore := make(PeerAddrMap) + err := pstore.AddPeer(selfPeer) + require.NoError(t, err) + + view := NewSortedPeersView(selfAddr, pstore) + initialAddrs := []string{selfAddr.String()} + initialPeers := []Peer{selfPeer} + + // assert initial state + require.ElementsMatchf(t, initialAddrs, view.sortedAddrs, "initial addresses don't match") + require.ElementsMatchf(t, initialPeers, view.sortedPeers, "initial peers don't match") + + addrsToAdd := strings.Split(testCase.addAddrs, "") + addrsToRemove := strings.Split(testCase.removeAddrs, "") + expectedAddrs := fromCharAddrs(testCase.expectedAddrs) + + // add peers + for _, addr := range addrsToAdd { + peer := &NetworkPeer{Address: []byte(addr)} + view.Add(peer) + t.Logf("sortedAddrs: %s", toCharAddrs(view.sortedAddrs)) + } + + // remove peers + for _, addr := range addrsToRemove { + view.Remove([]byte(addr)) + t.Logf("sortedAddrs: %s", toCharAddrs(view.sortedAddrs)) + } + + // assert resulting state + var expectedPeers []Peer + for _, addr := range expectedAddrs { + expectedPeers = append(expectedPeers, &NetworkPeer{Address: cryptoPocket.AddressFromString(addr)}) + } + + actualAddrsStr := toCharAddrs(view.sortedAddrs) + require.Equal(t, testCase.expectedAddrs, actualAddrsStr, "resulting addresses don't match") + require.ElementsMatchf(t, expectedPeers, view.sortedPeers, "resulting peers don't match") + }) + } +} + func TestSortedPeersView_Remove(t *testing.T) { t.Skip("TECHDEBT(#554): test that this method works as expected when target peer/addr is not in the list!") } + +func TestSortedPeersView(t *testing.T) { + testCases := []struct { + name string + selfAddr string + initialAddrs string + expectedSortOrder string + }{ + { + name: "lowest self address", + selfAddr: "A", + initialAddrs: "BDCEA", + expectedSortOrder: "ABCDE", + }, + { + name: "highest self address", + selfAddr: "E", + initialAddrs: "BDCEA", + expectedSortOrder: "EABCD", + }, + { + name: "middle self address", + selfAddr: "C", + initialAddrs: "BDCEA", + expectedSortOrder: "CDEAB", + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + selfAddr := cryptoPocket.Address(testCase.selfAddr) + + pstore := make(PeerAddrMap) + var initialPeers []Peer + initialAddrs := fromCharAddrs(testCase.initialAddrs) + for _, addr := range initialAddrs { + peer := &NetworkPeer{Address: cryptoPocket.AddressFromString(addr)} + initialPeers = append(initialPeers, peer) + err := pstore.AddPeer(peer) + require.NoError(t, err) + } + + view := NewSortedPeersView(selfAddr, pstore) + require.ElementsMatchf(t, initialAddrs, view.sortedAddrs, "initial addresses don't match") + require.ElementsMatchf(t, initialPeers, view.sortedPeers, "initial peers don't match") + + var actualPeersStr string + for _, peer := range view.sortedPeers { + actualPeersStr += string(peer.GetAddress().Bytes()) + } + + actualAddrsStr := toCharAddrs(view.sortedAddrs) + require.Equal(t, testCase.expectedSortOrder, actualAddrsStr, "resulting addresses don't match") + require.Equal(t, testCase.expectedSortOrder, actualPeersStr, "resulting addresses don't match") + }) + } +} + +// fromCharAddrs converts each char in charAddrs into a serialized pokt address +func fromCharAddrs(charAddrs string) (addrs []string) { + for _, ch := range strings.Split(charAddrs, "") { + addrs = append(addrs, cryptoPocket.Address(ch).String()) + } + return addrs +} + +// toCharAddrs converts each string in addrStrs to a raw pokt address binary +// string and concatenates them for return +func toCharAddrs(addrStrs []string) (charAddrs string) { + for _, addrStr := range addrStrs { + charAddrs += string(cryptoPocket.AddressFromString(addrStr).Bytes()) + } + return charAddrs +}