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

crypto/kzg4844: do lazy init in all ckzg funcs #27679

Merged
merged 3 commits into from
Jul 24, 2023
Merged

crypto/kzg4844: do lazy init in all ckzg funcs #27679

merged 3 commits into from
Jul 24, 2023

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Jul 8, 2023

  • Add ckzgIniter.Do(ckzgInit) call to all c-kzg functions.
  • Add b.ResetTimer() in benchmarks for accuracy.
  • Delete some unnecessary blank lines for consistency.
  • Fix a minor typo in a comment.

@holiman
Copy link
Contributor

holiman commented Jul 11, 2023

--- FAIL: TestCKZGWithPoint (0.01s)
panic: trusted setup isn't loaded [recovered]
	panic: trusted setup isn't loaded
goroutine 18 [running]:
testing.tRunner.func1.2({0x5f3d20, 0x764108})
	/home/appveyor/.cache/geth-go-1.20.3-linux-amd64/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/home/appveyor/.cache/geth-go-1.20.3-linux-amd64/go/src/testing/testing.go:1529 +0x39f
panic({0x5f3d20, 0x764108})
	/home/appveyor/.cache/geth-go-1.20.3-linux-amd64/go/src/runtime/panic.go:884 +0x213
github.com/ethereum/c-kzg-4844/bindings/go.BlobToKZGCommitment({0x15, 0xbc, 0x4c, 0x98, 0x23, 0xa3, 0xc0, 0xcd, 0xf, 0xdc, ...})
	/home/appveyor/.gvm/pkgsets/go1.20.1/global/pkg/mod/github.com/ethereum/[email protected]/bindings/go/main.go:208 +0x145
github.com/ethereum/go-ethereum/crypto/kzg4844.ckzgBlobToCommitment(...)
	/home/appveyor/projects/go-ethereum/crypto/kzg4844/kzg4844_ckzg_cgo.go:65
github.com/ethereum/go-ethereum/crypto/kzg4844.BlobToCommitment({0x15, 0xbc, 0x4c, 0x98, 0x23, 0xa3, 0xc0, 0xcd, 0xf, 0xdc, ...})
	/home/appveyor/projects/go-ethereum/crypto/kzg4844/kzg4844.go:70 +0xa5
github.com/ethereum/go-ethereum/crypto/kzg4844.testKZGWithPoint(0xc000085380, 0x1?)
	/home/appveyor/projects/go-ethereum/crypto/kzg4844/kzg4844_test.go:60 +0xe5
github.com/ethereum/go-ethereum/crypto/kzg4844.TestCKZGWithPoint(0x0?)
	/home/appveyor/projects/go-ethereum/crypto/kzg4844/kzg4844_test.go:48 +0x1e
testing.tRunner(0xc000085380, 0x72a6d8)
	/home/appveyor/.cache/geth-go-1.20.3-linux-amd64/go/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/home/appveyor/.cache/geth-go-1.20.3-linux-amd64/go/src/testing/testing.go:1629 +0x3ea
FAIL	github.com/ethereum/go-ethereum/crypto/kzg4844	0.012s
ok  	github.com/ethereum/go-ethereum/crypto/secp256k1	2.402s

Tests call it directly, without doing the proper loading, so this PR breaks that

@jtraglia
Copy link
Member Author

Ah sorry, I didn't notice that. I've made some changes to the test file that should fix this.

  • Replace test setup with call to UseCKZG.
  • Call b.ResetTimer() after setup, for accurate benchmarks.
  • Remove a couple blank lines for consistency.

@jtraglia
Copy link
Member Author

Also, from looking at the benchmarks I can tell that go-kzg is using concurrency. In Peter's comment about this a while back, it didn't seem like this was the desired behavior. Should I fix this in another PR?

$ go test -tags=ckzg -bench=.                       
goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/crypto/kzg4844
BenchmarkCKZGBlobToCommitment-20              27          43707494 ns/op
BenchmarkGoKZGBlobToCommitment-20            288           4147987 ns/op
BenchmarkCKZGComputeProof-20                  26          44053894 ns/op
BenchmarkGoKZGComputeProof-20                240           5009865 ns/op
BenchmarkCKZGVerifyProof-20                 1144           1044147 ns/op
BenchmarkGoKZGVerifyProof-20                 794           1496341 ns/op
BenchmarkCKZGComputeBlobProof-20              26          43993462 ns/op
BenchmarkGoKZGComputeBlobProof-20            230           5166157 ns/op
BenchmarkCKZGVerifyBlobProof-20              697           1721706 ns/op
BenchmarkGoKZGVerifyBlobProof-20             534           2249320 ns/op

