Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Upgrading Web3 >= 1.2.2 #2688

Closed
cgewecke opened this issue Dec 15, 2019 · 21 comments
Closed

Upgrading Web3 >= 1.2.2 #2688

cgewecke opened this issue Dec 15, 2019 · 21 comments

Comments

@cgewecke
Copy link
Contributor

cgewecke commented Dec 15, 2019

This is a tracking issue for changes necessary at Truffle to upgrade Web3 above 1.2.1.

Resolve #2654

In #2654, users with long-running test suites saw performance reduction and random ganache disconnection in Truffle versions which use Web3 1.2.2.

Based on experiments with some of the projects reporting the bug, I think the underlying issue is the way that truffle/contract manages confirmations handling in combination with a fix in Web3 1.2.2 for broken behavior in that feature.

In Web3 1.2.1 and below, confirmations over http (incorrectly) fire once per second, 24 times, without querying the client. 1.2.2 fixes this by block polling and emitting if a new block has been mined. (See code). Note that polling only occurs if a conf handler is attached. The number of confirmations the handler should wait for is configurable.

Truffle contract automatically attaches confirmation handlers for all transactions here:

emitter.on("confirmation", handlers.confirmation.bind(emitter, context));

This means that in a unit test context with thousands of txs, each one will incur a large number of additional background block polling requests. In addition, truffle evm_reverts between the tests suites, affecting the block number values relevant for polling. It looks like the combination of these swamps ganache with unnecessary calls, slows things down a lot, and creates instability.

