Touches to StakingEscrow.sample() by cygnusv · Pull Request #1280 · nucypher/nucypher

cygnusv · 2019-08-28T19:04:05Z

Corrects a minor bug in StakingEscrow.sample() that was affecting sampling (Fixes #1090)

Also adds a test that checks that sampling produces an acceptable distribution (Closes #1019)

michwill · 2019-09-01T17:22:48Z

nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol

-            }
-            if (sumOfLockedTokens > point) {
+            uint256 stakerTokens = getLockedTokens(info, currentPeriod, nextPeriod);
+            if (sumOfLockedTokens + stakerTokens > point) {


Need to think if we need also sumOfLockedTokens <= point, otherwise all good

michwill

I think, it's almost off-WIP!

codecov · 2019-09-02T21:38:28Z

Codecov Report

Merging #1280 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1280      +/-   ##
==========================================
+ Coverage   82.07%   82.36%   +0.29%     
==========================================
  Files          72       72              
  Lines        9959     9948      -11     
==========================================
+ Hits         8174     8194      +20     
+ Misses       1785     1754      -31

Impacted Files	Coverage Δ
nucypher/blockchain/eth/agents.py	`90.22% <100%> (+5.41%)`	⬆️
nucypher/policy/policies.py	`85.57% <100%> (-0.24%)`	⬇️
nucypher/blockchain/eth/token.py	`87.95% <0%> (-1.62%)`	⬇️
nucypher/blockchain/eth/actors.py	`83.41% <0%> (-1.42%)`	⬇️
nucypher/crypto/powers.py	`94.26% <0%> (-0.32%)`	⬇️
nucypher/blockchain/eth/interfaces.py	`71.98% <0%> (ø)`	⬆️
nucypher/config/storages.py	`77.31% <0%> (ø)`	⬆️
nucypher/network/nodes.py	`79.5% <0%> (+0.27%)`	⬆️
nucypher/characters/base.py	`86.8% <0%> (+0.36%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eda82cb...27d1109. Read the comment docs.

nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol

derekpierre · 2019-09-03T13:35:22Z

nucypher/blockchain/eth/agents.py

-            raise self.NotEnoughStakers(f'There are {stakers_population} published stakers, need a total of {quantity}.')
+        n_select = math.ceil(quantity * additional_ursulas)  # Select more Ursulas
+        if n_select > stakers_population:
+            raise self.NotEnoughStakers(f'There are {stakers_population} active stakers, need at least {n_select}.')


Should we clarify that we need a sample that may be larger than the quantity specified. As a user I may be confused that I get a NotEnoughStakers exception if I specify n = 6 and there are 6 active stakers that randomly says some value eg. need at least 9 (assuming additional_ursulas is 1.5)

Perhaps our message could be:

Suggested change

raise self.NotEnoughStakers(f'There are {stakers_population} active stakers, need at least {n_select}.')

raise self.NotEnoughStakers(f'There are {stakers_population} active stakers - for {quantity} stakers we need a sample size of at least {n_select}.')

Yes, I think that's a good suggestion. Thanks @derekpierre !

Does additional_ursulas still make sense as a static value? For such small m of n values, a proportional, smaller value might be more usable.

@derekpierre sorry I didn't include this in #1280. I'll do that in #1259.
@KPrasch Although in the method signature there's a static default, in reality it's always called explicitly with the BlockchainPolicy.selection_buffer value as defined in BlockchainPolicy.sample_essential(). We however don't configure BlockchainPolicy.selection_buffer yet, so for the moment it always uses the default value of 1.5. That's why #1061 is still open.

derekpierre · 2019-09-03T13:37:15Z

nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol

+    * For example:
+    *
+    * Stakes: |----- S0 ----|--------- S1 ---------|-- S2 --|---- S3 ---|-S4-|----- S5 -----|
+    * Points: ....R0.......................R1..................R2...............R3...........


tuxxy

Great additions, especially the neat test. 🐧

tuxxy · 2019-09-03T14:53:07Z

nucypher/policy/policies.py

-            handpicked_ursulas = set()
-        else:
-            handpicked_ursulas = set(handpicked_ursulas)
+        selected_ursulas = set(handpicked_ursulas) if handpicked_ursulas else set()


I'm not a fan of these little one-liners, but this is readable enough. :)

tuxxy · 2019-09-03T15:08:02Z

nucypher/policy/policies.py

+        if target_sample_quantity > 0:
+            sampled_ursulas = self.sample_essential(quantity=target_sample_quantity,
+                                                    handpicked_ursulas=handpicked_ursulas)
+            selected_ursulas.update(sampled_ursulas)


KPrasch · 2019-09-03T21:03:30Z

tests/blockchain/eth/contracts/main/staking_escrow/test_sampling.py

        escrow.functions.sample([], 1).call()
-    with pytest.raises((TransactionFailed, ValueError)):


Just a note - By removing the handling of ValueError, the test suite is less compatible with other backends, like geth, since the exception is EthereumTester-specific. Unsure if this is a reasonable goal for us, but I'd like to remain open to the possibility of running the test suite without eth-tester.

Oh so that's why! I always found weird those lines. In any case, if that's the reason, isn't it better that we have something like:

TransactionException = TransactionFailed if using_eth_tester() else ValueError ... with pytest.raises(TransactionException): foo.bar()

Otherwise, there may be the case that in our current setting the method being tested is currently raising a legitimateValueError not caused by a failed transaction and we're accepting that as valid.

KPrasch · 2019-09-03T21:17:38Z

tests/blockchain/eth/contracts/main/staking_escrow/test_sampling_distribution.py

+    SAMPLES = 100
+    quantity = 3
+    import random
+    from collections import Counter


Wondeful 👍

KPrasch · 2019-09-03T21:21:16Z

nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol

+            uint256 nextSumValue = sumOfLockedTokens.add(stakerTokens);
+
+            uint256 point = _points[pointIndex];
+            require(point >= previousPoint);  // _points must be a sorted array


KPrasch · 2019-09-03T21:44:19Z

I've reviewed this changeset, however I'd like to go through it with @szotov before giving approval.

vzotova

I like those changes and tests!
from logic point of view everything is great,
from optimization part - for big stakers (multiple point in one staker) sample in contract will call getLockedTokens multiple times, so method will work sligtly slower, but off corse it's not a problem for external call function

vzotova · 2019-09-04T09:41:03Z

nucypher/policy/policies.py

+        selected_ursulas = set(handpicked_ursulas) if handpicked_ursulas else set()

        # Calculate the target sample quantity
-        ADDITIONAL_URSULAS = self.selection_buffer


Just to clarify - no more selection buffer for federated policy, right?

michwill reviewed Sep 1, 2019

View reviewed changes

cygnusv force-pushed the sample branch from c910ff5 to 18238c1 Compare September 2, 2019 17:20

cygnusv added 4 commits September 2, 2019 19:22

Wiggle room for sampling is created in just one place

21a7a34

Correct and simplify sampling algorithm

25b3908

Adapt sampling tests to new changes

4bb83cf

Sets N=6 in test_blockchain_ursulas_reencrypt. Closes nucypher#1090.

b75d688

cygnusv force-pushed the sample branch from 18238c1 to b75d688 Compare September 2, 2019 17:23

cygnusv changed the title ~~[WIP] Touches to StakingEscrow.sample()~~ Touches to StakingEscrow.sample() Sep 2, 2019

cygnusv marked this pull request as ready for review September 2, 2019 18:37

cygnusv requested review from KPrasch, derekpierre, jMyles, michwill, tuxxy, vepkenez and vzotova September 2, 2019 18:38

cygnusv mentioned this pull request Sep 2, 2019

Secondary CI workflows #1288

Closed

Test that sampling distribution is as expected. Closes nucypher#1019

27d1109

michwill reviewed Sep 3, 2019

View reviewed changes

nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol Show resolved Hide resolved

michwill approved these changes Sep 3, 2019

View reviewed changes

derekpierre approved these changes Sep 3, 2019

View reviewed changes

tuxxy approved these changes Sep 3, 2019

View reviewed changes

KPrasch reviewed Sep 3, 2019

View reviewed changes

vepkenez approved these changes Sep 3, 2019

View reviewed changes

vepkenez merged commit cae5247 into nucypher:master Sep 3, 2019

vzotova reviewed Sep 4, 2019

View reviewed changes

	raise self.NotEnoughStakers(f'There are {stakers_population} active stakers, need at least {n_select}.')
	raise self.NotEnoughStakers(f'There are {stakers_population} active stakers - for {quantity} stakers we need a sample size of at least {n_select}.')

		escrow.functions.sample([], 1).call()
		with pytest.raises((TransactionFailed, ValueError)):

Conversation

cygnusv commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michwill left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuxxy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KPrasch commented Sep 3, 2019

Uh oh!

vzotova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cygnusv commented Aug 28, 2019 •

edited

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading