Fully Backed Sortition Pools tweaks #89
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removed interffaces that were defined for BondedSortitionPool and SortitionPool. The interfaces are not being used anywhere and they are not up-to-date. In keep-ecdsa we are using the contracts directly.
The pool was updating the minimum bondable value for each selectSetGroup call. The idea was to automatically remove from the pool operators that are no longer able to satisfy application needs about the minimum available bondable value and prevent operators with too low value griefing signer selection. We now have two minimum bond values: one defining the minimum unbonded value the operator needs to have so that it can join and stay in the pool, and another defining the required bond value for the current selection. If the given operator has enough unbonded value to stay in the pool but it does not have enough unbonded value for the current group selection, it is skipped. We assume a reasonable minimum bondable value is set on the pool creation and it can be later updated by the pool owner (application). We move the responsibility of keeping this value sane to the application. If the application defines allowed lot sizes, just like tBTC, the application may set the minimum bondable value to the minimum lot size and later skip operators that are not eligible to satisfy bond for the current selection instead of removing them from the pool. All operators without at least the minimum unbonded value are removed from the pool during the group selection. The change was originally implememnted to BondedSortitionPool in 46d187a.
We created a dedicated interface for the fully backed bonding as there will be specific rules to verify for the group selection. For regular BondedSortitionPool we had a staking contract reference holding informations like eligibleStake, here we don't use staking contract but all required informations are received from bonding contract.
When a bonding relationship gets delegated in the bonding contract it has to pass some initialization period. In getEligibleWeight that is invoked on operator registration in the pool we are checking if the initialization period has passed. If not we're not letting the operator to register in the pool.
Updated BondingContractStub to implement IBonding interface. Added stub for FullyBackedBonding that implements IFullyBackedBonding interface and extends BondingContractStub.
fullyBackedFactoryTest.js: - use correct bonding contract - added awaits - set operator initialization in bonding stub fullyBackedSortitionPoolTest.js: - use correct bonding contract - added awaits - set operator initialization in bonding stub - use bn-chai to verify BN values - use bond value instead of weighted value for operator preparation - get operator initialiation period from contract and use it for blocks mining - updated tests for group selection - added tests for minimum bondable value configuration - added tests for joinPool (getEligibleWeight).
2 tasks
eth-r
previously approved these changes
Sep 22, 2020
eth-r
previously approved these changes
Oct 13, 2020
Because operators are selected randomly, there was a 1/4 possibility that Carol might not be picked, causing the check for the removal of ineligible operators to fail. By increasing Carol's initial bond to 100x that of the others, the probability of Carol being skipped becomes roughly 1 in a million.
BondingContractStub doesn't implement initialization status functions. This caused the work selection distribution tests to fail. Switch to FullyBackedBondingStub and set initialization status when preparing operators.
eth-r
approved these changes
Oct 13, 2020
pdyraga
approved these changes
Oct 27, 2020
| @@ -118,13 +147,23 @@ contract FullyBackedSortitionPool is AbstractSortitionPool { | |||
| ); | |||
|
|
|||
| // Don't query stake if bond is insufficient. | |||
There was a problem hiding this comment.
This comment is not correct, it should say "don't check initialization if..."
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We removed interfaces for SortitionPools that were not implemented by the pools, so they were outdated. The interfaces are not being used in keep-ecdsa anyways as we're using the implementation of the contract.
This PR contains also couple of tweaks to Fully Backed Sortition Pools:
isInitializedfunction to check if the operator delegation in a bonding contract has passed initialization period,Depends on #87
Depends on #97
Closes #86
Refs keep-network/keep-ecdsa#483