Tracking metablocks' rounds in the Consensus contract. #103

pgev · 2019-12-09T11:05:55Z

Fixes #92
Fixes #95

benjaminbollen · 2019-12-09T11:46:31Z

contracts/consensus/Consensus.sol

@@ -91,29 +103,28 @@ contract Consensus is MasterCopyNonUpgradable, CoreStatusEnum, ConsensusI {
    /** Coinbase split per mille */
    uint256 public coinbaseSplitPerMille;

-    /** Block hash of heads of Metablockchains */
-    mapping(bytes20 /* chainId */ => bytes32 /* MetablockHash */) public metablockHeaderTips;
+    /** Mapping of a metablock number to a metablock (per chain).  */


"(per metachain)"; also on L109

benjaminbollen · 2019-12-09T11:47:06Z

contracts/consensus/Consensus.sol


-    /** Core statuses */
-    mapping(address /* core */ => CoreStatus /* coreStatus */) public coreStatuses;
+    /** Metablocks' heights per chain. */


/** Metablockchains' current height of tip per metachain. */

benjaminbollen · 2019-12-09T12:22:04Z

contracts/consensus/Consensus.sol

+    mapping(address /* core */ => CoreStatus /* coreStatus */) public coreStatuses;
+
+    /** Linked-list of committees. */
+    mapping(address => address) public committees;


mapping(bytes32 /* metachainId */ => CommitteeI) public committees;

that way, there is less state build-up on Consensus; when a new metablock is precommitted; this mapping can be overwritten.

In return the pruning of Committees can be done from the committee itself; it can check whether it can be selfdestructed by checking open balances, and whether it has reported the decision / latest, if the metablockchain has moved on to another blockheight

We can create a follow up ticket for this. What are your thoughts?

- Updates naming convention from chain -> metachin - Change web3 provider from `http` to `ws`

benjaminbollen

Hi ! last night i drafted the storage layout I had in mind for Consensus.sol after this ticket. you can find it here

https://github.com/mosaicdao/mosaic-pm/blob/master/specification-discussions/20191206-consensus.md#contract-summary

I can also give it a try to implement the functions according to this storage. Please evaluate from your side whether this is a sensible proposal cc @deepesh-kn @pgev

deepesh-kn

👍
Lot of changes. Added a few comments.

contracts/consensus/Consensus.sol

deepesh-kn

looks good. I added a comment and have created a followup ticket #147 to update enterCommittee

deepesh-kn · 2019-12-18T08:00:15Z

contracts/consensus/Consensus.sol

    )
        private
        view
        returns (bytes32 seed_)
    {
+        assert(length > 0);
+
+        uint256 begin = end.add(uint256(1)).sub(length);


hashBlockSegment is used only at one place, so we need to pass the length, its already a constant.

For me, It was difficult to understand this part of the code, just a suggestion.
Instead of adding add(uint256(1)) here, can be do the following in the for loop:

for (uint256 i = 1; i <= length; i = i.add(1)) { seedGenerator[i] = blockhash(begin.add(i)); }

assuming you meant "we don't" need to pass the length? agreeing with this comment

@deepesh-kn with regards to the 2. point I would rather keep existing implementation.
@deepesh-kn @benjaminbollen wrt 1. point, I am not sure why should not be function written as a standalone function. However, I will change it as you both have proposed.

assuming you meant "we don't" need to pass the length? agreeing with this comment

yes, I meant we don't.

contracts/consensus/Consensus.sol

benjaminbollen · 2019-12-18T14:38:23Z

contracts/committee/Committee.sol

@@ -272,7 +283,6 @@ contract Committee is MasterCopyNonUpgradable, ConsensusModule, CommitteeI {

        proposal = _proposal;

-        // @qn (pro): In case of 7 quorum should be 4 or 5.
        quorum = _committeeSize * COMMITTEE_SUPER_MAJORITY_NUMERATOR /
        COMMITTEE_SUPER_MAJORITY_DENOMINATOR;


missing indentation

benjaminbollen · 2019-12-18T15:20:23Z

contracts/consensus/Consensus.sol

-            abi.encodePacked(seedGenerator)
+        bytes32 seed = hashBlockSegment(
+            committeeFormationBlockHeight,
+            COMMITTEE_FORMATION_LENGTH


why are we passing the constant to an internal function?

consider instead passing the metablockHash, because otherwise if multiple precommits happen in the same Ethereum block, they will all have the same seed, and thus the same committee.

cc @deepesh-kn @jayeshbairagi can we make this a separate task?

bytes32 seed = hashBlockSegments( currentMetablock.metablockHash, committeeFormationHeight);

(reference https://github.com/mosaicdao/mosaic-pm/blob/master/specification-discussions/20191206-consensus.md)

benjaminbollen · 2019-12-18T15:47:16Z

contracts/consensus/Consensus.sol

+            "Further validator's address is 0."
+        );
+
+        require(


can we collapse these into a modifier

onlyAssignedCore(_metachainId)

in a new ticket?

In the current implementation of enterCommittee the core of the validator is specified by its address (not metablockchain id). In upcoming implementation of enterCommittee (according to the newest proposal) we can.

benjaminbollen · 2019-12-18T16:14:24Z

contracts/consensus/Consensus.sol

-        address core = assignments[_metachainId];
+        bytes32 metablockHash = goToCommittedRound(_metachainId);
+
+        CoreI core = assignments[_metachainId];
        require(


ok for now, but it's more of an assert than anything; the assigned core should be active

@benjaminbollen do you mean here, that cores in assignments storage mapping will not keep halted/corrupted cores?

benjaminbollen · 2019-12-18T16:31:56Z

contracts/consensus/Consensus.sol

            "Core does not exist for given metachain."
        );

        // Specified core must have creation lifetime status.
        require(
-            coreLifetimes[core] == CoreLifetime.creation,
+            coreLifetimes[address(core)] == CoreLifetime.creation,


the mapping should probably be from CoreI => CoreLifetime?

To my knowledge, solidity mapping key can be only elementary type.

Mapping types use the syntax mapping(_KeyType => _ValueType) and variables of mapping type are declared using the syntax mapping(_KeyType => _ValueType) _VariableName. The _KeyType can be any built-in value type plus bytes and string. User-defined or complex types such as contract types, enums, mappings, structs or array types apart from bytes and string are not allowed. _ValueType can be any type, including mappings, arrays and structs.

https://solidity.readthedocs.io/en/v0.6.0/types.html#mapping-types

contracts/consensus/Consensus.sol

benjaminbollen · 2019-12-18T16:57:29Z

contracts/consensus/Consensus.sol

+        uint256 begin = end.add(uint256(1)).sub(length);
+
+        require(
+            block.number >= end && block.number < begin.add(uint256(256)),


block.number must be strictly greater than end, bc the blockhash of the current block is not yet known

Yes. 🤦. Thanks!

contracts/consensus/Consensus.sol

benjaminbollen

Looks very good, great progress; thanks, and sorry for the long road to get here;

mostly requesting a small change, can you look and assert this point?
https://github.com/mosaicdao/mosaic-1/pull/103/files#r359458898

the other comments should largely be captured in new tickets; can you assist here @jayeshbairagi we can go over them tomorrow

benjaminbollen

LGTM!

Paruyr Gevorgyan added 8 commits December 5, 2019 10:30

mosaicdaoGH-92: Tracking metablocks' rounds in the Consensus contract.

952feb7

Fixes Committee unit tests

3019de8

Merge with 'origin/consensus-cleanup-naming'

4509cc1

Fixing failing unit tests

c1f3f00

Change consensus move*Round nameing to go*Round

d3eebe0

Fixes failing unittests

b82af0d

Replaces chainId with metachainId

70911f3

Merges with 'feature/consensus'

f5178a2

pgev requested a review from deepesh-kn December 9, 2019 11:06

benjaminbollen reviewed Dec 9, 2019

View reviewed changes

Adjusts chain and metachain naming

4e2685d

benjaminbollen reviewed Dec 9, 2019

View reviewed changes

Paruyr Gevorgyan added 7 commits December 9, 2019 19:24

Merges w/ feature/consensus

fe3ee29

- Updates naming convention from chain -> metachin - Change web3 provider from `http` to `ws`

Merges feature/consensus

85b7f54

Uses ws for web3

23c7cd1

Improves usage

f993b43

Running reputation test first

53d57ba

Runs each test separately to fix instability

a1ae788

Merges feature/consensus

1e450a2

pgev requested a review from benjaminbollen December 10, 2019 08:57

Paruyr Gevorgyan added 3 commits December 10, 2019 11:38

Drafts mosaicdaoGH-95

29a8d84

Merges metablock-round

a04023f

Drafts mosaicdaoGH-95. Compiling version.

1f2063e

benjaminbollen reviewed Dec 11, 2019

View reviewed changes

Paruyr Gevorgyan added 5 commits December 12, 2019 10:02

Merges with feature/consensus

152b459

Makes Consensus::isValidator internal

b83d062

Merges w/ pgev:metablock-round

53441fd

Merges w/ feature/consensus

64f9ce9

Merge w/ metablock-round

12c8b06

Paruyr Gevorgyan added 3 commits December 17, 2019 14:52

Cleanup.

e61de39

Ignores/fixes unit tests

6b6090e

Merge w/ feature/consensus

e4168a0

deepesh-kn suggested changes Dec 17, 2019

View reviewed changes

contracts/consensus/Consensus.sol Outdated Show resolved Hide resolved

contracts/consensus/Consensus.sol Show resolved Hide resolved

contracts/consensus/Consensus.sol Show resolved Hide resolved

contracts/consensus/Consensus.sol Show resolved Hide resolved

Paruyr Gevorgyan added 2 commits December 18, 2019 10:43

Fixes consensus unit test failure

9985754

Fixes Axiom unit test failure

76a08d4

deepesh-kn removed their assignment Dec 18, 2019

pgev requested a review from deepesh-kn December 18, 2019 08:02

deepesh-kn previously approved these changes Dec 18, 2019

View reviewed changes

benjaminbollen reviewed Dec 18, 2019

View reviewed changes

contracts/consensus/Consensus.sol Show resolved Hide resolved

benjaminbollen reviewed Dec 18, 2019

View reviewed changes

contracts/consensus/Consensus.sol Show resolved Hide resolved

benjaminbollen requested changes Dec 18, 2019

View reviewed changes

Paruyr Gevorgyan added 7 commits December 19, 2019 08:07

Merges w/ feature/consensus

9936d62

hashBlockSegment() reads blocksegment length from storage

9438c52

Fixes indentation

16f0ed6

Improves hashBlockSegment() to include metablockhash on seed calculation

c31f408

Calls reputation first on validator logout

182516d

Refines assertion for hashBlockSegment

b2a3f91

Improves hashBlockSegment() to include metablockhash on seed calculation

2c1c70b

pgev dismissed deepesh-kn’s stale review via 2c1c70b December 19, 2019 06:21

pgev requested a review from benjaminbollen December 19, 2019 06:57

benjaminbollen approved these changes Dec 19, 2019

View reviewed changes

benjaminbollen merged commit 86908bf into mosaicdao:feature/consensus Dec 19, 2019

pgev mentioned this pull request Dec 19, 2019

Tracking metablocks' rounds in the Consensus contract. #92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking metablocks' rounds in the Consensus contract. #103

Tracking metablocks' rounds in the Consensus contract. #103

pgev commented Dec 9, 2019 •

edited

Loading

benjaminbollen Dec 9, 2019

pgev Dec 9, 2019

benjaminbollen Dec 9, 2019 •

edited

Loading

pgev Dec 9, 2019

benjaminbollen Dec 9, 2019

benjaminbollen Dec 9, 2019

pgev Dec 10, 2019

benjaminbollen left a comment

deepesh-kn left a comment

deepesh-kn left a comment

deepesh-kn Dec 18, 2019

benjaminbollen Dec 18, 2019

pgev Dec 19, 2019

pgev Dec 19, 2019

deepesh-kn Dec 19, 2019

benjaminbollen Dec 18, 2019

pgev Dec 19, 2019

benjaminbollen Dec 18, 2019 •

edited

Loading

pgev Dec 19, 2019

benjaminbollen Dec 18, 2019

pgev Dec 19, 2019

benjaminbollen Dec 18, 2019

pgev Dec 19, 2019

benjaminbollen Dec 18, 2019

pgev Dec 19, 2019

pgev Dec 19, 2019 •

edited

Loading

benjaminbollen Dec 18, 2019

pgev Dec 19, 2019

benjaminbollen left a comment

benjaminbollen left a comment

Tracking metablocks' rounds in the Consensus contract. #103

Tracking metablocks' rounds in the Consensus contract. #103

Conversation

pgev commented Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen Dec 9, 2019 • 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

benjaminbollen left a comment

Choose a reason for hiding this comment

deepesh-kn left a comment

Choose a reason for hiding this comment

deepesh-kn 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

benjaminbollen Dec 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

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

pgev Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

pgev commented Dec 9, 2019 •

edited

Loading

benjaminbollen Dec 9, 2019 •

edited

Loading

benjaminbollen Dec 18, 2019 •

edited

Loading

pgev Dec 19, 2019 •

edited

Loading