One solution might be to disable confirmation handling in unit tests (where it's rarely used). It could be opt-in.

Resolve #2657

#2657 has TS errors in the HDWalletProvider unit tests. These result from HDWalletProvider not implementing some parts of the Web3 HTTPProvider interface. There are notes about the interface in 2657 comment.

If the above seems unclear / incorrect / incomplete or there's anything that could be fixed at Web3 to help with this, please just ping.

Environment

  • Operating System:
  • Ethereum client:
  • Truffle version (truffle version):
  • node version (node --version):
  • npm version (npm --version):
@nogo10
Copy link

nogo10 commented Apr 24, 2020

A few packages have not been upgraded

-- truffle-config@1.1.16
-- truffle-provider@0.1.16 +-- truffle-interface-adapter@0.2.5 | -- web3@1.2.1
`-- web3@1.2.1

@elenadimitrova
Copy link

Now that we have ganache-cli accepting --chainId option can we prioritise pushing web3 up to at least 1.2.2 as getChainId was introduced there and we can't do proper transaction replay attack protection as per EIP-155 without it.

@cgewecke
Copy link
Contributor Author

via #3446

@cgewecke
Copy link
Contributor Author

Re-opening, I think the confirmations handler problem is still live. Sorry.

@cgewecke cgewecke reopened this Oct 29, 2020
@eggplantzzz
Copy link
Contributor

@cgewecke Can you link to any repos that are experiencing this issue so that we can test against it/them?

@jjgonecrypto
Copy link

@eggplantzzz we seem to be hitting it in Synthetix after upgrading ganache-core and it's transitive deps.

https://github.com/Synthetixio/synthetix (on develop branch)

See CI output for running solidity-coverage

@eggplantzzz
Copy link
Contributor

@justinjmoses What version of Truffle are you using? Are you just running Ganache, configuring Truffle to connect, and then running truffle test --network <ganacheNetwork>?

@jjgonecrypto
Copy link

@eggplantzzz We're going through solidity-coverage to run truffle - https://github.com/sc-forks/solidity-coverage/blob/master/package.json#L28 and actually running it all through buidler. Though @cgewecke had mentioned he thinks it's coming in via builder-truffle5 - https://github.com/nomiclabs/hardhat/blob/buidler-truffle5-v1.3.4/packages/buidler-truffle5/package.json#L36 - which is using a fork of truffle-contract - https://github.com/nomiclabs/truffle/blob/master/packages/contract/package.json

@cgewecke
Copy link
Contributor Author

cgewecke commented Oct 31, 2020

Hi @eggplantzzz Ganache is being launched as a server within a Truffle plugin like this:

await pify(server.listen)(port);

Will try to do some additional testing over the weekend to see if commenting out the confirmations handler attachment line stabilizes things at Synthetix.

As noted in the opening comment in this issue, attaching that handler now results in a large number of additional eth_getBlockNumber requests per transaction. In Web3 1.2.1 confirmations over http was broken and didn't query the client to make sure the block had changed before firing a conf.

The relevant code at Web3 is here and here

@cgewecke
Copy link
Contributor Author

cgewecke commented Nov 2, 2020

@eggplantzzz

Results of the synthetix repo experiment are:

If you click into the CI jobs you can see current truffle/contract running much more slowly: only about ~half the tests execute over 55min (and they crash).

The passing jobs complete all tests in ~40min.

@eggplantzzz
Copy link
Contributor

eggplantzzz commented Nov 5, 2020

@cgewecke So is this something that it will be possible for me to reproduce locally? Since the number of confirmations is configurable, is it a possible solution to pass in a provider with it configured to 0 for testing? Just a thought.

I'm not sure if that solution makes sense, to be honest I'm not familiar with the way @truffle/contract deals with those handlers and how they work with everything. Let me know if you have a project that would allow me to experiment and test this.

@cgewecke
Copy link
Contributor Author

cgewecke commented Nov 5, 2020

is this something that it will be possible for me to reproduce locally

@eggplantzzz

It's challenging to reproduce as a simple test because it only becomes a problem at scale e.g with test suites that run for several minutes.

Maybe by running transactions in a loop and watching the log output from a separate ganache instance? You should see abnormally large numbers of eth_getBlockNumber requests.

[EDIT]

The number of confirmations to poll for is configurable via

@cgewecke
Copy link
Contributor Author

cgewecke commented Nov 9, 2020

Believe another example of this problem has been reported at trufflesuite/ganache#702 (comment)

@MaxFeldman1
Copy link

MaxFeldman1 commented Nov 10, 2020

This just started happening for me when I load a transaction into truffle debug after my project reached some size, it does not happen when I debug transactions from smaller projects

@MaxFeldman1
Copy link

MaxFeldman1 commented Nov 10, 2020

I don't know that it has anything to do with the amount/size of tests perhaps it has something to do with the total amount of bytecode/abi complexity within the project

I am getting this with Truffle v5.1.30

@eggplantzzz
Copy link
Contributor

Thanks for all the info guys, I'm going to meet with @gnidan and see if we can figure out a solution to this problem.

@MaxFeldman1
Copy link

I tried with geth and could not reproduce for me this seems to be specific to ganache

@gnidan
Copy link
Contributor

gnidan commented Nov 28, 2020

The workaround in #3566 has been released in https://github.com/trufflesuite/truffle/releases/tag/v5.1.55

@eggplantzzz
Copy link
Contributor

eggplantzzz commented Dec 1, 2020

@cgewecke or @MaxFeldman1 can either of you confirm that the workaround mentioned above fixes this problem?

We didn't really think it was great to disable the confirmation listener across the board so you basically configure networks['myNetwork'].disableConfirmationListener = true in truffle-config.js to turn it off for your testing network config.

So for example in truffle-config.js:

module.exports = {
  ... // some config stuff
  networks: {
    testNetwork: {
      host: <host>,
      port: <port>,
      network_id: "*",
      disableConfirmationListener: true
    },
    ... // rest of config stuff
};

If you want it to apply to the default test network (where you won't have to run Ganache separately) then you can name it "test" and that is what Truffle will use while testing.

@cgewecke
Copy link
Contributor Author

cgewecke commented Dec 1, 2020

Sorry, yes the fix LGTM. Thanks! Closing.

@cgewecke cgewecke closed this as completed Dec 1, 2020
@eggplantzzz
Copy link
Contributor

Thanks @cgewecke!

ci-syg pushed a commit to sygnumbank/syguard-contracts that referenced this issue Jan 18, 2024
Fix ganache crashing according to trufflesuite/truffle#2688

See merge request sygnum/blockchain-engineering/ethereum/solidity-edge-contracts/solidity-edge-dchf-contracts!34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants