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

Spectests #13940

Merged
merged 5 commits into from
May 2, 2024
Merged

Spectests #13940

merged 5 commits into from
May 2, 2024

Conversation

nalepae
Copy link
Contributor

@nalepae nalepae commented Apr 30, 2024 •
edited
Loading

What type of PR is this?
Bug fix

What does this PR do? Why is it needed?
Implement spec tests for get_custody_columns and fix the implementation.

@nalepae nalepae changed the base branch from develop to peerDAS April 30, 2024 16:25
@nalepae nalepae marked this pull request as ready for review May 2, 2024 12:28
@nalepae nalepae requested a review from a team as a code owner May 2, 2024 12:28
@nalepae nalepae requested review from rauljordan, rkapka and nisdas and removed request for a team May 2, 2024 12:28
@nalepae nalepae added Ready For Review A pull request ready for code review peerDAS labels May 2, 2024
nisdas
nisdas reviewed May 2, 2024
View reviewed changes
beacon-chain/core/peerdas/helpers.go Outdated
@@ -23,10 +23,13 @@ const (

// Number of cells in the extended matrix
extendedMatrixSize = fieldparams.MaxBlobsPerBlock * cKzg4844.CellsPerExtBlob

// Maxmimum value of an uint64
maxUint64 = ^uint64(0)
Copy link
Member

Choose a reason for hiding this comment

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

we should use the value from the std lib (math.MaxUint64)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in dc02b04.

beacon-chain/core/peerdas/helpers.go
hashedNextNodeId := hash.Hash(nextNodeId)
subnetId := binary.LittleEndian.Uint64(hashedNextNodeId[:8]) % dataColumnSidecarSubnetCount
// Handle the maximum value of a uint256 case.
if nodeIdUInt256.Cmp(maxUint256) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Why set it to 0 ? Do spec tests expect it

Copy link
Member

Choose a reason for hiding this comment

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

Just realized they changed the implementation,
https://github.com/ethereum/consensus-specs/blob/dev/specs/_features/eip7594/das-core.md#get_custody_columns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I proposed a new change: ethereum/consensus-specs#3742

@nisdas nisdas merged commit 326471d into peerDAS May 2, 2024
16 checks passed
@nisdas nisdas deleted the spectests branch May 2, 2024 15:05
nisdas pushed a commit that referenced this pull request May 16, 2024
@nalepae @nisdas
Spectests (#13940)
f572d1e 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request May 30, 2024
@nalepae
Spectests (#13940)
cffd5fd 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request May 30, 2024
@nalepae
Spectests (#13940)
559d7ea 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Jun 12, 2024
@nalepae
Spectests (#13940)
f0b5d15 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nisdas pushed a commit that referenced this pull request Jul 4, 2024
@nalepae @nisdas
Spectests (#13940)
eac2ce4 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Jul 17, 2024
@nalepae
Spectests (#13940)
d9d7350 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Jul 17, 2024
@nalepae
Spectests (#13940)
c562d07 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nisdas pushed a commit that referenced this pull request Jul 18, 2024
@nalepae @nisdas
Spectests (#13940)
c7def8a 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Jul 18, 2024
@nalepae
Spectests (#13940)
b719bce 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Jul 29, 2024
@nalepae
Spectests (#13940)
a388780 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nisdas pushed a commit that referenced this pull request Aug 14, 2024
@nalepae @nisdas
Spectests (#13940)
b535789 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nisdas pushed a commit that referenced this pull request Aug 15, 2024
@nalepae @nisdas
Spectests (#13940)
3288f15 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nisdas pushed a commit that referenced this pull request Aug 20, 2024
@nalepae @nisdas
Spectests (#13940)
3d2cf28 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Aug 27, 2024
@nalepae
Spectests (#13940)
6464798 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Aug 28, 2024
@nalepae
Spectests (#13940)
03810d6 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
nalepae added a commit that referenced this pull request Aug 28, 2024
@nalepae
Spectests (#13940)
47ef948 
* Update `consensus_spec_version` to `v1.5.0-alpha.1`.

* `CustodyColumns`: Fix and implement spec tests.

* Make deepsource happy.

* `^uint64(0)` => `math.MaxUint64`.

* Fix `TestLoadConfigFile` test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nisdas nisdas nisdas approved these changes

@rauljordan rauljordan Awaiting requested review from rauljordan rauljordan is a code owner automatically assigned from prysmaticlabs/core-team

@rkapka rkapka Awaiting requested review from rkapka rkapka is a code owner automatically assigned from prysmaticlabs/core-team

Assignees
No one assigned
Labels
peerDAS Ready For Review A pull request ready for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nalepae @nisdas