Skip to content

feat(synth-bm): allow multiple account prefixes #12935

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

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Feb 14, 2025

During benchmarking I wanted to spawn txs between different shards, and for that it was convenient to use prefixes in the form 2,c,h,m,x. Would it make sense in general? I retain sub_account_prefix as alias.

Also I had an issue with RPC node crashing: [2025-02-13T21:19:04Z WARN near_synth_bm::rpc] RPC call failed: error while sending payload: [error sending request for url (http://34.132.32.11:3030/)]
However, response_check_severity states that we can afford such errors, and it makes sense because they are very rare. So I suggest to use warn_or_panic for it as well.

@Longarithm Longarithm requested a review from a team as a code owner February 14, 2025 17:38
@Longarithm Longarithm requested a review from wacban February 14, 2025 17:38
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.

Thanks, overall LGTM, please just store prefixes in a Vec<String>.

/// - `a_user_<i>.<signer_account_id>,b_user_<i>.<signer_account_id>`
/// if `sub_account_prefixes == Some("a,b")`
#[arg(long, alias = "sub_account_prefix")]
pub sub_account_prefixes: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing prefixes in a Vec<String> would be more intuitive and requires just some minor tweaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +120 to +123
Err(err) => {
let msg = format!("RPC call failed: {}", err);
warn_or_panic(&msg, self.response_check_severity);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@mooori
Copy link
Contributor

mooori commented Feb 17, 2025

cc @Trisfald who is also working on using synth-bm with multiple shards.

Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

LGTM but please address comments from others

@Longarithm Longarithm enabled auto-merge February 17, 2025 13:07
@Longarithm Longarithm requested a review from mooori February 17, 2025 13:07
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.48%. Comparing base (489303d) to head (e1d8a49).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12935      +/-   ##
==========================================
+ Coverage   70.35%   70.48%   +0.13%     
==========================================
  Files         854      854              
  Lines      175420   175591     +171     
  Branches   175420   175591     +171     
==========================================
+ Hits       123412   123766     +354     
+ Misses      46881    46681     -200     
- Partials     5127     5144      +17     
Flag Coverage Δ
backward-compatibility 0.36% <ø> (-0.01%) ⬇️
db-migration 0.36% <ø> (-0.01%) ⬇️
genesis-check 1.42% <ø> (?)
linux 70.12% <ø> (-0.04%) ⬇️
linux-nightly 70.12% <ø> (+0.11%) ⬆️
pytests 1.73% <ø> (+0.17%) ⬆️
sanity-checks 1.53% <ø> (-0.01%) ⬇️
unittests 70.31% <ø> (+0.11%) ⬆️
upgradability 0.36% <ø> (-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.

@Longarithm Longarithm added this pull request to the merge queue Feb 17, 2025
Merged via the queue into near:master with commit 486590a Feb 17, 2025
29 checks passed
@Longarithm Longarithm deleted the synth-more-accounts branch February 17, 2025 15:08
shreyan-gupta pushed a commit to shreyan-gupta/nearcore that referenced this pull request Mar 28, 2025
During benchmarking I wanted to spawn txs between different shards, and
for that it was convenient to use prefixes in the form `2,c,h,m,x`.
Would it make sense in general? I retain `sub_account_prefix` as alias.

Also I had an issue with RPC node crashing: `[2025-02-13T21:19:04Z WARN
near_synth_bm::rpc] RPC call failed: error while sending payload: [error
sending request for url (http://34.132.32.11:3030/)]`
However, `response_check_severity` states that we can afford such
errors, and it makes sense because they are very rare. So I suggest to
use `warn_or_panic` for it as well.
shreyan-gupta pushed a commit to shreyan-gupta/nearcore that referenced this pull request Mar 28, 2025
During benchmarking I wanted to spawn txs between different shards, and
for that it was convenient to use prefixes in the form `2,c,h,m,x`.
Would it make sense in general? I retain `sub_account_prefix` as alias.

Also I had an issue with RPC node crashing: `[2025-02-13T21:19:04Z WARN
near_synth_bm::rpc] RPC call failed: error while sending payload: [error
sending request for url (http://34.132.32.11:3030/)]`
However, `response_check_severity` states that we can afford such
errors, and it makes sense because they are very rare. So I suggest to
use `warn_or_panic` for it as well.
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