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

Spec precise VRF use in EC #390

Merged
merged 23 commits into from
Jul 26, 2019
Merged

Spec precise VRF use in EC #390

merged 23 commits into from
Jul 26, 2019

Conversation

sternhenri
Copy link
Contributor

@sternhenri sternhenri commented Jul 13, 2019

This PR covers #290 and is built atop #334 rather than master (sorry, worth merging 334 ahead of reviewing this one)

Important background in https://github.com/filecoin-project/research-private/issues/189

I made a few changes worth noting:

  1. Opted for using Secp256k1 > BLS12-381 after conversations with @nikkolasg
    • We lose 20% space per ticket and election proof (Secp 80bytes vs BLS 64), but save in terms of storing miner keys and 10-100x in verif time.
    • Regardless of where we fall on this, I'd recommend addressing it in a separate PR, as this one is I believe an improvement in pure terms of precision
  2. Opted not to store VRFOutput (i.e. the deterministic output from the VRFProof) on chain: while it is always needed for interacting with the VRF, it can be derived from the VRFProof (which is also needed), so I chose to minimize on-chain footprint
  3. The use of operation identifiers in generating the VRF input string (i.e. input | "t" for tickets and input | "e") is I believe necessary for security reasons, lest SLE not be Secret (can you see why?). However, the current token addition is pretty clumsy. Any suggestions there @dignifiedquire ?
  4. I fully removed the mention of the edge case where a lookback looks to a round containing multiple "losing" tickets (ie don't pick min in that round, but rather the one that yields to the block with the min final ticket): I believe the edge case is actually not possible. Can you confirm we're on same page @whyrusleeping ?
    • TipSet blocks must have same parents and same number of tickets
    • Meaning two blocks mined over losing tickets (fka null blocks) will not be in a same TipSet
  5. There may need to be changes to the miner actor to account for the necessary PK,SK pairs that need to be valid. I'm not too sure.

@sternhenri sternhenri changed the title Feat/ec sig spec Spec precise VRF use in EC Jul 13, 2019
@nikkolasg
Copy link
Contributor

The use of operation identifiers in generating the VRF input string (i.e. input | "t" for tickets and input | "e") is I believe necessary for security reasons, lest SLE not be Secret (can you see why?). However, the current token addition is pretty clumsy. Any suggestions there @dignifiedquire ?

A small nitpick: Separating different usage of the same function with an additional input is called "domain separation" and is usually done by prefixing the message (rather than suffixing it) with a specific byte / integer whose length is known easily (i.e. if it's a byte it's easy). I could encode "t" in many ways while encoding a number on a byte is straightforward. So for simplicity of reading & implementing the specs, I would encourage the use of a clear fixed-size domain separation with a integer.
I'm not sure suffixing it changes the security (although it might bring some trouble in case there are some weaknesses found in the compression function of the hash function such as what has been found in SHA1) but as well, consistency wise, I would recommend prefixing the domain separation.

Of course both of these suggestions are based on the fact that.. it's OK (i.e. lightweight) to change. If it's already implemented, since these are backward incompatible suggestions, then it's ok to leave it as it is. In any case, domain separation is definitely required here !!

@sternhenri
Copy link
Contributor Author

Great! This is exactly what I was asking @dignifiedquire, @nikkolasg. Thanks for this reply. Is this done anywhere else already @dignifiedquire, so I may draw inspiration? Otherwise, where would you recommend I "assign" the appropriate byte for each operation in the spec?

@nikkolasg
Copy link
Contributor

To cite back the RFC, they're doing that in the hashing parts of the protocol, for example look at the Hash Points part here where they use the 0x02 byte as a domain separation.

@Kubuxu
Copy link

Kubuxu commented Jul 15, 2019

As far as I know one of the primary reasons for prefix-based domain separation is a risk of length extension attacks on hash functions.

@dignifiedquire
Copy link
Contributor

  • Agree with prefixes are better, and the "standard" way to do this as far as I know.
  • Suggestion regarding assign these.

Step 1. Define a table

VDF Personalizations (I believe this is the right technical term)

Type Prefix
Ticket 0x01

Step 2. Use the table

input <- VDFPersonalizationTicket | parentTicket.VDFOutput

Step 3. Profit

@sternhenri
Copy link
Contributor Author

A quick convo w @dignifiedquire and @nikkolasg yielded the following back of the envelope impact for choice of BLS (64 B, 30ms verif time) vs secp (80 B, .3ms verif time).

Over a year (~1M blocks) with 2 Verifs per block (ticket and electionProof), syncing chain from genesis would take:

  • BLS, ~9 hours
  • Secp, ~6 mins

Someone check my math. But seems both are fine. May be worth using BLS instead.

@whyrusleeping
Copy link
Member

on my macbook (not the fastest processor) I get 5ms per bls verify using our rust to go bindings.

Verifications can be batched as well for improved performance, and done in parallel. I'm not super worried about the cpu cost of BLS. I also expect it to improve with optimizations.

@sternhenri
Copy link
Contributor Author

I agree. Even if it didn't it's nbd. However, the size gain is less than I thought though I forget the exact reason atm. Anyways, The time has come to merge or request a change. Turn to BLS, or do I merge?

@whyrusleeping
Copy link
Member

@sternhenri the signature length for bls12-381 should be 384 bits == 48 bytes.

For some reason, the go code i'm using (go-bls-sigs) has signatures being 96 bytes. Not sure why that is.

@sternhenri
Copy link
Contributor Author

Per @nikkolasg out-of-band:

there are two ways to compute a BLS signature which have different trade-offs on the size of public keys vs size of signature.
(1) (default) public key is G2 and signature is on G1. In that case public key is 96 bytes and G1 is 48 bytes
(2) public key is on G1 and signature on G2. In that case, public key is 48 bytes and signature is 96 bytes.

So this is the tradeoff vs secp (80B) re sizing, if I grok correctly,

  • in case 1, we gain 64 Bytes per block against secp ((80-64)*2) but lose 16 Bytes per miner in the storage actor.
  • in case 2, we gain 32 Bytes per miner in the actor, but lost 32 Bytes per block.

Case 1 seems preferable to me. Either way Secp is a bit better than BLS in terms of size…

Assuming I haven't said mistakes here @nikkolasg @whyrusleeping, I think we're ready to make a call...

@sternhenri
Copy link
Contributor Author

sternhenri commented Jul 24, 2019

Last missing piece is hashing the VDFOutput into a usable source of randomness (colloquially known as fetching the ticket). We defer to #332 for defining the function which fetches randomness (hash fn used is blake2)

@sternhenri sternhenri changed the base branch from master to feature/weight_punishment July 25, 2019 16:54
@sternhenri sternhenri changed the base branch from feature/weight_punishment to master July 25, 2019 16:54
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed with @henri

switch to bls will happen in a follow up pr

@dignifiedquire dignifiedquire merged commit ca3f93b into master Jul 26, 2019
@nicola nicola deleted the feat/ec-sig-spec branch November 29, 2019 02:09
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.

6 participants