crypto/kzg4844: pull in the C and Go libs for KZG cryptography

[karalabe]

This PR pulls in the two contending KZG implementations, https://github.com/crate-crypto/go-kzg-4844 written in pure Go for systems where CGO is disabled and optionally https://github.com/ethereum/c-kzg-4844 for systems where CGO is enabled.

The backend implementation can be switched via --crypto.kzg, setting it to either gokzg (default) or ckzg.

% go test -v ./crypto/kzg4844 --bench=. --benchtime=3s
=== RUN   TestCKZGWithPoint
--- PASS: TestCKZGWithPoint (4.06s)
=== RUN   TestGoKZGWithPoint
--- PASS: TestGoKZGWithPoint (2.24s)
=== RUN   TestCKZGWithBlob
--- PASS: TestCKZGWithBlob (0.08s)
=== RUN   TestGoKZGWithBlob
--- PASS: TestGoKZGWithBlob (0.02s)
goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/crypto/kzg4844
BenchmarkCKZGBlobToCommitment
BenchmarkCKZGBlobToCommitment-12     	      94	  35924289 ns/op
BenchmarkGoKZGBlobToCommitment
BenchmarkGoKZGBlobToCommitment-12    	     603	   5990393 ns/op
BenchmarkCKZGComputeProof
BenchmarkCKZGComputeProof-12         	      93	  36533360 ns/op
BenchmarkGoKZGComputeProof
BenchmarkGoKZGComputeProof-12        	     532	   6702309 ns/op
BenchmarkCKZGVerifyProof
BenchmarkCKZGVerifyProof-12          	    3880	    895805 ns/op
BenchmarkGoKZGVerifyProof
BenchmarkGoKZGVerifyProof-12         	    2565	   1269782 ns/op
BenchmarkCKZGComputeBlobProof
BenchmarkCKZGComputeBlobProof-12     	      96	  36983056 ns/op
BenchmarkGoKZGComputeBlobProof
BenchmarkGoKZGComputeBlobProof-12    	     518	   6827653 ns/op
BenchmarkCKZGVerifyBlobProof
BenchmarkCKZGVerifyBlobProof-12      	    2425	   1430330 ns/op
BenchmarkGoKZGVerifyBlobProof
BenchmarkGoKZGVerifyBlobProof-12     	    1759	   1917272 ns/op
PASS
ok  	github.com/ethereum/go-ethereum/crypto/kzg4844	49.121s

% GOMAXPROCS=1 go test -v ./crypto/kzg4844 --bench=. --benchtime=3s
=== RUN   TestCKZGWithPoint
--- PASS: TestCKZGWithPoint (4.07s)
=== RUN   TestGoKZGWithPoint
--- PASS: TestGoKZGWithPoint (2.35s)
=== RUN   TestCKZGWithBlob
--- PASS: TestCKZGWithBlob (0.08s)
=== RUN   TestGoKZGWithBlob
--- PASS: TestGoKZGWithBlob (0.08s)
goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/crypto/kzg4844
BenchmarkCKZGBlobToCommitment
BenchmarkCKZGBlobToCommitment  	      96	  35844277 ns/op
BenchmarkGoKZGBlobToCommitment
BenchmarkGoKZGBlobToCommitment 	      87	  39549616 ns/op
BenchmarkCKZGComputeProof
BenchmarkCKZGComputeProof      	      96	  36484898 ns/op
BenchmarkGoKZGComputeProof
BenchmarkGoKZGComputeProof     	      86	  40379910 ns/op
BenchmarkCKZGVerifyProof
BenchmarkCKZGVerifyProof       	    3894	    895827 ns/op
BenchmarkGoKZGVerifyProof
BenchmarkGoKZGVerifyProof      	    2702	   1291486 ns/op
BenchmarkCKZGComputeBlobProof
BenchmarkCKZGComputeBlobProof  	      96	  36896916 ns/op
BenchmarkGoKZGComputeBlobProof
BenchmarkGoKZGComputeBlobProof 	      86	  40887138 ns/op
BenchmarkCKZGVerifyBlobProof
BenchmarkCKZGVerifyBlobProof   	    2446	   1440444 ns/op
BenchmarkGoKZGVerifyBlobProof
BenchmarkGoKZGVerifyBlobProof  	    1873	   1880667 ns/op
PASS
ok  	github.com/ethereum/go-ethereum/crypto/kzg4844	53.006s

