Add testmode #426

LukasGasior1 · 2018-03-19T10:43:17Z

added JSON RPC via Unix Domain Socket
added testmode
1831 out of 1837 Solidity test cases passing (6 failing)

TODO:

unit tests (at least for IPC, test_ endpoints are kind of covered by those Solidity tests anyway)
fix remaining 6 Solidity tests (possibly another PR) // edit: it seems, at lest some of these tests, are failing because of Mantis not properly handling "pending" block parameter in RPC api, see task: EC-510 for more details

dwightguth · 2018-03-22T16:30:29Z

src/main/scala/io/iohk/ethereum/jsonrpc/IeleJsonMethodsImplicits.scala

+      gas <- optionalQuantity(obj \ "gas")
+      gasPrice <- optionalQuantity(obj \ "gasPrice")
+      value <- optionalQuantity(obj \ "value")
+      functionName = (obj \ "functionName").extractOpt[String]


we've previously used the name "function" for this field instead of "functionName". If you don't have a strong opinion, it may be simpler to do the same thing here.

dwightguth · 2018-03-22T17:02:25Z

src/main/scala/io/iohk/ethereum/jsonrpc/TestJsonMethodsImplicits.scala

+      for {
+        eIP150ForkBlock <- extractQuantity(blockchainParamsJson \ "EIP150ForkBlock")
+        eIP158ForkBlock <- extractQuantity(blockchainParamsJson \ "EIP150ForkBlock")
+        accountStartNonce <- extractQuantity(blockchainParamsJson \ "EIP150ForkBlock")


this code looks a little suspicious to me. Is this a copy/paste error or was it deliberate?

lol! I totally overlooked that.

dwightguth · 2018-03-22T19:55:34Z

src/universal/conf/testmode.conf

+  }
+
+  network.rpc {
+    apis = "eth,web3,net,personal,daedalus,test"


this should include iele if we want to use testmode for the solidity-to-iele test suite, right?

dwightguth · 2018-03-23T18:32:22Z

src/main/scala/io/iohk/ethereum/jsonrpc/EthService.scala

+    }
+  }
+
+  def ieleCall(req: IeleCallRequest): ServiceResponse[CallResponse] = {


iele transactions return a list of values rather than a single value, so I believe you want to make a IeleCallResponse class and perform rlp decoding in this method.

dwightguth · 2018-03-26T15:53:30Z

src/main/scala/io/iohk/ethereum/extvm/VMClient.scala

    messageHandler: MessageHandler,
    testMode: Boolean)
  extends Logger {

  def sendHello(version: String, blockchainConfig: BlockchainConfig): Unit = {
    val config = BlockchainConfigForEvm(blockchainConfig)
-    val configMsg = msg.Hello.Config.EthereumConfig(buildEthereumConfigMsg(BlockchainConfigForEvm(blockchainConfig)))
+    val configMsg = externalVmConfig.vmType match {
+      case VmConfig.ExternalConfig.VmTypeIele => msg.Hello.Config.IeleConfig(buildIeleConfigMsg())


based on our discussion on slack, weren't we planning for our near-term solution to be to make IeleConfig also be passed on every CallContext?

@dwightguth done

…C458' into feature/testmode

…/testmode

rtkaczyk · 2018-03-29T10:06:28Z

Could you add some info on how to setup and run solidity tests?

LukasGasior1 · 2018-03-29T10:35:36Z

@rtkaczyk

first, you need to build solidity from sources: https://solidity.readthedocs.io/en/latest/installing-solidity.html#building-from-source
then you start Mantis in testmode (uncomment include testmode.conf)
and run the tests: ETH_TEST_IPC="~/.mantis-testmode/mantis.ipc" ./test/soltest -- --no-smt

by the way, DO NOT REVIEW 😄 I'm resolving conflicts and doing a small refactor on this occasion

LukasGasior1 · 2018-03-30T10:57:11Z

src/main/scala/io/iohk/ethereum/jsonrpc/TransactionRequest.scala

@@ -20,8 +20,19 @@ case class TransactionRequest(
      nonce = nonce.getOrElse(defaultNonce),
      gasPrice = gasPrice.getOrElse(defaultGasPrice),
      gasLimit = gasLimit.getOrElse(defaultGasLimit),
-      receivingAddress = to,
+      receivingAddress = if (to.contains(Address(0))) None else to,


It seems this is the default behavior in cpp-ethereum (and what solidity tests expect). To me it (kind of) makes sense, but is a bit counterintuitive for caller (what if you really want to send tx to address 0x0?). WDYT? Other solution is to use this logic only in testmode.

Well, you know... https://gastracker.io/addr/0x0000000000000000000000000000000000000000

This is of course a result of many mistakes, but maybe, for reasons beyond my imagination, someone has interest in sending Ether there. I'd say if it's not too much trouble go with testmode only, if not, don't worry about it.

Btw, the expression could be simplified as:

to.filter(_ != Address(0))

loverdos · 2018-03-30T12:57:20Z

src/main/resources/application.conf

+        # See: https://github.com/input-output-hk/mantis/wiki/Creating-self-signed-certificate-for-using-JSON-RPC-with-HTTPS
+        mode = "http"
+
+        # Whether to enable JSON-RPC endpoint


Should this be

Whether to enable JSON-RPC HTTP(S) endpoint

?

Also, minor, we could use the shorter HTTP(S) instead of HTTP/HTTPS

loverdos · 2018-03-30T13:20:04Z

There is a compilation error:

[error] /mantis/src/snappy/scala/io/iohk/ethereum/snappy/Prerequisites.scala:53: not found: type ValidatorsBuilder
[error]   private val components = new StdTestConsensusBuilder with ValidatorsBuilder with SyncConfigBuilder {
[error]                                                             ^
[error] one error found

loverdos · 2018-03-30T13:50:51Z

src/main/scala/io/iohk/ethereum/nodebuilder/NodeBuilder.scala

@@ -353,12 +382,6 @@ trait OmmersPoolBuilder {
  lazy val ommersPool: ActorRef = system.actorOf(OmmersPool.props(blockchain, ommersPoolSize))
 }

-trait ValidatorsBuilder {


This deletion causes a compilation error in snappy. Check io.iohk.ethereum.snappy.Prerequisites.components.

rtkaczyk · 2018-03-30T14:09:02Z

src/main/resources/application.conf

@@ -17,6 +17,9 @@ mantis {
  # timeout for shutting down the ActorSystem
  shutdown-timeout = "15.seconds"

+  # Whether to run Mantis in test mode (enables test_ RPC endpoints; similar to --test flag in cpp-ethereum)


Does it change other functionality? If not, could be moved into rpc section.

It does. If running in testmode we use different Ledger/Consensus implementations.

In that case maybe we should document how the functionality differs in test mode? Or is that already documented somewhere else?

rtkaczyk · 2018-04-09T15:22:40Z

src/main/scala/io/iohk/ethereum/jsonrpc/server/ipc/JsonRpcIpcServer.scala

+      override def run(): Unit = {
+        while (!serverSocket.isClosed) {
+          val clientSocket = serverSocket.accept()
+          new ClientThread(jsonRpcController, clientSocket).start()


Are we just piling up these threads?

It's one thread per connection; it stops when connection closes

Why not use a thread pool?

@loverdos after taking a second thought I'm not sure if it's the best idea. What are the benefits of using a thread pool here?

Avoid a new thread for each new connection?

You mean sharing one thread for multiple connections? I think it doesn't make sense with current approach (we're using blocking socket reading)

I am mostly concerned with preventing an unbounded number of threads and/or outstanding requests and I was thinking in the lines of using an appropriately configured Executor.

rtkaczyk · 2018-04-09T15:25:22Z

src/main/scala/io/iohk/ethereum/testmode/TestLedgerWrapper.scala

+    blockchain: BlockchainImpl,
+    syncConfig: SyncConfig,
+    consensus: Consensus,
+    var blockchainConfig: BlockchainConfig) { // var as it's modifiable by test_ RPC endpoints


Couldn't we just create a new instance of this class?

Not really. It's a bit tricky, because Ledger is being passed as a parameter to various other components on initialization (see NodeBuilder.scala). I wanted to create new instances, but I didn't find an elegant way to update those classes with new ledger (without introducing a var elsewhere).

rtkaczyk · 2018-04-09T15:26:16Z

src/main/scala/io/iohk/ethereum/testmode/TestmodeConsensus.scala

+    blockchain: BlockchainImpl,
+    blockchainConfig: BlockchainConfig,
+    consensusConfig: ConsensusConfig,
+    var blockTimestamp: Long = 0) // var, because it can be modified by test_ RPC endpoints


Similar as above, couldn't we create a new instance?

rtkaczyk · 2018-04-09T15:30:19Z

unit tests (at least for IPC, test_ endpoints are kind of covered by those Solidity tests anyway)

I take it this was done?

rtkaczyk · 2018-04-11T07:13:20Z

@LukasGasior1 I'm unable to run the tests. Trying the command you provided:

$ ETH_TEST_IPC="~/.mantis-testmode/mantis.ipc" ./test/soltest -- --no-smt
bash: ./test/soltest: No such file or directory

So I tried:

$ find . -name soltest
./build/test/soltest

$ ETH_TEST_IPC="~/.mantis-testmode/mantis.ipc" ./build/test/soltest -- --no-smt
Test setup error: std::exception: No test path specified. The --testpath argument is required.

Any hints?

loverdos · 2018-04-11T11:12:29Z

src/main/scala/io/iohk/ethereum/jsonrpc/server/ipc/JsonRpcIpcServer.scala

+
+  def close(): Unit = {
+    serverSocket.close()
+    removeSocketFile()


If close() throws an Exception, the file will not be removed.

loverdos · 2018-04-11T15:05:40Z

@LukasGasior1 Could you git cherry-pick 0b033e2 ? It will make testing a bit easier.

Also drop the errors in favour of warnings for just a bit better developer experience.

rtkaczyk · 2018-04-16T11:34:24Z

$ ETH_TEST_IPC="$HOME/.mantis-testmode/mantis.ipc" ./build/test/soltest -- --no-smt
Running 1837 test cases...
/home/radek/iele/solidity/test/libsolidity/SolidityEndToEndTest.cpp(1502): error in "blockchain": Invalid encoded data
   Result                                                           Expectation
   ..............................................................1c ..............................................................1c
   ........................1212121212121212121212121212121212121212 ........................1212121212121212121212121212121212121212
 X ...............................................................6 ...............................................................7

/home/radek/iele/solidity/test/libsolidity/SolidityEndToEndTest.cpp(1552): error in "now": Invalid encoded data
   Result                                                           Expectation
   ...............................................................1 ...............................................................1
 X .............................................................22c .............................................................22d

/home/radek/iele/solidity/test/libsolidity/SolidityEndToEndTest.cpp(10784): error in "snark": check callContractFunction("pair()") == encodeArgs(true) failed
/home/radek/iele/solidity/test/libsolidity/SolidityEndToEndTest.cpp(10785): error in "snark": check callContractFunction("verifyTx()") == encodeArgs(true) failed
/home/radek/iele/solidity/test/contracts/Wallet.cpp(574): error in "multisig_value_transfer": check balanceAt(destination) == 100 failed [0 != 100]
/home/radek/iele/solidity/test/contracts/Wallet.cpp(644): error in "revoke_transaction": check balanceAt(destination) == 100 failed [0 != 100]

*** 6 failures detected in test suite "SolidityTests"

Looking good, but do we have a plan to fix the remaining tests?

LukasGasior1 · 2018-04-16T11:39:29Z

@rtkaczyk I've spent some time trying to figure them out. I think they may be related to the bug with "pending" as block param. To be honest, I wouldn't like to spend more time investigating it. Especially that this task was originally a minor thing.

loverdos · 2018-04-18T12:28:25Z

@LukasGasior1
So, trying to understand what is going on: We introduce a new service, TestService, which we interface to the outer world via the existing JsonRpcController. The TestService initially starts with the original Mantis configuration but it will deviate as soon as some calls are made to:

  private def handleTestRequest(testService: TestService): PartialFunction[JsonRpcRequest, Future[JsonRpcResponse]] = ...

The crucial observation is that TestService and the related testmode facilities live in parallel to the rest of Mantis. If this is correct, then we do not have to worry about introducing inconsistencies to the rest of Mantis.

Having said that, using vars probably exposes the state a bit too eagerly. We could model state changes by respective updateXXX methods, e.g. updateBlockChainConfig. This resembles firing an event and gives the receiver the advantage of internally doing whatever else is necessary. This way we can fight possible inconsistencies inside the whole testmode.

So, for example, when we update blockchainConfig

mantis/src/main/scala/io/iohk/ethereum/testmode/TestLedgerWrapper.scala

Line 16 in 493da55

    
           var blockchainConfig: BlockchainConfig) { // var as it's modifiable by test_ RPC endpoints

in TestService

mantis/src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala

Line 103 in 3221837

testLedgerWrapper.blockchainConfig = newBlockchainConfig

what does it mean to just set a new value? Does it have to propagate further?

I am not saying there are inconsistencies with the current state change approach but I can identify a potential source.

LukasGasior1 · 2018-04-18T13:08:41Z

@loverdos

The main problem here is that, when we update the blockchain config in testmode, it should also be updated globally (wherever Ledger or related classes are used - miner etc). I solved that by introducing TestLedgerWrapper (notice the ledger implementation there is a def) and TestLedgerBuilder.TestLedgerProxy which delegates to the ledger in this mutable wrapper. What I wanted to avoid here is to couple non-test logic (sync, block execution etc) with testmode functionality (note, that with this solution all remaining classes don't even know they're using test/mutable ledger).

what does it mean to just set a new value? Does it have to propagate further?

yes, subsequent references to ledger (from any place) will use this new confg - thanks do this wrapper mechanism.
This approach may not be perfect, but it in my opinion it has two strong pros:

existing code stays untouched and is not aware of testmode existence
this mechanism is only used in testmode, it does not affect a "normally" running node in any way, see:

class StdNode extends BaseNode with StdLedgerBuilder with StdConsensusBuilder
class TestNode extends BaseNode with TestLedgerBuilder with TestmodeConsensusBuilder with TestServiceBuilder

so without testmode enabled, there are really no vars involved

loverdos

Let's add a // Note in JsonRpcIpcServer where we create Threads, so that we can revisit this.

loverdos · 2018-04-18T12:00:22Z

src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala

+  }
+
+  def mineBlocks(request: MineBlocksRequest): ServiceResponse[MineBlocksResponse] = {
+    def mineBlock(): Future[Unit] = {


If consensus is not ethash do we still want to run mineBlock?

^ OK I saw that there is a specific Consensus wired, not the one used by the rest of Mantis.

rtkaczyk · 2018-04-24T08:02:16Z

@LukasGasior1 can this be merged now?

…/testmode

Łukasz Gąsior added 5 commits March 19, 2018 11:20

wip - testmode

a5930d7

Cleanup

d9f1b5c

Fix scalastyle

a5d0cc9

Add iele_ transaction endpoints

c6a172f

Cleanup

ce3d726

dwightguth reviewed Mar 22, 2018

View reviewed changes

Łukasz Gąsior added 2 commits March 22, 2018 18:12

Rename 'functionName' param to 'function'; fix chain config msg

3448160

Fix

34018ab

dwightguth reviewed Mar 22, 2018

View reviewed changes

Łukasz Gąsior added 3 commits March 23, 2018 11:51

Fix

943fd3b

Fix test

9e8b4de

Send proper config based on vm type

786f0a7

dwightguth reviewed Mar 23, 2018

View reviewed changes

Update iele call response

a8c3a99

dwightguth reviewed Mar 26, 2018

View reviewed changes

Łukasz Gąsior added 4 commits March 26, 2018 20:01

Send config on every request; fix rlp decoding

99d0742

Kevm config in call ctx msg

69baedd

Merge remote-tracking branch 'origin/feature/consensus_as_a_service_E…

3a3195c

…C458' into feature/testmode

Merge remote-tracking branch 'origin/phase/iele_testnet' into feature…

04958f7

…/testmode

Łukasz Gąsior added 4 commits March 29, 2018 13:55

wip

d8f62df

wip

db7776d

Cleanup

eb18f82

Use block generator from consensus in TestService

3221837

LukasGasior1 commented Mar 30, 2018

View reviewed changes

loverdos reviewed Mar 30, 2018

View reviewed changes

rtkaczyk reviewed Mar 30, 2018

View reviewed changes

Łukasz Gąsior added 5 commits April 3, 2018 10:07

Sending to address 0x0 not allowed only in testmode

dfbc9ab

Update config

0d56e6d

Fix compile in snappy

2e2be41

Fix ethCompatibleStorage param in blockPreparator; remove default value

e43749a

Better config doc

8814879

rtkaczyk reviewed Apr 9, 2018

View reviewed changes

loverdos reviewed Apr 11, 2018

View reviewed changes

Christos KK Loverdos and others added 2 commits April 11, 2018 17:28

Upgrade sbt-verify to 0.4.1

78e5228

Also drop the errors in favour of warnings for just a bit better developer experience.

Wrap socket close in Try

fc4ffef

Łukasz Gąsior added 2 commits April 16, 2018 14:08

Fix it test

e8d2fb7

Fix ets

493da55

rtkaczyk approved these changes Apr 17, 2018

View reviewed changes

loverdos suggested changes Apr 19, 2018

View reviewed changes

Add thead pool note

0b6d237

loverdos approved these changes Apr 19, 2018

View reviewed changes

Łukasz Gąsior added 2 commits April 24, 2018 10:36

Merge remote-tracking branch 'origin/phase/iele_testnet' into feature…

380e8a0

…/testmode

Fix

fe62c56

LukasGasior1 merged commit 8f4961a into phase/iele_testnet Apr 24, 2018

LukasGasior1 deleted the feature/testmode branch April 24, 2018 10:07

Add testmode #426

Add testmode #426

Uh oh!

Conversation

LukasGasior1 commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtkaczyk commented Mar 29, 2018

Uh oh!

LukasGasior1 commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loverdos Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loverdos commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loverdos Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtkaczyk commented Apr 9, 2018

Uh oh!

rtkaczyk commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loverdos commented Apr 11, 2018

Uh oh!

rtkaczyk commented Apr 16, 2018

Uh oh!

LukasGasior1 commented Apr 16, 2018

LukasGasior1 commented Mar 19, 2018 •

edited

Loading

loverdos Mar 30, 2018 •

edited

Loading

loverdos Apr 18, 2018 •

edited

Loading

LukasGasior1 commented Apr 18, 2018 •

edited

Loading