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

Support changing stakers at runtime #2567

Merged
merged 11 commits into from Jun 1, 2019

Conversation

@bkase
Copy link
Contributor

commented May 30, 2019

We always start proposing now and with a set of keypairs. If the set is
empty it's effectively the same thing as not proposing. Proposing is not
interrupted by a change in staking keypairs, but it is woken up by this
change. If we are in the middle of staking with different keypairs, that
action is finished before the new keys kick in.

Local_state is now a table from public keys to delegation info and we
check vrfs for all the keypairs that we wish to stake.

I've included a handler in Ledger.ml as it may be useful later even
though I ended up not using it in this PR.

Tests pass, thorough testing of swapping proposers is todo

@bkase bkase force-pushed the runtime-dynamic-stake-changing branch 2 times, most recently from 474663e to bfaae27 May 30, 2019

Support changing stakers at runtime
We always start proposing now and with a set of keypairs. If the set is
empty it's effectively the same thing as not proposing. Proposing is not
interrupted by a change in staking keypairs, but it is woken up by this
change. If we are in the middle of staking with different keypairs, that
action is finished before the new keys kick in.

Local_state is now a table from public keys to delegation info and we
check vrfs for all the keypairs that we wish to stake.

I've included a handler in Ledger.ml as it may be useful later even
though I ended up not using it in this PR.

Haven't tested thoroughly yet.

@bkase bkase force-pushed the runtime-dynamic-stake-changing branch from bfaae27 to 97aead8 May 30, 2019

@johnwu93
Copy link
Contributor

left a comment

This looks good to me. I didn't take a careful look at the consensus code though.

@@ -784,8 +789,16 @@ module Make (Inputs : Inputs_intf) = struct
(Linear_pipe.iter (Snark_pool.broadcasts snark_pool) ~f:(fun x ->
Net.broadcast_snark_pool_diff net x ;
Deferred.unit )) ;
let proposer_kps_r, proposer_kps_w =
Strict_pipe.(

This comment has been minimized.

Copy link
@johnwu93

johnwu93 May 30, 2019

Contributor

Why can't this be a broadcast_pipe or an incremental variable? I think it would make the semantics of updating the proposer keys easier to read. Why does this have to be async?

This comment has been minimized.

Copy link
@nholland94

nholland94 May 30, 2019

Contributor

I don't know if I agree that this should be a broadcast_pipe as there would only be a single consumer at any point in time...

This comment has been minimized.

Copy link
@bkase

bkase May 30, 2019

Author Contributor

I wanted the proposer keys to live in the proposer and not (also) coda_lib. The nice thing about the pipes here is that it gets drained.

This comment has been minimized.

Copy link
@johnwu93

johnwu93 May 30, 2019

Contributor

@nholland94 . Oh, to be more specific, a synchronous pipe with a cache on top of it.

( Keypair.Set.to_list keypairs
|> List.map ~f:(fun kp ->
Public_key.compress kp.Keypair.public_key )
|> Public_key.Compressed.Set.of_list ) ;

This comment has been minimized.

Copy link
@bkase

bkase May 30, 2019

Author Contributor

reset to current slot instead of zero zero

This comment has been minimized.

Copy link
@bkase

bkase May 30, 2019

Author Contributor

keep track of current slots

* for a long while.
* *)
Agent.with_value keypairs_agent ~f:(fun _new_keypairs ->
Singleton_scheduler.schedule scheduler (Time.now time_controller)

This comment has been minimized.

Copy link
@bkase

bkase May 30, 2019

Author Contributor

cancel_and_do_now keep the same check for now,

  1. TODO: Don't reuse vrfs
  2. TODO: Don't take the first block
Show resolved Hide resolved src/lib/coda_lib/coda_lib.ml Outdated
@@ -784,8 +789,16 @@ module Make (Inputs : Inputs_intf) = struct
(Linear_pipe.iter (Snark_pool.broadcasts snark_pool) ~f:(fun x ->
Net.broadcast_snark_pool_diff net x ;
Deferred.unit )) ;
let proposer_kps_r, proposer_kps_w =
Strict_pipe.(

This comment has been minimized.

Copy link
@nholland94

nholland94 May 30, 2019

Contributor

I don't know if I agree that this should be a broadcast_pipe as there would only be a single consumer at any point in time...

Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml Outdated
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml Outdated
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml Outdated

@bkase bkase force-pushed the runtime-dynamic-stake-changing branch from f67b61f to 66ee3e3 May 30, 2019

Requested changes
1. Precompute compressed public keys via Keypair.And_compressed_pk.Set.t
2. Move Agent to Otp_lib and make it backed by a ref
3. Fix N+1 query problem on delegatees
4. Store unique epoch/slot pairs for each pk we're tracking and set
intersect with the new keys

@bkase bkase force-pushed the runtime-dynamic-stake-changing branch from 66ee3e3 to cc3cb5f May 30, 2019

Show resolved Hide resolved src/lib/coda_lib/coda_lib.ml
@@ -331,7 +329,8 @@ module Data = struct
type t =
{ mutable last_epoch_snapshot: Snapshot.t
; mutable curr_epoch_snapshot: Snapshot.t
; mutable last_checked_slot_and_epoch: Epoch.t * Epoch.Slot.t
; last_checked_slot_and_epoch:
(Epoch.t * Epoch.Slot.t) Public_key.Compressed.Table.t

This comment has been minimized.

Copy link
@nholland94

nholland94 May 31, 2019

Contributor

Doesn't it make more sense to track this in the delegator table? Then when you update keypairs, you would take your old table, remove the keys that are no longer there, and process the new keys with the epoch/slot set to current time.

This comment has been minimized.

Copy link
@bkase

bkase May 31, 2019

Author Contributor

Hmm there isn't really one delegator table. There are ones for each snapshot -- so it's not clear exactly where this should be stored. I can (and will) derive the proposer_public_keys field from the keys of this table however (as that will deal with filtering out the ones that aren't in the ledger also).

Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml Outdated
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml
Show resolved Hide resolved src/lib/consensus/proof_of_stake.ml
Show resolved Hide resolved src/lib/otp_lib/agent.mli Outdated
Show resolved Hide resolved src/lib/proposer/proposer.ml Outdated

bkase added some commits May 31, 2019

Off-by-one error in sparse_ledger.iteri
Plus quickcheck test for confirmation
@nholland94
Copy link
Contributor

left a comment

Looks good to go besides one type that is to general.

@@ -92,7 +92,7 @@ module type Proposer_intf = sig
-> completed_work_checked option)
-> transaction_pool:transaction_pool
-> time_controller:Block_time.Controller.t
-> keypairs:Keypair.And_compressed_pk.Set.t Agent.Read_only.t
-> keypairs:(_, Keypair.And_compressed_pk.Set.t) Agent.t

This comment has been minimized.

Copy link
@nholland94

nholland94 May 31, 2019

Contributor

Doesn't this need to be Agent.read_only Agent.flag?

@bkase

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

I'm not moving the delegators out of the Snapshot in this PR as it's too big of a control flow change at the last minute. See #2582 for note on this.

@bkase bkase added the ready-to-merge label Jun 1, 2019

@mergify mergify bot merged commit a911dff into master Jun 1, 2019

20 of 21 checks passed

ci/circleci: test--test_postake_holy_grail CircleCI is running your tests
Details
Summary 1 rule matches
Details
ci/circleci: build-artifacts--testnet_postake Your tests passed on CircleCI!
Details
ci/circleci: build-artifacts--testnet_postake_many_proposers Your tests passed on CircleCI!
Details
ci/circleci: build-artifacts--testnet_postake_snarkless_fake_hash Your tests passed on CircleCI!
Details
ci/circleci: build-wallet Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test--fake_hash Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_bootstrap Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_catchup Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_delegation Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_txns Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_txns Your tests passed on CircleCI!
Details
ci/circleci: test-unit--dev Your tests passed on CircleCI!
Details
ci/circleci: test-unit--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@mergify mergify bot deleted the runtime-dynamic-stake-changing branch Jun 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.