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

✨ Improved statistics with reporting of CU used by fuzzed transactions #203

Closed
wants to merge 14 commits into from

Conversation

st22nestrel
Copy link

@st22nestrel st22nestrel commented Sep 25, 2024

Description

This PR improves optional statistics logged at the end of fuzzing (introduced in #144). Function to process transaction with metadata was created, in order to be able to obtain CU used by the transaction. Reporting of minimum and maximum CU used for both failed and successful transactions was added.

Related Tickets & Documents

  • Related Issue #

  • Closes #

  • I clicked on "Allow edits from maintainers"

Copy link
Collaborator

@lukacan lukacan left a comment

Choose a reason for hiding this comment

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

Please resolve mentioned changes.

I forgot to mention, please update the documentation accordingly.

I see that the FuzzTestExecutor is getting a bit messy. I'd like to improve this in a separate PR. Also, I don’t like that users can't choose which type of stats they'd like to see. I think we can resolve this by loading the config before fuzzing starts, which would also eliminate unnecessary overhead. Gonna work on it in separate PR.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -93,10 +111,60 @@ impl FuzzingStatistics {
successful: 1,
failed: 0,
failed_check: 1,
cu_used_max: 0,
cu_used_min: u64::MAX,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be misleading if every transaction fails, as it would show that the minimum amount was u64::MAX. I understand why you approached it this way, but I think we need to consider if there’s a better solution.

Copy link
Author

Choose a reason for hiding this comment

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

I think checking for whether these values are still set to default in here f1e4fc4#diff-71ae76d79a6fc8fef2696e928840ebdd85005943030feaf457405706f81686c2R230 and if they are, outputting Not available or something like that would be ok. What do you think?

crates/fuzz/src/program_test_client_blocking.rs Outdated Show resolved Hide resolved
match transaction_result {
Ok(result_with_metadata) => {
// We got metadata => transaction was either successful or returned with TransactionError
match result_with_metadata.result {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I would consider to first unwrap the Metadata and then based on the result (Ok , or Err), update the appropriate stats, but as I meantioned previously , this should be handled in executor not here.

@lukacan lukacan closed this Jan 2, 2025
@lukacan lukacan deleted the feat/CU_stats branch January 9, 2025 20:16
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