Openzeppelin-contracts: ethSendTransaction doesn't wait for the transaction to be mined

[colltoaction]

🎉 Description

I'm running your test suite against the RSKj 0.5.0 node and found an issue with some tests.

• [X] 🐛 This is a bug report.

💻 Environment

• OpenZeppelin master branch (commit https://github.com/OpenZeppelin/openzeppelin-solidity/commit/6c4c8989b399510a66d8b98ad75a0979482436d2)
• RSK 0.5.0 regtest network
• npx truffle test --network rsk with my node listening at localhost:4444

📝 Details

Let's take for example the BreakInvariantBounty tests. The it can set reward test fails because sendReward returns immediately, before the block including the transaction is mined. The balance of this.bounty.address hasn't been updated yet and the check fails.

🔢 Code To Reproduce Issue

I ran the test code against the RSK node and it failed consistently.

I also verified that both of these changes make the test pass:

it('can set reward', async function () {
  await sendReward(owner, this.bounty.address, reward);
  await advanceBlock();
  const balance = await ethGetBalance(this.bounty.address);
  balance.should.be.bignumber.equal(reward);
});

it('can set reward', async function () {
  await transactionMined(await sendReward(owner, this.bounty.address, reward));
  const balance = await ethGetBalance(this.bounty.address);
  balance.should.be.bignumber.equal(reward);
});

I could send a PR if you want, let me know.

Thanks!

[nventuro]

Hi @tinchou! We only run our tests agaisnt ganache, which AFAIK mines a block immediately on each transaction, so I'd expect there to be multiple tests with this same issue. Is this the only one causing you trouble?

[colltoaction]

No, there are multiple issues but this is the first one I could isolate.

Is running your tests on full nodes something you want to do? Your test suite is very valuable for us, but we could just fork if this isn't something you're interested in.

[nventuro]

We've actually discussed that same thing very recently, since there are some minor differences between ganache and a real node (IIRC this caused a false positive here due to the default gas stipend not being the same, though I may be wrong). Improving our test suite so that it can be reused (and even exporting the test helpers as a separate package) is something we intended to work on for 2.1, though that will probably have to wait until 2.2 (about two months from now).

I'd hate however to have the test suite littered with transactionMined or advanceBlock in every single test case: isn't there a cleaner way of handling this?

[colltoaction]

It's hard for me to say. As a node developer, I would argue that the behavior of the ganache node doesn't reflect the reality of an Ethereum/RSK node. On the other hand, ganache makes it easier to test contracts, and the fact that we could use your suite as a sort of integration test suite for our node is incidental.

I'll let you know if I can work a little more on this to get a sense of the amount of code needed to make it compatible.

[frangio]

@tinchou We are definitely interested in running the test suite against actual nodes. The main obstacle is probably our use of ganache's evm_increaseTime method, though, which we need in order to test some time-dependent contracts such as TokenTimelock. Do you have any ideas for working around that?

I agree with @nventuro that it would be very bad to have the test suite littered with these function calls. We should be able to solve it at a lower level.

[colltoaction]

We started working on it, hopefully we can collaborate! Here's a preview:

https://github.com/rsksmart/openzeppelin-solidity
https://circleci.com/gh/rsksmart/openzeppelin-solidity

I saw just a minor issue with our evm_increaseTime implementation where there's an off by one second error, but it mostly works.

The main issue is that Circle CI aborts the test run after 5 hours. I plan to investigate and come up with alternatives on Monday. One idea I had was to reduce the time between blocks in the node configuration for these tests.

[colltoaction]

Good news! We just had a successful test run on the public Circle CI server:

https://circleci.com/gh/rsksmart/openzeppelin-solidity/23

Current status:

• 1606 passing (2h)
• 175 pending

This was the main goal of this sprint's task, and I will move on to another task for now.

These are some things we've identified that could allow us to have 100% compatibility, and hopefully we could tackle them soon:

• The bundled miner client takes too much time with the current hard-coded parameters. We should at least introduce a configuration parameter to avoid hitting Circle CI's 5 hour time limit https://github.com/rsksmart/rskj/commit/3e501916465f29ca11ad0b4bd3fc9d83ba85a8b7
• Change eth_call behavior when the transaction is reverted https://github.com/paritytech/parity-ethereum/pull/8448
• Implement emv_mine optional timestamp parameter https://github.com/trufflesuite/ganache-core/pull/13
• Implement _insta-mining_ https://github.com/ethereum/go-ethereum/pull/15323

[nventuro]

That's great news @tinchou! Are those blockers specific to RSKj, or will they affect all other nodes? Also, how did you get the promises to resolve only once the transaction was mined?

[colltoaction]

It seems that some Ethereum implementations would not have them. EthereumJ probably would.

For transferring value I added an await. For transactions that are created through contract methods, it seems that Web3 creates filters and waits automatically. It just worked :).

[nventuro]

Awesome! This will be of help once we set up our tests against a real node, thanks :)

For reference, this is the bit that we'd need to port over to OZ to fix this issue.

[colltoaction]

@ignaciopulicedonatto started working on insta-mining. We've seen test times go down to the sub-10 minute mark :). It would seem that Truffle has code in place specially for Geth which makes things easier for them (and we're stuck with the workarounds). We'll look into it next week.

Happy long weekend!