ferranbt / fastssz

Fast Ethereum2.0 SSZ encoder/decoder
MIT License
74 stars 44 forks source link

Invalid root for eip4844 containers #111

Closed Inphi closed 1 year ago

Inphi commented 1 year ago

We're using sszgen for an updated eip4844 beacon block body:

class BeaconBlockBody(Container):
    randao_reveal: BLSSignature
    eth1_data: Eth1Data  # Eth1 data vote
    graffiti: Bytes32  # Arbitrary data
    # Operations
    proposer_slashings: List[ProposerSlashing, MAX_PROPOSER_SLASHINGS]
    attester_slashings: List[AttesterSlashing, MAX_ATTESTER_SLASHINGS]
    attestations: List[Attestation, MAX_ATTESTATIONS]
    deposits: List[Deposit, MAX_DEPOSITS]
    voluntary_exits: List[SignedVoluntaryExit, MAX_VOLUNTARY_EXITS]
    sync_aggregate: SyncAggregate
    # Execution
    execution_payload: ExecutionPayload 
    blob_kzg_commitments: List[KZGCommitment, MAX_BLOBS_PER_BLOCK]  # [New in EIP-4844]

The BeaconBlockBody contains a new field, blob_kzg_commitments, where KZGCommitment is a Bytes48 ssz type and MAX_BLOBS_PER_BLOCK is 16. So we add the field to the new BeaconBlockBody struct in our prysm fork:

BlobKzgs [][]byte `ssz-size:"?,48" ssz-max:"16"`

However, the updated generated tree routine for the beacon block looks incorrect:

func (b *BeaconBlockBody) HashTreeRootWith(hh *ssz.Hasher) (err error) {
    // <omitted other fields for brevity>

    // Field (10) 'BlobKzgs'
    {
        if size := len(b.BlobKzgs); size > 16 {
            err = ssz.ErrListTooBigFn("--.BlobKzgs", size, 16)
            return
        }
        subIndx := hh.Index()
        for _, i := range b.BlobKzgs {
            if len(i) != 48 {
                err = ssz.ErrBytesLength
                return
            }
            hh.PutBytes(i)
        }

        numItems := uint64(len(b.BlobKzgs))
        hh.MerkleizeWithMixin(subIndx, numItems, ssz.CalculateLimit(16, numItems, 0))
    }

I'd expect that the size input for CalculateLimit to be 48 but here it's 0 which causes the new BeaconBlockBody root to be incorrect.

ferranbt commented 1 year ago

Hi, does it work if you change CalculateLimit with 48? Also, are there any official tests for these encodings I can use to unit test?

ferranbt commented 1 year ago

Please check if #116 works.

Inphi commented 1 year ago

The change looks OK, but the root doesn't match what I expect. I've narrowed it down to the merkleization of the kzg List itself. There aren't any official test vectors for the new types yet. But we use remerkeable in the official specs for consensus tests. The hash_tree_root I get using remerkleable doesn't match sszgen's:

from remerkleable.complex import List
from remerkleable.byte_arrays import ByteVector

kzgs = List[ByteVector[48], 16]()
kzgs.hash_tree_root().hex() == "792930bbd5baac43bcc798ee49aa8185ef76bb3b44ba62b91d86ae569e4bb535"

whereas the root the above struct definition in my OP for fastssz, the root is 0x52e2647abc3d0c9d3be0387f3f0d925422c7a4e98cf4489066f0f43281a899f3. I'm not sure which ssz implementation is correct here though.

Inphi commented 1 year ago

Hi. Any updates here?

ferranbt commented 1 year ago

Hi @Inphi. Sorry I was in an offsite and quite dumped with work. I am still checking this. I think I might find time in the coming days to check.

ferranbt commented 1 year ago

Please check again the PR linked, it returns now the same results as remerkleable. There was a problem in how the limit for the merkle trie was being calculated for complex types (Vector).

Inphi commented 1 year ago

Awesome! The fix works well. Thank you!