Skip to content

Separate worker role from miner/staker#949

Merged
cygnusv merged 13 commits intonucypher:alhambra-verdefrom
vzotova:worker
May 29, 2019
Merged

Separate worker role from miner/staker#949
cygnusv merged 13 commits intonucypher:alhambra-verdefrom
vzotova:worker

Conversation

@vzotova
Copy link
Member

@vzotova vzotova commented Apr 26, 2019

Before this PR a staker can't be an intermediary contract (such as User Escrow) because contract can't sign anything (and we can't slash it).
Now miner/staker can set worker who can confirm activity and doesn't matter how many intermediary contracts between worker and MinersEscrow.

Any suggestions on renaming "worker" are welcome.

MinersEscrow API cahnges:

  • lock method no longer includes mint and confirmActivity methods
  • setWorker(address) method - necessary to set worker before confirm activity
  • getWorkerByMiner(address)
  • getMinerByWorker(address)
  • Only worker can call confirmActivity()

Also MiningAdjudicator uses getMinerByWorker(address) to get miner for slashing

@vzotova vzotova added ETH Contracts Ursula 👩‍🚀 Effects the "Ursula" development area labels Apr 26, 2019
@vzotova vzotova self-assigned this Apr 26, 2019
@codecov
Copy link

codecov bot commented Apr 28, 2019

Codecov Report

Merging #949 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
+ Coverage   84.98%   85.05%   +0.07%     
==========================================
  Files          65       65              
  Lines        7908     7925      +17     
==========================================
+ Hits         6721     6741      +20     
+ Misses       1187     1184       -3
Impacted Files Coverage Δ
nucypher/blockchain/economics.py 100% <100%> (ø) ⬆️
nucypher/blockchain/eth/actors.py 76.06% <100%> (+0.34%) ⬆️
nucypher/blockchain/eth/agents.py 91.03% <100%> (+1.37%) ⬆️
nucypher/cli/characters/ursula.py 75.4% <100%> (+0.13%) ⬆️
nucypher/utilities/sandbox/ursula.py 98.27% <100%> (+0.06%) ⬆️
nucypher/utilities/sandbox/constants.py 98.03% <100%> (ø) ⬆️

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 f558e2f...ea3da24. Read the comment docs.

@vzotova vzotova changed the title Separate key holder/worker role from miner/staker [WIP] Separate key holder/worker role from miner/staker Apr 30, 2019
@vzotova vzotova mentioned this pull request May 10, 2019
@vzotova vzotova changed the title [WIP] Separate key holder/worker role from miner/staker Separate key holder/worker role from miner/staker May 17, 2019
@vzotova vzotova marked this pull request as ready for review May 17, 2019 16:46
self.minimum_allowed_locked, # Min amount of tokens that can be locked
self.maximum_allowed_locked # Max amount of tokens that can be locked
self.maximum_allowed_locked, # Max amount of tokens that can be locked
self.minimum_worker_periods # Min amount of periods while a worker can't be changed
Copy link
Member

Choose a reason for hiding this comment

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

"Minimum number of periods a contract can be an intermediary "

Copy link
Member Author

Choose a reason for hiding this comment

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

Did not get what do you mean,
this value to prevent too often changing of worker, but staker is always an intermediary contract or real address

@vzotova vzotova changed the title Separate key holder/worker role from miner/staker Separate worker role from miner/staker May 26, 2019
seconds_per_period = hours_per_period * 60 * 60 # Seconds in single period

# Time Constraints
minimum_worker_periods = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Though it seems that this is a pretty minimal attack surface, the "unknown-unknowns" here are really disturbing to me. @szotov tells me a worker history is the solution, but it has some downsides. I really want to think more about this and what effects it can have; I think we don't completely understand all the effects this can have on the network.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened an issue on this -- #1013

@KPrasch KPrasch changed the base branch from master to alhambra-verde May 27, 2019 13:09
@KPrasch KPrasch mentioned this pull request May 29, 2019
@cygnusv cygnusv merged commit 0549edb into nucypher:alhambra-verde May 29, 2019
cygnusv added a commit to cygnusv/nucypher that referenced this pull request Jun 4, 2019
Separate worker role from miner/staker
@vzotova vzotova deleted the worker branch April 28, 2020 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ursula 👩‍🚀 Effects the "Ursula" development area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants