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.

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

Update ckzg4844 to latest version of das branch #14223

Merged
merged 10 commits into from
Jul 17, 2024

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Jul 15, 2024

What type of PR is this?

Other

What does this PR do? Why is it needed?

  • ckzg4844.Cell is now an array of bytes, so the conversion is simpler.
  • Use fieldparams.NumberOfColumns instead of ckzg4844.CellsPerExtBlob.
  • Remove use of ckzg4844.CellsPerExtBlob since it's no longer a public constant.
  • We removed the rowIndices params & pass a commitment for each cell.
  • We renamed "cell ID" to "cell index"; updated some vars to reflect that.
  • Update load trusted setup to support the three types of bytes.
  • Pull in trusted setup from the specs which has g1 monomial points.

Other notes for review

I think it's best to review this split-view. The diff looks pretty ugly.

@jtraglia jtraglia requested a review from a team as a code owner July 15, 2024 19:57
@jtraglia jtraglia requested review from kasey, nalepae and prestonvanloon and removed request for a team July 15, 2024 19:57
@@ -2,14 +2,24 @@ package kzg

import (
"errors"
fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams"
Copy link
Contributor

Choose a reason for hiding this comment

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

This shim file was intentionally made to not rely on other parts of the code -- Can you define NumOfColumns in this file as an alias for CellsPerExtBlob?

The idea is that this file and code by itself can be tested and checked to be correct in isolation to the rest of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the length only exists in that package, I don't think it makes sense to have it at all. I've pushed a commit which changes CellsAndProofs to use slices instead of arrays. Open to changing it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it ideally gets checked by the caller who knows fieldparams.NumOfColumns -- There should probably be a check/test that breaks if ckzg.CellsPerExtBlob != fieldparams.NumOfColumns, since these values come from two different sources but should be the same.

If c-kzg for example, were to return less cells than the client code is expecting due to the params, then this would be caught by that test.

RE changing to slice, imo ideally this PR just updates the dependency and other changes which are not necessary are done in a separate PR.

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 don't like making this change in this PR either. But it was using a constant which no longer exists. ckzg.CellsPerExtBlob isn't available anymore, it's been made private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I didn't realise that CellsPerExtBlob was now private from ckzg -- whether that should be private is out of scope of this PR, so this change makes sense in that context.

@nalepae
Copy link
Contributor

nalepae commented Jul 17, 2024

This test fails.

panic: runtime error: slice bounds out of range [:2] with length 0

goroutine 4283 [running]:
github.com/crate-crypto/go-kzg-4844.trim0xPrefix(...)
	/Users/manu/go/pkg/mod/github.com/crate-crypto/go-kzg-4844@v0.7.0/trusted_setup.go:183
github.com/crate-crypto/go-kzg-4844.parseG2PointNoSubgroupCheck({0x0?, 0x0?})
	/Users/manu/go/pkg/mod/github.com/crate-crypto/go-kzg-4844@v0.7.0/trusted_setup.go:110 +0x218
github.com/crate-crypto/go-kzg-4844.parseG2PointsNoSubgroupCheck.func1(0x49)
	/Users/manu/go/pkg/mod/github.com/crate-crypto/go-kzg-4844@v0.7.0/trusted_setup.go:167 +0x64
created by github.com/crate-crypto/go-kzg-4844.parseG2PointsNoSubgroupCheck in goroutine 23
	/Users/manu/go/pkg/mod/github.com/crate-crypto/go-kzg-4844@v0.7.0/trusted_setup.go:166 +0x68
FAIL	github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain/kzg	0.306s
FAIL

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

Crypto(kzg.go) changes look good to me (Can't explicitly approve)

@nalepae
Copy link
Contributor

nalepae commented Jul 17, 2024

All green, thanks a lot!

@nalepae nalepae merged commit 164d743 into prysmaticlabs:peerDAS Jul 17, 2024
16 checks passed
@jtraglia jtraglia deleted the update-ckzg branch July 17, 2024 13:58
nalepae added a commit that referenced this pull request Jul 17, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Jul 17, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nisdas pushed a commit that referenced this pull request Jul 18, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Jul 18, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Jul 29, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nisdas pushed a commit that referenced this pull request Aug 14, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nisdas pushed a commit that referenced this pull request Aug 15, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nisdas pushed a commit that referenced this pull request Aug 20, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Aug 27, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Aug 28, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Aug 28, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 7, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 7, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 7, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 8, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 23, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 23, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Oct 24, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Nov 20, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Nov 22, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Nov 22, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Nov 25, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Nov 25, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
nalepae added a commit that referenced this pull request Nov 27, 2024
* Update ckzg4844 to latest version

* Run go mod tidy

* Remove unnecessary tests & run goimports

* Remove fieldparams from blockchain/kzg

* Add back blank line

* Avoid large copies

* Run gazelle

* Use trusted setup from the specs & fix issue with struct

* Run goimports

* Fix mistake in makeCellsAndProofs

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants