(fix) Fix storage usage bug with migration #4274

EgorKulikov · 2021-05-04T20:36:12Z

On some further though we are actually able to migrate this inside a single block

Test plan

Sanity test in migrations.rs
Integration test in process_blocks

core/primitives/src/runtime/migration_data.rs

core/primitives/src/views.rs

neard/src/runtime/mod.rs

core/primitives/src/types.rs

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

runtime/runtime/src/lib.rs

core/primitives/src/types.rs

neard/src/runtime/mod.rs

runtime/runtime/src/lib.rs

…o migration-fix-storage-usage

frol

I don't see what prevents this migration from being applied on every single epoch start. Do I miss something?

Another corner case I have in mind is: what if the very first block on the epoch won't be produced? Will this still work fine? I am not very familiar with the block production code.

chain/rosetta-rpc/src/adapters/mod.rs

frol · 2021-05-10T14:32:42Z

core/primitives/src/runtime/migration_data.rs

+#[derive(Default)]
+pub struct MigrationData {
+    #[cfg(feature = "protocol_feature_fix_storage_usage")]
+    pub storage_usage_delta: Vec<(AccountId, u64)>,


I would prefer to have a struct with named arguments as u64 is not really descriptive

If we use named structure in json this will lead to additional 70kb of data included executable

EgorKulikov · 2021-05-10T15:04:37Z

I don't see what prevents this migration from being applied on every single epoch start. Do I miss something?

migration_data in ApplyState in only Some when protocol version changes, not just epoch

EgorKulikov · 2021-05-10T15:05:47Z

Another corner case I have in mind is: what if the very first block on the epoch won't be produced? Will this still work fine? I am not very familiar with the block production code.

It follows same logic as changes to validators at the start of an epoch

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

frol

All my concerns are clarified.

@EgorKulikov This implementation is much cleaner, thank you for re-implementing it to this maintainable state! 🚀

frol · 2021-05-11T11:00:05Z

core/primitives/src/types.rs

@@ -152,6 +152,9 @@ pub enum StateChangeCause {
    /// State change that happens when we update validator accounts. Not associated with with any
    /// specific transaction or receipt.
    ValidatorAccountsUpdate,
+    /// State change that is happens due to migration that happens in first block of an epoch


nit:

Suggested change

/// State change that is happens due to migration that happens in first block of an epoch

/// State change that happens due to state-correction migration that happens in the first block of an epoch

Ideally, reference this PR as an example for the reader, so they don't need to do git-blame to look up what kind of "state-corrections" happen.

chain/client/tests/process_blocks.rs

olonho · 2021-05-11T14:54:04Z

neard/src/migrations.rs

+const GAS_USED_FOR_STORAGE_USAGE_DELTA_MIGRATION: Gas = 490_000_000_000_000;
+
+pub fn load_migration_data(chain_id: &String) -> MigrationData {
+    #[cfg(not(feature = "protocol_feature_fix_storage_usage"))]


Separate blocks under #[cfg(feature = "protocol_feature_fix_storage_usage")] and #[cfg(not(feature = "protocol_feature_fix_storage_usage"))] could be more readable.

olonho · 2021-05-11T15:40:06Z

runtime/runtime/src/lib.rs

+                    Some(mut account) => {
+                        // Storage usage is saved in state, hence it is nowhere close to max value
+                        // of u64, and maximal delta is 4196, se we can add here without checking
+                        // for overflow


Maybe checked add would be just easier than long comment why it's not needed :)?

olonho

LGTM, just few nits.

bowenwang1996 · 2021-05-11T18:52:18Z

chain/client/tests/process_blocks.rs

@@ -2267,6 +2275,135 @@ fn test_epoch_protocol_version_change() {
    assert_eq!(protocol_version, PROTOCOL_VERSION + 1);
 }

+#[test]
+#[cfg(feature = "protocol_feature_fix_storage_usage")]


It is better to group the tests for some protocol feature in one mod so that the imports are cleaner, e.g.

#[cfg(test)] mod protocol_feature_fix_storage_usage_tests { .... }

Will do, though I have to notice that there is a lot of different kinds of tests in this file and just one mod

bowenwang1996 · 2021-05-11T18:52:44Z

chain/client/tests/process_blocks.rs

+            assert_eq!(account_test0.storage_usage(), 182);
+        }
+    }
+    {


If it is a different test, please actually make it separate.

They are kinda the same, just with different chain ids

Then it makes sense to refactor the test to avoid code duplication. Something like

fn test_with_chain_id(chain_id: &str) { .... }

I did something like that, not sure it is more understandable though

bowenwang1996 · 2021-05-11T18:53:50Z

chain/client/tests/process_blocks.rs

+            let (encoded_chunk, merkle_paths, receipts) =
+                create_chunk_on_height(&mut env.clients[0], i);
+
+            let mut chain_store =
+                ChainStore::new(env.clients[0].chain.store().owned_store(), genesis_height);
+            env.clients[0]
+                .shards_mgr
+                .distribute_encoded_chunk(
+                    encoded_chunk.clone(),
+                    merkle_paths.clone(),
+                    receipts.clone(),
+                    &mut chain_store,
+                )
+                .unwrap();
+
+            let mut block = env.clients[0].produce_block(i).unwrap().unwrap();
+
+            let validator_signer = InMemoryValidatorSigner::from_seed(
+                &"test0".to_string(),
+                KeyType::ED25519,
+                &"test0".to_string(),
+            );
+
+            block.mut_header().get_mut().inner_rest.latest_protocol_version =
+                ProtocolFeature::FixStorageUsage.protocol_version();
+            block.mut_header().resign(&validator_signer);


Could you explain what is done here? This looks the same if you were to call TestEnv::produce_block

Maybe. I started with test_epoch_protocol_version_change as a template for my test. I'll look if this can be refactored

I need to update protocol version, so no, I cannot just use TestEnv::produce_block

runtime/runtime/src/lib.rs

…o migration-fix-storage-usage

chain/client/tests/process_blocks.rs

core/primitives/src/version.rs

Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>

EgorKulikov added 2 commits May 4, 2021 22:18

Fix storage usage bug with migration

d7c8fd6

Sanity test

c24c930

EgorKulikov requested a review from Longarithm May 4, 2021 20:36

EgorKulikov requested review from bowenwang1996, evgenykuzyakov, frol, MaksymZavershynskyi, olonho and SkidanovAlex as code owners May 4, 2021 20:36

EgorKulikov added 6 commits May 4, 2021 22:36

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

f61efed

Compilation fix, added description to migration cause

4e762c7

Added feature to root cargo

91d6ef8

Compilation fix

3299762

Compilation fix

45a9802

Compilation fix

8c42e7a

matklad reviewed May 5, 2021

View reviewed changes

core/primitives/src/runtime/migration_data.rs Outdated Show resolved Hide resolved

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

neard/src/runtime/mod.rs Outdated Show resolved Hide resolved

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

EgorKulikov and others added 3 commits May 5, 2021 11:50

Update core/primitives/src/views.rs

4f8363b

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

Suggestions by matklad

3a7426d

Suggestions by matklad

3817c08

Longarithm reviewed May 5, 2021

View reviewed changes

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved

matklad reviewed May 5, 2021

View reviewed changes

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

EgorKulikov added 2 commits May 5, 2021 12:35

Removing unneeded trait

d7e903f

Additional comment

af8d2fe

EgorKulikov linked an issue May 5, 2021 that may be closed by this pull request

Recalculate storage usage based on the actual state #3824

Closed

Longarithm mentioned this pull request May 5, 2021

Restore receipts after recent fix #4248

Merged

EgorKulikov added 3 commits May 5, 2021 15:09

Display is needed after all

fdcff6f

Getting read of EMPTY

3a2b50e

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

907ee3e

bowenwang1996 reviewed May 5, 2021

View reviewed changes

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

neard/src/runtime/mod.rs Outdated Show resolved Hide resolved

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved

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

771530e

EgorKulikov added 4 commits May 10, 2021 11:33

Merge remote-tracking branch 'origin/migration-fix-storage-usage' int…

f8aa99e

…o migration-fix-storage-usage

Compilation fix

11b6b02

Compilation fix

921d96e

Compilation fix

ec1f41c

frol requested changes May 10, 2021

View reviewed changes

EgorKulikov and others added 5 commits May 10, 2021 17:10

Update chain/rosetta-rpc/src/adapters/mod.rs

4c7e883

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

MigrationId no longer used

43511a2

MigrationId no longer used

571c082

Compilation fix

23e1319

Compilation fix

f54b1f8

frol approved these changes May 11, 2021

View reviewed changes

frol reviewed May 11, 2021

View reviewed changes

olonho reviewed May 11, 2021

View reviewed changes

chain/client/tests/process_blocks.rs Outdated Show resolved Hide resolved

olonho reviewed May 11, 2021

View reviewed changes

olonho approved these changes May 11, 2021

View reviewed changes

bowenwang1996 requested changes May 11, 2021

View reviewed changes

EgorKulikov added 5 commits May 12, 2021 21:17

Refactoring test

2d1dd18

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

1b0bbfc

Adressing comments and changing constant

ab12aed

Merge remote-tracking branch 'origin/migration-fix-storage-usage' int…

bd4af16

…o migration-fix-storage-usage

Adressing comments

60e970c

bowenwang1996 approved these changes May 13, 2021

View reviewed changes

chain/client/tests/process_blocks.rs Show resolved Hide resolved

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

EgorKulikov and others added 2 commits May 13, 2021 15:35

Update core/primitives/src/version.rs

aeb83ea

Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>

Addressing comments

35750fe

EgorKulikov added the automerge label May 13, 2021

near-bulldozer bot merged commit 473e71b into master May 13, 2021

near-bulldozer bot deleted the migration-fix-storage-usage branch May 13, 2021 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) Fix storage usage bug with migration #4274

(fix) Fix storage usage bug with migration #4274

EgorKulikov commented May 4, 2021 •

edited

Loading

frol left a comment

frol May 10, 2021

EgorKulikov May 10, 2021

EgorKulikov commented May 10, 2021

EgorKulikov commented May 10, 2021

frol left a comment

frol May 11, 2021

olonho May 11, 2021

olonho May 11, 2021

olonho left a comment

bowenwang1996 May 11, 2021

EgorKulikov May 11, 2021

bowenwang1996 May 11, 2021

EgorKulikov May 11, 2021

bowenwang1996 May 12, 2021

EgorKulikov May 12, 2021

bowenwang1996 May 11, 2021

EgorKulikov May 11, 2021

EgorKulikov May 12, 2021

	/// State change that is happens due to migration that happens in first block of an epoch
	/// State change that happens due to state-correction migration that happens in the first block of an epoch

(fix) Fix storage usage bug with migration #4274

(fix) Fix storage usage bug with migration #4274

Conversation

EgorKulikov commented May 4, 2021 • edited Loading

Test plan

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

EgorKulikov commented May 10, 2021

EgorKulikov commented May 10, 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

olonho 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

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 May 4, 2021 •

edited

Loading