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

Draft: Devnet support #171

Merged
merged 16 commits into from
Sep 19, 2023
Merged

Draft: Devnet support #171

merged 16 commits into from
Sep 19, 2023

Conversation

borispovod
Copy link
Contributor

Hey,

I've created a draft pull request because I'm not certain all changes from the PR will be accepted. We implemented support for the OP devnet while familiarizing ourselves with Magi. This PR contains fixes for several issues as well as the logic for the devnet.

The first part focuses on fixes that can be particularly useful, as they address potential math overflows concerning state.purge and the calculation of the l1 start block during the initiation of the node and during reorgs. There are also updates to the Docker setup to support custom genesis and rollup JSON configurations. I believe these overflows should be addressed; for instance, if someone were to launch their own local instance of optimism using dev PoS L1, this could be problematic.

The second part pertains to the support of the OP devnet itself. We introduced a devnet parameter for the config because the default devnet employs PoW mining and doesn't finalize blocks. As such, the L1 watcher would interpret the latest block as finalized. This might be a relevant feature for Magi, and the PR includes updates to documentation and Docker concerning the devnet flag. I'd love to get your feedback on this.

Quick summary of PR:

  • Fixed overflow issues in state.purge and corrected the calculation of l1 start blocks passed to ChainWatcher during initiation and reorgs.
  • Updated Docker compose to support custom genesis and/or rollup files.
  • Introduced an option to alter the ports of the L2 executor. This is crucial if you have L1 geth running on the same machine.
  • Slightly refactored the block_update_receiver logic in ChainWatcher. Previously, there was a default channel creation with a receiver which wasn't used at all and was subsequently replaced after startup.
  • Introduced the devnet flag, primarily utilized to determine the L1 finalized block. If devnet mode is enabled, it will default to the latest block. This is a contentious point, and I'm eager to hear your thoughts. It also involves changes in Docker, documentation, entrypoints, and more.

Let me know what you think. If necessary, I can create another pull request with only the selected changes/fixes.

Copy link
Contributor

@ncitron ncitron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Left a few questions and comments.

let prune_until = self
.safe_epoch
.number
.saturating_sub(self.config.chain.seq_window_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

fn get_l1_start_block(epoch_number: u64, channel_timeout: u64) -> u64 {
epoch_number
.checked_sub(channel_timeout)
.unwrap_or(epoch_number)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct logic? Shouldn't we return an epoch value of 0 if it underflows? Similar to how you changed the prune function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix: d49fcd6

@@ -43,7 +43,7 @@ pub struct ChainWatcher {
/// The L2 starting block
l2_start_block: u64,
/// Channel for receiving block updates for each new block
pub block_update_receiver: mpsc::Receiver<BlockUpdate>,
block_update_receiver: Option<mpsc::Receiver<BlockUpdate>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change. This is a lot clearer than what we were doing before with leaving a useless channel there on initialization.

docs/devnet.md Outdated
@@ -0,0 +1,135 @@
# Devnet Environment

The devnet environment is constructed atop the OP stack devnet, also known as "bedrock". Essentially, this environment runs `op-geth` and `geth` node in development mode, excluding the PoS client, and without finality. This is why the `--devnet` flag is necessary. This flag allows for the acceptance of any block as finalized. Otherwise, the Magi would operate exclusively with finalized blocks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of implies that the devnet is called bedrock, rather than the op stack. Can we make this clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, my eyes glazed over 👍 Fix 9f420c9

docs/devnet.md Outdated

For troubleshooting, please refer to the official [documentation](https://community.optimism.io/docs/developers/build/dev-node/#).

### Configure OP-Geth for Magic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: magi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

then
if [ $NETWORK = "devnet" ]
then
rm -rf $DATADIR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we deleting the datadir if it is a devnet? Wouldn't this cause us to resync from genesis on every restart?

Copy link
Contributor Author

@borispovod borispovod Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that when you're working on the devnet and making changes to the code of the node, you'd likely prefer to start syncing from a clean environment. But i agree it might not always be necessary.

UPD: e671918

@ncitron
Copy link
Contributor

ncitron commented Sep 18, 2023

This is looking good. Is there anything else you wanted to add before moving it from draft?

@borispovod borispovod marked this pull request as ready for review September 19, 2023 11:51
@borispovod
Copy link
Contributor Author

This is looking good. Is there anything else you wanted to add before moving it from draft?

No, i think it's good. Moved to PR.

Copy link
Contributor

@ncitron ncitron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ncitron ncitron merged commit b665632 into a16z:master Sep 19, 2023
5 checks passed
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.

2 participants