From 5ac8b25fb59dc5538aeb04402b0955481c62a0fd Mon Sep 17 00:00:00 2001 From: juanbono Date: Tue, 12 Sep 2023 17:55:43 -0300 Subject: [PATCH 1/6] add failing test that reproduce the issue --- rpc_state_reader/tests/sir_tests.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rpc_state_reader/tests/sir_tests.rs b/rpc_state_reader/tests/sir_tests.rs index 4c33dc95a..d8636b0e7 100644 --- a/rpc_state_reader/tests/sir_tests.rs +++ b/rpc_state_reader/tests/sir_tests.rs @@ -262,6 +262,7 @@ fn starknet_in_rust_test_case_tx(hash: &str, block_number: u64, chain: RpcChain) .. } = call_info.unwrap(); + // check Cairo VM execution resources assert_eq_sorted!( execution_resources, trace @@ -271,6 +272,8 @@ fn starknet_in_rust_test_case_tx(hash: &str, block_number: u64, chain: RpcChain) .execution_resources, "execution resources mismatch" ); + + // check amount of internal calls assert_eq!( internal_calls.len(), trace @@ -282,6 +285,7 @@ fn starknet_in_rust_test_case_tx(hash: &str, block_number: u64, chain: RpcChain) "internal calls length mismatch" ); + // check actual fee calculation if receipt.actual_fee != actual_fee { let diff = 100 * receipt.actual_fee.abs_diff(actual_fee) / receipt.actual_fee; @@ -293,3 +297,16 @@ fn starknet_in_rust_test_case_tx(hash: &str, block_number: u64, chain: RpcChain) } } } + +#[test] +fn test_sorted_events() { + let (tx_info, _trace, _receipt) = execute_tx( + "0x00164bfc80755f62de97ae7c98c9d67c1767259427bcf4ccfcc9683d44d54676", + RpcChain::MainNet, + BlockNumber(197000), + ); + + let events_len = tx_info.get_sorted_events().unwrap().len(); + + assert_eq!(3, events_len); +} From 4b53b3c6d1c53111c3285602704c488228746351 Mon Sep 17 00:00:00 2001 From: juanbono Date: Tue, 12 Sep 2023 19:55:26 -0300 Subject: [PATCH 2/6] fix the bug --- src/execution/mod.rs | 50 +++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/src/execution/mod.rs b/src/execution/mod.rs index 4acaf3fbe..bbac972b0 100644 --- a/src/execution/mod.rs +++ b/src/execution/mod.rs @@ -104,48 +104,72 @@ impl CallInfo { ) } - /// Yields the contract calls in DFS (preorder). + /// Returns the contract calls in DFS (preorder). pub fn gen_call_topology(&self) -> Vec { let mut calls = Vec::new(); + calls.push(self.clone()); if self.internal_calls.is_empty() { - calls.push(self.clone()) + return calls; } else { - calls.push(self.clone()); for call_info in self.internal_calls.clone() { calls.extend(call_info.gen_call_topology()); } } + calls } /// Returns a list of Starknet Event objects collected during the execution, sorted by the order /// in which they were emitted. pub fn get_sorted_events(&self) -> Result, TransactionError> { + // get the full call topology (current call + all the internal calls) let calls = self.gen_call_topology(); + // the total amount of events let n_events = calls.iter().fold(0, |acc, c| acc + c.events.len()); - let mut starknet_events: Vec> = (0..n_events).map(|_| None).collect(); + // if there is no events to collect we just return + if n_events == 0 { + return Ok(vec![]); + } + + let mut collected_events: Vec> = (0..n_events).map(|_| None).collect(); for call in calls { for ordered_event in call.events { let event = Event::new(ordered_event.clone(), call.contract_address.clone()); - starknet_events.remove((ordered_event.order as isize - 1).max(0) as usize); - starknet_events.insert( - (ordered_event.order as isize - 1).max(0) as usize, - Some(event), - ); + // we subtract 1 to the order of the events since they start at 1 + let event_index = (ordered_event.order as isize - 1).max(0) as usize; + // check if the there is no stored event at the event index + if collected_events[event_index].is_none() { + // if there is none we just store it. + // Replacing the None value. + collected_events.remove(event_index); + collected_events.insert( + (ordered_event.order as isize - 1).max(0) as usize, + Some(event), + ); + } else { + // it can be the case that some inner call already emitted + // an event with the same order, so there is already an + // event stored at position=event_index, so we just store + // it in the next position (event_index + 1). + collected_events.remove(event_index + 1); + collected_events.insert(event_index + 1, Some(event)); + } } } - let are_all_some = starknet_events.iter().all(|e| e.is_some()); + // check that all the positions are covered (they are not None). + // if that's not the case, return an error. + let are_all_some = collected_events.iter().all(|e| e.is_some()); if !are_all_some { return Err(TransactionError::UnexpectedHolesInEventOrder); } - Ok(starknet_events.into_iter().flatten().collect()) + Ok(collected_events.into_iter().flatten().collect()) } - /// Returns a list of Starknet L2ToL1MessageInfo objects collected during the execution, sorted + /// Returns a list of L2ToL1MessageInfo objects collected during the execution, sorted /// by the order in which they were sent. pub fn get_sorted_l2_to_l1_messages(&self) -> Result, TransactionError> { let calls = self.gen_call_topology(); @@ -586,6 +610,8 @@ impl TransactionExecutionInfo { }) } + /// Returns an ordered vector with all the event emitted during the transaction. + /// Including the ones emitted by internal calls. pub fn get_sorted_events(&self) -> Result, TransactionError> { let calls = self.non_optional_calls(); let mut sorted_events: Vec = Vec::new(); From 49e0c877baf916835e7a95ab4c68b08a3f7ca3f8 Mon Sep 17 00:00:00 2001 From: juanbono Date: Tue, 12 Sep 2023 20:39:11 -0300 Subject: [PATCH 3/6] fix test since now 2 events with the same order are ok --- src/execution/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/execution/mod.rs b/src/execution/mod.rs index bbac972b0..e21939247 100644 --- a/src/execution/mod.rs +++ b/src/execution/mod.rs @@ -854,7 +854,7 @@ mod tests { call_root.internal_calls = [child1, child2].to_vec(); - assert!(call_root.get_sorted_events().is_err()) + assert!(call_root.get_sorted_events().is_ok()) } #[test] From 8bbc15af474895e4fcac495dc09730ccf5c96a14 Mon Sep 17 00:00:00 2001 From: juanbono Date: Wed, 13 Sep 2023 13:40:59 -0300 Subject: [PATCH 4/6] handle multiple events --- rpc_state_reader/tests/sir_tests.rs | 13 ++++++ src/execution/mod.rs | 68 ++++++++++++----------------- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/rpc_state_reader/tests/sir_tests.rs b/rpc_state_reader/tests/sir_tests.rs index d8636b0e7..fa8c1e31f 100644 --- a/rpc_state_reader/tests/sir_tests.rs +++ b/rpc_state_reader/tests/sir_tests.rs @@ -310,3 +310,16 @@ fn test_sorted_events() { assert_eq!(3, events_len); } + +#[test] +fn test_sorted_events_with_huge_transaction() { + let (tx_info, _trace, _receipt) = execute_tx( + "0x03ec45f8369513b0f48db25f2cf18c70c50e7d3119505ab15e39ae4ca2eb06cf", + RpcChain::MainNet, + BlockNumber(219764), + ); + + let events_len = tx_info.get_sorted_events().unwrap().len(); + + assert_eq!(7, events_len); +} diff --git a/src/execution/mod.rs b/src/execution/mod.rs index e21939247..a46e18788 100644 --- a/src/execution/mod.rs +++ b/src/execution/mod.rs @@ -119,54 +119,40 @@ impl CallInfo { calls } - /// Returns a list of Starknet Event objects collected during the execution, sorted by the order + /// Returns a list of [`Event`] objects collected during the execution, sorted by the order /// in which they were emitted. pub fn get_sorted_events(&self) -> Result, TransactionError> { - // get the full call topology (current call + all the internal calls) + // collect a vector of the full call topology (all the internal + // calls performed during the current call) let calls = self.gen_call_topology(); - // the total amount of events - let n_events = calls.iter().fold(0, |acc, c| acc + c.events.len()); - - // if there is no events to collect we just return - if n_events == 0 { - return Ok(vec![]); + let mut collected_events = Vec::new(); + + // for each call, collect its ordered events + for c in calls { + collected_events.extend(c.events.iter().map(|oe| (oe.clone(), c.contract_address.clone()))); } - - let mut collected_events: Vec> = (0..n_events).map(|_| None).collect(); - - for call in calls { - for ordered_event in call.events { - let event = Event::new(ordered_event.clone(), call.contract_address.clone()); - // we subtract 1 to the order of the events since they start at 1 - let event_index = (ordered_event.order as isize - 1).max(0) as usize; - // check if the there is no stored event at the event index - if collected_events[event_index].is_none() { - // if there is none we just store it. - // Replacing the None value. - collected_events.remove(event_index); - collected_events.insert( - (ordered_event.order as isize - 1).max(0) as usize, - Some(event), - ); - } else { - // it can be the case that some inner call already emitted - // an event with the same order, so there is already an - // event stored at position=event_index, so we just store - // it in the next position (event_index + 1). - collected_events.remove(event_index + 1); - collected_events.insert(event_index + 1, Some(event)); - } + // sort the collected events using the ordering given by the order + collected_events.sort_by_key(|(oe, _)| oe.order); + + // check that there is no holes. + // Since it is already sorted, we only need to check for continuity + let mut i = 0; + for (oe, _) in collected_events.iter() { + if i == oe.order { + continue; + } + i += 1; + if i != oe.order { + return Err(TransactionError::UnexpectedHolesInEventOrder); } } - // check that all the positions are covered (they are not None). - // if that's not the case, return an error. - let are_all_some = collected_events.iter().all(|e| e.is_some()); - - if !are_all_some { - return Err(TransactionError::UnexpectedHolesInEventOrder); - } - Ok(collected_events.into_iter().flatten().collect()) + // Now it is ordered and without holes, we can discard the order and + // convert each [`OrderedEvent`] to the underlying [`Event`]. + let collected_events = collected_events.into_iter() + .map(|(oe, ca)| Event::new(oe, ca)) + .collect(); + Ok(collected_events) } /// Returns a list of L2ToL1MessageInfo objects collected during the execution, sorted From 620182d44d959bb9238532a9833b5da7279a89f2 Mon Sep 17 00:00:00 2001 From: juanbono Date: Wed, 13 Sep 2023 14:01:13 -0300 Subject: [PATCH 5/6] fix comments --- rpc_state_reader/tests/sir_tests.rs | 13 +++++++++++++ src/execution/mod.rs | 15 ++++++++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/rpc_state_reader/tests/sir_tests.rs b/rpc_state_reader/tests/sir_tests.rs index fa8c1e31f..9d8702277 100644 --- a/rpc_state_reader/tests/sir_tests.rs +++ b/rpc_state_reader/tests/sir_tests.rs @@ -323,3 +323,16 @@ fn test_sorted_events_with_huge_transaction() { assert_eq!(7, events_len); } + +#[test] +fn test_sorted_events_with_huge_transaction2() { + let (tx_info, _trace, _receipt) = execute_tx( + "0x01f891b13f9c15ca409f4ce2baa3e4c441298d7b8763a127a518bde7ecbc30ff", + RpcChain::MainNet, + BlockNumber(219798), + ); + + let events_len = tx_info.get_sorted_events().unwrap().len(); + + assert_eq!(7, events_len); +} diff --git a/src/execution/mod.rs b/src/execution/mod.rs index a46e18788..69e00108c 100644 --- a/src/execution/mod.rs +++ b/src/execution/mod.rs @@ -107,12 +107,13 @@ impl CallInfo { /// Returns the contract calls in DFS (preorder). pub fn gen_call_topology(&self) -> Vec { let mut calls = Vec::new(); + // add the current call calls.push(self.clone()); - if self.internal_calls.is_empty() { - return calls; - } else { - for call_info in self.internal_calls.clone() { - calls.extend(call_info.gen_call_topology()); + + // if it has internal calls we need to add them too. + if !self.internal_calls.is_empty() { + for inner_call in self.internal_calls.clone() { + calls.extend(inner_call.gen_call_topology()); } } @@ -135,7 +136,7 @@ impl CallInfo { collected_events.sort_by_key(|(oe, _)| oe.order); // check that there is no holes. - // Since it is already sorted, we only need to check for continuity + // since it is already sorted, we only need to check for continuity let mut i = 0; for (oe, _) in collected_events.iter() { if i == oe.order { @@ -147,7 +148,7 @@ impl CallInfo { } } - // Now it is ordered and without holes, we can discard the order and + // now that it is ordered and without holes, we can discard the order and // convert each [`OrderedEvent`] to the underlying [`Event`]. let collected_events = collected_events.into_iter() .map(|(oe, ca)| Event::new(oe, ca)) From 21644349ee3c397b5724bfaac9f24f235e2357f1 Mon Sep 17 00:00:00 2001 From: juanbono Date: Wed, 13 Sep 2023 14:28:58 -0300 Subject: [PATCH 6/6] cargo fmt --- src/execution/mod.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/execution/mod.rs b/src/execution/mod.rs index dc4ee2bef..9fed18a67 100644 --- a/src/execution/mod.rs +++ b/src/execution/mod.rs @@ -123,18 +123,22 @@ impl CallInfo { /// Returns a list of [`Event`] objects collected during the execution, sorted by the order /// in which they were emitted. pub fn get_sorted_events(&self) -> Result, TransactionError> { - // collect a vector of the full call topology (all the internal + // collect a vector of the full call topology (all the internal // calls performed during the current call) let calls = self.gen_call_topology(); let mut collected_events = Vec::new(); - + // for each call, collect its ordered events for c in calls { - collected_events.extend(c.events.iter().map(|oe| (oe.clone(), c.contract_address.clone()))); + collected_events.extend( + c.events + .iter() + .map(|oe| (oe.clone(), c.contract_address.clone())), + ); } // sort the collected events using the ordering given by the order collected_events.sort_by_key(|(oe, _)| oe.order); - + // check that there is no holes. // since it is already sorted, we only need to check for continuity let mut i = 0; @@ -148,9 +152,10 @@ impl CallInfo { } } - // now that it is ordered and without holes, we can discard the order and - // convert each [`OrderedEvent`] to the underlying [`Event`]. - let collected_events = collected_events.into_iter() + // now that it is ordered and without holes, we can discard the order and + // convert each [`OrderedEvent`] to the underlying [`Event`]. + let collected_events = collected_events + .into_iter() .map(|(oe, ca)| Event::new(oe, ca)) .collect(); Ok(collected_events)