# C-KZG-4844

## Item 1

**Description:** Usage of `m=i+1` in `compute_kzg_proof_impl` is not immediately clear.

**Link:** [code](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L1099)

**Suggestion:**

The non-intuitive part is mainly due to `m=i+1` instead of `m=i`. This is because if `m==0`, then the [this line](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L1115) won't trigger. In other languages we do this with an `Optional` type which is capable of checking if `m` was originally set.

My suggestion is to use `m=-1` as a sentinel value to denote `m` not being set, then our check becomes `if (m != -1)`

**Reason:** The codepath requires you to know about how C handles truthiness of values, ie 0 is false and anything else is true.

**George Comment:** Good point. Let's do this improvement!

**Gotti Comment:** Considered fixed by #208. It still uses `m==0` to indicate that the evaluation point is not among the roots of unity, but it is clearly documented now.

## Item 2

**Description:** `8 * sizeof(blst_scalar)` is used in both g1_mul and g2_mul.

**Link** : [g1_mul](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L351) and [g2_mul]()

**Suggestion:** Create a constant with some documentation explaining why its needed.

**Reason:** If you need to change that value, it would need to be changed in both places, so a constant is generally better and it also allows you to document what it is. For example, it is not clear how that value differs from the `BYTES_PER_FIELD_ELEMENT` constant.

**George Comment:** Agreed. Let's do it. I think we might also be able to make that [255](https://github.com/ethereum/c-kzg-4844/blob/main/src/c_kzg_4844.c#L799) instead of 256 as we are doing in the pippenger function for a tiny speed boost. But figuring this out properly needs some digging.

**Gotti Comment:** With the comment added in #212, it is sufficiently clear, I think. We could replace 8 by the `CHAR_BIT` standard macro, but it's not that we expect anything to work with non-8-bit bytes.

## Item 3

**Description:** `bytes_from_uint64` does not specify the endianness.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L480)

**Suggestion:** Specify an endianness in the comments or function name.

**Reason:** Mixing up endianness is a common source of bugs, so being explicit in every place its used goes a long way to avoiding these.

**George Comment:** Fixed by Gotti in #208.

## Item 4

**Description:** Extra parameter `n` in doc comments.

**Link:** [code](https://github.com/ethereum/c-kzg-4844/blob/549739fcb3aaec6fe5651e1912f05c604b45621b/src/c_kzg_4844.c#L685)

**Suggestion:** Remove parameter `n` as its not a parameter.

**George Comment:** Fixed by Gotti in #208.

## Item 5

**Description:** `bit_reversal_permutation` takes `n` as a `uint64`

**Link:** [code](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L555)

**Suggestion:** It seems that this is because `FFTSettings::MaxWidth` is a `uint64` instead of `uint32`. However note that [this assert](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L557) forces it to be a `uint32` anyways and the 2-adicity of bls12-381 is 32, so having it as a uint32 should be fine.

**Reason:** If we are using a `uint64` and asserting that it is a `uint32`. It makes sense to use a `uint32` from the beginning.

**George Comment:** Agreed let's do it. I wonder if `max_width` is uint64 for a reason and we are gonna get lots of type errors if we attempt to do this change.

**Gotti Comment:** I started doing this and ran into some robustness issue: Since the 2-adicity of bls12-381 is 32, the natural maximum value of `FFTSettings::MaxWidth` is `1<<32`, which just overflows `uint32_t`. This would mean having to artifically restrict `MaxWidth` to `1<<31`, which is kind-of ugly and error-prone. I would suggest instead to extend `log2_pow2` to work with 64-bits and then change the assertion to `<= (1<<32)`. The 64-bit value is then required for the possible valid value `1<<32`

## Item 6

**Description:** `CHALLENGE_INPUT_SIZE` has constants `32` and `48` but it is not clear why.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L675)

**Suggestion:** Specify what the `32` and `48` are related to.

**Note:** I wonder if there is anyway to not need to manually do pointer arithmetic; `offset + 8`.

**Reason:** Just a general nit to document constants; for `48` we could probably use `BYTES_PER_COMMITMENT` and for `32` I'm not completely sure why we need `32 byte` alignment. I have just used the `16` as a constant in the golang code.

**George Comment:** `CHALLENGE_INPUT_SIZE = 32 + BYTES_PER_BLOB + 48;` It's 32 bytes for the domain separator, then a blob, then a commitment. I agree we should document 32 and 48 with constants instead of magic numbers there. I don't know of a better way to do the pointer stuff there (without implementing a higher-level `buffer` abstraction).

**Gotti Comment:** Considered fixed by #212

## Item 7 (George No Action needed)

Description: `max_width+1` is used in the codebase and signifies the fact that some arrays have `4097` roots of unity and not 4096.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L1639)

**Suggestion:** Document why its 4097 elements. If I understand corretly, its so that its easier to reason about the inverse of a particular root of unity when the expanded roots are reversed (since the inverse of 1 is 1)

**Gotti Comment:** Addressed by #222

## Item 8 (Dankrad Confirmed)

Description: The `log2_pow2` method is using a lookup table algorithm and `register` keyword.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/549739fcb3aaec6fe5651e1912f05c604b45621b/src/c_kzg_4844.c#L443)

Suggestion: Two suggestions are noted:

- One could use `log2l` or `log2` from `math.h`
- One could use a *simpler* algorithm:

static int log2_pow2_(uint32_t n) {
    int result = 0;
    while (n >>= 1) ++result;
    return result;
}

**Gotti Comment:** Note that C23 will have a count-trailing-zeros in the standard library.

## Item 9 (George Confirmed)

Level: Informational

Description: doc comment uses `@p x` however there is no `x` parameter.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/9b91f3b8320cfd805b99fc04d96066e613132d12/src/c_kzg_4844.c#L989)

**Suggestion:** Change to `@p z`

**Gotti Comment:** Addressed in #227

## Item 10 (George Confirmed)

Description: the use of the phrase "trusted and validated" in the same sentence is a bit ambiguous as to how they differ.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/549739fcb3aaec6fe5651e1912f05c604b45621b/src/c_kzg_4844.c#L596)

**Suggestion:** We can remove the word `trusted` and specify what validation is happening.

## Item 11 (George Confirmed)

**Description:** Before using the pairings function, it is assumed that the inputs have been subgroup checked.

Link: [code](https://github.com/ethereum/c-kzg-4844/blob/549739fcb3aaec6fe5651e1912f05c604b45621b/src/c_kzg_4844.c#L406)

**Suggestion:** It would be good to specify this in the comments, since I don't think its specified in the blst code either.

**Note:** The go-kzg code does not have this comment either, but the gnark library has a comment on top of the pairings method.