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.

Sign up for GitHub

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

feat: reduce block overhead effect on estimations #7175

Merged
merged 1 commit into from
Jul 12, 2022
Merged

feat: reduce block overhead effect on estimations #7175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions genesis-tools/genesis-populate/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@ use near_primitives::types::{AccountId, Balance, EpochId, ShardId, StateChangeCa
use near_store::{get_account, set_access_key, set_account, set_code, Store, TrieUpdate};
use nearcore::{NearConfig, NightshadeRuntime};
use std::collections::BTreeMap;
use std::hash::{Hash, Hasher};
use std::path::{Path, PathBuf};
use std::sync::Arc;

fn get_account_id(account_index: u64) -> AccountId {
AccountId::try_from(format!("near_{}_{}", account_index, account_index)).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, never understood why we account-index twice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's a bit weird. It's to make it longer I suppose, as the account name length does matter quite a bit for how deep in a trie it can be.

Not sure if it was intentional by whomever designed this but this creates somewhat interesting paths for every account that look something like this:

  • extension node from root to "near_"
  • several branches to get through first account index
  • another extension for the second index

Almost the same thing could be achieved with "near_{account_index}_blablabla". But now there is a potential of sharing the extension node, as the state will be the same for all accounts and I think they will produce the same hashes.

Honestly, I don't think it matters. But let's not swap out too many gears at once.

pub fn get_account_id(account_index: u64) -> AccountId {
let mut hasher = std::collections::hash_map::DefaultHasher::new();
account_index.hash(&mut hasher);
let hash = hasher.finish();
AccountId::try_from(format!("{hash}_near_{account_index}_{account_index}")).unwrap()
}

pub type Result<T> = std::result::Result<T, Box<dyn std::error::Error>>;
Expand Down
6 changes: 4 additions & 2 deletions runtime/runtime-params-estimator/src/estimator_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use near_vm_logic::ExtCosts;
use crate::config::{Config, GasMetric};
use crate::gas_cost::GasCost;
use crate::testbed::RuntimeTestbed;
use crate::utils::get_account_id;
use genesis_populate::get_account_id;

use super::transaction_builder::TransactionBuilder;

Expand Down Expand Up @@ -47,7 +47,9 @@ impl<'c> EstimatorContext<'c> {
config: self.config,
inner,
transaction_builder: TransactionBuilder::new(
(0..self.config.active_accounts).map(get_account_id).collect(),
(0..self.config.active_accounts)
.map(|index| get_account_id(index as u64))
.collect(),
),
}
}
Expand Down
4 changes: 2 additions & 2 deletions runtime/runtime-params-estimator/src/transaction_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use std::collections::{HashMap, HashSet};

use genesis_populate::get_account_id;
use near_crypto::{InMemorySigner, KeyType};
use near_primitives::hash::CryptoHash;
use near_primitives::transaction::{Action, FunctionCallAction, SignedTransaction};
use near_primitives::types::AccountId;
use rand::prelude::ThreadRng;
use rand::Rng;

use crate::utils::get_account_id;
/// A helper to create transaction for processing by a `TestBed`.
#[derive(Clone)]
pub(crate) struct TransactionBuilder {
Expand Down Expand Up @@ -87,7 +87,7 @@ impl TransactionBuilder {
rand::thread_rng()
}

pub(crate) fn account(&mut self, account_index: usize) -> AccountId {
pub(crate) fn account(&mut self, account_index: u64) -> AccountId {
get_account_id(account_index)
}
pub(crate) fn random_account(&mut self) -> AccountId {
Expand Down
12 changes: 1 addition & 11 deletions runtime/runtime-params-estimator/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use crate::transaction_builder::TransactionBuilder;
use std::collections::HashMap;

use near_primitives::transaction::{Action, DeployContractAction, SignedTransaction};
use near_primitives::types::AccountId;
use near_vm_logic::{ExtCosts, VMConfig};
use rand::distributions::Alphanumeric;
use rand::Rng;
Expand Down Expand Up @@ -282,11 +281,7 @@ pub(crate) fn aggregate_per_block_measurements(
gas_cost.set_uncertain("HIGH-VARIANCE");
}
if let Some(overhead) = overhead {
// The assumption is that the overhead in the measurement due to applying blocks
// is negligible (<1%) and can therefore be ignored. This code is here to verify .
if overhead / block_size as u64 * 100 >= gas_cost {
gas_cost.set_uncertain("BLOCK-MEASUREMENT-OVERHEAD");
}
gas_cost = gas_cost.saturating_sub(&overhead, &NonNegativeTolerance::PER_MILLE);
}
(gas_cost, total_ext_costs)
}
Expand Down Expand Up @@ -336,11 +331,6 @@ pub(crate) fn percentiles(
.map(move |idx| costs[idx].clone())
}

/// Get account id from its index.
pub(crate) fn get_account_id(account_index: usize) -> AccountId {
AccountId::try_from(format!("near_{}_{}", account_index, account_index)).unwrap()
}

/// Produce a valid function name with `len` letters
pub(crate) fn generate_fn_name(index: usize, len: usize) -> Vec<u8> {
let mut name = Vec::new();
Expand Down