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

bench(synth-bench): Add read_nonces_from_network flag to benchmark binaries #12826

Merged
merged 4 commits into from
Feb 10, 2025

Conversation

ssavenko-near
Copy link
Contributor

@ssavenko-near ssavenko-near commented Jan 29, 2025

issue 12805
The flag (default=false) triggers the update of the account nonces from the network before generating the transactions.

impl details:

  • extract the update_account_nonces() function
  • add a corresponding flag to benhmark_native_transfers and benchmark_mpc binaries
    • apply the update_account_nonces() depending on the flag value

tested:

  • run the benchmark from the clean state
  • edit the a_user_0.test.near.json, set "nonce" to 7000001
  • update the log level to "trace" in the justfile
  • re-run the benchmark
  • find the [2025-01-29T13:06:56Z TRACE near_synth_bm::account] updating nonce for a user a_user_0.test.near (7000001->7000020) in the log

@ssavenko-near ssavenko-near linked an issue Jan 29, 2025 that may be closed by this pull request
3 tasks
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.41%. Comparing base (57b9b81) to head (567e7ad).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12826   +/-   ##
=======================================
  Coverage   70.41%   70.41%           
=======================================
  Files         848      848           
  Lines      174849   174930   +81     
  Branches   174849   174930   +81     
=======================================
+ Hits       123117   123185   +68     
- Misses      46485    46489    +4     
- Partials     5247     5256    +9     
Flag Coverage Δ
backward-compatibility 0.16% <ø> (-0.01%) ⬇️
db-migration 0.16% <ø> (?)
genesis-check 1.41% <ø> (-0.01%) ⬇️
linux 70.02% <ø> (-0.07%) ⬇️
linux-nightly 70.06% <ø> (-0.02%) ⬇️
pytests 1.70% <ø> (+0.29%) ⬆️
sanity-checks 1.52% <ø> (?)
unittests 70.25% <ø> (-0.04%) ⬇️
upgradability 0.20% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ssavenko-near ssavenko-near changed the title [benchmarks] Add read_nonces_from_network flag to benchmark binaries synth-bench: Add read_nonces_from_network flag to benchmark binaries Jan 29, 2025
@ssavenko-near ssavenko-near changed the title synth-bench: Add read_nonces_from_network flag to benchmark binaries bench(synth-bench): Add read_nonces_from_network flag to benchmark binaries Jan 29, 2025
@ssavenko-near ssavenko-near force-pushed the slavas/bm-nonce branch 2 times, most recently from 827d5c1 to 88bb222 Compare January 29, 2025 13:20
[issue 12805](#12805)
The flag (default=false) triggers the update of the account nonces from the network before generating the transactions.

impl details:
  - extract the `update_account_nonces()` function
  - add a corresponding flag to benhmark_native_transfers and benchmark_mpc binaries
    + apply the `update_account_nonces()` depending on the flag value

tested:
   - run the benchmark from the clean state
   - edit the `a_user_0.test.near.json`, set "nonce" to 7000001
   - update the log level to "trace" in the justfile
   - re-run the benchmark
   - find the `[2025-01-29T13:06:56Z TRACE near_synth_bm::account] updating nonce for a user a_user_0.test.near (7000001->7000020)` in the log
@ssavenko-near ssavenko-near marked this pull request as ready for review January 29, 2025 13:24
@ssavenko-near ssavenko-near requested a review from a team as a code owner January 29, 2025 13:24
Copy link
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

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

Looking good, just some nits regarding the implementation.

As we want to improve docs, I would suggest adding a section for --read-nonces-from-network to common parameters:

  • Explaining why it exists (see issue description).
  • Recommend it when using few accounts resp. when it's ok to wait for the nonces being queried.

benchmarks/synth-bm/src/account.rs Outdated Show resolved Hide resolved
benchmarks/synth-bm/src/account.rs Outdated Show resolved Hide resolved
benchmarks/synth-bm/src/native_transfer.rs Outdated Show resolved Hide resolved
benchmarks/synth-bm/src/native_transfer.rs Outdated Show resolved Hide resolved
@ssavenko-near
Copy link
Contributor Author

As we want to improve docs, I would suggest adding a section for --read-nonces-from-network to common parameters:

I will definitely update it after the other PRs touching the benchmarking docs land. That will probably be easier than de-conflicting it.

@ssavenko-near ssavenko-near requested a review from mooori January 29, 2025 15:48
Copy link
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

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

LGTM, please just change log to tracing.

@ssavenko-near
Copy link
Contributor Author

LGTM, please just change log to tracing.

done

@mooori mooori requested a review from nagisa February 7, 2025 14:01
@nagisa
Copy link
Collaborator

nagisa commented Feb 7, 2025

There are a number of spellcheck errors that you'll need to resolve before this can land.

sub_accounts = update_account_nonces(
client.clone(),
sub_accounts,
1_000_000 / args.interval_duration_micros,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason this code goes from duration to Hz and then back within the function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a side effect of having multiple PRs in flight.

@ssavenko-near ssavenko-near added this pull request to the merge queue Feb 10, 2025
Merged via the queue into master with commit 6cddee1 Feb 10, 2025
21 of 24 checks passed
@ssavenko-near ssavenko-near deleted the slavas/bm-nonce branch February 10, 2025 10:02
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.

synth-bench: query nonces from RPC before sending traffic
3 participants