(wip) (fix) Fix Storage Usage #4247

EgorKulikov · 2021-04-21T14:47:37Z

Storage usage is incorrect for some accounts on mainnet due to oversubtracting storage usage on access key deletion. Bug itself was already fixed. Delta between actual and expected storage usage was calculated on mainnet dump. Next time storage usage will be changed for such accounts they will be migrated to V2 and storage usage will be adjusted

Test plan

See test_storage_usage_delta

Moving Account to primitives

chain/rosetta-rpc/src/lib.rs

core/primitives/src/account.rs

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

bowenwang1996

where did you fix storage usage?

bowenwang1996 · 2021-04-21T18:11:39Z

core/primitives/src/account.rs

@@ -128,6 +145,16 @@ struct LegacyAccount {
    storage_usage: StorageUsage,
 }

+#[cfg(feature = "protocol_feature_add_account_versions")]
+#[derive(BorshSerialize, BorshDeserialize)]
+struct SerializableAccount {


should we implement From<SerializableAccount> for Account?

Considering it should ever be used only once I am not sure we need this

EgorKulikov · 2021-04-21T18:37:06Z

where did you fix storage usage?

It is not in yet, safely stashed until this builds, that's why is added wip to it

…fix-account-storage-usage

EgorKulikov · 2021-04-26T13:42:20Z

How do you plan to test this change?

I've introduced test, see test_storage_usage_delta

bowenwang1996

Can we add at least an integration test to test that this protocol feature works fine with upgrades? You can do it in client/tests/process_blocks.rs

bowenwang1996 · 2021-04-26T15:55:27Z

runtime/runtime/src/actions.rs

@@ -644,6 +673,13 @@ pub(crate) fn action_add_key(
        }
    );
    let storage_config = &apply_state.config.transaction_costs.storage_usage_config;
+    #[cfg(feature = "protocol_feature_add_account_versions")]
+    migrate_account(


it seems that you try to do this in every action. I wonder whether we can do it in apply_action or some higher level function to avoid code duplication

Not in every action, only in those that change storage usage

EgorKulikov · 2021-04-26T18:40:38Z

Can we add at least an integration test to test that this protocol feature works fine with upgrades? You can do it in client/tests/process_blocks.rs

Will do

…fix-account-storage-usage

frol

Is it possible to avoid the hardcoded CSV file and instead reproduce it on the fly? How long will it take? If that is not the way to go, how about generating .rs file instead of .csv? (I really don't like having more dependencies than necessary, and csv in runtime looks completely odd)

frol · 2021-04-27T15:56:18Z

core/primitives/src/runtime/apply_state.rs

+    /// True iff this ApplyState is on mainnet
+    #[cfg(feature = "protocol_feature_add_account_versions")]
+    pub is_mainnet: bool,


We introduce so many hacks right into our core primitives that it makes me uneasy. Why do we need to know if it is mainnet here? Let's just have the STORAGE_USAGE_DELTA_FROM_FILE empty on non-mainnet networks, and thus we won't need this ugly is_mainnet everywhere since the lookup for a given account in the STORAGE_USAGE_DELTA_FROM_FILE will never succeed on non-mainnet networks given the map is going to be empty.

runtime/runtime/src/actions.rs

frol · 2021-04-27T16:08:20Z

utils/storage-usage-delta-calculator/src/main.rs

+fn main() -> Result<(), Error> {
+    println!("Start");
+
+    let genesis = Genesis::from_file("output.json");


I feel like you expect to have a state dump in this output.json, but it is not clear. Please, provide more details in the doc comments as to how to use it.

frol · 2021-04-27T16:16:07Z

utils/storage-usage-delta-calculator/src/main.rs

+    let storage_usage =
+        Runtime::compute_storage_usage(&genesis.records.0[..], &RuntimeConfig::default());
+    println!("Storage usage calculated");
+    for record in genesis.records.0 {


I believe this should work:

Suggested change

let storage_usage =

Runtime::compute_storage_usage(&genesis.records.0[..], &RuntimeConfig::default());

println!("Storage usage calculated");

for record in genesis.records.0 {

let storage_usage =

Runtime::compute_storage_usage(genesis.records.as_ref(), &RuntimeConfig::default());

println!("Storage usage calculated");

for record in genesis.records.into() {

P.S. GenesisRecords needs one more derive DeriveMore::Into to support .into()

frol · 2021-04-27T16:25:03Z

runtime/runtime/src/actions.rs

+
+#[cfg(feature = "protocol_feature_add_account_versions")]
+lazy_static::lazy_static! {
+    static ref STORAGE_USAGE_DELTA_FROM_FILE: HashMap<AccountId, u64> = read_storage_usage_delta();


We already did enough hacks around by pulling a random CSV file right into the binary, let's at least have the HashMap properly propagated through the configuration. neard should be the single source of configuration, we should not hijack runtime and get changed its operation due to some random global variable. Let neard hold the file, parse it conditionally when the chain id is mainnet, and pass it through the runtime configs down here if necessary.

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

EgorKulikov · 2021-04-27T17:57:25Z

Is it possible to avoid the hardcoded CSV file and instead reproduce it on the fly? How long will it take? If that is not the way to go, how about generating .rs file instead of .csv? (I really don't like having more dependencies than necessary, and csv in runtime looks completely odd)

I'd rather parse csv manually than put more than 3k lines of data directly into rs file

matklad · 2021-04-28T13:15:48Z

I'd rather parse csv manually than put more than 3k lines of data directly into rs file

Another option is to convert CSV to borsh once, and then, at/in runtime, include!(data.borsh) and deserialize that. Does conceptually the same thing as embedding & parsing CSV, but re-usese existing deps

frol · 2021-04-28T13:46:04Z

Another option is to convert CSV to borsh

Well, it will be even more magical and impossible to review, so I would not go with it

EgorKulikov · 2021-04-28T16:34:59Z

Well, it will be even more magical and impossible to review, so I would not go with it

We can go with serde, I guess

bowenwang1996 · 2021-04-29T02:46:39Z

Is it possible to avoid the hardcoded CSV file and instead reproduce it on the fly? How long will it take? If that is not the way to go, how about generating .rs file instead of .csv? (I really don't like having more dependencies than necessary, and csv in runtime looks completely odd)

@frol it is impossible to reproduce it while the node is running as it is prohibitively expensive to iterate over the state. More complex solutions are possible, but I don't think it is worth the effort here.

frol · 2021-04-29T08:44:22Z

We can go with serde, I guess

JSON or manual CSV parsing is fine. Let's move the preparation logic and the artifact file to neard crate and pass the adjustment delta-table to runtime through configuration.

Account V2

134a899

Moving Account to primitives

EgorKulikov requested review from ailisp, bowenwang1996, evgenykuzyakov, frol, MaksymZavershynskyi and SkidanovAlex as code owners April 21, 2021 14:47

matklad reviewed Apr 21, 2021

View reviewed changes

chain/rosetta-rpc/src/lib.rs Outdated Show resolved Hide resolved

core/primitives/src/account.rs Outdated Show resolved Hide resolved

core/primitives/src/account.rs Show resolved Hide resolved

core/primitives/src/account.rs Show resolved Hide resolved

EgorKulikov and others added 4 commits April 21, 2021 17:16

Update core/primitives/src/account.rs

816a00f

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Update chain/rosetta-rpc/src/lib.rs

ab029b8

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Merge branch 'master' into fix-account-storage-usage

2e028a0

Fixing build

d73e475

EgorKulikov requested review from chefsale and khorolets as code owners April 21, 2021 17:15

Fixing build

ecaabfc

bowenwang1996 reviewed Apr 21, 2021

View reviewed changes

Fixing build

b110634

EgorKulikov requested a review from olonho as a code owner April 21, 2021 18:30

EgorKulikov added 2 commits April 21, 2021 21:39

Fixing build

1fb8a6b

Fixing build

dbef212

EgorKulikov added the S-donotmerge Status: do not merge this PR label Apr 21, 2021

EgorKulikov added 8 commits April 21, 2021 23:08

Fixing test

9f388b0

Merge branch 'master' into fix-account-storage-usage

2cb26d4

Storage usage fix

933d861

Merge remote-tracking branch 'origin/fix-account-storage-usage' into …

f76351b

…fix-account-storage-usage

Compliation fix

0bf112b

Compliation fix

869aa91

Compliation fix

77e4bc0

Merge branch 'master' into fix-account-storage-usage

40d7981

EgorKulikov added 2 commits April 26, 2021 14:10

Compilation fix

54a4335

Compilation fix

6d96ee0

EgorKulikov linked an issue Apr 26, 2021 that may be closed by this pull request

Recalculate storage usage based on the actual state #3824

Closed

Merge branch 'master' into fix-account-storage-usage

87b76c9

EgorKulikov requested a review from Longarithm April 26, 2021 16:07

bowenwang1996 reviewed Apr 26, 2021

View reviewed changes

EgorKulikov added 4 commits April 27, 2021 12:58

Merge branch 'master' into fix-account-storage-usage

1748c6c

Removing unnecessary into

7e68414

Merge remote-tracking branch 'origin/fix-account-storage-usage' into …

f6c5bd0

…fix-account-storage-usage

Merge branch 'master' into fix-account-storage-usage

53f2729

bowenwang1996 requested a review from frol April 27, 2021 16:11

frol requested changes Apr 27, 2021

View reviewed changes

Update runtime/runtime/src/actions.rs

b8ad914

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

Merge branch 'master' into fix-account-storage-usage

2a94d2b

EgorKulikov added 3 commits April 29, 2021 13:55

Merge branch 'master' into fix-account-storage-usage

7fc8509

Merge branch 'master' into fix-account-storage-usage

6e2f721

Compilation fix

8e61380

EgorKulikov changed the title ~~(wip)(fix) Fix Storage Usage~~ (fix) Fix Storage Usage May 6, 2021

EgorKulikov changed the title ~~(fix) Fix Storage Usage~~ (wip) (fix) Fix Storage Usage May 6, 2021

EgorKulikov marked this pull request as draft May 6, 2021 16:33

EgorKulikov closed this May 10, 2021

Ekleog-NEAR deleted the fix-account-storage-usage branch March 29, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(wip) (fix) Fix Storage Usage #4247

(wip) (fix) Fix Storage Usage #4247

EgorKulikov commented Apr 21, 2021 •

edited

Loading

bowenwang1996 left a comment

bowenwang1996 Apr 21, 2021

EgorKulikov Apr 21, 2021

EgorKulikov commented Apr 21, 2021

EgorKulikov commented Apr 26, 2021

bowenwang1996 left a comment

bowenwang1996 Apr 26, 2021

EgorKulikov Apr 26, 2021

EgorKulikov commented Apr 26, 2021

frol left a comment

frol Apr 27, 2021

frol Apr 27, 2021

frol Apr 27, 2021

frol Apr 27, 2021

EgorKulikov commented Apr 27, 2021

matklad commented Apr 28, 2021

frol commented Apr 28, 2021

EgorKulikov commented Apr 28, 2021

bowenwang1996 commented Apr 29, 2021

frol commented Apr 29, 2021

(wip) (fix) Fix Storage Usage #4247

(wip) (fix) Fix Storage Usage #4247

Conversation

EgorKulikov commented Apr 21, 2021 • edited Loading

Test plan

bowenwang1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorKulikov commented Apr 21, 2021

EgorKulikov commented Apr 26, 2021

bowenwang1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorKulikov commented Apr 26, 2021

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorKulikov commented Apr 27, 2021

matklad commented Apr 28, 2021

frol commented Apr 28, 2021

EgorKulikov commented Apr 28, 2021

bowenwang1996 commented Apr 29, 2021

frol commented Apr 29, 2021

EgorKulikov commented Apr 21, 2021 •

edited

Loading