The last argument in go-kzg functions is numGoRoutines. Right now it's currently configured (with 0) to use all available CPU cores. If you want to disable this, change it to 1. For example, this function call:

proof, claim, err := context.ComputeKZGProof((gokzg4844.Blob)(blob), (gokzg4844.Scalar)(point), 0)

@fjl
Copy link
Contributor

fjl commented Jul 18, 2023

We reviewed this in a team call, and it seems not great to call UseCKZG in every test. The original 'issue' this PR was supposed to fix is that there is a call like ckzgIniter.Do(ckzgInit) in the low level function. Thing is, that call is basically free, so I think we could just go ahead and leave the lazy initialization in.

@karalabe
Copy link
Member

This seems a simpler fix that's also consistent with the go version

diff --git a/crypto/kzg4844/kzg4844_ckzg_cgo.go b/crypto/kzg4844/kzg4844_ckzg_cgo.go
index d62ca3fad..540028569 100644
--- a/crypto/kzg4844/kzg4844_ckzg_cgo.go
+++ b/crypto/kzg4844/kzg4844_ckzg_cgo.go
@@ -74,6 +74,8 @@ func ckzgBlobToCommitment(blob Blob) (Commitment, error) {
 // ckzgComputeProof computes the KZG proof at the given point for the polynomial
 // represented by the blob.
 func ckzgComputeProof(blob Blob, point Point) (Proof, Claim, error) {
+       ckzgIniter.Do(ckzgInit)
+
        proof, claim, err := ckzg4844.ComputeKZGProof((ckzg4844.Blob)(blob), (ckzg4844.Bytes32)(point))
        if err != nil {
                return Proof{}, Claim{}, err
@@ -84,6 +86,8 @@ func ckzgComputeProof(blob Blob, point Point) (Proof, Claim, error) {
 // ckzgVerifyProof verifies the KZG proof that the polynomial represented by the blob
 // evaluated at the given point is the claimed value.
 func ckzgVerifyProof(commitment Commitment, point Point, claim Claim, proof Proof) error {
+       ckzgIniter.Do(ckzgInit)
+
        valid, err := ckzg4844.VerifyKZGProof((ckzg4844.Bytes48)(commitment), (ckzg4844.Bytes32)(point), (ckzg4844.Bytes32)(claim), (ckzg4844.Bytes48)(proof))
        if err != nil {
                return err
@@ -99,6 +103,8 @@ func ckzgVerifyProof(commitment Commitment, point Point, claim Claim, proof Proo
 //
 // This method does not verify that the commitment is correct with respect to blob.
 func ckzgComputeBlobProof(blob Blob, commitment Commitment) (Proof, error) {
+       ckzgIniter.Do(ckzgInit)
+
        proof, err := ckzg4844.ComputeBlobKZGProof((ckzg4844.Blob)(blob), (ckzg4844.Bytes48)(commitment))
        if err != nil {
                return Proof{}, err
@@ -108,6 +114,8 @@ func ckzgComputeBlobProof(blob Blob, commitment Commitment) (Proof, error) {
 
 // ckzgVerifyBlobProof verifies that the blob data corresponds to the provided commitment.
 func ckzgVerifyBlobProof(blob Blob, commitment Commitment, proof Proof) error {
+       ckzgIniter.Do(ckzgInit)
+
        valid, err := ckzg4844.VerifyBlobKZGProof((ckzg4844.Blob)(blob), (ckzg4844.Bytes48)(commitment), (ckzg4844.Bytes48)(proof))
        if err != nil {
                return err

@fjl fjl removed the status:triage label Jul 18, 2023
@jtraglia jtraglia changed the title crypto/kzg4844: remove unnecessary init call & fix typo crypto/kzg4844: do lazy init in all ckzg funcs Jul 24, 2023
@jtraglia
Copy link
Member Author

@fjl Okay, no worries. I understand. I've made a new commit that should resolve this.

@karalabe karalabe added this to the 1.12.1 milestone Jul 24, 2023
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@karalabe karalabe merged commit 2274a03 into ethereum:master Jul 24, 2023
@jtraglia jtraglia deleted the kzg-nits branch July 24, 2023 17:37
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
* crypto/kzg4844: remove unnecessary init call & fix typo

* Fix kzg4844 tests/benchmarks

* Make init lazy & revert changes to tests
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Nov 29, 2024
* crypto/kzg4844: remove unnecessary init call & fix typo

* Fix kzg4844 tests/benchmarks

* Make init lazy & revert changes to tests
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 2, 2024
* crypto/kzg4844: remove unnecessary init call & fix typo

* Fix kzg4844 tests/benchmarks

* Make init lazy & revert changes to tests
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.

5 participants