feat: add support for external coordinator mode #13
Conversation
bendanzhentan
force-pushed
the
opbnb-testnet-op-coordinator
branch
from
17113eb to
6ed426b
Compare
|
I have dropped the previous first one commit 845168fae7527aa3e8b327dbf244d12002d7deb6 "feat: add API admin_sequencerStopped" since the Optimism team has added a similar API |
| if err := d.CanStartBuildingBlock(); err != nil { | ||
| d.log.Error("sequencer is not allowed to build block", "err", err) | ||
| d.nextAction = d.timeNow().Add(time.Second) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
i have some questions about coordinator logic:
- should cancel the whole sequencer action(not just startBuildingBlock step) if cooridinatorClient return error?
- need move this part in front of
case <-sequencerCh:? in other word, we should not run intocase <-sequencerCh:if node is inactive sequencer - if cooridinatorClient return error, what's the proper frequency to check if cooridinatorClient return ok in future?
- if the node is not active sequencer, seems the SequencerActive api will return true?
- there's a case could result in blockchain fork, if the active sequencer was turned inactive by coordinator in the middle of a block building(startBuildingBlock finished, completeBuildingBlock not)
There was a problem hiding this comment.
Hi @bnoieh, thanks for your review.
1/2. I agree with your comments on (1) and (2). If we move the check coordinator_startBuildingBlock to the front of case <- sequencerCh, we can achieve the behavior of "inactive sequencer not entering case <- sequencerCh" and cancel all sequencer actions. This suggestion is great, and I will update it as soon as possible.
3. The change is related to 1/2. I will make some experiments and then reply.
4. admin_sequencerActive should return false for inactive sequencers.
5. It's a big question. In simple terms, the current HA mechanism cannot ensure to prevent fork 100% effectively.
| if err != nil { | ||
| return fmt.Errorf("failed to call coordinator_requestBuildingBlock: %w", err) | ||
| } | ||
| if respErr != nil { |
There was a problem hiding this comment.
error is passed by value in golang. So I guess respErr will always be nil?
There was a problem hiding this comment.
I am not sure about it. Would you be able to confirm?
Returns true if the node is actively sequencing, otherwise false. This is essentially a parallel with the `eth_mining` API from execution clients.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
The Optimism sequencer is single-entity and the current implementation does not include a fail-over mechanism. Which means that the current Optimism sequencer is a single point of failure. If it goes down, the whole optimistic rollup stops producing blocks.
External Coordinator comes to prevent this from happening, it ensures that only one instance of Optimism sequencer is producing and if it goes down, it will be replaced by another instance.
This PR adds support for optional external coordinator mode, by providing the following flags,
e.g.
In order to produce new unsafe blocks, the sequencer must request building-block permission from the coordinator first.