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

Fix problems with new Electra structs #136

Merged
merged 6 commits into from
May 8, 2024
Merged

Fix problems with new Electra structs #136

merged 6 commits into from
May 8, 2024

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented May 7, 2024

This PR fixes the issues discovered by testing with the Electra reference structures.

  • Add electra.AggregationAndProof structure.
  • Add electra.SignedAggregationAndProof structure.
  • Add electra.AttesterSlashing structure.
  • Add VersionedAttesterSlashing structure.
  • Add VersionedIndexedAttestation structure.
  • Fix ssz-size for CommitteeBits (it's 8 bytes not 1 bit vector, confusing).
  • Fix typo when unmarshaling consolidations.
  • In SignedConsolidation, make message a pointer.
  • Remove ExitsRoot from ExecutionPayloadHeader.
  • Use yaml (not json) struct tag for Consolidations.
  • Fix input to MerkleizeWithMixin for ExecutionPayload.

One of the tests is still failing, but I'm pretty sure the ref test is wrong here.

I sent this message to hww:

image

@@ -436,7 +436,7 @@ func (e *ExecutionPayload) HashTreeRootWith(hh ssz.HashWalker) (err error) {
hh.MerkleizeWithMixin(elemIndx, byteLen, (1073741824+31)/32)
}
}
hh.MerkleizeWithMixin(subIndx, num, 1073741824)
hh.MerkleizeWithMixin(subIndx, num, 1048576)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me quite a while to find this issue. Why was this auto-generated value used? Have you been manually changing this for previous versions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was addressed in sszgen as per ferranbt/fastssz#127 but not sure if it has made it into a release version yet.

@mcdee
Copy link
Contributor

mcdee commented May 7, 2024

Extra data of 0x is an empty value. In YAML strings don't have to be quoted, as far as I'm aware. You will find that consensus and execution nodes are a little lax when it comes to formatting, so it's likely that you will have to handle that situation.

@jtraglia
Copy link
Contributor Author

jtraglia commented May 7, 2024

@mcdee I'm not entirely sure how to fix this 0x issue. I can fix our marshaled output, but the test code will marshal it to a string, which has a different value. It will marshal it to 0 (not 0x) there.

func testYAMLFormat(input []byte) string {
	val := make(map[string]any)
	if err := yaml.UnmarshalWithOptions(input, &val, yaml.UseOrderedMap()); err != nil {
		panic(err)
	}
        ...

For example, the following. The top line is the expected output, the bottom line is the actual output:

image

Any advice here?

@jtraglia
Copy link
Contributor Author

jtraglia commented May 7, 2024

Ah I found that you did this for Capella. I'll copy this:

image

@jtraglia
Copy link
Contributor Author

jtraglia commented May 7, 2024

Pushed a commit which handles that edge case. Also, Hsiao-Wei has acknowledged the issue. I'm going to make a PR to the consensus-specs to get that fixed. The fix might not make it into the next release though.

@mcdee mcdee merged commit 3117883 into attestantio:electra May 8, 2024
3 checks passed
@jtraglia jtraglia deleted the pass-electra-tests branch May 8, 2024 11:28
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 this pull request may close these issues.

2 participants