Change BLS Public Key Length Check in Beacon RPC Server #1571
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1571 +/- ##
=========================================
Coverage ? 71.84%
=========================================
Files ? 99
Lines ? 6674
Branches ? 0
=========================================
Hits ? 4795
Misses ? 1470
Partials ? 409 |
| @@ -43,8 +43,8 @@ func (vs *ValidatorServer) ValidatorEpochAssignments( | |||
| ctx context.Context, | |||
| req *pb.ValidatorEpochAssignmentsRequest, | |||
| ) (*pb.ValidatorEpochAssignmentsResponse, error) { | |||
| if len(req.PublicKey) != 48 { | |||
There was a problem hiding this comment.
Why change this? Didn't we have panics where invalid public keys were sent?
There was a problem hiding this comment.
IMO, this needs to be fixed to the expectation size. If it's 96 now then it should be 96.
There was a problem hiding this comment.
pub key size should really become its own variable if it's referenced in multiple places
There was a problem hiding this comment.
yes, it can be a params constant
| @@ -171,7 +188,7 @@ func TestValidatorCommitteeAtSlot_CrosslinkCommitteesFailure(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestValidatorCommitteeAtSlot_Ok(t *testing.T) { | |||
| func TestValidatorEpochAssignments_CorrectAssign(t *testing.T) { | |||
There was a problem hiding this comment.
CorrectAssign? Can you make this human readable?
|
@rauljordan This would clash with the spec though, a lot of the data structures take the public keys as 48 bytes, cant we just lop off half the bits to create a public key with 48 bytes as a temporary solution ? |
|
From eth2.0 spec's point of view, it doesn't care for the size of the pub key, it's all abstracted out. As long this doesn't break |
|
@terenc3t data structures like this https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#depositinput , and also https://github.com/ethereum/eth2.0-specs/blob/master/specs/bls_signature.md#bls_verify in the bls spec assume 48 bytes. Also the |
|
I see. That makes sense for the time being but we won't be compatible with rest of the clients :P |
|
In my opinion, once we implement our own BLS we'll have 48byte pubkeys, so this won't be an issue. The cutoff thing sounds a bit weird to do, so I'd rather just stick to this for now until we have our BLS that is in line with the spec. |
Part of #1565
Description
Write why you are making the changes in this pull request
Currently, our beacon RPC server checks if the public key in the assignments request is of length 48, however, our BLS library created by Herumi has pubkeys of length 96. Until we find a better BLS library or roll out our own, we will be stuck with a crashing runtime due to this strict check.
Write a summary of the changes you are making
This changes the length to global config param which can be checked against instead of a magic number.