Chunk aligned merge upstream

[kasey]

Adds padding for byte slices that are not multiples of the htr chunk size (32 bytes) to align to the next chunk boundary. Failing to do this yields incorrectly encoded values which fail spec tests. This surfaced in changes for Altair, which introduces a new field with aligned byte slices that are not pre-aligned to chunk boundaries.

[ferranbt]

I do not usually include test in sszgen. Instead, every change in sszgen should include an example case in spectests with the corresponding generation (i.e. #53). Then, we know that the modification did not change any other thing. I am going to document this tomorrow now that more people is contributing to fastssz.

Then, do you mind removing the test and generating the files with "make build-spec-tests". Add a new case struct as an example if necessary.

[ferranbt]

Is this specific to any new struct from Altair? I think I might remember something like this.

[kasey]

Is this specific to any new struct from Altair? I think I might remember something like this.

SyncCommitteeDuty.Pubkey https://github.com/prysmaticlabs/prysm/blob/hf1/proto/eth/v2/validator.pb.go#L139

yes this is a new type for Altair.

[prestonvanloon]

Then, do you mind removing the test and generating the files with "make build-spec-tests". Add a new case struct as an example if necessary.

Could we keep these tests or add more tests to sszgen? I could see how sszgen might be easily broken while the spec-test cases pass.

[kasey]

I do not usually include test in sszgen. Instead, every change in sszgen should include an example case in spectests with the corresponding generation (i.e. #53). Then, we know that the modification did not change any other thing. I am going to document this tomorrow now that more people is contributing to fastssz.

Then, do you mind removing the test and generating the files with "make build-spec-tests". Add a new case struct as an example if necessary.

Looking at #53 and I'm not sure what you mean, it doesn't seem to include a test.

The reason I added a unit test here is that I was changing some of the template rendering code to try to improve its maintainability, but I also recognized I was making changes that would impact existing cases, so I wanted to test that the new code was doing no harm. It can also be difficult when reading the fastssz code to understand how a local change in a method like this translates to the combined rendered output. So I also thought that a unit test would help to make clear how this specific method translates to the final result. Do you think there could be some value in unit tests like that?

[kasey]

I tried to follow the readme to update the spec test fixtures like this:

go run github.com/ferranbt/fastssz/sszgen --path $HOME/src/prysmaticlabs/prysm/proto/eth/v2/ --objs SyncCommitteeDuty --include $HOME/src/prysmaticlabs/eth2-types/ --output ./synccommitteeduty.ssz.go
panic: No files to generate

goroutine 1 [running]:
main.encode(0x7ffee6fa8a9c, 0x31, 0xc00010c330, 0x1, 0x1, 0x7ffee6fa8b24, 0x1a, 0xc00010c340, 0x1, 0x1, ...)
        /home/kasey/src/kasey/fastssz/sszgen/main.go:112 +0x5fe
main.main()
        /home/kasey/src/kasey/fastssz/sszgen/main.go:43 +0x3cb
exit status 2

This seems like it should work, I'll need to do some more debugging to see why this is panicking. If I had gotten output, I think I would need to read source code to understand how to integrate it into the spectests, so the docs you suggested would be totally appreciated. Thanks!

[ferranbt]

If you check the repo there is a folder called spectests. It represents all the structs for Eth2.0 + the official tests (the same things you do in prysm/proto/eth/v2/). Usually, the protocol to add a new ssz generation is:

• Write it in sszgen.
• Add a test case in fastssz/spectests (like the interface in #53)
• Regenerate the spectests with "make build-spec-tests". Currently, this does not work in your branch because you have included the hash_test.go.

Thus, it is possible to know if the new modification has changed any other encoding without any unit test. I am not sure if unit tests are worth it here since to make it complete you should test code -> IR and IR -> marshal/unmarshal/hash. The fastssz/spectests are like an e2e test that encompasses all the testing.

[ferranbt]

Looking at #53 and I'm not sure what you mean, it doesn't seem to include a test.

The lack of changes in spectests/structs_encoding.go is the test.

[ferranbt]

I tried to follow the readme to update the spec test fixtures like this:

Sorry, I did not make myself clear. In ./spectests/structs.go in the fastssz repo add something like this:

type Test1 struct {
   Data []byte `ssz-size:48`
}

Now, regenerate the structs with:

$ make build-spec-tests 

Then, we can see how does your change encodes the new Test1 struct and how it affects the other eth2.0 structs.

[prestonvanloon]

Any update on this?

[kasey]

@prestonvanloon please see https://github.com/ferranbt/fastssz/pull/60

[kasey]

Closing this per https://github.com/ferranbt/fastssz/pull/60#issuecomment-908540639