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

Delegated mining #1011

Merged
merged 9 commits into from
Feb 2, 2022
Merged

Delegated mining #1011

merged 9 commits into from
Feb 2, 2022

Conversation

area
Copy link
Member

@area area commented Jan 12, 2022

Closes #1002

Draft status, as not ready to merge, but after feedback.

Implements what Daniel and I vaguely talked about a while ago. Basically, everything stays the same, but instead of using msg.sender during mining, we instead call a function that either returns msg.sender (i.e. the fallback case should be the existing functionality) or, if a delegation has been awarded to msg.sender, returns the address they are allowed to mine on behalf of.

@area area marked this pull request as draft January 12, 2022 15:50
Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

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

Interesting, when we discussed this earlier it seemed like we'd need two mappings, but you've done it with one -- using the miner address to look up the delegator during the mining process, and requiring the delegator to pass the mining address explicitly while setting.

@area area force-pushed the feat/delegated-mining branch 8 times, most recently from 905732c to f217f97 Compare January 14, 2022 17:32
@area
Copy link
Member Author

area commented Jan 14, 2022

So I've added restrictions that mean that anyone doing any sort of mining must either have tokens staked, or be a delegate miner for another address. If they have both, the former takes priority. This is to avoid the possibility that existed in the original implementation of 'stealing' miners by naming them as your delegate. In this implementation, you still can, but it has no effect (while they have tokens staked). Previously, confirmNewHash could be called by anyone, but now needs to be called by someone with tokens staked.

There were a good number of knock-on effects in the tests, unfortunately, but I think broadly speaking this is better. I don't think it presents a risk of getting in to issues either.

@area area force-pushed the feat/delegated-mining branch 2 times, most recently from 2b7570c to 2ddf7db Compare January 17, 2022 14:51
@area area force-pushed the feat/delegated-mining branch from 2ddf7db to 586308a Compare January 17, 2022 15:31
@kronosapiens
Copy link
Member

What was the reason for restricting confirmNewHash to staked miners?

@area
Copy link
Member Author

area commented Jan 20, 2022

What was the reason for restricting confirmNewHash to staked miners?

So it originally came up because we call getMinerAddress in responsePossible, and the changes I made to getMinerAddress to deal with the griefing angle. I could have solved it by only calling getMinerAddress if we're inside the SUBMITTER_ONLY_WINDOW_DURATION, but realised it makes the mining cycle as a whole slightly more resilient. Picture a scenario where only one person has tokens staked.

  1. This person unstakes their tokens
  2. Someone calls confirmNewHash
  3. The mining cycle is now broken. No-one has had tokens staked since before this cycle started, and so cannot submit the next hash.

By requiring this function call to be made by someone with tokens staked, we guarantee at least one person is able to continue the mining cycle. If the only person with tokens staked is malicious, then they're still able to break things (by unstaking after calling confirmNewHash), but we are now at least not able to get in to such a situation by everyone behaving non-maliciously, which was the case before (both steps 1 and 2 could be done by well-behaving participants).

@area area marked this pull request as ready for review January 31, 2022 16:17
kronosapiens
kronosapiens previously approved these changes Feb 1, 2022
packages/reputation-miner/ReputationMiner.js Outdated Show resolved Hide resolved
test/reputation-system/happy-paths.js Show resolved Hide resolved
@kronosapiens kronosapiens merged commit e6b4f92 into master Feb 2, 2022
@kronosapiens kronosapiens deleted the feat/delegated-mining branch February 2, 2022 18:12
@area area mentioned this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants