Stagger allowed miner responses during a dispute #842

area · 2020-05-07T16:17:11Z

First off, this is (notionally) based on #812 hence draft status for now.

We obviously hope that multiple people will run reputation mining clients. For submissions, we have a window over which it gets easier and easier to submit new hashes, with the difficulty being randomised per user, and with it being easier to submit if you have more staked. We did this because we wanted rewards to be 'fairly' distributed and for miners to not be wasting gas, making submissions that wouldn't succeed because all the slots filled up before their transaction was mined (which wasn't the case when they broadcast it). This PR adds similar functionality to dispute resolutions.

In this implementation, there is a window during a dispute resolution where only those that have submitted a hash are able to respond to a challenge, and the point in this window where a submitter is able to respond is randomised per-submitter for each stage. This additional window lasts 10 minutes, so in the event of a 'full' mining cycle submission, each submitter can expect to get (waves hands vaguely) a minute after they are eligible to respond to a dispute before someone else becomes eligible, which seems adequate. This increases the potential severity of a DDOS attack, because each dispute round can take twice as long to resolve if bad submissions are defended judiciously, but the well-behaving miners shouldn't step on each others toes during submissions, which is well worth the cost.

There's one subtlety in the implementation worth drawing attention to; in the case of all but one of these windows, the window is based on the last response timestamp of the submission being defended. The one case that's not true is the invalidateHash call where an entry is being invalidated (i.e. not the case where a bye is awarded), where the window is based on the last response timestamp of the submission being invalided.

area · 2020-06-04T15:41:37Z

Further comments as this hopefully nears being done:

ReputationMiningCycleBinarySearch.sol and ReputationMiningCycleCommon.sol have been introduced as the contract sizes were getting too large.

lastResponseTimestamp is now set on submission to the time the window closes. This is fairer (even-numbered submissions now pay the same gas) and also makes working out when a valid window is easier in the case of the last submission being able to get a bye in the first round (when previously, that timestamp wouldn't be set).

I have added a BinarySearchStep event - every other stage of a mining dispute had a corresponding event.

I have added a helper function makeTxAtTimestamp which does what it says on the tin. It stops mining, sends a transaction, and then mines a block at the specified timestamp before re-enabling mining. This helps massively in making tests reproduceable (and was necessary here) for the main test added (should not allow miners to respond immediately during a dispute, but they should be able to some time before the end of the window). It arguably should be used instead of forwardTime wherever possible, but that is much more of a maintenance PR than something that should be here I think.

kronosapiens · 2020-08-11T01:14:30Z

contracts/reputationMiningCycle/ReputationMiningCycleBinarySearch.sol

+
+contract ReputationMiningCycleBinarySearch is ReputationMiningCycleCommon {
+  function respondToBinarySearchForChallenge(
+    uint256 round,


Are we not using the underscore convention in these contracts? We use them in the interface contract...

It's barely used in the interface contract either for what it's worth, but I guess it's best to be consistent across all contracts.

kronosapiens · 2020-08-11T02:19:08Z

contracts/reputationMiningCycle/ReputationMiningCycleCommon.sol

+      return false;
+    }
+
+    uint256 delta = now - since;


Reading through this and I'm still getting confused around which way the window works. One one level it seems like it should be >=, i.e. more restrictive when the delta is large, but we have the opposite. Can we rename delta to responseTimeRemaining or something? since could also be clearer.

Rearranged this a bit, that hopefully makes it clearer to the reader.

kronosapiens · 2020-08-11T02:32:04Z

packages/reputation-miner/ReputationMinerClient.js

@@ -367,30 +377,44 @@ class ReputationMinerClient {

        // If we're here, we do have an opponent.
        // Has our opponent timed out?
+        // TODO: Remove these magic numbers


Does the client not have a constants.js file?

It does not. We could add one, ideally though we'd want these values being picked up off the contracts, which I think is what I had in mind when I wrote this comment.

kronosapiens · 2020-08-11T02:37:37Z

test/contracts-network/reputation-basic-functionality.js

+      await forwardTime(MINING_CYCLE_DURATION, this);
+
+      let nSubmissionsForHash = await repCycle.getNSubmissionsForHash("0x12345678", 10, "0x00");
+      expect(nSubmissionsForHash).to.eq.BN(0);


to.be.zero

kronosapiens · 2020-08-11T02:42:17Z

test/reputation-system/reputation-mining-client/client-auto-functionality.js

+          const noChallengeCompleted = noEventSeen(repCycleEthers, "ChallengeCompleted");
+          const noHashInvalidated = noEventSeen(repCycleEthers, "HashInvalidated");
+
+          await noChallengeCompleted;


Remind me why we don't just await these promises directly?

Because it's testing the auto functionality. If we awaited directly where they're created, the actions to trigger them have not yet taken place on chain so the test would time out. If we moved the creation to where they're awaited, then the tests become spotty, as the thing the promises are waiting for might have happened before they've been set up.

kronosapiens · 2020-08-11T02:42:39Z

test/reputation-system/root-hash-submissions.js

+
+      await goodClient.submitRootHash();
+      let reward = await repCycle.getDisputeRewardSize();
+      expect(reward).to.eq.BN("0");


to.be.zero

kronosapiens · 2020-08-18T22:11:46Z

contracts/reputationMiningCycle/ReputationMiningCycleCommon.sol

      // require user made a submission
      if (reputationHashSubmissions[msg.sender].proposedNewRootHash == bytes32(0x00)) {
        return false;
      }
-      uint256 target = delta * Y;
+      uint256 windowOpenFor = now - _responseWindowOpened;


If you want to make the most of this variable I'd be happy using it up in the conditional as

if (windowOpenFor <= SUBMITTER_ONLY_WINDOW_DURATION) {

Even if there's only one

How did we ever write bulletproof tests without makeTxAtTimestamp?

This brings them in line with most of the rest of the codebase i.e. prefixed with _

kronosapiens

Nice! This has been in the works a while.

area force-pushed the feat/gate-dispute-responses branch 5 times, most recently from 48e70bc to 9910e0c Compare May 26, 2020 13:32

area force-pushed the feat/gate-dispute-responses branch 8 times, most recently from ab699cc to 5548d2c Compare June 4, 2020 15:08

area force-pushed the feat/gate-dispute-responses branch 4 times, most recently from 56ac63b to c7c4635 Compare June 9, 2020 09:28

area force-pushed the feat/gate-dispute-responses branch 2 times, most recently from d4dc043 to 57e78d9 Compare June 12, 2020 10:39

area force-pushed the feat/gate-dispute-responses branch from 57e78d9 to ffe55ba Compare June 19, 2020 08:34

area added the ready-for-review label Jun 19, 2020

area marked this pull request as ready for review June 19, 2020 10:38

kronosapiens assigned area Jun 19, 2020

area force-pushed the feat/gate-dispute-responses branch 5 times, most recently from 909e18d to 7c59ab2 Compare June 22, 2020 15:49

area force-pushed the feat/gate-dispute-responses branch from 551dc1a to 46a7a3b Compare July 13, 2020 08:49

area force-pushed the feat/gate-dispute-responses branch from 46a7a3b to 8c4214e Compare July 29, 2020 09:40

kronosapiens reviewed Aug 11, 2020

View reviewed changes

area force-pushed the feat/gate-dispute-responses branch from e6539f3 to e5dee55 Compare August 12, 2020 12:31

kronosapiens previously approved these changes Aug 18, 2020

View reviewed changes

area added 18 commits August 19, 2020 09:30

First pass at rewards during dispute

d0213ea

Improvements following Daniel's review

cc8264d

First pass at rewards during dispute

d52b6a2

Avoid landrush from miners during disputes

66a8fcd

Fix ESLint issues

52fb0d3

Add testing for responsePossible

9c33e4d

Deal with consequences of a hard rebase

46a9b3a

Fix linter, add tests

6fd3d00

Try and harden a test, fixing an inconsistency

a16882d

Fix up docs

193a69d

Change how responsePossible handles the future

98dd6d5

Always update timestamps for submissions

3e3f9a0

Even if there's only one

Changes after Daniel's review

9c2d721

Add output to middle of test for sometimes slow CI

aa76a29

Make test more resilient

70627c0

How did we ever write bulletproof tests without makeTxAtTimestamp?

Update parameter names for reputation mining

0ee9993

This brings them in line with most of the rest of the codebase i.e. prefixed with _

Rest of changes following review

fb7b6c1

Response to feedback 2

9bee220

area dismissed kronosapiens’s stale review via 9bee220 August 19, 2020 08:32

area force-pushed the feat/gate-dispute-responses branch from e5dee55 to 9bee220 Compare August 19, 2020 08:32

kronosapiens approved these changes Aug 20, 2020

View reviewed changes

kronosapiens merged commit f82c660 into develop Aug 20, 2020

kronosapiens deleted the feat/gate-dispute-responses branch August 20, 2020 16:26

kronosapiens mentioned this pull request Nov 7, 2022

Fix random ordering of miners in response windows #1102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stagger allowed miner responses during a dispute #842

Stagger allowed miner responses during a dispute #842

area commented May 7, 2020

area commented Jun 4, 2020

kronosapiens Aug 11, 2020

area Aug 12, 2020

kronosapiens Aug 11, 2020

area Aug 12, 2020

kronosapiens Aug 11, 2020

area Aug 12, 2020

kronosapiens Aug 11, 2020

kronosapiens Aug 11, 2020

area Aug 12, 2020

kronosapiens Aug 11, 2020

kronosapiens Aug 18, 2020

kronosapiens left a comment

Stagger allowed miner responses during a dispute #842

Stagger allowed miner responses during a dispute #842

Conversation

area commented May 7, 2020

area commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kronosapiens left a comment

Choose a reason for hiding this comment