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

fix: optimist resync over a bad block, tidy docker-compose yml #870

Merged
merged 33 commits into from
Sep 9, 2022

Conversation

Westlad
Copy link
Contributor

@Westlad Westlad commented Aug 12, 2022

fixes #844
fixes #862

This PR fixes the issue whereby, if Optimist resyncs when there is an un-challenged bad block, it will become stuck - unable to start making blocks because it knows there is a bad block but unable to challenge the bad block because its challenge function is switched off during re-sync. The challenge function is switched off so that it doesn't challenge already resolved bad blocks during sync and waste huge amounts of gas.

To fix this issue, we note that, previously, whenever optimist detects a challengeable block, either during re-sync or in normal operation, it pushes a dummy function into the stop queue. This correctly prevents optimist making blocks until the challenge is resolved, at which point the dummy function is removed from the queue and normal block-making resumes.

In this PR we no longer push a dummy function - we push the actual challenge commitment. If optimist is in normal operation, this has no effect - it's queued and dequeued just like the dummy function was (and not run); if however optimist is syncing, it will wait until synchronisation is complete and then run any challenge commitments that remain in the stop queue (because the re-sync did not resolve them and so they really are outstanding). This fixes the issue of a bad block.

This PR also:

  • adds the ability to manually start and stop challenging via an API - this was needed for testing but could be more generally useful.
  • makes the nf3.startChallenger() function return an event emitter for consistency with startProposer(). There's now no need to call a separate getter.
  • adds a rollback complete event to that emitter (again used for testing but probably more generally useful).
  • removes the need to register a challenger. Instead, we make the challenger itself responsible for deciding whether it should respond to a CommitToChallenge event (received from optimist via its websocket) with a reveal, rather than delegating the decision to optimist. This is much cleaner because the Challenger definitely knows its own address and can well make the response decision itself. Thus optimist will send all such events to all connected challengers rather than a specific one that has registered the correct address. As there should be very few such events, this is not a significant comms overhead.
  • The docker-compose files have been updated to remove the second optimist and client and dependent containers/volumes. These are not used and so are just a waste of resource. The remaining ones are renamed without the trailing ordinal (optimist1 -> optimist etc).
  • optimist synchronisation tests are added to the CI tests.
  • The setup-nightfall script has been modified to build containers sequentially with individual build commands because otherwise the docker compose build command often fails.

@Westlad Westlad force-pushed the westlad/challenge-sync branch from 4734ece to f580aca Compare August 15, 2022 16:21
@Westlad Westlad added DNM Do not merge and removed Merge conflict labels Aug 16, 2022
@Westlad Westlad added the Help wanted Extra attention is needed label Aug 16, 2022
@druiz0992
Copy link
Contributor

I took a look and found that when the challenge is signed, there is a revert

Transaction: 0x3426b99d97416a1f23308d0941ac3742adac2e4db18a710ce945c967d6989a2a
  Gas usage: 2055154
  Block Number: 636
  Block Time: Tue Aug 16 2022 22:35:48 GMT+0000 (Coordinated Universal Time)
  Runtime Error: revert
  Revert reason: The sibling path is invalid

Sibling path is fixed by #894, but there seems to be new problems now with websockets that prevent new blocks from being proposed.

nightfall_3-optimist-1  | [2022-08-31 10:28:11.629] WARN: Websocket to proposer is closed for rollback complete.  Waiting for challenger to reconnect

@Westlad Westlad removed Help wanted Extra attention is needed DNM Do not merge labels Sep 6, 2022
@Westlad
Copy link
Contributor Author

Westlad commented Sep 6, 2022

Adversary issues are fixed in latest master so this PR should be good to go now.

@ChaitanyaKonda
Copy link
Contributor

All the code looks good.

Something to clarify, because an optimist will produce a challenge commit when it sees a bad block, if multiple challengers are connected to the same optimist, all of them will receive this transaction. At this point, if all of them submit the transaction, the first mined transaction becomes the valid one. The rest lose gas. This is something that is bound to happen if each of them used different optimists anyway but in the case of the same optimist it will happen every time. Assuming this, the best recommendation would be for challenger to run their own optimist as opposed to sharing one ?

@ChaitanyaKonda ChaitanyaKonda added the One more approval needed One reviewer has approved this PR but another is needed label Sep 7, 2022
@Westlad
Copy link
Contributor Author

Westlad commented Sep 7, 2022

All the code looks good.

Something to clarify, because an optimist will produce a challenge commit when it sees a bad block, if multiple challengers are connected to the same optimist, all of them will receive this transaction. At this point, if all of them submit the transaction, the first mined transaction becomes the valid one. The rest lose gas. This is something that is bound to happen if each of them used different optimists anyway but in the case of the same optimist it will happen every time. Assuming this, the best recommendation would be for challenger to run their own optimist as opposed to sharing one ?

Yes, this is true. Nothing in this PR has changed that though; it's always been the case. The first challenger to post a successful challenge is the winner. Sharing an optimist if you are a challenger is indeed a bit silly because you want to have a faster optimist than anyone else (and also be luckier than anyone else because you can't control when your optimist will see the bad transaction). Challengers sharing an Optimist would be a bit like trying to win a Grand Prix with all of us in the same car!

@Westlad Westlad requested a review from pawelgrzybek September 8, 2022 15:08
@druiz0992 druiz0992 merged commit 3f67a57 into master Sep 9, 2022
@druiz0992 druiz0992 deleted the westlad/challenge-sync branch September 9, 2022 07:37
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
3 participants