The PR also does not yet add batched proving and verification as the C library does not implement batched proving and I'm unsure what the value is vs. say higher level parallelisation. Should do some benchmarks before committing to a batched API.

The GoKZG library has some weird concurrency baked in that cannot be controlled from the outside. We should investigate this whether that's something we want to live with or make it less smart.

The CKZG library uses ADX instruction sets and will crash on older CPUs with SIGILL. Fixing this requires a custom build flag, which we can't set by default for go build. To try to maximise flexibility and portability, the ci.go build scripts have been extended to both enable CKZG as well as switch it to portable mode. For users manually building via go build/install or using go-ethereum as a dependency, they will need to add --tags=ckzg to the builder to enable the lib and should they want to run in portable mode, manually set the CGO_CFLAGS="-O -D__BLST_PORTABLE__" env var.

[asn-d6]

What is the benefit of using c-kzg-4844 in systems where cgo is enabled? IIUC c-kzg-4844 and go-kzg-4844 have comparable performance. Do you want both libs for diversity or are there other reasons as well?

I'm asking because from a diversity perspective, c-kzg will be used in all other clients, so there might be an argument for geth using the golang lib.

[karalabe]

Number are not that clean:

C

BenchmarkBlobToCommitment
BenchmarkBlobToCommitment-12    	      96	  35899266 ns/op
BenchmarkComputeProof
BenchmarkComputeProof-12        	      94	  36451832 ns/op
BenchmarkVerifyProof
BenchmarkVerifyProof-12         	    3873	    904125 ns/op
BenchmarkComputeBlobProof
BenchmarkComputeBlobProof-12    	      96	  36920463 ns/op
BenchmarkVerifyBlobProof
BenchmarkVerifyBlobProof-12     	    2485	   1412954 ns/op

Go:

BenchmarkBlobToCommitment
BenchmarkBlobToCommitment-12    	     603	   5986624 ns/op
BenchmarkComputeProof
BenchmarkComputeProof-12        	     530	   6771594 ns/op
BenchmarkVerifyProof
BenchmarkVerifyProof-12         	    2496	   1271459 ns/op
BenchmarkComputeBlobProof
BenchmarkComputeBlobProof-12    	     512	   6924226 ns/op
BenchmarkVerifyBlobProof
BenchmarkVerifyBlobProof-12     	    1782	   1861672 ns/op

This is on M2-Max CPU.

Computing the proofs in CGO is for some reason very slow. IMHO there's a bug somewhere. Wrt verifications, there's a 50% speedup of using C vs Go. That seems quite a bit.

As for everyone else using one library and us using something else "for diversity" seems like a bad take. It will just ensure that in case of a consensus fault in the libs the network will fall apart. If there were equal distributions it might make sense, but having everyone else run one lib and Geth run another. Unsure if that's a good idea.

[karalabe]

Seems CKZG will blow up on older CPUs with SIGILL. Ugh, does nobody test their stuff nowadays?

[jtraglia]

Seems CKZG will blow up on older CPUs with SIGILL. Ugh, does nobody test their stuff nowadays?

I'm not certain, but I suspect this is because BLST is trying to use optimized instructions. There's a blurb about this at the bottom of the "build" section in the README.

If the test or target application crashes with an "illegal instruction" exception [after copying to an older system], rebuild with CGO_CFLAGS environment variable set to -O -D__BLST_PORTABLE__. Don't forget -O!

So try this & let me know if you're still having issues:

CGO_CFLAGS="-O -D__BLST_PORTABLE__" go test

[karalabe]

Fixing it compile time is not good. For one, all binary distributions (e.g docker images) will either be crippled or will randomly blown up runtime. Also compile time flags for C needs to be added to the package that contains the C(CGO) code. Passing flags from the outside would require us to introduce a build system for Geth and users wouldn’t be able to just go build. I don’t want to expose C faults up to the user.

…

