Introduced a private Ethereum network for testing purposes #397

screwyprof · 2017-10-12T15:22:28Z

Implements issue #312. (Introduces a private testing network used for testing purposes)

Changes:

A new private testing network (StatusChain) is introduced.
An example test case implemented (TestSendEtherOnStatusChainTx)
Genesis file added static/config/status-chain-genesis.json

Changes: 1. A new private testing network (StatusChain) is introduced. 2. An example test case implemented (TestSendEtherOnStatusChainTx) 3. Genesis file added static/config/status-chain-genesis.json

adambabik · 2017-10-13T07:34:52Z

@screwyprof Looks great! Can you also submit a docker-compose config file so that I can run it locally, please?

screwyprof · 2017-10-13T07:41:33Z

@adambabik Docker is not required at the moment. You can just run tests normally and everything should work. The test itself runs a node.

adambabik

LGTM! A few minor comments.

adambabik · 2017-10-13T07:39:07Z

e2e/transactions/transactions_test.go

@@ -314,6 +314,89 @@ func (s *TransactionsTestSuite) TestSendEtherTx() {
 	s.Zero(s.Backend.TxQueueManager().TransactionQueue().Count(), "tx queue must be empty at this point")
 }

+func (s *TransactionsTestSuite) TestSendEtherOnStatusChainTx() {
+


This new line is not needed.

Do we have a style guide section on that? Should I really fix it?

Very minor. I just noticed it and though it's a typo or copy&paste. Generally, we don't put a new line at the beginning of a function body.

Ok, thanks. It would be great if it was stated in Style Guide so that the other contributors kept it in mind as well.

We don't add blank lines after a function declaration, I'll add it to our upcoming style guide. Please, fix.

adambabik · 2017-10-13T07:41:22Z

e2e/transactions/transactions_test.go

+			)
+			s.NoError(err, fmt.Sprintf("cannot complete queued transaction[%v]", event["id"]))
+
+			log.Info("contract transaction complete", "URL", "https://ropsten.etherscan.io/tx/"+txHash.Hex())


This log is not needed as it's not Ropsten.

I agree, will fix it.

adambabik · 2017-10-13T07:42:18Z

e2e/transactions/transactions_test.go

+
+	select {
+	case <-completeQueuedTransaction:
+	case <-time.After(2 * time.Minute):


As we use a private blockchain and it's supposed to be fast, maybe we can decrease it to seconds?

Totally agree with you, more than that, If it were up to me, I would not use such a construction at all. Timeouts should be managed by context. Context is used everywhere across our calls, but for some reasons is not passed in the tests. The reason I didn't touch it is that It was said just to copy paste the test and make it pass on a private network as a basic showcase.

I would also split the test into 3-4 different test cases (with active user, with inactive user, with no user. It seems to me that there are too many asserts per test case right now

I agree with all your points. We will start fixing our e2e tests when we have all pieces. This private network is one of them.

1) Unnecessary log message is eliminated as non-applicable in this case

tiabc

Looks good!

First of all, it's really great to see the tests run so fast on real blockchain! Feels great.

The only thing I'd like to settle is the genesis block to be more compliant with public test networks.

tiabc · 2017-10-15T11:08:15Z

e2e/transactions/transactions_test.go

@@ -314,6 +314,89 @@ func (s *TransactionsTestSuite) TestSendEtherTx() {
 	s.Zero(s.Backend.TxQueueManager().TransactionQueue().Count(), "tx queue must be empty at this point")
 }

+func (s *TransactionsTestSuite) TestSendEtherOnStatusChainTx() {
+


We don't add blank lines after a function declaration, I'll add it to our upcoming style guide. Please, fix.

tiabc · 2017-10-15T11:08:34Z

e2e/transactions/transactions_test.go

@@ -314,6 +314,89 @@ func (s *TransactionsTestSuite) TestSendEtherTx() {
 	s.Zero(s.Backend.TxQueueManager().TransactionQueue().Count(), "tx queue must be empty at this point")
 }

+func (s *TransactionsTestSuite) TestSendEtherOnStatusChainTx() {
+


We don't add blank lines after a function declaration, I'll add it to our upcoming style guide. Please, fix.

tiabc · 2017-10-15T11:23:54Z

static/config/status-chain-genesis.json

+      "balance": "0x1B1AE4D6E2EF500000"
+    }
+  },
+  "coinbase": "0x0000000000000000000000000000000000000000",


Let's revise the genesis block. I'd like it to be easier to grasp and we can do that provided that most default values should be fine.
For example, coinbase will be 0x0000000000000000000000000000000000000000, specifying it here doesn't give any valuable information. Let's remove it.

tiabc · 2017-10-15T11:25:00Z

static/config/status-chain-genesis.json

+    "eip158Block": 10,
+    "ethash": {},
+    "homesteadBlock": 0
+  },


