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 for twelve L1 confirmations before updating L2 data. #426

Merged
merged 86 commits into from
Feb 4, 2022

Conversation

Westlad
Copy link
Contributor

@Westlad Westlad commented Jan 25, 2022

fixes #440

This is a significant change. It adds the latest version of the ping-pong tests, but that does not, of itself, affect mainstream code. However, to get our code to run reliably on Ropsten, this PR implements code to wait for 12 confirmations of a blockchain transaction before updating layer 2.

Our code won't run reliably on Ropsten without this PR.

The changes occur in two places:

  1. In the Nf3.submitTransaction method. The returned Promise now resolves after twelve confirmations (it used to resolve as soon as a transaction receipt was available). This uses web3js's logic, which seems to be able to cope with a chain reorg and doesn't require further input from the user (the effect of a chain-reorg is not documented, but experimentally it seems to wait for the transaction to be re-mined if a re-org occurs and then continue to wait for 12 confirmations of the re-mined transaction).
  2. The event queue. When an event is pushed into the queue it is bundled with code that will wait until the event has been confirmed 12 times. Thus, when the event reaches the front of the queue and runs, the Promise won't resolve until the event has been seen twelve times. Web3js doesn't directly handle a chain reorg in this circumstance, so we handle it ourselves: If, during this wait, the event is removed as part of a chain reorg, then the event is disregarded. It will then be re-mined and re-added to the event queue, and treated as a new event.

The tests are now slower, because they wait for the 12 confirmations. Down the road, we should revisit our original method of dynamically changing the L2 database in response to chain reorgs because this approach is more responsive. However, this will take more time than we have available now.

To test - you can test in the usual way (./start-nightfall, npm t) but to test the ping-pong code specifically, do the following:

cd /test/ping-pong
./pong-down -v # clears any volumes and containers from a previous run
./ganache-standalone -s # starts a mining ganache instance exposed to localhost, for the test to use
./pong-nightfall -d -s # starts nightfall, using the local ganache chain, deploys contracts and uses stubs

Then, in a separate terminal, use run

./pong-apps

The readme in the test/ping-pong directory has further details, e.g. for running on Ropsten.

NB: You will need to bring these containers down before you can run conventional tests. To do that:

./ganache-standalone -d
./pong-down -v

@Westlad Westlad changed the title Westlad/ping pong test Wait for twelve L1 confirmations before updating L2 data. Jan 27, 2022
@IlyasRidhuan
Copy link
Contributor

I think that as waitForConfirmation is blocking the state queue (queue[0]) it will also throttle block production as they use the same queue. Once a single blockProposed event is emitted, block production will be halted until 12 further L1 blocks have been seen. It might even be halted further if more blockProposed events are enqueued.

If this is the case, I don't think it is something we can figure out by modifying this PR (as I cannot think of how to get around this at the moment). Perhaps this warrants opening a new issue to figure out how we can prevent this. Finally, ROTATE_PROPOSER_BLOCKS is set to 4, we should remember to set this to something "reasonable" in the face of this 12 block confirmation.

@Westlad
Copy link
Contributor Author

Westlad commented Feb 1, 2022

Yes, this is true but seems to be a fairly fundamental limitation of the 12 confirmations approach. We discussed and can't see a viable way around it. The situation will improve considerably after the merge because of the faster PoS confirmation.

@IlyasRidhuan IlyasRidhuan added the One more approval needed One reviewer has approved this PR but another is needed label Feb 3, 2022
@Westlad Westlad merged commit 34aec0e into master Feb 4, 2022
@Westlad Westlad deleted the westlad/ping-pong-test branch February 4, 2022 09:30
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

🎉 This PR is included in version 1.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
One more approval needed One reviewer has approved this PR but another is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wait twelve confirmations of L1 before updating L2
4 participants