(WIP) feat: sequencer #179

borispovod · 2023-11-23T23:15:46Z

Hello everyone,

This PR includes changes we've made for running the sequencer on top of Magi. It also impacts types, configurations (additional parameters and refactoring), networking (broadcast and configuration), and RPC (implementation of missing endpoints).

I would appreciate a review, particularly if you notice something functioning incorrectly, and I'd like to hear your opinions on the changes, etc. In the future, I believe the sequencer should be moved into its own module, launched by a timer, etc. This is planned as part of future updates.

Additionally, I have primarily tested it on our devnet, so the tests should probably also be run on top of existing networks.

Thank you.

P.S. Sorry for the single commit. We have a monorepo, and it would be difficult to separate all the changes into individual commits. I hope we can improve this process in the future.

LEAFERx · 2023-11-27T08:45:00Z

Hi I think run_sequencer_step implementation src/driver/mod.rs:347 is incorrect (https://github.com/a16z/magi/pull/179/files#diff-c97e92b44dfb27733ada13c547be940cddfd184bc1a87dd32fa5046916f117caR347)
The current build_payload implementation will call get_payload right after the forkchoice_update call, which gives the execution client no time to build the attributes with transactions in the txpool.

see opnode implementation: https://github.com/ethereum-optimism/optimism/blob/develop/op-node/rollup/driver/sequencer.go#L125
it essentially waits blocktime - sealingtime before you call get_payload.

also see Engine API spec:

(Payload Building)
Client software SHOULD start the process of updating the payload. The strategy of this process is implementation dependent. The default strategy is to keep the transaction set up-to-date with the state of local mempool.

Client software SHOULD stop the updating process when either a call to engine_getPayload with the build process's payloadId is made or SECONDS_PER_SLOT (12s in the Mainnet configuration) have passed since the point in time identified by the timestamp parameter.

(engine_getPayload)
Given the payloadId client software MUST return the most recent version of the payload that is available in the corresponding build process at the time of receiving the call.

borispovod · 2023-11-27T16:16:08Z

Hey @LEAFERx,

Thank you for your catch.
We tested it on devnet with Geth and later with Reth; however, we have our own Reth fork, so the behavior might be slightly different.

I will look into it shortly. However, block generations, along with transactions, have worked and continue to work. Still, I agree that we should implement sealingtime according to the protocol.

ncitron

This is awesome! I just gave it a first pass and left some comments, and will probably make another pass through it to make sure I didn't miss anything since this PR is so large.

I'll also work on getting this synced to some of the op stack chains to make sure there are no regressions.

ncitron · 2023-11-27T19:51:07Z

bin/magi.rs

+    #[clap(long = "sequencer-enabled")]
+    sequencer_enabled: bool,
+    #[clap(long = "sequencer-max-safe-lag", default_value = "0")]
+    sequencer_max_safe_lag: String,


Why is this a string? Could we use an unsigned integer here instead?

bin/magi.rs

ncitron · 2023-11-27T20:06:34Z

bin/network.rs

@@ -1,28 +1,91 @@
-use std::str::FromStr;
+#![allow(unused_imports)]


Can we remove this and delete any unused imports or is there another reason this is here?

Well, as I understood, the idea behind bin/network.rs is just for testing purposes, so we've added code to generate its own Enr and commented it out, and kept imports, will fix that part.

ncitron · 2023-11-27T20:10:18Z

bin/network.rs

+    // let bootnodes = vec![enr];
+
+    let bootnodes: Vec<discv5::enr::Enr<CombinedKey>> =
+        vec![Enr::from_str("enr:-Je4QKqISnjZwcUSRQqLTbOoqFvmQX8sVlPIud5sWPrUp_8hPJXnzSyY-fqXhzqWGKDHjNSLJRbBGjC9VILm_HGuhHkBgmlkgnY0gmlwhH8AAAGHb3BzdGFja4OFBwCJc2VjcDI1NmsxoQMqv564GlblO4zWKiGSn0-lcr70dYrzwiieFETLNEy8xoN0Y3CCJvyDdWRwgib8").map_err(|e| eyre::eyre!("err: {}", e))?];


Where is this node coming from? Can we make sure that this uses the optimism mainnet ones like as we specify in discovery::bootnodes?

ncitron · 2023-11-27T20:11:01Z

bin/network.rs

    let (_, recv) = watch::channel(Address::from_str(
-        "0x715b7219d986641df9efd9c7ef01218d528e19ec",
+        "0xF64c29538cAE4E69eac62c50CDfebAC22b378044",


Can we also keep this as the op mainnet value?

ncitron · 2023-11-27T20:41:52Z

src/derive/stages/attributes.rs

+            prev_randao,
+            suggested_fee_recipient,
+            transactions,
+            no_tx_pool: false,


Same concern as earlier with this not matching the spec, or is it supposed to add new transactions to the block since it is only run during the sequencer step?

Well, i think i need to rework that part as i already told.
Yet would need some time.

ncitron · 2023-11-27T20:50:04Z

src/driver/info.rs

+
+    pub async fn get_finalized<P: InnerProvider>(p: &P, chain: &ChainConfig) -> HeadInfo {
+        let block_number = BlockNumber::Finalized;
+        Self::get_head_info(p, block_number)


nit: can we refactor these three to use an internal function to dedupe the unwrap_or_else handling? Maybe we can call it get_head_info_or_genesis or something similar?

That's a good idea. Also, we noticed that if we relaunch the L2 node and, at the same time, launch Magi, it can roll back heads to the genesis ones. So, I think maybe we should add additional checks there.

ncitron · 2023-11-27T20:56:57Z

src/driver/mod.rs

+    /// Produces a block if the conditions are met.
+    /// If successful the block would be signed by sequencer and shared by P2P.
+    async fn run_sequencer_step(&mut self) -> Result<()> {
+        if let Some(seq_config) = self.sequencer_config.clone() {


I think we can avoid cloning the entire config here by doing an as_ref or something.,

Although I guess since its just a uint in there it doesn't really matter. But if more stuff ends up being added to that config it could become a big clone.

ncitron · 2023-11-27T21:01:00Z

src/driver/mod.rs

+    async fn run_sequencer_step(&mut self) -> Result<()> {
+        if let Some(seq_config) = self.sequencer_config.clone() {
+            // Get unsafe head to build a new block on top of it.
+            let head = self.engine_driver.unsafe_info.head;


Rename to unsafe_head to match with the next line and avoid any confusion.

ncitron · 2023-11-27T21:01:59Z

src/driver/mod.rs

+            let head = self.engine_driver.unsafe_info.head;
+            let unsafe_epoch = self.engine_driver.unsafe_info.epoch;
+
+            if seq_config.max_safe_lag() > 0 {


I'm not sure if this extra check here really matters in terms of efficiency. Might be better to just remove it for readability reasons.

Do you want to remove the parameter at all?

I don't think that we need to remove the parameter, but doesn't the check on L294 cover this case, making it redundant, or am I misunderstanding something?

If we remove the zero check, the code will always return max safe lag reached... when max_safe_lag == 0, because safe block + 0 <= unsafe will always be true. As a result, no new blocks would be generated, among other issues.

We could use an Option for max_safe_lag, but I still think there would be issues if someone passes 0 as the value.

Oh I see that makes sense!

* Removed unused imports * Configurated to optimism mainnet * Improved comments

# Conflicts: # bin/magi.rs # src/config/mod.rs # src/derive/stages/attributes.rs # src/network/handlers/block_handler.rs

# Conflicts: # src/config/mod.rs # src/derive/mod.rs # src/derive/stages/attributes.rs # src/derive/stages/batches.rs # src/derive/state.rs # src/driver/mod.rs

0x677261706562616261 · 2024-01-21T05:15:16Z

what is the progress of this PR?

dk-pontem · 2024-01-21T05:52:45Z

@0x677261706562616261

what is the progress of this PR?

I'd like @borispovod to look into it before review of a16z team.

borispovod · 2024-01-23T03:39:52Z

I think most of issues fixed by me and @dk-pontem , we are going to do review, unit tests, manual tests

borispovod added 2 commits November 24, 2023 03:06

feat: sequencer

03e28ae

fix: clippy checks

aa5d896

borispovod force-pushed the sequencer branch from 5800e79 to aa5d896 Compare November 24, 2023 00:08

ncitron requested changes Nov 27, 2023

View reviewed changes

borispovod and others added 16 commits December 1, 2023 00:08

fix(network)

ec5fee2

* Removed unused imports * Configurated to optimism mainnet * Improved comments

fix:(config): sequencer max safe lag as u64

2b6933d

fix(sequencer): rename head to unsafe_head

f662e31

Merge branch 'master' into sequencer

3191972

fix(sequencer): don't want for next epoch

d81ddef

feat: paralleled and refactored of getting heads

9c963d8

Merge remote-tracking branch 'origin/master' into sequencer

826eb36

# Conflicts: # bin/magi.rs # src/config/mod.rs # src/derive/stages/attributes.rs # src/network/handlers/block_handler.rs

fix: compilation errors after merging

013585c

feat: derive_attributes* refactoring

c0c0996

Merge branch 'master' of github.com:pontem-network/magi into sequencer

b1c58bf

# Conflicts: # src/config/mod.rs # src/derive/mod.rs # src/derive/stages/attributes.rs # src/derive/stages/batches.rs # src/derive/state.rs # src/driver/mod.rs

fix: compilation errors after merge, config refactoring

6b07ec4

fix: errors in tests

d178200

fix: small refactoring in run_sequencer_step

6c77a29

fix: small refactoring in run_sequencer_step

fca6cb6

fix(sequencer): wait for txpool processing

a1d9829

fix(sequencer): incorrect no_tx_pool calculation

2c464be

dk-pontem added 3 commits January 31, 2024 12:16

feat(sequencer): refactored derive_attributes_internal, added test

0b82a75

fix(sequencer): derived user deposited transactions

d490061

feat(sequencer): refactored, added test_prepare_block_data

d1f1cf2

gregdhill mentioned this pull request Feb 14, 2024

Magi doesn't support sequencing #212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) feat: sequencer #179

(WIP) feat: sequencer #179

borispovod commented Nov 23, 2023

LEAFERx commented Nov 27, 2023

borispovod commented Nov 27, 2023 •

edited

Loading

ncitron left a comment

ncitron Nov 27, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

borispovod Nov 30, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

borispovod Nov 30, 2023 •

edited

Loading

ncitron Nov 27, 2023

borispovod Nov 30, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

ncitron Nov 27, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

dk-pontem Jan 11, 2024

ncitron Nov 27, 2023

borispovod Nov 30, 2023

ncitron Nov 30, 2023

borispovod Nov 30, 2023

ncitron Nov 30, 2023

0x677261706562616261 commented Jan 21, 2024

dk-pontem commented Jan 21, 2024 •

edited

Loading

borispovod commented Jan 23, 2024

		@@ -1,28 +1,91 @@
		use std::str::FromStr;
		#![allow(unused_imports)]

(WIP) feat: sequencer #179

Are you sure you want to change the base?

(WIP) feat: sequencer #179

Conversation

borispovod commented Nov 23, 2023

LEAFERx commented Nov 27, 2023

borispovod commented Nov 27, 2023 • edited Loading

ncitron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borispovod Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x677261706562616261 commented Jan 21, 2024

dk-pontem commented Jan 21, 2024 • edited Loading

borispovod commented Jan 23, 2024

borispovod commented Nov 27, 2023 •

edited

Loading

borispovod Nov 30, 2023 •

edited

Loading

dk-pontem commented Jan 21, 2024 •

edited

Loading