I may be wrong but let's try to only specify these values to be more consistent with real testnets:

"chainId": 777 "homesteadBlock": 0 "daoForkBlock": 0 "daoForkSupport": true "byzantiumBlock": 0 "eip150Hash": <0 block hash> "eip155Block": 0 "eip158Block": 0

The genesis block I added is fully based on Ropsten's genesis block. (DefaultTestnetGenesisBlock). I can change it if necessary.

Yes, please, let's try. Ropsten has already undergone those scheduled changes while our private network not. And it won't do it because we have no miners to mine blocks.

tiabc · 2017-10-15T11:28:23Z

static/config/status-chain-genesis.json

+    "homesteadBlock": 0
+  },
+  "difficulty": "0x100000",
+  "extraData": "0x537461747573436861696e20507269766174652054657374696e67204e6574776f726b",


What is this?

extraData contains a hex-string: "StatusChain Private Testing Network" :)

Ok, sounds good :)

1) Genesis block revised 2) Unnecessary empty line at the begging of the function removed

… issue/introduce-private-network-for-testing-purposes-#312

tiabc · 2017-10-16T12:59:35Z

static/config/status-chain-genesis.json

-    "eip155Block": 10,
-    "eip158Block": 10,
-    "ethash": {},
-    "homesteadBlock": 0


Why have you deleted all that?

You asked me to make genesis as simple as possible, so I left only the minimal required options.
All the rest will be set to their default values.

Should I add some specific options/values?

No, I haven't. I've asked it to be as close to public testnets as possible and proposed some specific values to test here: #397 (comment)

The reason is that nil is the default, not 0:

status-go/vendor/github.com/ethereum/go-ethereum/params/config.go

Line 107 in 90acfed

EIP150Block *big.Int `json:"eip150Block,omitempty"` // EIP150 HF block (nil = no fork)

and nil would be incorrect in our case.

The very first revision of genesis was fully based on Ropsten genesis used in the project. I only changed chain id, pre-allocated accounts and extra data :) The options you proposed significantly differ from the default values of Ropsten.

Would be so kind as to specify exactly which values would you like me to use?

If I get it correctly, all you really want at the moment is to add the options with their corresponding values as follows:

"config": {
"byzantiumBlock": 1700000,
"chainId": 777,
"daoForkSupport": true,
"eip150Block": 0,
"eip150Hash": "0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d",
"eip155Block": 10,
"eip158Block": 10,
"ethash": {},
"homesteadBlock": 0
},

The reason why I've requested that change is because Ropsten has already reached forks specified in its genesis, for example byzantiumBlock, eip150Block, eip155Block and eip158Block and its actual head is about 1882811.
While our private net with the same config won't reach those forks unless we specify 0 block there. So we need to modify our genesis so that those forks have already been reached from the very beginning.

Summarily, although my changes significantly differ from the ones from Ropsten, they allow to reach almost the same state in which Ropsten is in at the moment.

Makes sense?

