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

chore: Use monomial form when computing the quotient #98

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kevaundray
Copy link
Contributor

No description provided.

@@ -62,8 +63,11 @@ func (c *Context) ComputeBlobKZGProof(blob *Blob, blobCommitment KZGCommitment,
// 2. Compute Fiat-Shamir challenge
evaluationChallenge := computeChallenge(blob, blobCommitment)

domain.BitReverse(polynomial)
polyCoeff := c.domain.IfftFr(polynomial)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This additional IFFT costs 1ms which is not that much compared to the MSM

// 2. Create opening proof
openingProof, err := kzg.Open(c.domain, polynomial, inputPoint, c.commitKeyLagrange, numGoRoutines)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be able to get rid of commitKeyLagrange entirely, if we can stomach the 1ms ifft

Comment on lines +10 to +19

outputPoint := poly.PolyEval(polyCoeff, evaluationPoint)

quotient := poly.DividePolyByXminusA(polyCoeff, evaluationPoint)

comm, err := ck.Commit(quotient, 0)
if err != nil {
return OpeningProof{}, nil
}

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 have not deleted the code, we no longer need but this is comparatively simpler than what we were doing before

@kevaundray kevaundray changed the title chore: Use monomial form chore: Use monomial form when computing the quotient Aug 20, 2024
@kevaundray
Copy link
Contributor Author

Need to take benchmarks to compute what the difference is before moving forward

@kevaundray
Copy link
Contributor Author

Currently on master I get:

Running tool: /usr/local/bin/go test -benchmem -run=^$ -bench ^Benchmark$ github.com/crate-crypto/go-eth-kzg

goos: darwin
goarch: arm64
pkg: github.com/crate-crypto/go-eth-kzg
Benchmark/BlobToKZGCommitment-10         	     129	   9069520 ns/op	  405012 B/op	     153 allocs/op
Benchmark/ComputeKZGProof-10             	     126	   9790412 ns/op	  930463 B/op	     164 allocs/op
Benchmark/ComputeBlobKZGProof-10         	     100	  10086890 ns/op	  930860 B/op	     171 allocs/op
ok  	github.com/crate-crypto/go-eth-kzg	33.101s

On this branch, I get:

Running tool: /usr/local/bin/go test -benchmem -run=^$ -bench ^Benchmark$ github.com/crate-crypto/go-eth-kzg

goos: darwin
goarch: arm64
pkg: github.com/crate-crypto/go-eth-kzg
Benchmark/BlobToKZGCommitment-10         	     136	   9300691 ns/op	  405015 B/op	     153 allocs/op
Benchmark/ComputeKZGProof-10             	     100	  10629476 ns/op	 3681812 B/op	   12439 allocs/op
Benchmark/ComputeBlobKZGProof-10         	     100	  11383314 ns/op	 3682208 B/op	   12446 allocs/op
PASS
ok  	github.com/crate-crypto/go-eth-kzg	31.926s

So its about 10% slower though the allocations have grown

@kevaundray
Copy link
Contributor Author

I created another branch that merges the iterative IFFT code into these changes, see #99 -- we get:

goos: darwin
goarch: arm64
pkg: github.com/crate-crypto/go-eth-kzg
Benchmark/BlobToKZGCommitment-10         	     140	   9368765 ns/op	  405019 B/op	     153 allocs/op
Benchmark/ComputeKZGProof-10             	     120	  11115686 ns/op	  667633 B/op	     179 allocs/op
Benchmark/ComputeBlobKZGProof-10         	     100	  10302858 ns/op	  668033 B/op	     186 allocs/op

@kevaundray
Copy link
Contributor Author

So that branch has essentially the same performance, with the newer branch having a slighter better memory profile, but nothing significant

@kevaundray
Copy link
Contributor Author

Note to self: local branch is named kw/simple-kzg-code

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.

1 participant