State sync from l1 #1296

jerrybaoo · 2023-12-05T05:39:48Z

Add sync state from L1 feature

Pull Request type

Feature

What is the current behavior?

Currently, there is no functionality to synchronize data from L1.

What is the new behavior?

When the functionality to synchronize state from L1 is enabled, the node fetches a trusted state diff from L1.
Node will package the state diff into a substrate block, which includes a starknet block header in the block header digest, and then apply it locally.
Due to the absence of a transaction list on L1, this feature cannot construct a complete blockchain; it can only build a complete state.

Does this introduce a breaking change?

No

tdelabro · 2023-12-06T10:21:28Z

Hey thanks for the good work.
Can I ask why did you develop that?
In the context of a Madara appchain, what is the use you envision for this? What should the chain do with that l1 state once it get it?

I think it can be used as a way to finalize the l2 state, but it needs something build on top of it. Did you had other stuffs in mind?

tdelabro · 2023-12-06T10:31:15Z

Also, are you aware of this PR: #1282
It's about getting the l1->l2 messaging, by reading l1.
We are going to merge it on main soon.
Can you give it a look and tell me if it is redundant with your PR?

jerrybaoo · 2023-12-07T01:09:21Z

Hey thanks for the good work. Can I ask why did you develop that? In the context of a Madara appchain, what is the use you envision for this? What should the chain do with that l1 state once it get it?

I think it can be used as a way to finalize the l2 state, but it needs something build on top of it. Did you had other stuffs in mind?

Based on this issue #1224, we developed the code in this pull request (PR). This feature can be used to rapidly rebuild the state of L2 from L1.
After understanding the synchronization mechanisms of other Starknet sequencer implementation approaches, we found that they currently heavily rely on centralized feedline gateways, which are going to be phased out. The alternative to feedline gateways is full-node RPC. However, this remains fundamentally centralized as we must trust the linked RPC. Therefore, by utilizing data synchronized from L1, as the data on L1 is proven, we can rapidly build the L2 state without the need for trust assumptions.

jerrybaoo · 2023-12-07T02:06:33Z

Also, are you aware of this PR: #1282 It's about getting the l1->l2 messaging, by reading l1. We are going to merge it on main soon. Can you give it a look and tell me if it is redundant with your PR?

I've read the code, and there are no redundant with my PR. This PR (#1282) is about retrieving and applying messages from L1 to L2, whereas mine is about fetching L2 state from L1 and applying it locally.

tdelabro · 2023-12-07T10:40:18Z

crates/node/src/commands/run.rs

+
+    /// When enable, the node will sync state from l1,
+    #[clap(long)]
+    pub sync_from_l1: Option<String>,