Got it. Just in case, would it be ok if I set the values as follows:
"config": {
"chainId": 777
"homesteadBlock": 0
"daoForkBlock": 0
"daoForkSupport": true
"byzantiumBlock": 0
"eip150Hash": "0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d",
"eip155Block": 0
"eip158Block": 0
}

Please, remove:
daoForkBlock

Please, fix:
eip150hash should contain the hash of the current genesis block while you're setting it to Ropsten's 0 block.

The rest seems fine.

Suggestions are taken from:

status-go/vendor/github.com/ethereum/go-ethereum/params/config.go

Line 49 in 90acfed

TestnetChainConfig = &ChainConfig{

I may be wrong but let's try.

I pushed all the fixes discussed. Could you please prove me right or wrong on eip150Hash. In order to set its value I did the following:

Ran the StatusChain network

Attached to it

called eth.getBlock(0) and retrieved the hash => "0x28c4da1cca48d0107ea5ea29a40ac15fca86899c52d02309fa12ea39b86d219c"

Sounds good, let's see :)

influx6

LGTM!

1) Genesis block revised 2) StatusChain network is now created in its own folder

… issue/introduce-private-network-for-testing-purposes-#312

tiabc

Thank you for the contribution, Max! I'm going to merge this PR soon.

We'll have a conversation about you joining the core developers team and I'll get back to you with the result.

Implements issue #312.

3ff6423

Changes: 1. A new private testing network (StatusChain) is introduced. 2. An example test case implemented (TestSendEtherOnStatusChainTx) 3. Genesis file added static/config/status-chain-genesis.json

screwyprof closed this Oct 12, 2017

screwyprof reopened this Oct 12, 2017

screwyprof closed this Oct 12, 2017

screwyprof reopened this Oct 12, 2017

screwyprof closed this Oct 12, 2017

screwyprof reopened this Oct 12, 2017

adambabik approved these changes Oct 13, 2017

View reviewed changes

After CR's fixes applied:

ac16ca3

1) Unnecessary log message is eliminated as non-applicable in this case

tiabc changed the title ~~Implements issue #312. (Introduces a private testing network used for testing purposes)~~ Introduced a private Ethereum network for testing purposes Oct 15, 2017

tiabc suggested changes Oct 15, 2017

View reviewed changes

Maxim Shcherbo added 2 commits October 15, 2017 15:04

CR notices fixed:

a6518b2

1) Genesis block revised 2) Unnecessary empty line at the begging of the function removed

Merge branch 'develop' of https://github.com/status-im/status-go into…

22325f3

… issue/introduce-private-network-for-testing-purposes-#312

tiabc reviewed Oct 16, 2017

View reviewed changes

influx6 approved these changes Oct 16, 2017

View reviewed changes

Maxim Shcherbo added 2 commits October 17, 2017 00:35

CR notices fixed:

3cc4c74

1) Genesis block revised 2) StatusChain network is now created in its own folder

Merge branch 'develop' of https://github.com/status-im/status-go into…

de60698

… issue/introduce-private-network-for-testing-purposes-#312

tiabc approved these changes Oct 16, 2017

View reviewed changes

tiabc merged commit 26fcfda into status-im:develop Oct 16, 2017

screwyprof deleted the issue/introduce-private-network-for-testing-purposes-#312 branch October 16, 2017 22:00

Introduced a private Ethereum network for testing purposes #397

Introduced a private Ethereum network for testing purposes #397

Conversation

screwyprof commented Oct 12, 2017 • edited

adambabik commented Oct 13, 2017

screwyprof commented Oct 13, 2017

adambabik 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

screwyprof Oct 13, 2017 • 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

Choose a reason for hiding this comment

tiabc 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

screwyprof Oct 16, 2017 • 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

Choose a reason for hiding this comment

influx6 left a comment

Choose a reason for hiding this comment

tiabc left a comment

Choose a reason for hiding this comment

screwyprof commented Oct 12, 2017 •

edited

screwyprof Oct 13, 2017 •

edited

screwyprof Oct 16, 2017 •

edited