Solidity p14 v1 #594

NicholasDotSol · 2019-02-18T15:53:41Z

Discussion:

Time between ticket challenge period and first submitter eligibility.

We don't know how long DKD is going to last, but we still need immutable time variables set beforehand. Time for first submitter to submit , T_step

Voting is a mandatory action, enough non voters break honest majority.

This sounds intuitive, but we had a discussion about this with @ngrinkevich and @pschlump.

function naming

currently old submitDkgResult() is named receiveSubmission() and getFinalResult() from the white-paper is named submitGroupPublicKey. this seems a little awkward.

cleanup() not refunding gas. @ngrinkevich

I called the same transaction twice, once with cleanup() as the last instruction and once without. The gas cost was identical. I know that there is some kind of refund mechanic but i'm not too clear on how it works. From what I gathered, calling delete costs gas but a percentage >100% is refunded for items deleted. I'll do some more research to make sure we call this the best way possible.

can't forget Request Id

I think we concluded on using beacon value as unique identifier

Solidity NatSpec comments causing compilation errors.

Haven't fixed this yet. The ones that are there work fine, but when i add one its not fine....

To Do:

Fix solidity testing errors and clean up TestDkgConflictResolution.js
Fix Event parameters
go CI testing should eventually be modified.
Find good function naming
Remove eligibility check after first submission, but keep a voting timeout.
Should not be able to get final result until voting period is over or enough votes are in.
Remove the final getBlockNumber method and use web3 in testing instead.
Edit test to work with no check after first submission.
cleanup() research

removed RequestID and logic asociated with it since simultanious group creation is unlikely removed SubmitDkgResult and submission methods in preparation for phase14 logic

added reveiveSubmission, receiveVote and submitGroupPublicKey allong with some helper methods. This allows users to submit a DKG result, vote on the currect one, and retreive the final result with the highest votes.

Included a newer web3 version as the older version does not mimic solidity's compact hashing made some phase 14 logic better by removing clutter (mainly in eligibleSubmitter()) Removed TestPublishDkgResult.js tests as the old submission method will no longer be used Added a helper method to get the block during which a user at a given index becomes eligible to submit a DKG result

ngrinkevich

@NicholasDotSol Just to remind you truffle.js should be ignored based on #515

Also looks like package-lock.json files shouldn't be in this commit since you're not modifying any packages

ngrinkevich

Unverified add p14 logic commit, pls fix

ngrinkevich · 2019-02-18T18:33:03Z

contracts/solidity/contracts/KeepGroupImplV1.sol

@@ -21,11 +21,11 @@ contract KeepGroupImplV1 is Ownable {
        bytes inactive;
    }

-    event DkgResultPublishedEvent(uint256 requestId, bytes groupPubKey);
+    event DkgResultPublishedEvent(address publisher, bytes groupPubKey);


ngrinkevich · 2019-02-18T18:34:19Z

contracts/solidity/package.json

