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

Storage usage #1202

Merged
merged 4 commits into from Aug 22, 2019

Conversation

@mikhailOK
Copy link
Collaborator

commented Aug 20, 2019

Storage usage for account is counted as:

  • sum of lengths of keys and values stored by the contract
  • sum of lengths of access keys (serialized) and public keys
  • fixed cost per key-value record of account data and access keys
  • contract code size
  • fixed cost of 100 for metadata overhead

TODO: integration test

Storage usage
Storage usage for account is counted as:
- sum of lengths of keys and values stored by the contract
- sum of lengths of access keys (serialized) and public keys
- contract code size
- fixed cost of 100 for metadata overhead

TODO: integration test

@mikhailOK mikhailOK requested a review from nearmax Aug 20, 2019

@@ -297,6 +311,9 @@ pub(crate) fn action_add_key(
return;
}
set_access_key(state_update, account_id, &add_key.public_key, &add_key.access_key);
account.storage_usage += (add_key.public_key.as_ref().len()

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Aug 20, 2019

Collaborator

AccessKeys also use account_id and a separator as a prefix. Should we include them or we ignore them, because they are shared?

This comment has been minimized.

Copy link
@mikhailOK

mikhailOK Aug 20, 2019

Author Collaborator

We don't include them for contract data, so not doing it here either.

This comment has been minimized.

Copy link
@nearmax

nearmax Aug 21, 2019

Collaborator

We don't include them for contract data, so not doing it here either.

You mean account data? Account storage cost has a fixed storage value, which works because it is one. We can have multiple access keys prefixed with very long account names. I suggest we add two fields to RuntimeConfig : storage usage of account in bytes (computed assuming Account is populated to the max) and base storage usage of the access key (computed assuming max account id length).

This comment has been minimized.

Copy link
@mikhailOK

mikhailOK Aug 21, 2019

Author Collaborator

Account data / data stored by contract executions. All of it it stored in the trie prefixed with account name, but we don't count the prefix.

This comment has been minimized.

Copy link
@nearmax

nearmax Aug 21, 2019

Collaborator

We then should have a base cost for having an entry in KV storage, both for account data and access keys. Because someone can create a ton of empty KVs (Let's say all combinations of 4 bytes per key) pay almost nothing but force us to use a ton of storage because of prefixes.

let code = ContractCode::new(deploy_contract.code.clone());
account.as_mut().unwrap().code_hash = code.get_hash();
let prev_code = get_code(state_update, account_id);
account.storage_usage -= prev_code.map(|code| code.code.len() as u64).unwrap_or_default();

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Aug 20, 2019

Collaborator

should code also should pay for the key?

This comment has been minimized.

Copy link
@mikhailOK

mikhailOK Aug 20, 2019

Author Collaborator

key length is account name length, we could add extra fixed cost to punish long names but shouldn't matter too much because names have a limit.

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

It's also going to be a pain to calculate storage_usage during the migration in #1201

@@ -20,7 +20,14 @@ pub struct Account {

impl Account {
pub fn new(amount: Balance, code_hash: CryptoHash, storage_paid_at: BlockIndex) -> Self {
Account { amount, staked: 0, code_hash, storage_usage: 0, storage_paid_at }
let account_storage_overhead = 100;

This comment has been minimized.

Copy link
@bowenwang1996

bowenwang1996 Aug 21, 2019

Member

Should we make a constant for this number? Also is it fixed or do we potentially want to change it in the future?

This comment has been minimized.

Copy link
@mikhailOK

mikhailOK Aug 21, 2019

Author Collaborator

To change it we have to update existing accounts

@nearmax nearmax closed this Aug 21, 2019

@nearmax

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

Accidentally closed PR. Fat fingers.

@nearmax nearmax reopened this Aug 21, 2019

}

fn storage_remove(&mut self, key: &[u8]) -> Result<Option<Vec<u8>>, ExternalError> {
let storage_key = self.create_storage_key(key);
Ok(self.trie_update.remove(&storage_key))
let evicted = self.trie_update.get(&storage_key).map(DBValue::into_vec);

This comment has been minimized.

Copy link
@nearmax

nearmax Aug 21, 2019

Collaborator

This does not actually remove it?

This comment has been minimized.

Copy link
@nearmax

nearmax Aug 21, 2019

Collaborator

What was the reason of removing return values from TrieUpdate::{set, remove}? It was done to match the conventional API of std collections where inserting or removing element returns an potentially evicted element. Also would've helped to avoid this kind of mistakes.

This comment has been minimized.

Copy link
@mikhailOK

mikhailOK Aug 21, 2019

Author Collaborator

This does not actually remove it?

whoops

What was the reason of removing return values from TrieUpdate::{set, remove}?

they had a bug, only returned the previous value from uncommitted changes set

@@ -297,6 +311,9 @@ pub(crate) fn action_add_key(
return;
}
set_access_key(state_update, account_id, &add_key.public_key, &add_key.access_key);
account.storage_usage += (add_key.public_key.as_ref().len()

This comment has been minimized.

Copy link
@nearmax

nearmax Aug 21, 2019

Collaborator

We don't include them for contract data, so not doing it here either.

You mean account data? Account storage cost has a fixed storage value, which works because it is one. We can have multiple access keys prefixed with very long account names. I suggest we add two fields to RuntimeConfig : storage usage of account in bytes (computed assuming Account is populated to the max) and base storage usage of the access key (computed assuming max account id length).

@@ -20,7 +20,14 @@ pub struct Account {

impl Account {
pub fn new(amount: Balance, code_hash: CryptoHash, storage_paid_at: BlockIndex) -> Self {
Account { amount, staked: 0, code_hash, storage_usage: 0, storage_paid_at }
let account_storage_overhead = 100;
Account {

This comment has been minimized.

Copy link
@nearmax

nearmax Aug 21, 2019

Collaborator

We should have this as a number in the config. See my comment below. And assign storage_usage upon executing action_create_account instead of doing it inside the constructor of Account. In this way we avoid having this number hardcoded in the primitives crate.

@nearmax

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

Please fix the tests that are failing due to this change.

mikhailOK added 2 commits Aug 22, 2019

@mikhailOK mikhailOK merged commit deee97a into staging Aug 22, 2019

0 of 2 checks passed

GitLab CI job test_nearlib (pull request) script_failure on GitLab CI
Details
GitLab CI pipeline (pull request) Pipeline completed with errors on GitLab CI
Details

@bowenwang1996 bowenwang1996 deleted the storage_usage branch Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.