Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

WIP: Update devnet to post-constantinople version #1775

Closed
wants to merge 5 commits into from
Closed

WIP: Update devnet to post-constantinople version #1775

wants to merge 5 commits into from

Conversation

albrow
Copy link
Contributor

@albrow albrow commented Apr 11, 2019 •
edited
Loading

Description

This PR updates devnet to use our updated fork of Geth, which has support for post-Constantinople features.

Still WIP only because I haven't published the latest Docker image and re-enabled Geth tests in CI yet.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@albrow albrow added the WIP label Apr 11, 2019
albrow
albrow commented Apr 11, 2019
View reviewed changes
contracts/test-utils/src/assertions.ts
@@ -42,7 +42,7 @@ async function _getInsufficientFundsErrorMessageAsync(): Promise<string> {
}

async function _getTransactionFailedErrorMessageAsync(): Promise<string> {
return _getGanacheOrGethErrorAsync('revert', 'always failing transaction');
return _getGanacheOrGethErrorAsync('revert', 'Transaction failed');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest version of Geth changed this error message, so I had to update our assertions.

albrow
albrow commented Apr 11, 2019
View reviewed changes
contracts/test-utils/src/web3_wrapper.ts
@@ -40,7 +40,7 @@ export const txDefaults = testProvider === ProviderType.Ganache ? ganacheTxDefau
const gethConfigs = {
shouldUseInProcessGanache: false,
rpcUrl: 'http://localhost:8501',
shouldUseFakeGasEstimate: false,
shouldUseFakeGasEstimate: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the gas estimator in the latest version of Geth cannot estimate gas correctly when deploying our smart contracts. We've had some issues in the past but on the latest version it basically never works correctly. As a workaround we decided to disable it and just use the fake gas estimator.

albrow
albrow commented Apr 11, 2019
View reviewed changes
packages/dev-utils/src/blockchain_lifecycle.ts
@@ -2,13 +2,6 @@ import { logUtils } from '@0x/utils';
import { NodeType, Web3Wrapper } from '@0x/web3-wrapper';
import * as _ from 'lodash';

// HACK(albrow): 🐉 We have to do this so that debug.setHead works correctly.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I finally figured out why we needed this hack in the first place. The genesis.json file allows you to configure the block number for certain hard forks (e.g. the DAO hard fork or the Byzantium update). We had some of these configured to block number 1, 2, 3, and 4. I updated the genesis.json file to set all these block numbers to 0 so we no longer need to mine dummy blocks at the start of every test run.

@coveralls
Copy link

coveralls commented Apr 11, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 84.822% when pulling 0220a2b on fix/devnet-constantinople-update into c78a602 on development.

@albrow albrow force-pushed the fix/devnet-constantinople-update branch from 437e3b5 to 7f78756 Compare April 11, 2019 22:40
@albrow
Copy link
Contributor Author

albrow commented Apr 11, 2019 •
edited
Loading

Exchange core tests are failing in CI:


 |   16 passing (5s)
 |   1 failing
 | 
 |   1) Exchange core
 |        Testing exchange of ERC20 tokens with no return values
 |          should transfer the correct amounts when makerAssetAmount === takerAssetAmount:
 | 
 |       AssertionError: expected '0' to equal '-100000000000000000000'
 |       + expected - actual
 | 
 |       -0
 |       +-100000000000000000000

I can't reproduce this locally. Might take some time to figure out.

albrow
albrow commented Apr 12, 2019
View reviewed changes
packages/devnet/run.sh
@@ -5,6 +5,7 @@ mkdir -p /var/log

# Start Geth and direct output to stdout
/geth \
--allow-insecure-unlock \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to a new security feature added in the latest version of Geth, you can't unlock accounts and enable the RPC API without this flag.

@albrow
Copy link
Contributor Author

albrow commented Apr 12, 2019 •
edited
Loading

Just to update you on the current state of our Geth fork, our devnet is now using the 0x-testing-constantinople branch. I added 4 commits there. One adds support for the new debug_increaseTime method (cherry-picked from ethereum/go-ethereum#16834). The other three commits are hotfixes/workarounds to address some bugs in the current upstream version. Most of these hotfixes are probably not appropriate to merge upstream (e.g., flushing the transaction pool when you call debug_setHead might not be the behavior everyone wants) but they do work for our use case.

commit ee91ac42c646631c642f2e987dd9b3cc2689dfbf (HEAD -> 0x-testing-constantinople, 0x/0x-testing-constantinople)
Author: Alex Browne <[email protected]>
Date:   Fri Apr 12 14:59:52 2019 -0700

    Flush tx pool in debug_setHead

    Without flushing the tx pool, old transactions linger and are sometimes
    included in future blocks. This can mess up our tests, which assume the
    state changes in each test case are independent of all others and are
    completely reverted when you call setHead.

commit e9c114691e10a8d92ce556fb090ab13796024138
Author: Alex Browne <[email protected]>
Date:   Tue Jan 29 15:04:25 2019 -0800

    common, consensus, miner: introduce debug_increaseTime RPC method

commit 4b97eed19d55d0ec694cb4c08c1511c423c8dabd (fix-clique-set-head)
Author: Alex Browne <[email protected]>
Date:   Thu Apr 11 14:09:48 2019 -0700

    Add workaround for issue #19186

    See https://github.com/ethereum/go-ethereum/issues/19186.

    The root cause of this issue is a race condition in rpc/handler.go which
    causes eth_calls to return null (encoded in hex as "0x0"). The
    workaround, which was proposed by a community member on that issue page,
    is simply to change the order of two lines of code.

commit 3c6221e8d7bec445367da27db51f74572217e7c3
Author: Alex Browne <[email protected]>
Date:   Thu Apr 11 14:07:26 2019 -0700

    Add workaround for issue #19346

    See https://github.com/ethereum/go-ethereum/issues/19346.

    The root cause is a nonce check that prevents the miner from adding
    any new transactions to the block after debug_setHead is called due to
    the nonce being higher than expected. The workaround is simply to remove
    this nonce check.

@albrow albrow mentioned this pull request May 23, 2019
Closed
@albrow albrow mentioned this pull request Jun 5, 2019
Merged
4 tasks
@albrow albrow closed this Jun 5, 2019
@albrow
Copy link
Contributor Author

albrow commented Jun 5, 2019

See #1831 (comment). We're not going to try to maintain our own set of extra features on Geth moving forward.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abandeali1 abandeali1 Awaiting requested review from abandeali1 abandeali1 is a code owner

@fabioberger fabioberger Awaiting requested review from fabioberger

@hysz hysz Awaiting requested review from hysz

@LogvinovLeon LogvinovLeon Awaiting requested review from LogvinovLeon

Assignees
No one assigned
Labels
contracts @0x/contracts-dev-utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@albrow @coveralls