-
Notifications
You must be signed in to change notification settings - Fork 17
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
fixes & cleanup #28
fixes & cleanup #28
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.
Not sure where the spec of this code is, but the code is missing a signature domain for safety. You may want to update the spec with something like this.
HashTreeRoot() ([32]byte, error) | ||
} | ||
|
||
func VerifySignature(obj HashTreeRoot, pk, s []byte) (bool, error) { |
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.
It would be much better to define the signing API in a way where you are forced to include a signing domain and fork version etc. Signatures should be constructed in a way where they are unique for the purpose of the work and version of the protocol. See compute_signing_root
and get_domain
in the phase0 beacon spec. You essentially want to compute a "domain" (32 bytes) from a fork digest (constructed from genesis + fork info) and a type-domain (e.g. proposals and attestations have different domains to be sure they never have the same signature for two different intents). Or you can implement your own non-standard domain method. But definitely don't just create a BLS signature over data that might have a different meaning later on in the protocol. cc @lightclient
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.
Also relevant: ethereum/consensus-specs#2884 for the domain
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.
The current validation code is already merged, in this PR it's just moved. Maybe we should transfer this into it's own issue and make an update independent to this PR, which just complements the code currently in master.
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.
👉 #29
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.
It seems a little weird to use hexutil.Bytes
for extra_data
since it is a fixed 32 byte value, like many values already in the header. Okay for now though.
(oops, I see your msg now about |
Yeah i wrote to you before, if we use a fixed size then the block hashes don't match, because go-ethereum uses a variable width byte array. |
Cleanup of #26 (addresses all the review comments)