-
Notifications
You must be signed in to change notification settings - Fork 985
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
Part 3 of Restructuring BeaconBlock Proto - Remove Special Record #1067
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also use ranges here for proto fields to be safe
// Specials consist of exist, penalities, etc. | ||
repeated SpecialRecord specials = 7 [deprecated=true]; // Deprecated in favor of unify specials object w/ attestations. | ||
repeated bytes proposer_signature = 8; // Type of [uint384]? | ||
repeated bytes ancestor_hash32s = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 32s implies there is a multiple of ancestor hashes because repeated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No typo here. Hash32s is plural
proto/beacon/p2p/v1/types.proto
Outdated
|
||
// Block Body | ||
BeaconBlockBody body =79; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing
Codecov Report
@@ Coverage Diff @@
## master #1067 +/- ##
==========================================
- Coverage 71.19% 71.19% -0.01%
==========================================
Files 76 76
Lines 4576 4575 -1
==========================================
- Hits 3258 3257 -1
Misses 1011 1011
Partials 307 307 |
// Specials consist of exist, penalities, etc. | ||
repeated SpecialRecord specials = 7 [deprecated=true]; // Deprecated in favor of unify specials object w/ attestations. | ||
repeated bytes proposer_signature = 8; // Type of [uint384]? | ||
repeated bytes ancestor_hash32s = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No typo here. Hash32s is plural
proto/beacon/p2p/v1/types.proto
Outdated
// Block Body | ||
BeaconBlockBody body = 9; | ||
BeaconBlockBody body = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 1000? Do we expect a lot of fields in this message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not.. I'll change it back to 9
repeated SpecialRecord specials = 7 [deprecated=true]; // Deprecated in favor of unify specials object w/ attestations. | ||
repeated bytes signature = 8; // Type of [uint384]? | ||
repeated bytes ancestor_hash32s = 2; | ||
bytes state_root_hash32 = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, once we launch a v1, we can never change the IDs. It's OK for now though
I'll be sure to document this, but read this for your FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Reading it now
Part of #781
This PR removes special record from beacon block