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

Add script to check block composition #131

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

FrancoGiachetta
Copy link
Contributor

@FrancoGiachetta FrancoGiachetta commented Jan 22, 2025

This PR adds a script that checks the average tx, transfers and swap an average block has (separated by the day of its execution) and generates a plot which also aggregates this information by day. It also adds a new command which executes a block range, collecting every entrypoint in block_composition/<block_execution_range_name>.json. Every .json generated is a range of execution, we can have separated block ranges executing. Then, the script will loop through the files inside the folder and process all of them at once.

To be able to run the script first you need to collect some data:

cargo run --release -F block-composition block-compose 862063 862065 mainnet

Then you run the script:

python plotting/plot_block_composition.py <path_to_block_info>

PD: by deafult <path_to_block_info> is block_composition

Copy link

github-actions bot commented Jan 22, 2025

❌ Code is not formatted! Please run cargo format and push the changes.

@FrancoGiachetta FrancoGiachetta marked this pull request as ready for review January 28, 2025 20:54
@FrancoGiachetta FrancoGiachetta marked this pull request as draft January 30, 2025 23:57
@FrancoGiachetta FrancoGiachetta marked this pull request as ready for review January 31, 2025 15:02
Copy link
Contributor

@JulianGCalderon JulianGCalderon left a comment

Choose a reason for hiding this comment

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

Hi @FrancoGiachetta! I left you some comments we can discuss. Let me know what you think.

Also, can you add some documentation of this feature in the README?

Comment on lines 15 to 19
type BlockExecutionInfo = Vec<(
u64,
String,
Vec<Result<TransactionExecutionInfo, TransactionExecutionError>>,
)>;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the meaning of the u64 and the string? I think it would be better to declare a struct with named field:

struct BlockExecutionInfo {
    block_number: u64,
    timestamp?: String,
    transactions: Vec<...>
}

Copy link
Contributor Author

@FrancoGiachetta FrancoGiachetta Jan 31, 2025

Choose a reason for hiding this comment

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

It was for simplicity, since it just acts as a holder I didn't think it was worth to create another struct.

Comment on lines 21 to 32
#[derive(Debug, Serialize)]
struct BlockEntryPoints {
block_number: u64,
block_timestamp: String,
entrypoints: Vec<HashMap<String, Vec<EntryPointExecution>>>,
}

#[derive(Debug, Serialize)]
struct EntryPointExecution {
class_hash: ClassHash,
selector: EntryPointSelector,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we aim for re-usability of the data, I think it's better to save the execution trace as a tree, rather than a flattened list. That way we have that information if we ever need it, and if you need it as a vector you can easily flatten it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand what would be the benefit of using a tree instead of a vector in this case.

replay/src/block_composition.rs Show resolved Hide resolved
Comment on lines 52 to 69
if let Some(call) = execution.validate_call_info {
tx_execution.insert(
"validate_call_info".to_string(),
get_inner_class_executions(call),
);
}
if let Some(call) = execution.execute_call_info {
tx_execution.insert(
"execute_call_info".to_string(),
get_inner_class_executions(call),
);
}
if let Some(call) = execution.fee_transfer_call_info {
tx_execution.insert(
"fee_transfer_call_info".to_string(),
get_inner_class_executions(call),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If you only ever save these three values in the Hashmap (validate_call_info, execute_call_info, fee_transfer_call_info), I think its better to define a new struct (i.e: TransactionEntryPoints) with those fields, instead of using a generic HashMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that then

replay/src/main.rs Show resolved Hide resolved
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.

3 participants