Alan/no fork fix unit tests #1442

pavelkrolevets · 2024-06-27T15:41:25Z

fix message/validation/validation_test.go
partial fix network/p2p/p2p_test.go

moshe-blox · 2024-06-30T13:54:21Z

network/p2p/p2p_sync.go

@@ -82,11 +82,11 @@ func (n *p2pNetwork) handleStream(logger *zap.Logger, handler p2pprotocol.Reques
 }

 // getSubsetOfPeers returns a subset of the peers from that topic
-func (n *p2pNetwork) getSubsetOfPeers(logger *zap.Logger, senderID []byte, maxPeers int, filter func(peer.ID) bool) (peers []peer.ID, err error) {
+func (n *p2pNetwork) getSubsetOfPeers(logger *zap.Logger, vpk spectypes.ValidatorPK, maxPeers int, filter func(peer.ID) bool) (peers []peer.ID, err error) {


Why revert back to validator pk, if it could be either validator or committee? (so "sender")

https://github.com/ssvlabs/ssv/blob/alan/no-fork-fix-tests/network/p2p/p2p_sync.go#L191

Ill change here to senderID []byte.

MatusKysel · 2024-07-04T11:55:31Z

message/validation/validation_test.go

+		for i := 0; i < 1000; i++ {
 			msg.Messages = append(msg.Messages, msg.Messages[0])
 		}

 		_, err := msg.Encode()
-		require.ErrorContains(t, err, "max expected 13 and 14 found")
+		require.ErrorContains(t, err, "max expected 1000 and 1001 found")


why do we need to increase this?

we have max at ssz

type PartialSignatureMessages struct {
Type PartialSigMsgType
Slot phase0.Slot
Messages []*PartialSignatureMessage ssz-max:"1000"
}

MatusKysel · 2024-07-04T12:18:16Z

network/p2p/p2p_pubsub.go

-	if msg.SSVMessage.MsgID.GetRoleType() == spectypes.RoleCommittee {
-		committeeID = spectypes.CommitteeID(msg.SSVMessage.MsgID.GetDutyExecutorID()[16:])
-	} else {
-		share := n.nodeStorage.ValidatorStore().Validator(msg.SSVMessage.MsgID.GetDutyExecutorID())
-		if share == nil {
-			return fmt.Errorf("could not find validator: %x", msg.SSVMessage.MsgID.GetDutyExecutorID())
-		}
-		committeeID = share.CommitteeID()


are you sure about this change?

This brakes tests. Not sure here. @y0sher @moshe-blox what do you think?

MatusKysel · 2024-07-04T12:18:39Z

network/p2p/p2p_pubsub.go

@@ -208,6 +219,7 @@ func (n *p2pNetwork) subscribe(logger *zap.Logger, pk spectypes.ValidatorPK) err
 	cmtid := n.nodeStorage.ValidatorStore().Validator(pk[:]).CommitteeID()
 	topics := commons.CommitteeTopicID(cmtid)
 	for _, topic := range topics {
+		n.interfaceLogger.Debug("subscribing to topic", fields.Topic(topic))


please remove

MatusKysel · 2024-07-04T12:20:24Z

network/p2p/p2p_pubsub.go

+
+		signedSSVMessage := &spectypes.SignedSSVMessage{}
+		if err := signedSSVMessage.Decode(msg.GetData()); err != nil {
+			return fmt.Errorf("message was not decoded: %w", err)


add signed so we know which error we triggered

MatusKysel · 2024-07-04T12:20:58Z

network/p2p/p2p_setup.go

@@ -150,6 +150,7 @@ func (n *p2pNetwork) SetupServices(logger *zap.Logger) error {
 	if err := n.setupDiscovery(logger); err != nil {
 		return errors.Wrap(err, "could not setup discovery service")
 	}
+	fmt.Printf("Setting up pub sub...")


also please remove

MatusKysel · 2024-07-04T12:23:16Z

network/p2p/p2p_test.go

-		Nodes:        n,
-		MinConnected: n/2 - 1,
+	ks := spectestingutils.Testing4SharesSet()
+	pks := []string{"8e80066551a81b318258709edaf7dd1f63cd686a0e4db8b29bbb7acfe65608677af5a527d9448ee47835485e02b50bc0"}


is it ok that we use only one private key?

We have all logic working with one PK, but if we want to add another PK, we need to use validator storage create shares etc. When we use spectestingutils.Testing4SharesSet() we have shares for only one PK.
@moshe-blox what do you think?

MatusKysel · 2024-07-04T12:24:25Z

network/p2p/p2p_test.go

+	// Signature: []byte("sVV0fsvqQlqliKv/ussGIatxpe8LDWhc9uoaM5WpjbiYvvxUr1eCpz0ja7UT1PGNDdmoGi6xbMC1g/ozhAt4uCdpy0Xdfqbv"),
+	// Signers:   []spectypes.OperatorID{1, 3, 4},


we do not need this?

I guess remove it

yeah its from old code

MatusKysel · 2024-07-04T12:25:24Z

network/p2p/p2p_test.go

 	signedSSVMsg, err := spectypes.SSVMessageToSignedSSVMessage(ssvMsg, 1, dummySignSSVMessage)
 	require.NoError(t, err)

 	return id, signedSSVMsg
 }

 func dummyMsgAttester(t *testing.T, pkHex string, height int) (spectypes.MessageID, *spectypes.SignedSSVMessage) {
-	return dummyMsg(t, pkHex, height, spectypes.BNRoleAttester)
+	return dummyMsg(t, pkHex, height, spectypes.RunnerRole(spectypes.BNRoleAttester))


not sure if this is needed you can put there committee right away

MatusKysel · 2024-07-04T12:25:38Z

network/p2p/p2p_validation_test.go

-		validatorCount = 20
+		validatorCount = 1


is this ok?

Same here, we use spectestingutils.Testing4SharesSet() there only one PK. We use validator storage now, so to have 20 we need to create shares and store. I can try to mock it, if 20 is critical, but looks like with one we test all the logic.

@moshe-blox @y0sher

MatusKysel · 2024-07-04T12:26:10Z

network/p2p/p2p_validation_test.go

 		var validator [48]byte
 		cryptorand.Read(validator[:])
-		validators[i] = hex.EncodeToString(validator[:])
+		validators[i] = "8e80066551a81b318258709edaf7dd1f63cd686a0e4db8b29bbb7acfe65608677af5a527d9448ee47835485e02b50bc0"


what about this? are we creating validator with same key?

yeah Ill rewrite it. Ill leave some logic commented.

MatusKysel · 2024-07-04T12:27:23Z

network/p2p/p2p_validation_test.go

+// func (v *MockMessageValidator) ValidateSSVMessage(ssvMessage *queue.DecodedSSVMessage) (*queue.DecodedSSVMessage, validation.Descriptor, error) {
+// 	panic("not implemented") // TODO: Implement
+// }
+


I guess you can delete this

MatusKysel · 2024-07-04T12:28:01Z

network/p2p/test_utils.go

 	operatorPubkey, err := keys.OperatorKey.Public().Base64()
 	if err != nil {
 		return nil, err
 	}

-	hash, err := keys.OperatorKey.StorageHash()
+	// hash, err := keys.OperatorKey.StorageHash()
 	if err != nil {


this is not needed anymore

MatusKysel · 2024-07-04T12:28:23Z

network/p2p/test_utils.go

+	// cfg.NodeStorage = mock.NodeStorage{
+	// 	MockPrivateKeyHash:              hash,
+	// 	RegisteredOperatorPublicKeyPEMs: []string{},
+	// }


MatusKysel · 2024-07-04T12:29:07Z

network/peers/connections/handshaker.go

+	// if !strings.Contains(ni.Metadata.NodeVersion, "ALANTEST") {
+	// 	return errors.New("non Alan node version is not supported")
+	// }


hmm did you need to change it due to tests? because we need this for stage

yeah, this brakes tests. Probably we need another approach

MatusKysel · 2024-07-04T12:29:48Z

network/topics/msg_validator_test.go

-	// TODO: enable once topic validation is in place
-	//t.Run("wrong topic", func(t *testing.T) {
-	//	pkHex := "b5de683dbcb3febe8320cc741948b9282d59b75a6970ed55d6f389da59f26325331b7ea0e71a2552373d0debb6048b8a"
-	//	msg, err := dummySSVConsensusMsg(share.ValidatorPubKey, 15160)
-	//	require.NoError(t, err)
-	//	raw, err := msg.Encode()
-	//	require.NoError(t, err)
-	//	pk, err := hex.DecodeString("a297599ccf617c3b6118bbd248494d7072bb8c6c1cc342ea442a289415987d306bad34415f89469221450a2501a832ec")
-	//	require.NoError(t, err)
-	//	topics := commons.ValidatorTopicID(pk)
-	//	pmsg := newPBMsg(raw, topics[0], []byte("16Uiu2HAkyWQyCb6reWXGQeBUt9EXArk6h3aq3PsFMwLNq3pPGH1r"))
-	//	res := mv.ValidateP2PMessage(context.Background(), "16Uiu2HAkyWQyCb6reWXGQeBUt9EXArk6h3aq3PsFMwLNq3pPGH1r", pmsg)
-	//	require.Equal(t, res, pubsub.ValidationReject)
-	//})


MatusKysel · 2024-07-04T12:29:54Z

network/topics/msg_validator_test.go

-	// TODO: enable once topic validation is in place
-	//t.Run("invalid validator public key", func(t *testing.T) {
-	//	msg, err := dummySSVConsensusMsg("10101011", 1)
-	//	require.NoError(t, err)
-	//	raw, err := msg.Encode()
-	//	require.NoError(t, err)
-	//	pmsg := newPBMsg(raw, "xxx", []byte{})
-	//	res := mv.ValidateP2PMessage(context.Background(), "xxxx", pmsg)
-	//	require.Equal(t, res, pubsub.ValidationReject)
-	//})


pavelkrolevets added 2 commits June 27, 2024 17:32

fix message/validation/validation_test.go

d1097a0

partial fix network/p2p/p2p_test.go

394e2b6

pavelkrolevets requested a review from MatusKysel June 27, 2024 15:41

MatusKysel approved these changes Jun 28, 2024

View reviewed changes

MatusKysel requested a review from y0sher June 28, 2024 18:39

moshe-blox reviewed Jun 30, 2024

View reviewed changes

pavelkrolevets and others added 8 commits June 30, 2024 17:08

update network/topics tests

d5456bf

fix protocol/v2/qbft/spectest/qbft_mapping_test.go

050c5e8

use sender ID instead of validator pub

dd18b99

fix network/topics/msg_validator_test.go

c20b3e2

todos at network/topics/msg_validator_test.go

dfb80f8

permit network/peers/connections/handshaker_test.go

abcf2cf

fix p2p tests

c5cd59e

fix the topic bug

00020ce

MatusKysel reviewed Jul 4, 2024

View reviewed changes

pavelkrolevets added 2 commits July 4, 2024 15:47

remove log

c2462c9

minor fixes

04d0a0c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alan/no fork fix unit tests #1442

Alan/no fork fix unit tests #1442

pavelkrolevets commented Jun 27, 2024

moshe-blox Jun 30, 2024

pavelkrolevets Jul 1, 2024 •

edited

Loading

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024 •

edited

Loading

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024 •

edited

Loading

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024

pavelkrolevets Jul 4, 2024

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

pavelkrolevets Jul 4, 2024

MatusKysel Jul 4, 2024

MatusKysel Jul 4, 2024

		// Signature: []byte("sVV0fsvqQlqliKv/ussGIatxpe8LDWhc9uoaM5WpjbiYvvxUr1eCpz0ja7UT1PGNDdmoGi6xbMC1g/ozhAt4uCdpy0Xdfqbv"),
		// Signers: []spectypes.OperatorID{1, 3, 4},

Alan/no fork fix unit tests #1442

Are you sure you want to change the base?

Alan/no fork fix unit tests #1442

Conversation

pavelkrolevets commented Jun 27, 2024

Choose a reason for hiding this comment

pavelkrolevets Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkrolevets Jul 4, 2024 • 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

Choose a reason for hiding this comment

pavelkrolevets Jul 4, 2024 • 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

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

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

pavelkrolevets Jul 1, 2024 •

edited

Loading

pavelkrolevets Jul 4, 2024 •

edited

Loading

pavelkrolevets Jul 4, 2024 •

edited

Loading