On Tue, 25 Apr 2023 at 00:42, Justin Traglia ***@***.***> wrote: Seems CKZG will blow up on older CPUs with SIGILL. Ugh, does nobody test their stuff nowadays? I'm not certain, but I suspect this is because BLST is trying to use optimized instructions. There's a blurb about this at the bottom of the "build" section in the README <https://github.com/supranational/blst/blob/master/bindings/go/README.md#build> . If the test or target application crashes with an "illegal instruction" exception [after copying to an older system], rebuild with CGO_CFLAGS environment variable set to -O -D__BLST_PORTABLE__. Don't forget -O! So try this & let me know if you're still having issues: CGO_CFLAGS="-O -D__BLST_PORTABLE__" go test — Reply to this email directly, view it on GitHub <#27155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA7UGMZLB35ZFKOH5MVAJLXC3XVTANCNFSM6AAAAAAXJM3RKY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jtraglia]

Yeah, I understand. I really don't like how BLST handles this either. This is a big reason why we recommended Go clients use Go-KZG-4844. The main purpose of the Go bindings is to facilitate differential fuzzing with Go implementations.

[kevaundray]

As for everyone else using one library and us using something else "for diversity" seems like a bad take. It will just ensure that in case of a consensus fault in the libs the network will fall apart. If there were equal distributions it might make sense, but having everyone else run one lib and Geth run another. Unsure if that's a good idea.

I believe prysm and erigon will also use go-kzg-4844 -- cc @roberto-bayardo

[jtraglia]

binary distributions (e.g docker images) will either be crippled or will randomly blown up runtime.

@karalabe I've made a PR to C-KZG-4844 that forces the Go bindings to always use the portable version of blst. What do you think of this solution? It will probably be slower than Go-KZG-4844 for most CPUs, but it wouldn't "blow up" anymore.

• Force go bindings to use portable blst c-kzg-4844#292

I do not think it's possible to add CPU feature detection in the bindings that changes how blst operates. This would need to be added upstream in blst. If you have a better idea, please let me know.

[karalabe]

My better idea is only to convince upstream to add the dynamic detection.

We've discussed the KZG status a bit internally and the current direction is to link both libs into the binary, defaulting to Go, but exposing the choice via a flag. That way if there's a consensus issue users can switch without requiring us to release a fix.

[jtraglia]

Are you okay with using the portable version of blst? If so, I'll work on getting that PR merged.

[karalabe]

I feel we're going around in circles :) We need to get BLST fixed.

The choices are:

• Fix the problem
• Work around the problem by being slower
• Work around the problem by crashing sometimes

So much effort is wasted on everyone trying to find a workaround. Lets just use that to apply pressure upstream.

[jtraglia]

This will still SIGILL when run on CPUs without ADX and using go-kzg (--crypto.kzg=gokzg) because blst checks this when the package is imported, not when a method is called. I suspect this is not how you thought it worked.

For example, when blst isn't configured to be portable, this will cause a SIGILL on older hardware:

package main

import _ "github.com/ethereum/c-kzg-4844/bindings/go"

func main() {}

[karalabe]

Damn, thanks for the heads up

[karalabe]

Have a new attempt at CKGZ integration:

• The library is disabled/unlinked by default and is rather tied to the --tags=ckzg build tag. This way the library is still in our codebase but does not build by default, so anyone running go build or go install blindly will not get surprises with that pooplib (blst)
• I modified our ci.go builder to add both the ckzg build tag, as well as the CGO_FLAGS="-O -D__BLST_PORTABLE__" CGO env var. This will make sure that anything built with our build scripts (docker, debian, ppt, brew, make, etc) will have CKZG available to allow failsafe switching, but in portable mode so no surprises.
  The only people losing out of CKZG is people who use their own build pipeline via go build/go install and people linking against Geth as a dependency. For these two sets of users CKGZ will only be available if they specify the build tags on their own end and with or without the damn cgo flag

All our pre-built binaries will have the portable CKZG included so we should be fairly safe with what we distribute but also fairly flexible. We'd need to update the README with the potential memo that anyone using their own build system needs to adapt. ¯_(ツ)_/¯

Best I could come up with.

[jtraglia]

Sounds good. Hopefully blst adds runtime detection soon (or allows the community to). By the way, I reached out to someone on the team privately and they have seen yours & Martin's comments. They said:

Hey there! Yes we did discuss that briefly last week and are going to think through some options for how to support the request. We have some really large software releases we're in the middle of now over the next couple weeks but did see the comments.

[karalabe]

Martin said he doens't opposed merging this :P Imma merge it away.