Validator manager contract in solidity #4

enriquefynn · 2018-01-15T22:09:42Z

Work in progress.

Missing:

Withdraw
addHeader

prestonvanloon · 2018-01-29T16:00:23Z

sharding/contracts/validator_manager.sol

+
+import "RLP.sol";
+
+interface ValidatorContract {


This is no longer needed

prestonvanloon

@enriquefynn we could check in this contract with the finished methods and have skeleton/stub methods for the others that aren't complete yet.

See this thread for source on some of my comments: https://gitter.im/ethereum/py-evm?at=5a6d73da0ad3e04b1b79d5df

prestonvanloon · 2018-01-29T16:01:56Z

sharding/contracts/validator_manager.sol

+    // Amount of wei the validator holds
+    uint deposit;
+    // The address which the validator's signatures must verify to (to be later replaced with validation code)
+    ValidatorContract validationCodeAddr;


We should store the validator account. Perhaps having the returnAddr and the validator address be the same

Maybe there's no need for validationCodeAddr anymore then. We can storeaddr instead

prestonvanloon · 2018-01-29T16:03:05Z

sharding/contracts/validator_manager.sol

+    var msgHash = keccak256("withdraw");
+    // TODO: Check this function
+    var result = validators[_validatorIndex].validationCodeAddr.withdraw.gas(sigGasLimit)(_sig);
+    if (result) {


This goes away and we assert(msg.sender == validators[_validatorIndex].address)

prestonvanloon · 2018-01-29T16:03:42Z

sharding/contracts/validator_manager.sol

+    }
+  }
+
+  function sample(int _shardId) public constant returns(address) {


Is this meant to be getEligibleProposer?

I think so, ref: https://github.com/ethereum/py-evm/blob/sharding/evm/vm/forks/sharding/contracts/validator_manager.v.py#L169

prestonvanloon · 2018-01-29T16:14:23Z

Also see ethereum/sharding#37

Remove validator's code and use the transaction's signature to verify the validator's identity

prestonvanloon · 2018-02-05T00:45:30Z

We should sync with the changes in this PR: ethereum/py-evm#348 This removes the validator code requirement.

Also, can we prepare this to merge? Even if some of it is incomplete

Missing CollationAdded log

prestonvanloon

Left a few comments. If any feel out of scope, please create an issue and mention it in response to my comment then we can resolve in another pull request.

Thanks for putting this together

prestonvanloon · 2018-02-05T15:53:33Z

sharding/contracts/validator_manager.sol

+  function VMC() public {
+  }
+
+  function isStackEmpty() internal view returns(bool) {


Please reorganize functions to this order:

constructor
fallback function (if exists)
external
public
internal
private

http://solidity.readthedocs.io/en/develop/style-guide.html#order-of-functions

prestonvanloon · 2018-02-05T15:54:22Z

sharding/contracts/validator_manager.sol

+  function isStackEmpty() internal view returns(bool) {
+    return emptySlotsStackTop == 0;
+  }
+  function stackPush(int index) internal {


Please add space between function declarations.

prestonvanloon · 2018-02-05T15:56:35Z

sharding/contracts/validator_manager.sol

+    address to;
+  }
+
+  mapping (int => Validator) validators;


Could you comment what the keys are for these mappings?

If the keys are shard ID or period number, shouldn't this be uint?

If the keys are shard ID or period number, shouldn't this be uint?

I want to do this afterwards, when I test the code gets bigger when modifying the ints to uint.
Some of those can also be packed in smaller sizes.

prestonvanloon · 2018-02-05T15:59:01Z

sharding/contracts/validator_manager.sol

+      deposit: msg.value,
+      addr: msg.sender
+    });
+  ++numValidators;


prestonvanloon · 2018-02-05T16:00:32Z

sharding/contracts/validator_manager.sol

+    // [TODO] Should check further if this safe or not
+    return validators[
+      int(
+      uint(keccak256(uint(block.blockhash(_period - lookAheadPeriods)) * periodLength, _shardId))


Formatting, please indent these three lines so it is clear that the result of modulus is cast to int.

prestonvanloon · 2018-02-05T16:03:13Z

sharding/contracts/validator_manager.sol

+    return receiptId;
+  }
+
+  function updataGasPrice(int _receiptId, uint _txGasprice) public payable returns(bool) {


/updata/update/s

prestonvanloon · 2018-02-05T16:04:40Z

sharding/contracts/validator_manager.sol

+      )].addr;
+  }
+
+  struct HeaderVars {


HeaderVars? Is this the collation header? Can we drop "Vars" and replace with something more meaningful?

That's just to pack variables used in the addHeader function

prestonvanloon · 2018-02-05T16:07:28Z

sharding/contracts/validator_manager.sol

+  }
+  function addHeader(int _shardId, uint _expectedPeriodNumber, bytes32 _periodStartPrevHash,
+                     bytes32 _parentCollationHash, bytes32 _txListRoot, address _collationCoinbase,
+                     bytes32 _postStateRoot, bytes32 _receiptRoot, int _collationNumber) public returns(bool) {


Always returns true? When is a false state? Can this be a void method?

From the sharding spec:

addHeader(bytes header) returns bool: attempts to process a collation header, returns True on success, reverts on failure.

I think it can be a void method. We should probably clone also the spec if we start differing so much, no?

Sounds good to me

prestonvanloon · 2018-02-05T16:08:51Z

sharding/contracts/validator_manager.sol

+      data: _data
+    });
+    var receiptId = numReceipts;
+    ++numReceipts;


I'm not 100% on solidity, but if receiptId is the same reference as numReceipts then wouldn't they both be incremented?

No, receiptId is a variable in memory.

Sounds good

prestonvanloon · 2018-02-05T16:09:16Z

sharding/contracts/validator_manager.sol

+  // during a future collation. Saves a `receipt ID` for this request,
+  // also saving `msg.sender`, `msg.value`, `to`, `shard_id`, `startgas`,
+  // `gasprice`, and `data`.
+  function txToShard(address _to, int _shardId, uint _txStartgas, uint _txGasprice, bytes12 _data) public payable returns(int) {


Line too long, can you limit to 100 characters for review?

rauljordan · 2018-02-05T17:53:50Z

sharding/contracts/validator_manager.sol

+
+  // Constant values
+  uint constant periodLength = 5;
+  int constant shardCount = 100;


Please make shardcount public

rauljordan · 2018-02-05T18:22:57Z

Looks all good to me...would you all be ok with merging??

prestonvanloon · 2018-02-05T18:50:49Z

sharding/contracts/validator_manager.sol

+    // if (parent_collation_hash == 0), i.e., is the genesis,
+    // then there is no need to check.
+    if (_parentCollationHash != 0x0)
+        assert((_parentCollationHash == 0x0) || (collationHeaders[_shardId][_parentCollationHash].score > 0));


_parentCollationHash == 0x0 can never be true from within this if statement, please update

prestonvanloon · 2018-02-05T18:53:32Z

sharding/contracts/validator_manager.sol

+      data: _data
+    });
+    var receiptId = numReceipts;
+    ++numReceipts;


Sounds good

Validator manager contract in solidity

Validator manager contract in solidity Former-commit-id: 5c8e92a

Validator manager contract in solidity Former-commit-id: 41453f9a6262ca73e52ed93aa5e9539b7d6edc46 [formerly 5c8e92a] Former-commit-id: 75b3b72

enriquefynn added 5 commits January 15, 2018 23:01

WIP: Validator manager contract in solidity

b1475e7

RLP implementation

8d41b1b

withdraw implemented

4bcee8d

Better abstraction to Validator Address

f8cbbf0

addHeader almost done

7467f35

prestonvanloon mentioned this pull request Jan 27, 2018

Use generated go bindings for validator management contract #13

Merged

prestonvanloon reviewed Jan 29, 2018

View reviewed changes

sharding/contracts/validator_manager.sol Outdated

import "RLP.sol";

interface ValidatorContract {

Copy link

Member

prestonvanloon Jan 29, 2018

Choose a reason for hiding this comment

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

This is no longer needed

prestonvanloon requested changes Jan 29, 2018

View reviewed changes

enriquefynn added 2 commits January 29, 2018 17:43

Simplify validator's logic

efafc71

Remove validator's code and use the transaction's signature to verify the validator's identity

constant variables & no need for sigGasLimit

8959685

enriquefynn added 7 commits February 5, 2018 09:47

new event

96116af

no need for RLP lib :)

8b31557

VMC contract adjustments

4558b45

Missing CollationAdded log

removed validator's cycle var

5638b78

Fix type errors

4abbbdd

Trying to outsmart the compiler

19c1f5a

styling

b561bc4

prestonvanloon reviewed Feb 5, 2018

View reviewed changes

rauljordan reviewed Feb 5, 2018

View reviewed changes

enriquefynn added 2 commits February 5, 2018 19:06

adjusting code to review

db62caa

make shardCount public

0c1d77c

prestonvanloon requested changes Feb 5, 2018

View reviewed changes

Renaming variables, removing unnecessary requires

6cfdef4

prestonvanloon approved these changes Feb 5, 2018

View reviewed changes

prestonvanloon changed the title ~~WIP: Validator manager contract in solidity~~ Validator manager contract in solidity Feb 5, 2018

prestonvanloon merged commit 11dc61f into prysmaticlabs:master Feb 5, 2018

prestonvanloon added a commit that referenced this pull request Jul 22, 2018

Merge pull request #4 from enriquefynn/geth-sharding

5c8e92a

Validator manager contract in solidity

prestonvanloon added a commit that referenced this pull request Jul 22, 2018

Merge pull request #4 from enriquefynn/geth-sharding

75b3b72

Validator manager contract in solidity Former-commit-id: 5c8e92a

prestonvanloon added a commit that referenced this pull request Jul 22, 2018

Merge pull request #4 from enriquefynn/geth-sharding

0a458de

Validator manager contract in solidity Former-commit-id: 41453f9a6262ca73e52ed93aa5e9539b7d6edc46 [formerly 5c8e92a] Former-commit-id: 75b3b72

terencechain mentioned this pull request Oct 10, 2018

Clean Up Todos and Add in Issue Checker #640

Merged

mdehoog added a commit to mdehoog/prysm that referenced this pull request Jun 14, 2022

Replace [32]byte with common.Hash (prysmaticlabs#4)

eb1c7bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validator manager contract in solidity #4

Validator manager contract in solidity #4

enriquefynn commented Jan 15, 2018 •

edited by rauljordan

prestonvanloon Jan 29, 2018

prestonvanloon left a comment •

edited

prestonvanloon Jan 29, 2018

enriquefynn Jan 29, 2018 •

edited

prestonvanloon Jan 29, 2018

prestonvanloon Jan 29, 2018

enriquefynn Jan 29, 2018

prestonvanloon commented Jan 29, 2018

prestonvanloon commented Feb 5, 2018 •

edited

prestonvanloon left a comment

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

enriquefynn Feb 5, 2018 •

edited

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

enriquefynn Feb 5, 2018

prestonvanloon Feb 5, 2018

enriquefynn Feb 5, 2018 •

edited

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

enriquefynn Feb 5, 2018

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

rauljordan Feb 5, 2018

rauljordan commented Feb 5, 2018

prestonvanloon Feb 5, 2018

prestonvanloon Feb 5, 2018

Validator manager contract in solidity #4

Validator manager contract in solidity #4

Conversation

enriquefynn commented Jan 15, 2018 • edited by rauljordan

Choose a reason for hiding this comment

prestonvanloon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriquefynn Jan 29, 2018 • edited

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

prestonvanloon commented Jan 29, 2018

prestonvanloon commented Feb 5, 2018 • edited

prestonvanloon 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

enriquefynn Feb 5, 2018 • edited

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

enriquefynn Feb 5, 2018 • edited

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

rauljordan commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriquefynn commented Jan 15, 2018 •

edited by rauljordan

prestonvanloon left a comment •

edited

enriquefynn Jan 29, 2018 •

edited

prestonvanloon commented Feb 5, 2018 •

edited

enriquefynn Feb 5, 2018 •

edited

enriquefynn Feb 5, 2018 •

edited