Version Block Support - testing infrastructure #223

olegshmuelov · 2023-05-08T14:11:33Z

This PR adds support for different beacon blocks in tests.
Currently, only Bellatrix is supported, but Capella will be added in a future PR.

To support multiple beacon block versions, we have added versioned functions with a "V" suffix to the existing beacon_node/ssv_msgs/runner functions. This allows us to keep the original functions while also supporting new versions.
We have also added versioned tests for proposer functions to ensure that there are no changes in roots, which use the versioned functions. Once this PR is approved and merged, we will adjust all proposer tests to use the versioned functions in a separate PR and delete the duplicates.
Once all proposer/randao tests have been aligned with the versioned functions, we can ~~remove the "V" suffix and~~ delete the deprecated functions. This will help us maintain a more efficient and streamlined codebase going forward.

…rt-base

y0sher · 2023-05-09T13:50:13Z

ssv/spectest/tests/runner/postconsensus/duplicate_msg_different_roots.go

+				Duty: testingutils.TestingProposerDutyV(spec.DataVersionBellatrix),
+				Messages: []*types.SSVMessage{
+					testingutils.SSVMsgProposer(nil, testingutils.PostConsensusProposerMsgV(ks.Shares[1], 1, spec.DataVersionBellatrix)),
+					testingutils.SSVMsgProposer(nil, testingutils.PostConsensusWrongProposerMsgV(ks.Shares[1], 1, spec.DataVersionBellatrix)),


so we're testing only Bellatrix as part of this PR?

yes, u can find the info in description

y0sher · 2023-05-09T14:16:36Z

types/testingutils/beacon_node_versioned.go

+	"github.com/bloxapp/ssv-spec/types"
+)
+
+var TestingBeaconBlockV = func(version spec.DataVersion) *spec.VersionedBeaconBlock {


I don't understand why you don't just write functions? instead of vars that hold functions..

keeping the current design
up to @GalRogozinski to decide if we want to break it

I think both of ways are fine...
So I don't see a reason to do a needless change

if @y0sher feels strongly about changing it we can discuss

if this is what existed before than yeah this isn't in the scope of this PR to change. generally I don't see why to write this way. in terms of IDE tooling like being able to see the function signature before using it's better to use the classic function structure than this.

GalRogozinski · 2023-05-10T12:46:09Z

types/testingutils/beacon_node_versioned.go

+	"github.com/bloxapp/ssv-spec/types"
+)
+
+var TestingBeaconBlockV = func(version spec.DataVersion) *spec.VersionedBeaconBlock {


I think both of ways are fine...
So I don't see a reason to do a needless change

if @y0sher feels strongly about changing it we can discuss

GalRogozinski · 2023-05-10T12:50:50Z

ssv/spectest/tests/runner/pre_consensus_justifications/invalid_msg_type.go

@@ -15,7 +15,7 @@ func InvalidMsgType() tests.SpecTest {
 		fullData, _ := obj.Encode()
 		root, _ := qbft.HashDataRoot(fullData)
 		msg := &qbft.Message{
-			MsgType:    qbft.PrepareMsgType,  //invalid, qbft.ProposeMsgType expected
+			MsgType:    qbft.PrepareMsgType, //invalid, qbft.ProposeMsgType expected


This happened automatically right?

We need to setup a formatting standard for Go for all Blox's projects. Once everyone will have the same settings we will see less of those.

yes, automatically. agree

GalRogozinski · 2023-05-10T12:58:27Z

types/testingutils/beacon_node_versioned.go

+		CommitteeIndex:          3,
+		CommitteesAtSlot:        36,
+		CommitteeLength:         128,
+		ValidatorCommitteeIndex: 11,


Will those numbers make sense for future forks?

should not affect future forks

If those fields are not used by our spec (and from my checks I believe there aren't) then I am not sure why we bother assigning them values.

I find adding code that looks somehow meaningful confusing.
I would delete this in another PR (because you copy-pasted existing logic, and we don't want to add changes here).

In the meanwhile add the following comment.

// ISSUE 233: We are initializing unused struct fields here

GalRogozinski · 2023-05-10T12:58:47Z

types/testingutils/beacon_node_versioned.go

+		CommitteeIndex:          3,
+		CommitteesAtSlot:        36,
+		CommitteeLength:         128,
+		ValidatorCommitteeIndex: 11,


types/testingutils/beacon_node.go

GalRogozinski · 2023-05-10T13:14:16Z

types/testingutils/beacon_node_versioned.go

+
+	switch version {
+	case spec.DataVersionBellatrix:
+		duty.Slot = TestingDutySlot2


How did you decide on this value?

see new commits

types/testingutils/runner_versioned.go

GalRogozinski · 2023-05-10T13:20:57Z

types/testingutils/ssv_msgs.go

@@ -52,7 +54,7 @@ var TestAggregatorConsensusDataByts, _ = TestAggregatorConsensusData.Encode()

 var TestProposerConsensusData = &types.ConsensusData{
 	Duty:    TestingProposerDuty,
-	Version: spec2.DataVersionBellatrix,
+	Version: spec.DataVersionBellatrix,


This whole file will be deleted and this is just temporary?

this file should not be deleted, there is a logic besides proposals

GalRogozinski · 2023-05-10T13:23:42Z

types/testingutils/ssv_msgs_versioned.go

+	"github.com/bloxapp/ssv-spec/types"
+)
+
+var TestProposerConsensusDataV = func(version spec.DataVersion) *types.ConsensusData {


All the functions in this file are currently being used in tests?
Meaning we can know they are ok since roots didn't change?

All the functions in this file are currently being used in tests?

almost, there is a function PreConsensusRandaoNoMsgV that there is no usage for it in ssv_msgs as well

Meaning we can know they are ok since roots didn't change?

yes

GalRogozinski · 2023-05-10T14:32:01Z

types/testingutils/beacon_node_versioned.go

+	return duty
+}
+
+var TestingProposerDutyNextEpochV = func(version spec.DataVersion) *types.Duty {


GalRogozinski · 2023-05-10T15:21:09Z

ssv/spectest/tests/runner/postconsensus/invalid_msg_slot.go

 				Messages: []*types.SSVMessage{
-					testingutils.SSVMsgProposer(nil, invalidateSlot(testingutils.PostConsensusProposerMsg(ks.Shares[1], 1))),


what happened to the impl of the old invalidate slot?

https://github.com/bloxapp/ssv-spec/blob/24ceff2656ab2219359940f9282cecd4d12c562f/ssv/spectest/tests/runner/postconsensus/invalid_msg_slot.go#L16

GalRogozinski · 2023-05-10T15:22:29Z

ssv/spectest/tests/runner/pre_consensus_justifications/invalid_slot.go

+	msgF := func(obj *types.ConsensusData, id []byte) *qbft.SignedMessage {
+		// change slot
+		if len(obj.PreConsensusJustifications) > 0 {
+			obj.PreConsensusJustifications[0].Message.Slot = testingutils.TestingDutySlot2
+		}
+		return signedMsg(obj, id)
+	}
+
+	msgFV := func(obj *types.ConsensusData, id []byte) *qbft.SignedMessage {
+		// change slot
+		if len(obj.PreConsensusJustifications) > 0 {
+			obj.PreConsensusJustifications[0].Message.Slot = testingutils.TestingInvalidDutySlotV(obj.Version)
+		}
+		return signedMsg(obj, id)
+	}


we still need both?

theoretically not, but if we want to keep the same design for all the versioned examples in current PR, so yes

types/testingutils/beacon_node.go

alonmuroch · 2023-05-11T09:52:38Z

types/testingutils/runner_versioned.go

+	"github.com/bloxapp/ssv-spec/types"
+)
+
+var SSVDecidingMsgsV = func(consensusData *types.ConsensusData, ks *TestKeySet, role types.BeaconRole) []*types.SSVMessage {


why is this here?

You are right that the description said all the V suffixed tests should have the "V" removed out of them, since they become the default only option.
However, we noticed that doing so will create lots of changes that will have to be reviewed, slowing us down farther.

So I will change the description.

ssv/spectest/tests/runner/postconsensus/invalid_msg_slot.go

y0sher

after reading the top msg, LGTM. approved.

GalRogozinski · 2023-05-11T11:27:52Z

So @y0sher and @alonmuroch
What is happening right now is that there are "versioned" functions for proposer duty, and old non-versioned functions for other duties that are not going to be affected by Capella.

In essence, @y0sher is correct. Since this is an infra PR, and in theory future forks can change data for any duty, we should have only versioned code now.

However, let's take the following engineering principles into account:

Keep PRs small as possible: more changes -> more cognitive load -> bigger chances of having something slip by
Don't fix problems that don't exist: We shouldn't waste time preparing our code for changes that won't happen in the near future, and maybe won't come up in the far future.

So I am not sure if those changes should happen in this PR, or that they are even a priority.

olegshmuelov added 29 commits May 1, 2023 11:21

implementation

d73e088

tmp

518c3d5

remove unused

59cc9aa

use latest go-eth2-client version

2ceb26a

use latest go-eth2-client version

82a241b

return block data as versioned & hashroot

3c967f9

improve log

8538093

upgrade go-eth2-client v0.15.8

a65f312

proposerCalls interface change

47ba782

versioned block spec tests

611c19f

Merge remote-tracking branch 'upstream/main' into version-block-suppo…

e7b4a53

…rt-base

generate test json

06f8ccb

include all versioned block spec tests

4701836

remove unused fields

03295fb

use block version instead of hardcoded

cfb3893

Merge branch 'main' into version-block-support-base

48a865b

keep only bellatrix block version

86c2aa1

mention github issue

464b2fa

SSV comparable state ssvlabs#215

fdee3c7

use proper package names

1baf187

Create beacon_node_versioned.go

fc85574

Create ssv_msgs_versioned.go

8679236

versioned state comparison example

a69fc06

TestProposerWithJustificationsConsensusData versioned example

9fe7938

versioned TestingProposerDutyNextEpoch example

f229f06

versioned PostConsensusWrongProposerMsg example

dd2a2ba

versioned PostConsensusWrongSigProposerMsg example

4749be4

versioned PostConsensusProposerTooManyRootsMsg example

3f03a45

versioned PostConsensusSigProposerWrongBeaconSignerMsg example

e0438e6

olegshmuelov self-assigned this May 8, 2023

olegshmuelov added 4 commits May 9, 2023 12:59

versioned PreConsensusRandaoWrongBeaconSigMsg example

753740b

versioned PreConsensusRandaoDifferentSignerMsg example

cb1dc6e

go fmt ./...

052c2a2

use versioned PreConsensusRandaoMsg

cd01e48

GalRogozinski requested a review from y0sher May 9, 2023 12:26

use versioned TestingProposerDuty

a592bc2

y0sher reviewed May 9, 2023

View reviewed changes

GalRogozinski and others added 2 commits May 9, 2023 17:01

delete redundant tests (#1)

eea2685

remove (versioned) name

61a1a28

y0sher reviewed May 9, 2023

View reviewed changes

y0sher approved these changes May 9, 2023

View reviewed changes

olegshmuelov added 4 commits May 9, 2023 17:54

remove unused

21756f5

versioned GetBeaconBlock/GetBlindedBeaconBlock

f810ebf

Merge branch 'main' into version-block-support-testing-infra

f5bf038

fix merge conflict

068766c

GalRogozinski reviewed May 10, 2023

View reviewed changes

olegshmuelov added 4 commits May 10, 2023 17:39

use TestingDutySlotBellatrixNextEpoch

b47a9ad

add versioned TestingInvalidDutySlot

f904ebe

versioned TestingInvalidDutySlot example

876bd62

versioned TestingInvalidDutySlot example2

24ceff2

GalRogozinski reviewed May 10, 2023

View reviewed changes

GalRogozinski requested a review from alonmuroch May 11, 2023 07:15

add an issue comment

2ca1614

alonmuroch requested changes May 11, 2023

View reviewed changes

y0sher approved these changes May 11, 2023

View reviewed changes

alonmuroch approved these changes May 11, 2023

View reviewed changes

liorrutenberg merged commit b8be134 into ssvlabs:main May 11, 2023

olegshmuelov deleted the version-block-support-testing-infra branch May 11, 2023 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version Block Support - testing infrastructure #223

Version Block Support - testing infrastructure #223

olegshmuelov commented May 8, 2023 •

edited by GalRogozinski

Loading

y0sher May 9, 2023

olegshmuelov May 9, 2023

y0sher May 9, 2023

olegshmuelov May 9, 2023

GalRogozinski May 10, 2023

y0sher May 11, 2023

GalRogozinski May 10, 2023

GalRogozinski May 10, 2023

olegshmuelov May 10, 2023

GalRogozinski May 10, 2023

olegshmuelov May 10, 2023

GalRogozinski May 10, 2023

olegshmuelov May 11, 2023

GalRogozinski May 10, 2023

olegshmuelov May 11, 2023

GalRogozinski May 10, 2023

olegshmuelov May 10, 2023

GalRogozinski May 10, 2023

olegshmuelov May 10, 2023

GalRogozinski May 10, 2023

olegshmuelov May 10, 2023 •

edited

Loading

GalRogozinski May 10, 2023

GalRogozinski May 10, 2023

olegshmuelov May 11, 2023

GalRogozinski May 10, 2023

olegshmuelov May 11, 2023

alonmuroch May 11, 2023

GalRogozinski May 11, 2023

y0sher left a comment •

edited

Loading

GalRogozinski commented May 11, 2023

		Messages: []*types.SSVMessage{
		testingutils.SSVMsgProposer(nil, invalidateSlot(testingutils.PostConsensusProposerMsg(ks.Shares[1], 1))),

Version Block Support - testing infrastructure #223

Version Block Support - testing infrastructure #223

Conversation

olegshmuelov commented May 8, 2023 • edited by GalRogozinski 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olegshmuelov May 10, 2023 • 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

y0sher left a comment • edited Loading

Choose a reason for hiding this comment

GalRogozinski commented May 11, 2023

olegshmuelov commented May 8, 2023 •

edited by GalRogozinski

Loading

olegshmuelov May 10, 2023 •

edited

Loading

y0sher left a comment •

edited

Loading