Adding support for declare v0, l1 handler and l1->l2 messages #321

Mohiiit · 2024-10-10T16:01:51Z

Support has been added for declare v0 transaction, this would be required in order to declare txn with zero fees.
Support for l1 handler txn is also added
l1->l2 feature was implemented was not in use, added that as well.

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

Feature

What is the current behavior?

the code don't have support for declare v0
currently the l1 handler txn are ignore
it's not added in the l1 sync service

Resolves: #NA

What is the new behavior?

declare v0 support has been added
supports l1 handler txn
should be able to execute l1->l2 messages

Does this introduce a breaking change?

Shouldn't

Other information

apoorvsadana · 2024-10-11T08:20:51Z

crates/client/eth/src/l1_messaging.rs

    let mut event_stream = event_filter
        .from_block(last_synced_event_block.block_number)
-        .select(BlockNumberOrTag::Finalized)


we should add this back?

it was causing the issue actually, was not able to get the messages

maybe it's an issue because anvil creates blocks only when a txn comes. so your block isn't finalised until you create more blocks on top (I guess). however, if we read a message which isn't finalised (and so it rolls back on Ethereum which is very rare) there can be big downstream issues

crates/client/mempool/src/block_production.rs

crates/primitives/transactions/src/broadcasted_to_blockifier.rs

apoorvsadana · 2024-10-11T08:36:05Z

crates/client/mempool/src/lib.rs

