EIP4844: Suggestion to remove ssz from the Fiat-Shamir function

[kevaundray]

The method to create Fiat-Shamir challenges looks like:

def hash_to_bls_field(x: Container) -> BLSFieldElement:
    """
    Compute 32-byte hash of serialized container and convert it to BLS field.
    The output is not uniform over the BLS field.
    """
    return bytes_to_bls_field(hash(ssz_serialize(x)))

For security, the Fiat-Shamir protocol requires a bijection from an object to bytes. This is the case if the serialise methods on an ssz struct is setup correctly since ssz was intended to be bijective.

This however indirectly ties the security of the commitment scheme to the ssz serialisation strategy, ie its not possible to test the Fiat-Shamir component entirely in the cryptography library without first assuming that the serialisation strategy that caller is using is bijective (The cryptography would only receive bytes). This seems to be an abstraction leak.

To keep the cryptography completely modularised, one could implement this function as:

def hash_to_bls_field(polys: List[Tuple[Polynomial | Blob]], comms: List[KZGCommitment]) -> BLSFieldElement:
   """
   Compute 32-byte hash of serialised polynomials and commitments concatenated
   This hash is then converted to a BLS field.
   The output is not uniform over the BLS field.
   """
   
   bytes = []
   
   # Append each polynomial
   for poly in polys:
       for serialised_evaluation in poly:
           bytes.extend(serialised_evaluation)
   
   # Append serialised g1 points
   for serialised_comm in comms:
       bytes.extend(serialised_comm)
       
   return bytes_to_bls_field(hash(bytes))

The cryptography already knows how to de/serialise field and group elements, so the hash_to_bls_field can live in the cryptography code and tested in isolation.

Changes implied

Instead of:

hash_to_bls_field(BlobsAndCommitments(blobs=blobs, kzg_commitments=kzg_commitments))
hash_to_bls_field(PolynomialAndCommitment(polynomial=aggregated_poly,kzg_commitment=aggregated_poly_commitment)

It would be called as:

hash_to_bls_field(blobs, kzg_commitments)
hash_to_bls_field([aggregated_poly],[aggregated_poly_commitment])

@hwwhww and @asn-d6 The ssz addition seems to have been introduced by you folks, If this change seems reasonable, I can push a PR. Eager to hear your thoughts.

[asn-d6]

Hello,

You are effectively saying that you don't trust that ssz_serialize() is a bijective function. And that instead we should handle the Fiat-Shamir in the crypto library where we have more control.

I think that's a reasonable defense in-depth argument given what's at stake. I also think your pseudocode for hash_to_bls_field() looks reasonable, but it will probably require modifications to run.

I guess the counter argument for this is that the crypto library does not know that Fiat-Shamir is used above it. It only exposes a bytes_to_bls_field() func, and it's the responsibility of the caller to use that correctly for her agenda. In this case the caller's choice of using ssz_serialize() might be a reasonable decision. Why does the crypto library need to test the Fiat-Shamir functionality, since it doesn't expose such a function?

All in all, I think a patch for this would be a reasonable improvement.

[dankrad]

Why does it need to be bijective? An injective function would be enough, right?

I think it is fairly easy to prove that SSZ is injective

[kevaundray]

Why does it need to be bijective? An injective function would be enough, right?

I think it is fairly easy to prove that SSZ is injective

Right, but this dependency on ssz is not necessary and doesn't seem to provide any advantages, since the cryptography library is able to handle the Fiat-Shamir protocol internally.

Should something ever change with the way clients implement the necessary methods on a user defined struct for ssz, the cryptography will not need to be re-analysed, for example.

I don't know how likely that example is, but if we can avoid the dependency, I see no reason not to?

[kevaundray]

Hello,

You are effectively saying that you don't trust that ssz_serialize() is a bijective function. And that instead we should handle the Fiat-Shamir in the crypto library where we have more control.

I think that's a reasonable defense in-depth argument given what's at stake. I also think your pseudocode for hash_to_bls_field() looks reasonable, but it will probably require modifications to run.

I guess the counter argument for this is that the crypto library does not know that Fiat-Shamir is used above it. It only exposes a bytes_to_bls_field() func, and it's the responsibility of the caller to use that correctly for her agenda. In this case the caller's choice of using ssz_serialize() might be a reasonable decision. Why does the crypto library need to test the Fiat-Shamir functionality, since it doesn't expose such a function?

All in all, I think a patch for this would be a reasonable improvement.

Good point, I find testing and modularising the Fiat-Shamir protocol hardens the code and makes it easier to reason about security.

An example of what I was referring to can be seen here:

https://github.com/crate-crypto/proto-danksharding-crypto/blob/master/crypto/src/kzg/transcript.rs#L46

So the Fiat-Shamir protocol is encapsulated in this Transcript struct which allows you to add polynomials and points.

I tend to do it this way because the Fiat-Shamir protocol usually has its own set of recommended practices such as domain separators, so this isolates it even further.

Not relevant to this issue, but domain separator usage can be seen here for example: https://github.com/zcash/halo2/blob/main/halo2_proofs/src/transcript.rs#L140

If the Fiat-Shamir protocol ever needs changing, the code for kzg can stay exactly the same and we just modify the transcript file.

[dankrad]

Should something ever change with the way clients implement the necessary methods on a user defined struct for ssz, the cryptography will not need to be re-analysed, for example.

Right, but I guess SSZ (which is a strict dependency of Ethereum) would also be fundamentally broken if SSZ were not injective. So from my point of view this does not introduce any new assumptions.

[kevaundray]

Should something ever change with the way clients implement the necessary methods on a user defined struct for ssz, the cryptography will not need to be re-analysed, for example.

Right, but I guess SSZ (which is a strict dependency of Ethereum) would also be fundamentally broken if SSZ were not injective. So from my point of view this does not introduce any new assumptions.

Yep, though it would still be an unnecessary abstraction leak into the cryptography module.

I think it's advantageous to not have the cryptography rely on any extraneous assumptions. I linked some code above to show how one can encapsulate the Fiat-Shamir protocol if this extra assumption is removed.

In terms of conventions, I think using ssz or some other objective function for Fiat-Shamir is more out of the norm than using the serialisation format that comes with points and field elements, see the halo2 impl I linked above

[kevaundray]

Closing as #3030 and #3038 have been merged