Add EIP7594 #455

gerceboss · 2024-08-11T12:24:14Z

@mratsim ,This is Nilav, I am permissionlessly taking part in EPF-5 this year, mentors on this project are agnish and tersec.

This PR is meant for adding one of the public methods in EIP-7594 namely compute_cells_and_kzg_proofs

The implementation is incomplete yet, needed help in figuring out some things.

How to import functions and types from EIP-4844 file?
Do the tests need to be written only for the public methods?
Please mention the suggestions as well about refactoring or redundant code or any other improvements

I know I am already quite behind in starting the contributions but now, will keep up with pace

mratsim · 2024-08-19T08:58:39Z

Hi,

Thanks for your contribution, I think it would need to be split into multiple parts, 1 with just this and the rest as 8k lines PR are quite scary even if only just test vectors.

Regarding your questions:

How to import functions and types from EIP-4844 file?

2 solutions, either use the export marker * or likely in this case as those are likely internal functions use the special {.all.} import with

import ./ethereum_eip4844_kzg {.all.} to access internal functions

Do the tests need to be written only for the public methods?

We should at the very least use the same tests as the reference implementations in C and Rust, c-kzg-4844 and peerdas-kzg. If internal tests help you can either write them within the file with when isMainModule: at the bottom of the file or use the import ethereum_eip7594_kzg {.all.} and create a test files for internal functions.

I'll proceed with the review and we can merge this first part after.

mratsim · 2024-08-19T08:59:36Z

constantine/ethereum_eip7594_kzg.nim

+  ./platforms/[abstractions, allocs],
+  ./serialization/[codecs_status_codes, codecs_bls12_381, endians],
+  ./commitments_setups/ethereum_kzg_srs
+  # ./ethereum_eip4844_kzg


Suggested change

# ./ethereum_eip4844_kzg

import ./ethereum_eip4844_kzg {.all.}

mratsim · 2024-08-19T08:59:58Z

constantine/ethereum_eip4844_kzg.nim

@@ -64,7 +64,7 @@ const RANDOM_CHALLENGE_KZG_BATCH_DOMAIN = asBytes"RCKZGBATCH___V1_"

 # Derived
 # ------------------------------------------------------------
-const BYTES_PER_BLOB = BYTES_PER_FIELD_ELEMENT*FIELD_ELEMENTS_PER_BLOB
+const BYTES_PER_BLOB* = BYTES_PER_FIELD_ELEMENT*FIELD_ELEMENTS_PER_BLOB


Suggested change

const BYTES_PER_BLOB* = BYTES_PER_FIELD_ELEMENT*FIELD_ELEMENTS_PER_BLOB

const BYTES_PER_BLOB = BYTES_PER_FIELD_ELEMENT*FIELD_ELEMENTS_PER_BLOB

not needed with the {.all.} import marker

mratsim · 2024-08-19T09:00:10Z

constantine/ethereum_eip4844_kzg.nim

@@ -177,7 +177,7 @@ func bytes_to_kzg_proof(dst: var KZGProof, src: array[48, byte]): CttCodecEccSta
    return cttCodecEcc_Success
  return status

-func blob_to_bigint_polynomial(
+func blob_to_bigint_polynomial*(


Suggested change

func blob_to_bigint_polynomial*(

func blob_to_bigint_polynomial(

not needed with the {.all.} import marker

mratsim · 2024-08-19T09:00:23Z

constantine/ethereum_eip7594_kzg.nim

+
+# Derived
+# ------------------------------------------------------------
+const BYTES_PER_CELL= FIELD_ELEMENTS_PER_CELL*BYTES_PER_FIELD_ELEMENT


Suggested change

const BYTES_PER_CELL= FIELD_ELEMENTS_PER_CELL*BYTES_PER_FIELD_ELEMENT

const BYTES_PER_CELL = FIELD_ELEMENTS_PER_CELL*BYTES_PER_FIELD_ELEMENT

mratsim · 2024-08-19T09:00:32Z

constantine/ethereum_eip7594_kzg.nim

+# Derived
+# ------------------------------------------------------------
+const BYTES_PER_CELL= FIELD_ELEMENTS_PER_CELL*BYTES_PER_FIELD_ELEMENT
+const FIELD_ELEMENTS_PER_EXT_BLOB=2*FIELD_ELEMENTS_PER_BLOB


Suggested change

const FIELD_ELEMENTS_PER_EXT_BLOB=2*FIELD_ELEMENTS_PER_BLOB

const FIELD_ELEMENTS_PER_EXT_BLOB = 2*FIELD_ELEMENTS_PER_BLOB

mratsim · 2024-08-19T09:09:56Z

constantine/ethereum_eip7594_kzg.nim

+    # for i in 0 ..< FIELD_ELEMENTS_PER_CELL:
+    #     var bytes = bls_field_to_bytes(cosetEvals[i])
+    #     for j in 0 ..< bytes.len:
+    #         dst[i* bytes.len + j] = bytes[j]


Suggested change

# dst[i* bytes.len + j] = bytes[j]

# dst[i* bytes.len + j] = bytes[j]

2 spaces everywhere, you can configure vs-code with 2 spaces for Nim with the following

ctrl+shift+P: Open users settings (JSON)

and add a language specific config

"[nim]": { "editor.tabSize": 2, },

like this:

mratsim · 2024-08-19T09:11:10Z

constantine/ethereum_eip7594_kzg.nim

+    # Compute a KZG multi-evaluation proof for a set of `k` points.
+
+    # This is done by committing to the following quotient polynomial:
+    #     Q(X) = f(X) - I(X) / Z(X)


It's unclear whether it's (f(X) - I(X)) / Z(X) or f(X) - (I(X) / Z(X))

mratsim · 2024-08-19T09:13:32Z

constantine/ethereum_eip7594_kzg.nim

+    # For all points, compute the evaluation of those points
+
+    for i in z:
+      ys[i]=evaluate_polynomialcoeff(polynomial_coeff, z[i])


As this is not defined, I assume this is pseudocode from the spec so I'll stop reviewing the file here.

It's OK to delete those, only check in the coset_eval_to_cell and the 8k lines of test vectors.

mratsim · 2024-08-19T09:17:15Z

constantine/math/polynomials/polynomials.nim

@@ -143,6 +143,32 @@ type InvDiffArrayKind* = enum
  kArrayMinus
  kMinusArray

+# How do we do modulo? Field.fromBigInt()?


Are you talking about polynomial division / remainder or coefficients?

If it's coefficient, it's handled internally when you use fields.

@mratsim ,Yes, I wanted to ask about that:

How do I perform n % BLS_MODULUS?

How do I do division between 2 polynomials given in form of their polynomial coefficients? I see that there are methods for sum and product.

How do I perform n % BLS_MODULUS?

Field element do not need this, all operations are implicitly reduced modulo BLS_MODULUS.

How do I do division between 2 polynomials given in form of their polynomial coefficients? I see that there are methods for sum and product.

Where is it used in the spec? EIP-4844 introduces polynomial division but for polynomial in Lagrange/evaluation form. I would expect the spec to introduce the polynomial division code if it's used.

Ultimately it's similar to integer division because integers are polynomials but with a twist, they carry.

mratsim · 2024-08-19T09:20:59Z

constantine/math/polynomials/polynomials.nim

+  for i in 0 ..< f.coefs.len:
+    f.coefs[i] -= g.coefs[i]
+
+func prod*[N,F](r: var PolynomialCoef[N, F], s: F, f: PolynomialCoef[N, F])=


This is incorrect for polynomials in coefficient form.

(a + bx + cx²) * (u + vx + wx²) is not au + bvx + cwx²

@mratsim, Agnish mentioned once that these are redundant as already there are methods for PolynomialEvals. Can you confirm? Because, from what I see multiplying here is of 2 polynomial coefficients and in the PolynomialEvals one , there is scalar getting multiplied to whole evaluated form.

EIP-4844 focused heavily on polynomial in evaluation form because they are asymptotically more efficient than polynomial in coefficient form.

Multiplication in evaluation form is O(n) with n the number of coefficient/evaluation.

Naive multiplication in coefficient form is O(n²).

Efficient multiplication in coefficient form would be O(n log n) with n log n FFT + O(n) eval multiplication + n log n inverse FFT

so I'm surprised if this is actually necessary in the spec?

gerceboss · 2024-08-25T10:45:05Z

Also, I don't know how to divide this PR in two parts as you mentioned, can you please guide on that?

And at some places tuples are used in consensus specs , and in nim tuples are immutable, so how do I approach writing such functions? @mratsim

mratsim · 2024-08-26T14:42:08Z

Also, I don't know how to divide this PR in two parts as you mentioned, can you please guide on that?

You can use a technique called "staggered PRs".

You create a branch for example eip7594-1, you put your first commits, here it would be just the test vectors.
Then you create a branch eip7594-2 from this eip7594-1 branch, adds your commit for your first feature(s).
And create a eip7594-3 from this eip7594-2 branch, etc

When opening a PR just make sure to target the previous branch

This allows you to make progress without waiting for me merging the code.
It allows reviewing features independently and fix the branch as well without mixing up commits, you can just rebase follow-up branch afterwards.
And more importantly it keeps PR size down so that they are easier to review and focus on.

And at some places tuples are used in consensus specs , and in nim tuples are immutable, so how do I approach writing such functions? @mratsim

Tuples are not immutable.

var myTuple = (1, 2)
myTuple[0] = 3
echo myTuple

var (a, b) = (1, 2)
a = 4
echo a

proc foo(tup: var (int, int)) =
  tup[0] += 1

proc bar(tup: var Tuple[x, y: int]) =
  tup.x += 1

are examples of mutable tuples.

This specific signature

def compute_kzg_proof_multi_impl(
        polynomial_coeff: PolynomialCoeff,
        zs: Coset) -> Tuple[KZGProof, CosetEvals]:

Should be transformed into:

proc compute_kzg_proof_multi_impl(
  cosetEvals: var XXXCosetEvalsXXX, # insert proper type here, it looks like openArray[Fr[BLS12_381]]
  proof: var KzgProof,
  poly: PolynomialCoeff,
  zs: Coset): ErrorType =

to follow the return value guideline here: https://github.com/mratsim/constantine/blob/master/ARCHITECTURE.md#return-values

gerceboss marked this pull request as draft August 13, 2024 13:15

mratsim requested changes Aug 19, 2024

View reviewed changes

gerceboss added 6 commits August 25, 2024 14:22

initiated eip7594 added polynomial helpers

96a17de

added basic operations for PolynomialCoef

94977c0

added consensus-test vectors

7af4e77

added implementations for a public method

ccbb911

removed unnecessary changes

7429b6f

Updated format

a2ecd61

gerceboss force-pushed the kzg_eip7594 branch from 3bfc8e2 to a2ecd61 Compare August 25, 2024 08:54

gerceboss requested a review from mratsim August 25, 2024 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EIP7594 #455

Add EIP7594 #455

gerceboss commented Aug 11, 2024 •

edited

Loading

mratsim commented Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

mratsim Aug 19, 2024

gerceboss Aug 25, 2024

mratsim Aug 26, 2024

mratsim Aug 19, 2024

gerceboss Aug 25, 2024 •

edited

Loading

mratsim Aug 26, 2024

gerceboss commented Aug 25, 2024 •

edited

Loading

mratsim commented Aug 26, 2024

	# ./ethereum_eip4844_kzg

	import ./ethereum_eip4844_kzg {.all.}

	const BYTES_PER_BLOB* = BYTES_PER_FIELD_ELEMENT*FIELD_ELEMENTS_PER_BLOB
	const BYTES_PER_BLOB = BYTES_PER_FIELD_ELEMENT*FIELD_ELEMENTS_PER_BLOB

	func blob_to_bigint_polynomial*(
	func blob_to_bigint_polynomial(

	const BYTES_PER_CELL= FIELD_ELEMENTS_PER_CELL*BYTES_PER_FIELD_ELEMENT
	const BYTES_PER_CELL = FIELD_ELEMENTS_PER_CELL*BYTES_PER_FIELD_ELEMENT

	const FIELD_ELEMENTS_PER_EXT_BLOB=2*FIELD_ELEMENTS_PER_BLOB
	const FIELD_ELEMENTS_PER_EXT_BLOB = 2*FIELD_ELEMENTS_PER_BLOB

	# dst[i* bytes.len + j] = bytes[j]
	# dst[i* bytes.len + j] = bytes[j]

Add EIP7594 #455

Are you sure you want to change the base?

Add EIP7594 #455

Conversation

gerceboss commented Aug 11, 2024 • edited Loading

mratsim commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerceboss Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerceboss commented Aug 25, 2024 • edited Loading

mratsim commented Aug 26, 2024

gerceboss commented Aug 11, 2024 •

edited

Loading

gerceboss Aug 25, 2024 •

edited

Loading

gerceboss commented Aug 25, 2024 •

edited

Loading