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

Possible aesthetic rework to get_domain #865

Merged
merged 8 commits into from Apr 19, 2019
Merged

Possible aesthetic rework to get_domain #865

merged 8 commits into from Apr 19, 2019

Conversation

vbuterin
Copy link
Contributor

In general I dislike how domains, which should be an unobtrusive out-of-the-way thing that we don't think about much, are taking up so much space in code to express, to the point of them being the single thing preventing bls_verify from being expressed in one line of code. Here I reorder arguments and add a default, and make bls_verify a one-liner.

Not necessarily convinced that exactly this approach is the way to go, but IMO it's worth considering.

vbuterin and others added 4 commits March 31, 2019 04:55
In general I dislike how domains, which should be an unobtrusive out-of-the-way thing that we don't think about much, are taking up so much space in code to express, to the point of them being the single thing preventing `bls_verify` from being expressed in one line of code. Here I reorder arguments and add a default, and make `bls_verify` a one-liner.

Not necessarily convinced that exactly this approach is the way to go, but IMO it's worth considering.
@JustinDrake
Copy link
Collaborator

All instances of bls_verify, with the exception of RANDAO, involve a self_signed_container where self_signed_container.signature is checked against signed_root(self_signed_container). We could make RANDAO fit into this pattern also and abstract away the repeated pattern.

@djrtwo
Copy link
Contributor

djrtwo commented Apr 3, 2019

This looks fine to me.

Justin are you suggesting making a RandaoReveal object that gets embedded in the block?

{
    'epoch': 'uint64',
    'signature': 'bytes96'
}

This seems reasonable. It doesn't allow you to fully abstract away the pattern because some objects are verified against the fork epoch current_epoch whereas others are verified against the fork epoch of the slot of the object in question

@JustinDrake
Copy link
Collaborator

JustinDrake commented Apr 3, 2019

are you suggesting making a RandaoReveal object that gets embedded in the block?

Making a RandaoReveal object yes, but not embedding it in the block. That is, the RandaoReveal can be populated at run-time. It's purely a cosmetic change to reduce the number of arguments in bls_verify from 4 to 3 (essentially merging message_hash and signature into a single self_signed_container). I think all bls_verify calls would then fit in a single line, including domain.

@JustinDrake JustinDrake added the general:presentation Presentation (as opposed to content) label Apr 4, 2019
@vbuterin
Copy link
Contributor Author

vbuterin commented Apr 4, 2019

I would say altering bls_verify itself would be a bad idea; it mixes layers, as it would introduce notions of how SSZ works etc etc into what is meant to be a pure cryptographic standard. But I would favor a bls_verify_container function (defined in the phase 0 spec) along with bls_verify. So we can still use "pure" bls_verify for the RANDAO (and possible future functions where adding a container would just complicate things), but get the efficiencies of bls_verify_container in all the cases where we are verifying a container.

@JustinDrake
Copy link
Collaborator

it mixes layers

Agreed that we do not want to mix layers :) On that note, it has been bothering me that the current bls_verify mixes layers. Specifically, it mixes the "cryptographic" layer (pubkey, signature, message_hash) with the "application" layer (domain).

My suggestion would be to have cleanly separated functions. First, a "pure" bls_verify, and second an Eth2-specific wrapper with self-signed containers and signature domains.

This was referenced Apr 14, 2019
@djrtwo
Copy link
Contributor

djrtwo commented Apr 17, 2019

Any verdict on this PR?

@JustinDrake
Copy link
Collaborator

JustinDrake commented Apr 18, 2019

Any verdict on this PR?

Very much pro :) Further changes I'd like to see:

  1. Decouple domain from the cryptographic layer.
  2. Make RANDAO fall into the general signature application-layer pattern by adding a new SSZ object.
  3. Further abstract the application-layer pattern with a verification function that takes only three arguments: pubkey, self_signed_container, domain.

@djrtwo
Copy link
Contributor

djrtwo commented Apr 18, 2019

okay. I'll fix merge conflict and merge

@djrtwo djrtwo merged commit 23fffe6 into dev Apr 19, 2019
@djrtwo djrtwo deleted the vbuterin-patch-2 branch April 19, 2019 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants