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

Zero sig checks #2733

Closed
wants to merge 4 commits into from
Closed

Zero sig checks #2733

wants to merge 4 commits into from

Conversation

mratsim
Copy link
Contributor

@mratsim mratsim commented Jul 22, 2021

We sent blocks with a zero signature instead of an infinity signature in Altair.

This adds checks to ensure that on serialization of signatures, in particular empty aggregates or defautl init ones we send 0xc000...0000 which is the infinity point serialized.


Source of the bug:

By delaying the deserialization of signatures up until they are needed (#2259)

ValidatorSig* = object
blob*: array[RawSigSize, byte]
, sometimes they are never needed.
In that case empty signatures will be hex-serialized as 0x0000...0000.

Solution, we need to check for a full zero byte array in a custom toHex function.

@mratsim mratsim requested a review from zah July 22, 2021 09:07
@mratsim
Copy link
Contributor Author

mratsim commented Jul 22, 2021

I've added a check to ensure default init signatures are serialized as the infinity points.

Though it seems like it impacts SignedBlockHeader serialization for some reason

image

@github-actions
Copy link

github-actions bot commented Jul 22, 2021

Unit Test Results

0 files   -      24  0 suites   - 492   0s ⏱️ - 14m 7s
0 tests  -    747  0 ✔️  -    745  0 💤  - 2  0 ❌ ±0 
0 runs   - 5 004  0 ✔️  - 4 996  0 💤  - 8  0 ❌ ±0 

Results for commit e662698. ± Comparison against base commit f0c30e3.

♻️ This comment has been updated with latest results.

@mratsim
Copy link
Contributor Author

mratsim commented Jul 26, 2021

So the genesis block needs a real full zero signature not an infinity signature #374 while for the empty Altair sync committees we need infinity signatures #2736

So we need to choose a proper default for serializing a full zero ValidatorSig = array[96, byte] and an escape hatch for one of the special case (genesis or empty committee).
Note that a blscurve.Signature is serialized as infinity signature and cannot be full zero.

@mratsim
Copy link
Contributor Author

mratsim commented Jul 26, 2021

aka: back to ethereum/consensus-specs#1713

@mratsim
Copy link
Contributor Author

mratsim commented Jul 26, 2021

Actually the zero sig test that was initially failing when introduced in https://github.com/status-im/nimbus-eth2/pull/400/files predates ethereum/consensus-specs#1713 and it's possible that it's irrelevant and can be deleted.

@arnetheduck
Copy link
Member

This looks complicated - ie ValidatorSig can already hold both an infinity and a full-zero signature, so NullableValidatorSig looks redundant / wrong - on the other hand, we should never be able to construct a CookedSig from a full-zero signature (it should fail to deserialize and there should be no other way to construct a CookedSig that represents a full-zero) - we should be able to create an infinity-point CookedSig however - in fact, the default constructor for CookedSig should create an infinity point, and this would get serialized into 0xc... - the committee code would then init itself using cookedsig (or a ValidatorSig.infinity() constructor.

@mratsim mratsim closed this Jul 28, 2021
@mratsim
Copy link
Contributor Author

mratsim commented Jul 28, 2021

Closing. From Discord discussion.

One Altair issue was due to different expectations between the BLS spec and the Altair test vector (which isn't supposed to happen in prod anyway). Fixed in #2741.

image

see ethereum/consensus-specs#1713
image

Next steps is adding tests to ensure that a default init ValidatorSig cannot be deserialized (aka, only infinity points can).

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

Successfully merging this pull request may close these issues.

2 participants