ferranbt / fastssz

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

Add Dynamic List of Byte Lists Support #41

Closed rauljordan closed 2 years ago

rauljordan commented 3 years ago

This PR adds support for

type ETHMergeTransactions struct {
    OpaqueList [][]byte `json:"opaque_list" ssz-size:"?,?" ssz-max:"2048,2048"`
}

Which does not exist before. Being able to specify ssz-max:"2048,2048" is important for the eth1 <> eth2 merge functionality

rauljordan commented 3 years ago

Forgot about unmarshal, will edit

ferranbt commented 3 years ago

Please also run "make build-spec-tests" so that we can see the difference in the encodings to see if this changes affects any other struct.

ferranbt commented 3 years ago

Is this the correct encoding for a dynamic list? At least for other dynamic types without fixed size length you have to write offsets first.

rauljordan commented 3 years ago

@ferranbt getting this cannot happen when running the build

go run sszgen/*.go --path ./spectests/structs.go --include ./spectests/external,./spectests/external2
panic: it cannot happen

goroutine 1 [running]:
main.(*Value).unmarshalList(0xc000138e80, 0xc000012900, 0xc00030e170)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/unmarshal.go:133 +0x6a5
main.(*Value).unmarshal(0xc000138e80, 0x11a46a8, 0x3, 0x2, 0xc000319978)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/unmarshal.go:95 +0xbfd
main.(*Value).umarshalContainer(0xc000138e00, 0xc0002c5301, 0xc00030e170, 0xa, 0xc0002c14c8, 0xc0002abb00)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/unmarshal.go:301 +0x120a
main.(*env).unmarshal(0xc00016fe38, 0xc0000164a0, 0x7, 0xc000138e00, 0xc000255000, 0x3da)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/unmarshal.go:20 +0xd7
main.(*env).print(0xc00016fe38, 0x7ffeefbff501, 0xc00011a600, 0x1e, 0x20, 0xc00001c500, 0x1f, 0xc00010be38, 0x115faa0, 0xc00010be38, ...)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/main.go:390 +0x62d
main.(*env).generateEncodings(0xc00016fe38, 0x0, 0x0, 0x1f, 0xc000073d00)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/main.go:323 +0x1d4
main.encode(0x7ffeefbff521, 0x16, 0x12e4250, 0x0, 0x0, 0x0, 0x0, 0xc00005e020, 0x2, 0x2, ...)
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/main.go:99 +0x266
main.main()
        /Users/me/Documents/Go/src/github.com/ferranbt/fastssz/sszgen/main.go:41 +0x3cb
exit status 2

hard to identify what's going on, and yes it seems like we might need to add the offset

rauljordan commented 3 years ago

Figured it out!

ferranbt commented 3 years ago

Is there any way to test this with the spectests?

dshulyak commented 3 years ago

Is it intetional that ssz-size:"?,1024" ssz-max:"?,1024" doesn't work as it can be expected? maybe an error should be raised then.

ferranbt commented 3 years ago

Is this PR still working? AFAIK this is the only new type required for merge right?

claravanstaden commented 2 years ago

Does this PR need to be merged before Transactions can be merkleized?

ferranbt commented 2 years ago

Closing the PR since this has already been implemented.