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

Accept wit/80 from Nethermind #4279

Closed
macfarla opened this issue Aug 19, 2022 · 3 comments · Fixed by #4283
Closed

Accept wit/80 from Nethermind #4279

macfarla opened this issue Aug 19, 2022 · 3 comments · Fixed by #4283
Assignees
Labels
mainnet peering TeamRevenant GH issues worked on by Revenant Team

Comments

@macfarla
Copy link
Contributor

I have created this issue against Nethermind: NethermindEth/nethermind#4443
Should we allow this encoding of the witness capability in Besu for now, so that we can decode the hello from Nethermind? This is a bug in Nethermind, but other clients probably allow that encoding, otherwise they would have noticed that they cannot connect to other clients …
#4443 Nethermind hello message reports wrong version for witness protocol
Describe the bug
The hello message sent by Nethermind contains the capability for the witness protocol. It is RLP encoded as:
c5: list with length 5
83: String of length 3
776974: “wit”
Show more
https://github.com/NethermindEth/nethermind|NethermindEth/nethermindNethermindEth/nethermind | Today at 11:05 AM | Added by GitHub

quite possible that the number 0 is encoded as a 0 length byte string.

@macfarla macfarla added mainnet peering TeamRevenant GH issues worked on by Revenant Team labels Aug 19, 2022
@shemnon
Copy link
Contributor

shemnon commented Aug 19, 2022

This reads right to me. Zeros are encoded as 80 because Deserialized positive integers with leading zeroes get treated as invalid. - making zero a zero length string trimmed to an empty string (spelled out explicitly here https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp) so c58377697480 is wit/0, rather than c58377697400 - which the latter is incorrect.

@pinges
Copy link
Contributor

pinges commented Aug 22, 2022

Thanks @shemnon I think you are right. I'm not 100% convinced that in this case that you have to interpret this as a "leading zero", but I guess we have to accomodate that interpretation. I will create a new issue to make sure that we are looking into how we are encoding zeros and other cases where we might have to accomodate 0x80 as an encoding for zero.

@shemnon
Copy link
Contributor

shemnon commented Aug 22, 2022

Ethereum yellow paper spells out the removal of all leading zeros in appendix B just below equation 197, and that zero scaler writes as 0x80 is explicitly tested for in the ethereum reference tests. and a number of invalid tests flag 0x00 as a scalar length as invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mainnet peering TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants