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

Little vs big endian #556

Closed
jannikluhn opened this issue Feb 2, 2019 · 8 comments
Closed

Little vs big endian #556

jannikluhn opened this issue Feb 2, 2019 · 8 comments
Labels
general:RFC Request for Comments

Comments

@jannikluhn
Copy link
Contributor

SSZ has been updated to little endian integer encoding, but the phase 0 spec still uses big endian encoding at a few of sites. Is there a specific reason for this? If not I'd advocate for using little endian here as well for the sake of consistency.

@djrtwo
Copy link
Contributor

djrtwo commented Feb 2, 2019

Good call. Agreed on consistency.

@hwwhww hwwhww added the general:RFC Request for Comments label Feb 3, 2019
@hwwhww
Copy link
Contributor

hwwhww commented Feb 3, 2019

Hmmm, I feel not fully confident of this change for some reasons:

  1. eth1.0 is still a big-endian world - so all the bytes conversion on deposit contract are still in big-endian.
  2. Our BLS spec uses big-endian now. Moreover, BLS12-381curve uses big-endian now. <-- Does it make sense to modify it?

That's said, IMHO it's okay to only use little-endian in SSZ.

@hwwhww
Copy link
Contributor

hwwhww commented Feb 3, 2019

/cc @arnetheduck 😆

@djrtwo
Copy link
Contributor

djrtwo commented Feb 4, 2019

Yeah, there is actually a bug in the spec right now. The amount and timestamp logged in deposit_data from the deposit contract is assumed to be in a well formatted SSZ DepositData object. If we are to keep ssz little endian, then we need to either do the conversion in the deposit contract or clean it up on the 2.0 side. I asked the vyper guys if there is a built-in for int to little-endian bytes and they said no. This is definitely an argument for big endian...

contract code snippet below:

deposit_amount: bytes[8] = slice(concat("", convert(msg.value / GWEI_PER_ETH, bytes32)), start=24, len=8)
deposit_timestamp: bytes[8] = slice(concat("", convert(block.timestamp, bytes32)), start=24, len=8)
deposit_data: bytes[528] = concat(deposit_amount, deposit_timestamp, deposit_input)

@djrtwo
Copy link
Contributor

djrtwo commented Feb 4, 2019

/cc @vbuterin

@arnetheduck
Copy link
Contributor

My general take on this would be that for types that have a canonical serialization defined outside of the logic of the Eth2 spec, we should use bytesX types in the Eth2 spec - for example, a keypair is generally not considered to be an integer or a point on the curve, from the point of view of the consumer of the key - instead, it's just a blob that you pass as-is to the cryptography black box, and you let the cryptography black box define that serialization on its own terms, compartmentalized.

This goes for hashes, keys and other similar types.

The deposit is a little bit more tricky indeed. We can drop the idea of little endian completely and lose the other benefits, or one of the sides will have to make way - one alternative is to do the decoding on the Eth2 side by encoding deposits as a canonical bytes structure and specify a specific interop deserialization function which is not SSZ, for that exceptional case.

@djrtwo
Copy link
Contributor

djrtwo commented Feb 5, 2019

I'm thinking we properly encode deposit_amount and deposit_data to little endian within the deposit contract. We just need to define one method to do the byte manipulation for the encoding. Having the beacon chain and the VM (wasm) have the same encoding will be a win in the end, and handling the fix within the deposit contract ensures that DepositData to comes into eth2 well-formatted so we don't have to write an exceptional rule.

@djrtwo
Copy link
Contributor

djrtwo commented Feb 7, 2019

fixed the deposit contract endian formatting bug here ethereum/deposit_contract#8

I think it makes sense to convert everything in the state transition spec to little endian. Fine with converting BLS spec too, but would rather just conform that to whatever other blockchains are using so we can wait on that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:RFC Request for Comments
Projects
None yet
Development

No branches or pull requests

4 participants