@@ -9,6 +9,8 @@
  "scripts": {
    "truffle": "truffle",
    "test": "truffle test",
+    "testm": "truffle test ./test/TestPublishDkgResult.js",


I don't think we need to add those separately? truffle test should pick them up anyway

Nope definitely don't need that here, just used them locally. Same with Truffle.js. Thought it got .gitIgnored

ngrinkevich · 2019-02-18T18:45:05Z

contracts/solidity/package.json

@@ -20,7 +20,8 @@
  },
  "homepage": "https://github.com/keep-network/contracts",
  "dependencies": {
-    "openzeppelin-solidity": "^2.0.0"
+    "openzeppelin-solidity": "^2.0.0",
+    "web3": "^1.0.0-beta.37"


Probably best would be to make a separate PR to upgrade truffle and web3? what do you think?
In the meantime you can use soliditySHA3 from already included ethereumjs-abi i.e. abi.soliditySHA3(...

Couldn't get that to work after a couple twists and turns. It doesn't have support for encoding byes, and the hash was different when passed as Uints or Buffers. Can remove it if abi.soliditySHA3() works and i'm missing something of course

and the hash was different when passed as Uints or Buffers

Right I see, ok how do you feel opening a PR with truffle 5 upgrade? and we'll see if thats a straightforward migration?

It's bundled with web3 1.0 , should help us keep our code cleaner overall, also we'll get rid of ethereumjs-abi. Bundled BN library will be useful for BLS relay entry tests (I had to include it separately in #559)

I'm running v5.0.1 locally already, That's why I need to specify solidity compiler in truffle.js

…p-core into solidity-p14-v1 Fix unverified commit.

pdyraga

Left some comment. I really like where it's heading!

pdyraga · 2019-02-20T21:33:34Z

contracts/solidity/contracts/KeepGroupImplV1.sol


    // Legacy code moved from Random Beacon contract
    // TODO: refactor according to the Phase 14
-    event SubmitGroupPublicKeyEvent(bytes groupPublicKey, uint256 requestID, uint256 activationBlockHeight);
+    event SubmitGroupPublicKeyEvent(bytes groupPublicKey, uint256 activationBlockHeight);//add RequestId replacement


What is the potential request ID replacement here?

@pdyraga previous beacon value should work well as unique identifier

pdyraga · 2019-02-20T21:35:42Z

contracts/solidity/contracts/KeepGroupImplV1.sol

-    mapping (bytes => uint256) internal _submissionVotes;
-    mapping (address => mapping (bytes => bool)) internal _hasVoted;
+    mapping (bytes32 => bool) internal _votedDkg;
+    mapping (bytes32 => bool) internal _resultPublished;


Do we need _resultPublished? Submitting a result entails voting on this result so maybe we can go with just _votedDkg?

nope, _voteDkg is all we need as there is no instance where _resultPublished[result] != _voteDkg[result] nice (:

Never mind, Voting for a result does not entail Submitting a result. We need to keep both

But publishing result entails voting on it. That's why I suggested to remove _resultPublished but keep _votedDkg.

we need to separate the two in the method. When 2 participants call the method in the same block one will publish and the other will vote.
if not _resultPublished -> publish
else -> vote

They also check different conditions. We can't use _voteDkg to check the condition because we won't know if the user needs to just vote for a result or publish it. besides when entering the method we require() that the user hasn't voted.

Hmmm... If you submitted a result, you are not allowed to vote later. If you voted, you are not allowed to submit a result later. There is a possibility I miss some important detail...

pdyraga · 2019-02-20T21:37:03Z

contracts/solidity/contracts/KeepGroupImplV1.sol

-    mapping (address => mapping (bytes => bool)) internal _hasVoted;
+    mapping (bytes32 => bool) internal _votedDkg;
+    mapping (bytes32 => bool) internal _resultPublished;
+    mapping (address => DkgResult) internal _publisherToDkgResult;


Do we need to store the information who submitted the given result?

Not right now, but we had conversations about using the publisher as an identifier to DKG result. isInactive and isDisqualified methods used it as an input to retrieve the IA and DQ bytes. Should remove it for now as it is just an extra instruction and not worth hoarding :)

Yep, let's remove it if it's not currently used 💯

pdyraga · 2019-02-20T21:40:04Z

contracts/solidity/contracts/KeepGroupImplV1.sol

+        //(2* (T_step)) -> time for first submitter to submit DKG Result.
+        //No way to calculate DKG time on-chain, so DKG result submission opens on ticket-challenge close.
+        //first submitter usable time period is really (2 * T_step - DKG time).
+        else if(block.number >= ((T_init + (2 * (T_step))) + ((index-2) * T_step))){


Worth noting, there is an ongoing discussion if this check makes sense at all.

Yup, following that (:

pdyraga · 2019-02-20T21:42:04Z

contracts/solidity/contracts/KeepGroupImplV1.sol

+     * @param index the claimed index of the user.
+     * @return true if the ticket at the given index is issued by msg.sender. False otherwise.
+     */
+    function validateIndex(uint index)public returns(bool){   


👌 Attention to details level 💯

pdyraga · 2019-02-20T22:01:05Z

contracts/solidity/contracts/KeepGroupImplV1.sol

+                leadingResult = _DkgResultHashes[i];
+            }
+            totalVotes += highestVoteNtemp;
+        }


Hmm... can't we achieve the same without highestVoteNtemp? 🤔

yup, Had a feeling this would save gas as it would avoid the lookup during reassignment but I may have been delusional 😓

As a result of some Gas testing, holding highestVoteNtemp does save some gas (about 800 here)

pdyraga · 2019-02-20T22:02:17Z

contracts/solidity/contracts/KeepGroupImplV1.sol

+        uint highestVoteN;
+        uint highestVoteNtemp;
+        uint totalVotes;
+        uint f_max = _groupSize/2 + 1;


This should be evaluated from _groupThreshold in group contract.

...and probably called maximumDishonest or something similar 😉

okay, this is the perfect comment! thank you @pdyraga.
I thought that in a case where groupSize != groupThreshold what matters is how large the group currently is, (aka #of stakers potentially eligible to vote) and honest majority is assessed based on that number. That doesn't make sense though 😀

Group size and threshold are fixed constants for the random beacon.

In theory, key generation protocol allows to parametrize threshold (above some secure minimum for the given group size) but we are not allowing to parametrize it.

For us, threshold < group size because we want to support scenario when one or more nodes either does not respond or respond incorrectly. If we set threshold = group size, then all nodes need to respond and all nodes need to behave according to the protocol. This is how it should work in a perfect world but since we don't live in a perfect world, we don't want a single node to stop the group from generating key and slowing down network in a long term.

pdyraga · 2019-02-20T22:06:08Z

contracts/solidity/contracts/KeepGroupImplV1.sol

+            highestVoteNtemp = _submissionVotes[_DkgResultHashes[i]];
+            if(highestVoteNtemp > highestVoteN){
+                highestVoteN = highestVoteNtemp;
+                leadingResult = _DkgResultHashes[i];


We'll need to add a check for strictly leading. For example, for the following distribution of votes: 2 1 2 1 1, we have no strictly leading result.

As an extension to the previous comment about _groupSize, This works if more than half of the voters agree on a result and fails in all other cases

assuming all group-members vote:
[4,4,1] - > this fails
[2,2,3,2] -> this fails

[1,1,1,1,1,2,1,1] -> Should this succeed?

There are two pieces here:

strictly leading result

honest majority

In the example you mentioned: [1,1,1,1,1,2,1,1], the result with 2 votes is strictly leading but the entire operation should end with failure since other results received 7 votes in total and the strictly leading result has only 2 votes, hence, we don't have the honest majority.

This is explained here:

[3] If more than fmax participants vote for a non-leading result, our honest majority assumption has failed and we cannot determine the correct outcome.

http://docs.keep.network/cryptography/beacon_dkg.html#3

Though, there is a typo there and f_max should be M_max.

okay awesome, That's the way I thought it was initially but mixed up groupSize and threshold

But we probably don't need strictly leading check here, if honest majority vote for a single result it will surely be the leading result as well.

honest_majority = _groupThreshold/2 + 1

checking if leadingResult has at least honest_majority votes should be sufficient. This is the way to method is currently implemented.

but should change
if(totalVotes - highestVoteN >= f_max)
to
if(totalVotes - highestVoteN > f_max)

The key here is that not all members need to vote - you need to look at off-chain phase 13 and 14:
http://docs.keep.network/random-beacon/dkg.html#phase-13

Say we have a group consisting of 5 players: P_1, P_2, P_3, P_4, P_5

Scenario 1

In phase 13 P_1 publishes DKG result

Since all other members agree P_1 is the correct result, they don't submit their own results and they don't vote. From phase 14 off-chain pseudocode:
elif (correctResult == leadResult and strictlyLeading) or alreadySubmitted: wait()

We have just one result with just one vote (from P_1). It's strictly leading result but it has no honest majority of votes looking at the counter of votes. This is fine, though. The condition is inverted here: if other results have more than M_max votes then we have a problem. In this particular case there is only one result, so no other results have votes so we are fine.

Scenario 2

In phase 13, P_1 publishes DKG result

In phase 14, P_3 does not agree with the result published by P_1 and publishes its own result

P_2, P_4, P_5 need to vote. Ideally, only one of them should vote because this will make either P_1 result or P_3 result strictly leading but we can't guarantee this behavior. Let's say they all agree P_1 is the correct result. We may end up with the following distribution of votes:
2 1, 3 1, 4 1. In all those cases P_1 is stricly leading but in not all of them all number of votes on strictly leading result minus number of votes on other result is greater than M_max. But this is fine, not all of them need to vote. That's again, why this condition is inverted:

If more than M participants vote for a non-leading result, our honest majority assumption has failed and we cannot determine the correct outcome.

Ah okay, assuming a vote was mandatory is incorrect then.
Thank you Piotr, Should be easy to change.

pdyraga · 2019-02-20T22:09:00Z

contracts/solidity/contracts/KeepGroupImplV1.sol

+     * @param index the claimed index of the submitter.
+     * @return the block number when a user is eligible.
+     */
+    function eligibleTime(uint index) public view returns(uint){


This should be used in eligibleSubmitter, right? (Assuming that we need this check at all, per the ongoing discussion)

ah, no this is just a helper for testing. I just plucked the logic from eligible submitter. I did not remove it because I thought it could potentially be useful. Will see if we still need it after the discussion is resolved

pdyraga · 2019-02-20T22:10:45Z

contracts/solidity/truffle.js

+      gas: 4712388
+    }
+  },
+  ompilers: {


ompilers? 😜

We probably shouldn't alter this file here - guess it was committed by accident.

Yup, didn't see it coming. it's also odd because I checked .gitignore and it should ignore truffle.js in contracts/solidity.

hmmm also it's spelled correctly on my end. else I can't compile.

Merge with Master

We don't currently hold information about who published a DKG result

…selectedParticipants() This way DKG won't occur if there are not enough participants, and the check will be avoided for every DKG result submission

…shes

ngrinkevich · 2019-02-27T14:07:45Z

contracts/solidity/test/TestDkgConflictResolution.js

+        }
+        await keepGroupImplViaProxy.receiveSubmission(i,success,groupPubKey,disqualified,inactive, {from: sender});
+      }
+      assert.equal(await keepGroupImplViaProxy.submitGroupPublicKey.call(), newWeb3.utils.soliditySha3(success,groupPubKey, disqualified, inactive) , "get correct final result");


@NicholasDotSol If you rebase with master now you'll get updated web3 and can remove newWeb3

Update to 0.5.0

pdyraga · 2019-03-08T09:48:44Z

Closing it as we work on a new version of P13/14.

Nicholas Evans added 5 commits February 13, 2019 11:45

Remove RequestID and DKG result submission methods

7e8d50e

removed RequestID and logic asociated with it since simultanious group creation is unlikely removed SubmitDkgResult and submission methods in preparation for phase14 logic

add p14 logic

e1fdfc5

added reveiveSubmission, receiveVote and submitGroupPublicKey allong with some helper methods. This allows users to submit a DKG result, vote on the currect one, and retreive the final result with the highest votes.

Remove old submission logic

e383663

fix typo and remove unused variable

c4b7a14

NicholasDotSol requested a review from a team February 18, 2019 15:53

change height() comment until it is removed

e6d91f6

ngrinkevich reviewed Feb 18, 2019

View reviewed changes

Nicholas Evans added 2 commits February 18, 2019 13:27

change height() comment until it is removed

850fcf7

Merge branch 'solidity-p14-v1' of https://github.com/keep-network/kee…

9f61688

…p-core into solidity-p14-v1 Fix unverified commit.

pdyraga reviewed Feb 20, 2019

View reviewed changes

pdyraga added the ⛓️ solidity label Feb 22, 2019

Nicholas Evans added 7 commits February 24, 2019 15:09

Merge branch 'master' into solidity-p14-v1

9802af9

Merge with Master

rename _DkgResultHashses to _dkgResultHashes

cca9164

Remove _publisherToDkgResult mapping

12aea1a

We don't currently hold information about who published a DKG result

move _tickets.length >= _groupSize check from receiveSubmission() to …

16fcd13

…selectedParticipants() This way DKG won't occur if there are not enough participants, and the check will be avoided for every DKG result submission

Make function names less confusing

b26ee9a

Add increment for voting Time

2cf3452

Add view function to return uint[] of votes and bytes32[] of resultHa…

d601261

…shes

ngrinkevich reviewed Feb 27, 2019

View reviewed changes

Nicholas Evans added 3 commits March 3, 2019 12:05

Merge branch 'master' into solidity-p14-v1

abf179a

Update to 0.5.0

Update KeepGroupImplV1.sol to match 0.5.0

9ed3082

Add time controls for voting and eddit testing

d90f4d8

pdyraga closed this Mar 8, 2019

pdyraga deleted the solidity-p14-v1 branch March 8, 2019 09:48

Solidity p14 v1 #594

Solidity p14 v1 #594

Conversation

NicholasDotSol commented Feb 18, 2019 • edited Loading

Discussion:

To Do:

ngrinkevich left a comment

Choose a reason for hiding this comment

ngrinkevich left a comment • edited Loading

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

NicholasDotSol Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdyraga left a 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

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

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

pdyraga Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scenario 1

Scenario 2

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

pdyraga commented Mar 8, 2019

NicholasDotSol commented Feb 18, 2019 •

edited

Loading

ngrinkevich left a comment •

edited

Loading

NicholasDotSol Feb 18, 2019 •

edited

Loading

pdyraga Feb 22, 2019 •

edited

Loading