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

Feature/use delegate vrf evals #1148

Merged
merged 24 commits into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@imeckler
Contributor

imeckler commented Nov 14, 2018

This PR does 2 things.

  1. It makes the proof of stake module use the VRF evaluations of ones' delegates. So, if one has more stake delegated to oneself, they will actually win more often now.
  2. It makes it so that the witness data needed for the proof of stake snark (i.e., the sparse ledger containing the account of the winning delegator and the proposer's private key) is actually passed to the prover, who for now just ignores it.

@imeckler imeckler requested a review from nholland94 Nov 14, 2018

@imeckler imeckler changed the base branch from master to feature/track-delegates Nov 14, 2018

@imeckler imeckler changed the base branch from feature/track-delegates to feature/track-delegates-on-master Nov 14, 2018

@imeckler imeckler changed the base branch from feature/track-delegates-on-master to master Nov 14, 2018

@nholland94

I'm a little bit confused about the Prover_state stuff. Perhaps I'm missing something, but it looks like you define Prover_state for proof of stake to be rather large, and then stuff it into Snark_transition, which is the layer of Internal_transition which is used as an input into the snark. How is prover_state even used on Snark_transition?

include Inputs
type ('blockchain_state, 'consensus_data, 'sok_digest, 'supply_increase) t =
{ blockchain_state: 'blockchain_state
; consensus_data: 'consensus_data
; sok_digest: 'sok_digest
; supply_increase: 'supply_increase
; prover_state: Prover_state.t

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

Should prover_state really be an input to the snark?

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

we can talk about this when you get in -- basically I think snark transition is wrong and it should maybe all be prover state and have no Typ here etc, but I didn't want to unwind that right now.

let previous_consensus_state =
Protocol_state.consensus_state previous_protocol_state
in
(* TODO: sign protocol_state instead of blockchain_state *)
let consensus_transition_data =
Consensus_transition_data.create_value
~private_key:keypair.Signature_lib.Keypair.private_key blockchain_state
Consensus_transition_data.create_value ~private_key:proposal_data

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

Why is it preferable to put values from keypair into proposal_data from next_proposal rather than just providing the keypair to generate_transition?

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

just to eliminate arguments -- I think it makes sense conceptually too since generically there's no reason a consensus mechanism should need a key pair (consider eg PoW)

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

PoW doesn't need a keypair for next_proposal either, but I get your point.

@@ -1102,47 +1122,47 @@ module Make (Inputs : Inputs_intf) :
failwith
"System time is out of sync. (hint: setup NTP if you haven't)" )
in
let%bind proposer_stake, total_stake =
let%bind _proposer_stake, total_stake =

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

If you aren't going to use the stake you query in Epoch_ledger.get_stake, you should split that into two functions or something. This is doing a lookup in a ledger that is immediately thrown away.

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

sounds good -- I will do so

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

done

{ delegator: Account.Index.t
; ledger: Sparse_ledger.t
; private_key: Signature_lib.Private_key.t }
[@@deriving bin_io, sexp]

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

Is Stake_proof actually a good name for this?

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

I'm not sure -- I was thinking Proof_of_stake but I wasn't sure about the name collision. could also be Proof_of_stake_witness or something

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

Maybe Proof_of_slot_ownership or some derivative like that?

Also, I just noticed the [@@deriving bin_io]... does this need to have bin_io? I'm a little afraid of accidentally leaking the private key somewhere. Seems like this object should be explicitly not binable.

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

hmm -- the private key already has bin_io on it, but maybe it's more obvious that you shouldn't directly bin_io that?

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

this has bin_io because we need to send it to the prover

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

Ok, well I'm fine with it for now, but we should probably log an issue to not send a private key to the prover over RPC. It's very insecure.

@@ -120,13 +137,15 @@ module Make (Inputs : Inputs_intf) :
; consensus_data
; sok_digest
; supply_increase
; prover_state

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

Should prover_state really just be passed through in the typ like this? Shouldn't prover_state define it's own typ?

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

no, this data is not going to be used thru a typ directly, but rather used by a handler. the way it is organized right now is bad, but we can discuss a potential better way when you are in

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

Probably not a good name actually as we'll have a version without the private key when we send around vrf evaluations later

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

Actually I'm going to start the restructuring now as the current way is just so messy

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

ok, done

@nholland94

Your changes look good, but there's one more issue I noticed to address and then I will approve.

let open Local_state in
let open Option.Let_syntax in
let%bind ledger =
if Coda_base.Frozen_ledger_hash.equal hash genesis_ledger_hash then

This comment has been minimized.

@nholland94

nholland94 Nov 14, 2018

Contributor

This check for which ledger to access is missing now, isn't it? Wherever we grab the ledger for looking up vrf evaluations will need to handle this genesis ledger case. It should also probably assert the hash equality in this else branch as well.

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

oo good call -- I will fix now

This comment has been minimized.

@imeckler

imeckler Nov 14, 2018

Contributor

ok think I've done it

@nholland94

Looks pretty good, so I'm approving. Just two more optional asks before you merge:

  1. The old code used to ensure that the epoch ledger we check for vrfs has the hash stored in the consensus state. It would be nice if you could add a debug assert that still ensures this is true when evaluating the vrf.
  2. Check my comment about the binable private key.

bkase and others added some commits Nov 14, 2018

@imeckler imeckler merged commit 3b83a7c into master Nov 15, 2018

8 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_public Your tests passed on CircleCI!
Details
ci/circleci: build_withsnark Your tests passed on CircleCI!
Details
ci/circleci: test-all_sig_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-all_stake_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-unit-test Your tests passed on CircleCI!
Details
ci/circleci: test-withsnark Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@imeckler imeckler deleted the feature/use-delegate-vrf-evals branch Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment