search

ethereum / consensus-specs

Ethereum Proof-of-Stake Consensus Specifications
Creative Commons Zero v1.0 Universal
3.56k stars 973 forks source link

DepositParameterRecord Primitive Ambiguity #219

Closed schroedingerscode closed 5 years ago

schroedingerscode commented 5 years ago

The newly defined data structure DepositParameterRecord defines 2 fields (among others): pubkey and proof_of_possession, both of which are implemented by int384 primitives.

While these BLS types are effectively no different than a theoretical bytes48 type, spec implementers have assumably used integers instead, anticipating arithmetic operations.

To be consistent with the rest of the spec, the 2 BLS fields named above should be updated to be of type uint384, as there is no need for underlying implementations to explicitly use a signed type.

djrtwo commented 5 years ago

whoops. nice catch!

schroedingerscode commented 5 years ago

No worries, just kind of double checking out loud myself. Perhaps I'll bring it one step further: while a BLS implementation may be internal to the Beacon Chain spec itself, how the specific BLS library each implementer uses should handle all of the BLS arithmetic, meaning the Beacon Chain itself does not have to worry with much more than storing and comparing BLS pubkeys/sigs. Therefore:

why are the BLS types defined as uint384's as opposed to bytes48 in the Beacon Chain spec?

djrtwo commented 5 years ago

uint384 and bytes48 are exactly the same wrt SSZ serialization. You are correct that there is no big integer arithmetic being performed on these values outside of the BLS curve operations.

Elliptic curves are usually concerned with the bit values (bn256, bls12-381). uint384 more clearly ties the datatype to the BLS12-381 curve than bytes48

schroedingerscode commented 5 years ago

Yep, makes sense. Better be more verbose than underexplain and cause confusion. Thanks.