+
+        if let Transaction::AccountTransaction(account_tx) = clone_transaction(&tx) {
+            let _ = validator.perform_validations(account_tx, deploy_account_tx_hash.is_some());
+        }

        if !is_only_query(&tx) {


probably not for this PR but shouldn't we throw an error if someone calls add_invoke_transaction as a query? query versions should only be allowed in estimate_fee and estimate_message_fee imo. so if it's a query version on a write call we can revert much earlier maybe?

good question, i'm not entirely sure?

apoorvsadana · 2024-10-11T08:48:43Z

crates/primitives/receipt/src/from_blockifier.rs

+            events,
+            execution_resources,
+            execution_result,
+            message_hash: Felt::from(1),


add a TODO?

i dont remember how the message hash is computed, i think the stuff that's commented just on top may work?
we should make a unit test for that tbh

it's implemented now

crates/primitives/receipt/src/from_blockifier.rs

Trantorian1 · 2024-10-11T09:35:58Z

crates/client/eth/src/l1_messaging.rs


    // TODO: remove unwraps
+    // Ques: shall it panic if no block number of event_index?


I have checked and could not find any check for None on any of these fields from the moment they are created to this point. This is quite worrying. I do not know what the expected behavior would be in that case but this should not cause a node failure.

crates/client/mempool/src/block_production.rs

crates/primitives/class/src/lib.rs

crates/primitives/receipt/src/from_blockifier.rs

crates/primitives/transactions/src/from_broadcasted_transaction.rs

cchudant

good!

mempool handling: l1 handler tx nonces is not handled properly, but I will probably fix that myself
rpc endpoint declare v0 should not be public, declare v0 is only for bootstrapper. Ideally in a dedicated node operator endpoint.
l1 to l2 messages - good, i'd love a test of some kind though
- message hash is wrong

crates/client/mempool/src/block_production.rs

crates/client/mempool/src/inner.rs

cchudant · 2024-10-11T09:34:47Z

crates/client/mempool/src/inner.rs

there is a

#![proptest_config(ProptestConfig::with_cases(5))] // comment this when developing, this is mostly for faster ci & whole workspace `cargo test`

at the bottom of the file that you should try commenting to see if the proptest passes locally
(don't commit it though)

This is fine for now, but please add a comment saying that the handling of nonces on l1 handler txs is wrong

Also, when I get to fixing it, what tests should I run to see that I did not break l1 messages in the process?

.gitignore

cchudant · 2024-10-11T09:40:16Z

crates/client/mempool/src/lib.rs

@@ -63,11 +63,13 @@ impl Error {
 #[cfg_attr(test, mockall::automock)]
 pub trait MempoolProvider: Send + Sync {
    fn accept_invoke_tx(&self, tx: BroadcastedInvokeTransaction) -> Result<InvokeTransactionResult, Error>;
+    fn accept_declare_v0_tx(&self, tx: BroadcastedDeclareTransactionV0) -> Result<DeclareTransactionResult, Error>;


might be worth to put a comment here to say why we need declare v0 txs in the first place

crates/primitives/class/src/lib.rs

cchudant · 2024-10-11T10:09:04Z

crates/primitives/receipt/src/from_blockifier.rs

+            events,
+            execution_resources,
+            execution_result,
+            message_hash: Felt::from(1),


i dont remember how the message hash is computed, i think the stuff that's commented just on top may work?
we should make a unit test for that tbh

crates/client/eth/src/l1_messaging.rs

crates/tests/src/lib.rs

crates/primitives/chain_config/src/chain_config.rs

crates/tests/src/lib.rs

crates/node/src/service/rpc/middleware.rs

crates/proc-macros/src/lib.rs

crates/client/mempool/src/inner.rs

Missclick :P

… default addeing of starknet namespace in middleware

crates/proc-macros/src/lib.rs

crates/node/src/service/rpc/middleware.rs

…ent in the rpc call

jbcaron · 2024-10-18T14:34:42Z

crates/client/eth/src/state_update.rs

 ) -> anyhow::Result<()> {
    // This is a provisory check to avoid updating the state with an L1StateUpdate that should not have been detected
    //
    // TODO: Remove this check when the L1StateUpdate is properly verified
-    if state_update.block_number > 500000u64 || chain_id == MAIN_CHAIN_ID {
+    if state_update.block_number > 500000u64 || chain_id.to_felt() == MAIN_CHAIN_ID {


I don't really understand this logic and the 'or'

I believe the logic was in place before this and if I remember correctly it was because there was some mismatch before specific on mainnet

jbcaron · 2024-10-18T14:46:12Z

crates/node/src/service/rpc.rs

@@ -46,8 +46,8 @@ impl RpcService {
                (true, false)
            }
        };
-        let (read, write, trace) = (rpcs, rpcs, rpcs);
-        let starknet = Starknet::new(Arc::clone(db.backend()), chain_config.clone(), add_txs_method_provider);
+        let (read, write, trace, internal) = (rpcs, rpcs, rpcs, node_operator);


It will eventually be necessary to secure the origin of internal calls (verification of origin, bind on a specific port, etc.)

ideally internal call shouldn't be public, so yeah I agree, shall we cover this in this PR tho?

jbcaron · 2024-10-18T14:47:59Z

crates/primitives/receipt/src/from_blockifier.rs

+        Transaction::L1HandlerTransaction(tx) => match get_l1_handler_message_hash(tx) {
+            Ok(hash) => Some(hash),
+            Err(err) => {
+                panic!("Error getting l1 handler message hash: {:?}", err);


the error should be reported

changed it to log the error

jbcaron · 2024-10-18T14:51:21Z

crates/primitives/transactions/src/broadcasted_to_blockifier.rs

+            TransactionHash(hash),
+            class_info,
+            None,
+            deployed_address.map(|address| address.try_into().unwrap()),


uses expect to explain that this should not panic

actually we don't need it for declare v0 but it was missing in other case so added there

jbcaron · 2024-10-18T14:51:45Z

crates/primitives/transactions/src/from_broadcasted_transaction.rs

+    ) -> Self {
+        let is_query = tx.is_query;
+        let transaction: Transaction =
+            Transaction::Declare(DeclareTransaction::from_broadcasted_declare_v0(tx, class_hash.unwrap()));


jbcaron · 2024-10-18T14:53:55Z

crates/primitives/transactions/src/lib.rs

@@ -40,6 +46,26 @@ impl TransactionWithHash {
    }
 }

+#[derive(Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]


maybe de/serialization tests

…omments resolved

…number

…ed block

jbcaron · 2024-10-21T06:06:20Z

there is an issue to start madara in sequencer without L1 synchronization:
L1 gas prices require the ethereum service to be enabled. Either disable gas prices syncing using --gas-price 0, or disable L1 sync using the --no-l1-sync argument.

Mohiiit · 2024-10-21T08:33:42Z

there is an issue to start madara in sequencer without L1 synchronization:
L1 gas prices require the ethereum service to be enabled. Either disable gas prices syncing using --gas-price 0, or disable L1 sync using the --no-l1-sync argument.

This makes sense right? if the user wants to set a price and don't want a l1-sync they can just pass --no-l1-sync

cchudant

cool!

cchudant · 2024-10-21T10:21:19Z

crates/client/mempool/src/inner.rs

        let mempool_tx_hash = mempool_tx.tx_hash();

        let replaced = if force {
-            if self.transactions.insert(OrderMempoolTransactionByNonce(mempool_tx), ()).is_some() {
+            if self.transactions.insert(OrderMempoolTransactionByNonce(mempool_tx.clone()), ()).is_some() {


cchudant · 2024-10-21T10:21:27Z

crates/client/mempool/src/inner.rs

                ReplacedState::Replaced
            } else {
                ReplacedState::NotReplaced
            }
        } else {
-            match self.transactions.entry(OrderMempoolTransactionByNonce(mempool_tx)) {
+            match self.transactions.entry(OrderMempoolTransactionByNonce(mempool_tx.clone())) {


mohiiit added 13 commits October 3, 2024 16:11

feat: declare v0 working

789f457

debug: debuging declare v0

d448b65

main merged

b842ae6

adding comments for debugging

3da2f1a

debugging with bootstraper

2938bef

making l1 handler execute in the mempool

b8c554c

feat: refactored Mempool to accept L1Handler txn

62e174f

fixing test for l1 message and mempool inner

4c16c8e

merged with main

7f31dde

formatting and linting

50dcc7a

formatting and changelog

f9650f5

comments removed

334c258

main merged

7c98ebd

Mohiiit marked this pull request as ready for review October 10, 2024 16:04

Mohiiit requested review from apoorvsadana and Trantorian1 October 11, 2024 07:24

apoorvsadana reviewed Oct 11, 2024

View reviewed changes

Merge branch 'main' into feat/declare_v0

bc05e4f

Trantorian1 assigned Mohiiit Oct 11, 2024

Trantorian1 requested changes Oct 11, 2024

View reviewed changes

cchudant requested changes Oct 11, 2024

View reviewed changes

mohiiit added 9 commits October 11, 2024 16:27

validation added for txns

7c63396

feat: main merged

1485eca

resolving issues

c857b79

refactor: declare v0 refactored with different rpc namespace

8e510c9

main merged

0ca66af

merged with upstream

501eaf8

merged with main

21d12f5

msgL2 hash added

2ad4d19

formatting and linting

fc16ff3

jbcaron reviewed Oct 17, 2024

View reviewed changes

crates/tests/src/lib.rs Outdated Show resolved Hide resolved

Trantorian1 requested changes Oct 17, 2024

View reviewed changes

crates/node/src/service/rpc/middleware.rs Show resolved Hide resolved

crates/proc-macros/src/lib.rs Show resolved Hide resolved

crates/client/mempool/src/inner.rs Show resolved Hide resolved

Trantorian1 previously approved these changes Oct 17, 2024

View reviewed changes

crates/client/mempool/src/inner.rs Outdated Show resolved Hide resolved

mohiiit added 4 commits October 18, 2024 16:13

tests added for proc macros, error added for empty namespace, removed…

2209885

… default addeing of starknet namespace in middleware

fixing comments

ed5b6c0

main merged

eaf5411

fix: build fix

f21baad

Mohiiit requested review from Trantorian1, cchudant and jbcaron October 18, 2024 11:58

Trantorian1 requested changes Oct 18, 2024

View reviewed changes

crates/proc-macros/src/lib.rs Show resolved Hide resolved

crates/node/src/service/rpc/middleware.rs Outdated Show resolved Hide resolved

feat: added error when namespace is not present or method is not pres…

77038a2

…ent in the rpc call

jbcaron reviewed Oct 18, 2024

View reviewed changes

Trantorian1 approved these changes Oct 18, 2024

View reviewed changes

mohiiit added 5 commits October 18, 2024 22:06

feat: de/serialize tests added for BroadcastedDeclareTransactionV0, c…

7669670

…omments resolved

fix: state diff special address should have current_block - 10 block …

51054f8

…number

feat: updating filter of l1 message to check between last and finaliz…

b7a2a3e

…ed block

linting and formatting

8ed287c

refactor: reverting the event filter in l1 messaging

638c645

jbcaron approved these changes Oct 20, 2024

View reviewed changes

Merge branch 'main' into feat/declare_v0

859e50c

cchudant approved these changes Oct 21, 2024

View reviewed changes

extra clones removed

2fa3b46

antiyro merged commit de11ced into main Oct 21, 2024
10 checks passed

jbcaron mentioned this pull request Nov 18, 2024

feat: get_compiled_casm (RPC) #374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for declare v0, l1 handler and l1->l2 messages #321

Adding support for declare v0, l1 handler and l1->l2 messages #321

Mohiiit commented Oct 10, 2024

apoorvsadana Oct 11, 2024

Mohiiit Oct 15, 2024

apoorvsadana Oct 17, 2024

apoorvsadana Oct 11, 2024

cchudant Oct 11, 2024

apoorvsadana Oct 11, 2024

cchudant Oct 11, 2024

Mohiiit Oct 19, 2024

Trantorian1 Oct 11, 2024

cchudant left a comment

cchudant Oct 11, 2024

cchudant Oct 11, 2024

cchudant Oct 11, 2024

cchudant Oct 11, 2024

jbcaron Oct 18, 2024

Mohiiit Oct 18, 2024

jbcaron Oct 18, 2024

Mohiiit Oct 18, 2024

jbcaron Oct 18, 2024

Mohiiit Oct 18, 2024

jbcaron Oct 18, 2024

Mohiiit Oct 18, 2024

jbcaron Oct 18, 2024

Mohiiit Oct 18, 2024

jbcaron Oct 18, 2024

Mohiiit Oct 18, 2024

jbcaron commented Oct 21, 2024

Mohiiit commented Oct 21, 2024

cchudant left a comment

cchudant Oct 21, 2024

Mohiiit Oct 21, 2024

cchudant Oct 21, 2024

Mohiiit Oct 21, 2024


		// TODO: remove unwraps
		// Ques: shall it panic if no block number of event_index?

Adding support for declare v0, l1 handler and l1->l2 messages #321

Adding support for declare v0, l1 handler and l1->l2 messages #321

Conversation

Mohiiit commented Oct 10, 2024

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

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

cchudant 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

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

jbcaron commented Oct 21, 2024

Mohiiit commented Oct 21, 2024

cchudant 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