Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test: Types - ConsensusData #344

Merged
merged 11 commits into from
Jan 16, 2024
Merged

Test: Types - ConsensusData #344

merged 11 commits into from
Jan 16, 2024

Conversation

MatheusFranco99
Copy link
Contributor

@MatheusFranco99 MatheusFranco99 commented Jan 9, 2024 •
edited
Loading

Implement tests for ConsensusData.

Tests:

  • Attestation consensus data encoding
  • Aggregation consensus data encoding
  • Proposer consensus data encoding
  • Blinded proposer consensus data encoding
  • Sync committee consensus data encoding
  • Sync committee contribution consensus data encoding
  • Invalid duty
  • Valid proposer consensus data with Capella block
  • Valid proposer consensus data with blinded Capella block
  • Proposer consensus data with no justification
  • Invalid proposer consensus data with for Capella block
  • Invalid proposer consensus data with for blinded Capella block
  • Valid attestation consensus data
  • Invalid attestation consensus data with pre-consensus justification
  • Invalid attestation consensus data with wrong data
  • Valid aggregation consensus data
  • Invalid aggregation consensus data with no pre-consensus justification
  • Invalid aggregation consensus data with wrong data
  • Valid sync committee consensus data
  • Invalid sync committee consensus data with pre-consensus justification
  • Invalid sync committee consensus data with wrong data
  • Valid sync committee contribution consensus data
  • Invalid sync committee contribution consensus data with no pre-consensus justification
  • Invalid sync committee contribution consensus data with wrong data

Note: some files were deleted because they had duplicated test specifications.

@MatheusFranco99 MatheusFranco99 self-assigned this Jan 9, 2024
GalRogozinski
GalRogozinski reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I review this after we conclude on value check

@MatheusFranco99 MatheusFranco99 mentioned this pull request Jan 15, 2024
Merged
1 task
GalRogozinski
GalRogozinski reviewed Jan 15, 2024
View reviewed changes
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just answer my questions and I'll approve

types/spectest/tests/consensusdata/capella_blinded_block_validation.go
// CapellaBlindedBlockValidation tests a valid consensus data with capella blinded block
func CapellaBlindedBlockValidation() *SpecTest {
panic("implement")
func CapellaBlindedBlockValidation() *ConsensusDataTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Deneb PR we should maybe add tests if they are not there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we will

types/spectest/tests/consensusdata/contributions_encoding.go

// ContributionsEncoding tests encoding and decoding contributions
func ContributionsEncoding() *SpecTest {
panic("implement")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already inside encoding.go (duplicate)

types/spectest/tests/consensusdata/invalid_sync_committee_block_validation.go
return &ConsensusDataTest{
Name: "invalid sync committee",
ConsensusData: cd,
ExpectedError: "could not unmarshal ssz: expected buffer of length 32 receiced 1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a typo in the error.. it is from our code?

Copy link
Contributor Author

@MatheusFranco99 MatheusFranco99 Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check

types/spectest/tests/consensusdata/invalid_sync_committee_cntribution_validation.go
return &ConsensusDataTest{
Name: "invalid sync committee contribution",
ConsensusData: *cd,
ExpectedError: "could not unmarshal ssz: four",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you know what "four" means here?
it comes from ssz lib

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be like an error code or something, that's pretty funny, from the fastssz lib:

		if offset > endOffset {
			return fmt.Errorf("four")
		}
		if endOffset > size {
			return fmt.Errorf("five")
		}

types/spectest/tests/consensusdata/proposer_justifications.go

// ProposerJustifications tests a valid consensus data with proposer justifications
func ProposerJustifications() *SpecTest {
panic("implement")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate (same as proposer_valid test)

y0sher
y0sher reviewed Jan 15, 2024
View reviewed changes
types/spectest/tests/consensusdata/sync_committee_contrib_no_justifications.go
// SyncCommitteeContributionNoJustifications tests an invalid consensus data with no sync committee contribution pre-consensus justifications
func SyncCommitteeContributionNoJustifications() *ConsensusDataTest {

// To-do: add error when pre-consensus justification check is added.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a test todo or we missing this check in general?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we bring back pre-consensus justification it shouldn't be empty

GalRogozinski
GalRogozinski approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatheusFranco99
I approve but still answer my questions please 🙏

@GalRogozinski GalRogozinski merged commit 0af3f69 into main Jan 16, 2024
2 checks passed
@GalRogozinski GalRogozinski deleted the consensus-data-test branch January 16, 2024 00:17
@GalRogozinski GalRogozinski mentioned this pull request Jan 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GalRogozinski GalRogozinski GalRogozinski approved these changes

@MatusKysel MatusKysel MatusKysel approved these changes

@y0sher y0sher y0sher approved these changes

@moshe-blox moshe-blox Awaiting requested review from moshe-blox

Assignees

@MatheusFranco99 MatheusFranco99

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MatheusFranco99 @y0sher @MatusKysel @GalRogozinski