Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid root for eip4844 containers #111

Closed
Inphi opened this issue Sep 26, 2022 · 7 comments · Fixed by #116
Closed

Invalid root for eip4844 containers #111

Inphi opened this issue Sep 26, 2022 · 7 comments · Fixed by #116

Comments

@Inphi
Copy link

Inphi commented Sep 26, 2022

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
Copy link
Owner

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
Copy link
Owner

Please check if #116 works.

@Inphi
Copy link
Author

Inphi commented Sep 26, 2022

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
Copy link
Author

Inphi commented Oct 5, 2022

Hi. Any updates here?

@ferranbt
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

Inphi commented Oct 24, 2022

Awesome! The fix works well. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants