Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait one block confirmation in tests #1438

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fullkomnun
Copy link
Contributor

@fullkomnun fullkomnun commented Aug 27, 2023

What does this implement/fix? Explain your changes.

  • Currently CONFIRMATIONS from 'config' is set to the default value of '12' or according to the matching env var if defined. It is referenced in common-files/utils/event-queue.mjs which is used by 'nightfall-optimist' and 'nightfall-client' to wait for a configured number of block confimrations before a L1 event is propagated and processed allowing for L1 re-org.
    During tests you usually want to either use a lower number of confirmations (such as 1) or when tests rely on this behaviour using a higher number of confimrations (like when testing L1 re-org) but leveraging Ganache like that:

test/utils.mjs

  async evmMine(n = 1) {
    await this.web3.currentProvider.send({
      jsonrpc: '2.0',
      method: 'evm_mine',
      params: [{ blocks: n }],
      id: 0,
    });
  }

some test file

await nf3Proposer.makeBlockNow();
await web3Client.waitForEvent(eventLogs, ['blockProposed']);
await web3Client.evmMine(CONFIRMATIONS + CONFIRMATIONS_BUFFER);
  • In .github/workflows/on-pull-request-master.yml most e2e test are setup with CONFIRMATIONS: 1, however value of env vars are not automatically propagated to containers and therefore the default value of '12' applies. Fixed that by editing docker-compose.*.yml files.
  • test/utils.mjs also waits for a number of block confirmations in 'submitTransaction' and in 'waitForEvent'. Curently it is hardcoded and set to '12', fixed it to grab the confirmations number from 'config'
  • Nf3 (from /cli) used by tests and 'proposer' / 'challenger' currently sets web3.eth.transactionConfirmationBlocks to the hardcoded value of '12'. Fixed that to use process.env.CONFIRMATIONS || 12 instead ('config' is not currently used by Nf3 / CLI so I did not use node 'config').
  • By default Ganache which is used for dev/test mines a block every second, so running tests when effectively client/optimist or test utils waits for 12 confirmations leads to excessive waiting and increases tests runtime.

Does this close any currently open issues?

Not that I'm aware of

What commands can I run to test the change?

Run all tests as usuall both locally and in CI (focus on these e2e tests that have CONFIRMATIONS: 1 which is all e2e tests except for 'client-authentication-test', 'test-apps')

Any other comments?

  • 'adversary-test' total runtime has decreased from '55m 4s' to '35m 19s' - minus 20 minutes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant