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

Correct ENR decoding on extension trait #6402

Merged
merged 5 commits into from
Sep 20, 2024

Conversation

AgeManning
Copy link
Member

@AgeManning AgeManning commented Sep 17, 2024

In #6385 we upgraded to a new version of ENR. This version uses alloy-rlp. We have now explicit typing in decoding values to and from an ENR.

The previous update assumed Vec<u8> would be decoded as bytes, but the alloy-rlp implements deocde for Vec<T> as an RLP list. So it was trying to decode a list of u8's rather than raw bytes. As the eth2 and attestation fields are not RLP lists this decoding failed and we marked the ENRs as invalid.

This should correct this issue by force the type to be Bytes allowing us to decode the raw bytes in the ENR.

@pawanjay176
Copy link
Member

Confirmed that discovery searches are working now. Added a commit to fix other occurrences of EnrBuilder::add_value

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. Networking labels Sep 18, 2024
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Nice find!

@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Sep 19, 2024

queue

🛑 The pull request has been removed from the queue default

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@michaelsproul
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Sep 20, 2024

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

Copy link

mergify bot commented Sep 20, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a97d77c

mergify bot added a commit that referenced this pull request Sep 20, 2024
@mergify mergify bot merged commit a97d77c into sigp:unstable Sep 20, 2024
27 of 28 checks passed
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
* Correct enr extension encodings

* Clippy my ol friend

* Correct all encoding and comparisons

* Found some more encodings

* Fix remaining tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants