EIP4844: Update cryptography API and Fiat-Shamir logic

[asn-d6]

This PR does a few things with the cryptography of EIP-4844.

Most importantly, it changes the public API of the KZG library to the following high-level API:

- verify_kzg_proof()
- compute_aggregate_kzg_proof()
- verify_aggregate_kzg_proof()
- blob_to_kzg_commitment()

compared to the old much more low-level API:

- compute_powers()
- matrix_lincomb()
- lincomb()
- bytes_to_bls_field()
- evaluate_polynomial_in_evaluation_form()
- verify_kzg_proof()
- compute_kzg_proof()

This means that all the cryptographic logic (including Fiat-Shamir) is now isolated and hidden in the KZG library and the validator.md file ends up being significantly simplified, only calling high-level KZG functions.

Some additional things that this PR does:

• Moves all EIP4844 cryptography into polynomial-commitments.md
• Improves the Fiat-Shamir stack by removing the need for SSZ and by introducing domain separators.

Future TODO tasks:

• Add unittests in test_validator.py that tests the high-level API

[kevaundray]

Closes #3026

[djrtwo]

Good separation of concerns 👍

can you give more on the debate over ssz vs domain separators vs neither?

[kevaundray]

Good separation of concerns 👍

can you give more on the debate over ssz vs domain separators vs neither?

Currently we all agree to remove ssz from the Fiat-Shamir function as this allows for client teams to not implement any of the cryptography.

The debate is on whether these separator labels that we append just before we add a serialised point or field element or polynomial into the bytes array, add any extra security vs having no label at all.

The argument for having them is that you want a precise mapping from your higher level objects to the bytes array. For example, if I was adding a point into the byte array, I could simply serialise the point as 48 bytes then append onto the byte array, or I could first append a label "POINT" and then add the 48 bytes. In this way, the byte array also encodes the types.

Although this seems like a reasonable thing to do, there is no strong rationale as to whether it improves security, and seems to lean on the side of being "extra safe"

[kevaundray]

As of two minutes ago, I think we now have enough information to come to a conclusion on how to proceed with this

[kevaundray]

Except description of BLS Modulus. Looks good to me.

Have left some optional house-keeping comments and notes.

[hwwhww]

Great work on extracting the crypto lib helpers!

Some minor suggestions. Signing my approval stamp so that @asn-d6 can merge it.

[hwwhww]

1. Is there any better name than r2? 😅 You also call the return value r2 in the caller function.
2. It seems a bit confusing that we use r for the result (blob_to_polynomial) and the random number (r2) in this file.

[asn-d6]

Did an attempt at this in cb46b11

[hwwhww]

return type is wrong. how about:

def retrieve_blobs_sidecar(slot: Slot, beacon_block_root: Root) -> Optional[BlobsSidecar]:
    return None  # Testing'''

[asn-d6]

Fixed in af48987

[dankrad]

Hmm, slightly annoying because None could also mean that the sidecar is not available (I know in python we would do this by raising an exception but not all languages would have that available).

[hwwhww]

Does bytes_to_bls_field function make BYTES_PER_FIELD_ELEMENT must be 32?

[asn-d6]

Hmm, not sure exactly what you mean here. BYTES_PER_FIELD_ELEMENT being 32 is essentially a parameter of the BLS field we are using.

[tbenr]

my 2c.
I think the point is that if you specify BYTES_PER_FIELD_ELEMENT as a preset, you should expect different values to be potentially configured. But thebytes_to_bls_field only works if BYTES_PER_FIELD_ELEMENT is strictly 32.
So I see two options:

1. BYTES_PER_FIELD_ELEMENT becomes a constant
2. generalize bytes_to_bls_field

[asn-d6]

my 2c. I think the point is that if you specify BYTES_PER_FIELD_ELEMENT as a preset, you should expect different values to be potentially configured. But thebytes_to_bls_field only works if BYTES_PER_FIELD_ELEMENT is strictly 32. So I see two options:

1. `BYTES_PER_FIELD_ELEMENT` becomes a constant

2. generalize `bytes_to_bls_field`

Ah, that makes sense! Thanks!

Seems like BYTES_PER_FIELD_ELEMENT was added as a preset in cbc170b, but I agree that it's not really a configurable value. Maybe it was done as a quick way to fix the tests if I read the commit correctly.

I mean like in theory we could encode field elements in 64 bytes or whatever, but I don't see why we would ever do that.

I think it's a good idea to leave it as a constant (which it already seems to be in polynomial-commitments.md).

[hwwhww]

[not-strong suggestion] Since Polynomial is defined as "a polynomial in evaluation form", I think you can make this function name shorter, e.g., evaluate_polynomial or evaluate_polynomial_at_point.

[asn-d6]

No strong opinion either. Inertia won and I left it as is.

[kevaundray]

Where is ENDIANNESS defined?

[asn-d6]

Where is ENDIANNESS defined?

https://github.com/ethereum/annotated-spec/blob/master/phase0/beacon-chain.md#constants

[asn-d6]

Merged! Thanks everyone!