In ExtendedRunCommand pub base: RunCmd, pub network_params: NetworkParams, pub sync: SyncMode,` we find this, provided by substrate:

/// Syncing mode. #[derive(Debug, Clone, Copy, ValueEnum, PartialEq)] #[value(rename_all = "kebab-case")] pub enum SyncMode { /// Full sync. Download end verify all blocks. Full, /// Download blocks without executing them. Download latest state with proofs. Fast, /// Download blocks without executing them. Download latest state without proofs. FastUnsafe, /// Prove finality and download the latest state. Warp, }

Do you think we can reuse those already defined modes? They look a bit similar to what we offer.

What is the list of the different sync mode we aim to support? I think we should define them.

What you said makes a lot of sense. I think we can expand this enumeration. Currently, pub sync_from_l1: Option<String>, actually refers to a configuration file path, and the content of this configuration file at present is like this：

{ "l1_start": 5854324, "core_contract": "0xde29d060D45901Fb19ED6C6e959EB22d8626708e", "verifier_contract": "0x5EF3C980Bf970FcE5BbC217835743ea9f0388f4F", "memory_page_contract": "0x743789ff2fF82Bfb907009C9911a7dA636D34FA7", "l2_start": 0, "l1_url_list": ["https://eth-goerli.g.alchemy.com/v2/nMMxqPTld6cj0DUO-4Qj2cg88Dd1MUhH","https://eth-goerli.g.alchemy.com/v2/AktNdFZZplqKaD2NrEfowmeYAwmEn4db"], "v011_diff_format_height": 28566, "constructor_args_diff_height": 4873 }

So, we can extend this enumeration to:

/// Syncing mode. #[derive(Debug, Clone, Copy, ValueEnum, PartialEq)] #[value(rename_all = "kebab-case")] pub enum SyncMode { /// Full sync. Download end verify all blocks. Full, /// Download blocks without executing them. Download latest state with proofs. Fast, /// Download blocks without executing them. Download latest state without proofs. FastUnsafe, /// Prove finality and download the latest state. Warp, /// Sync state from L1 FromL1(String), }

Has this made it into the codebase in the end?

The SyncMode defined in substrate specifies the synchronization method between nodes within the same network. However, sync from l1 is another type of synchronization that only exists within the rollup chain. Therefore, we cannot reuse the substrate SyncMode. Additionally, extending Substrate SyncMode is not quite suitable since substrate does not provide rollup functionality. Therefore, it is more appropriate to continue placing this command parameter in the ExtendedRunCommand.

antiyro · 2023-12-07T13:03:47Z

thanks for this impl! finishing some stuff on the rpc side and I'll deep dive here 👍

antiyro · 2024-01-06T09:16:05Z

Reviewing it now, my apologies for the delays I've been busy bumping madara specs.

antiyro · 2024-01-06T12:34:44Z

@jerrybaoo could you please sync with upstream when you have time so I can test it locally?

jerrybaoo · 2024-01-07T01:28:40Z

@jerrybaoo could you please sync with upstream when you have time so I can test it locally?

Okay, I'll start syncing with upstream now.

antiyro · 2024-01-07T12:28:19Z

Thanks!

jerrybaoo · 2024-01-07T14:04:43Z

Thanks!

Sync with upstream has been completed. Additionally, we have a quick start guide that we hope will be helpful to you. Alternatively, we can include it in the Readme.

tdelabro · 2024-01-16T14:37:06Z

crates/client/db/src/meta_db.rs

+pub struct L1L2BlockMapping {
+    pub l1_block_hash: H256,
+    pub l1_block_number: u64,
+    pub l2_block_hash: U256,


We are talking starknet hash here, not substrate hash.
Can we add a comment to make it explicit?

Why is it a U256 and not a H256? We are talking about a hash

Why is it a U256 and not a H256? We are talking about a hash

You are right, this is a mistake.

We are talking starknet hash here, not substrate hash. Can we add a comment to make it explicit?

Indeed, it is necessary to provide detailed comments explaining the mapping between L1 block hash and starknet block hash.

tdelabro · 2024-01-16T14:41:41Z

crates/client/state-sync/src/ethereum.rs

+pub struct EthOrigin {
+    block_hash: H256,
+    block_number: u64,
+    _transaction_hash: H256,


Why is it here if it's never used?

Indeed, it is not being used and has already been removed.

tdelabro · 2024-01-16T14:43:46Z

crates/client/state-sync/src/ethereum.rs

+    eth_origin: EthOrigin,
+    update: LogStateUpdate,


Both field contains block_hash and block_number. It looks like duplicated data, is it? Or is there a good reason to this?

There is no duplicate data; they have different meanings. eth_origin represents the L2 rollup data to L1, including the block and transaction information for this transaction. On the other hand, update conveys information about the L2 state update.

It is misleading for something called EthOrigin to be about l2, which is not eth.
Can you figure out a way to make it more understandable (changing names or adding more doc)?

I have indeed renamed it to avoid confusion and added comments for clarity.

tdelabro · 2024-01-16T14:45:28Z

crates/client/state-sync/src/ethereum.rs

+    pub block_hash: U256,
+}
+
+/// Ethereum contract event representing a log state update in old contract.


Can you add a bit more background on that, please? When was the update from old to new done (at which block) and link the code of both

In the testnet, we noticed an upgrade in the Starknet Core contract, leading to an additional field in the LogStateUpdate event. This adjustment is made solely for forward compatibility with the older events. If there is no need to maintain consistency with the Starknet mainnet and testnet, you may safely ignore LogStateUpdateOld.

Add it to the doc. I think it's interesting for people to understand that

Some comment documentation has been added.

tdelabro · 2024-01-16T14:46:41Z

crates/client/state-sync/src/ethereum.rs

+        return Ok(LogStateUpdate {
+            global_root: update.global_root,
+            block_number: update.block_number,
+            block_hash: Default::default(),


Is this safe?
Wouldnt' it be better to have an enum

pub enum LogStateUpdate { Old(..), New(..) }

In the current processing logic, if the L2 block hash cannot be obtained from the chain, a local l2 block hash will be computed. culcualte l2 block hash.

During the calculation of the local block hash, we are unable to retrieve all the information of the L2 block header. Therefore, this block hash cannot be computed correctly. To ensure security, it is crucial to find a method to obtain the correct block hash.

Do you have any good ideas about this? Is it possible for us not to consider compatibility with such changes in Starknet? In fact, we haven't found any documentation explaining these changes; we've only observed the changes in contract events from the chain explorer.

@antiyro how did you manage that? When getting data from l1, before some contract update the l2 block hash wasn't part of the payload. How to you manage to still retrieve the blockhash?

crates/client/state-sync/src/ethereum.rs

tdelabro · 2024-01-16T17:21:40Z

crates/client/state-sync/src/ethereum.rs

+
+        let state_updates = self.query_state_update(l1_from, l2_start).await?;
+        let tasks = state_updates.iter().map(|updates| {
+            debug!(target: LOG_TARGET, "crate task fro update l1:{} l2: {}", updates.eth_origin.block_number, updates.update.block_number);


crates/client/state-sync/src/ethereum.rs

tdelabro · 2024-01-16T17:29:03Z

crates/client/state-sync/src/ethereum.rs

+        let mut states_res = Vec::new();
+        for fetched_state in fetched_states {
+            match fetched_state {
+                Ok(state) => states_res.push(state),
+                Err(e) => return Err(e),
+            }
+        }


let states_res: Result<Vec<_>> = fetched_states.into_iter().collect();

https://stackoverflow.com/a/63798748/9967008

tdelabro · 2024-01-16T17:31:31Z

crates/client/state-sync/src/lib.rs

+    /// Error occurring during transaction construction with a specific message.
+    ConstructTransaction(String),
+    /// Error while committing data to storage with a specific message.
+    CommitStorage(String),
+    /// Error related to connection issues with L1 chain with a specific message.
+    L1Connection(String),
+    /// Error decoding an event from L1.
+    L1EventDecode,
+    /// Error related to state handling on L1 with a specific message.
+    L1StateError(String),
+    /// Error due to a type mismatch or inconsistency with a specific message.
+    TypeError(String),
+    /// Any other unspecified error with a specific message.
+    Other(String),


Don't use a String as a way to represent the original error.
Use the error type itself.
You can achieve this easily with thiserror and their #[from] macro

I have rewritten the Error enum using thiserror.

remove unused code

add comments

replace std::sync::mutex with use parking_lot::Mutex

github-actions · 2024-02-19T00:10:19Z

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

codemax and others added 9 commits December 5, 2023 12:59

implement state-sync basic component

237125e

init uint tests

8a86cd8

improve parser and state fetcher

da0953c

implement state sync async service

de2c82f

implement sync status oracle, and mock eth provider

1000177

improve docs

2148922

Merge branch 'main' into state-sync-from-l1

86252e1

Add control strategy for rpc requests

ccb18fc

remove unused

77615a6

jerrybaoo mentioned this pull request Dec 5, 2023

feat: Sync from L1 #1224

Open

jerrybaoo added 2 commits December 5, 2023 14:13

cargo clippy

b7384d7

Update CHANGELOG.md

a71adf3

tdelabro reviewed Dec 7, 2023

View reviewed changes

merge main

24fbcda

fix unit tests

450c695

jerrybaoo force-pushed the state-sync-from-l1 branch from bf1e5e6 to 450c695 Compare January 7, 2024 14:08

tdelabro requested changes Jan 16, 2024

View reviewed changes

jerrybaoo added 3 commits January 17, 2024 10:27

update sync from l1 command parameter

7583639

update the type of l2_block_hash in the L1L2BlockMapping

31754f4

improve code

f3512b8

jerrybaoo added 5 commits January 18, 2024 09:52

add comments

38728a7

remove unused code

rewrite query_state_update

2786740

Changed the error handling approach

31da0cd

add comments

improve code

415ee80

rewrite sync-state Error type

ac1b1d5

replace std::sync::mutex with use parking_lot::Mutex

jerrybaoo requested a review from tdelabro January 18, 2024 17:25

Add comments for rpc retry mechanism

191c3ce

github-actions bot added the stale label Feb 19, 2024

This was referenced Feb 25, 2024

Base Ethereum config for DA/settlement tasks #1452

Merged

feat: submit state diff to the memory pages contract on Ethereum (sovereign DA mode) #1480

Closed

State sync from l1 #1296

Are you sure you want to change the base?

State sync from l1 #1296

Conversation

jerrybaoo commented Dec 5, 2023

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

tdelabro commented Dec 6, 2023

tdelabro commented Dec 6, 2023

jerrybaoo commented Dec 7, 2023 • edited

jerrybaoo commented Dec 7, 2023 • edited

Choose a reason for hiding this comment

jerrybaoo Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrybaoo Jan 17, 2024 • edited

Choose a reason for hiding this comment

antiyro commented Dec 7, 2023

antiyro commented Jan 6, 2024

antiyro commented Jan 6, 2024

jerrybaoo commented Jan 7, 2024

antiyro commented Jan 7, 2024

jerrybaoo commented Jan 7, 2024 • edited

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

jerrybaoo Jan 17, 2024 • edited

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

jerrybaoo Jan 17, 2024 • edited

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

jerrybaoo Jan 18, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2024

jerrybaoo commented Dec 7, 2023 •

edited

jerrybaoo commented Dec 7, 2023 •

edited

jerrybaoo Dec 8, 2023 •

edited

jerrybaoo Jan 17, 2024 •

edited

jerrybaoo commented Jan 7, 2024 •

edited

jerrybaoo Jan 17, 2024 •

edited

jerrybaoo Jan 17, 2024 •

edited

jerrybaoo Jan 18, 2024 •

edited