Support state migration on upgrade #103

birchmd · 2021-05-20T18:36:45Z

This PR is much smaller than it looks based on the diff line count. The majority of the changes come from adding a proper test for this functionality (the original test_upgrade was not sufficient).

The key differences in the main code are:

The addition of state_migration as a new public method of the contract
Changing sdk::self_deploy to call state_migration after it has deployed the new code.

To test this functionality I define a test contract and upgrade to it, checking that the upgrade succeed by calling a new function it exposes. To prevent duplicating logic, the test contract imports the production contract as a library, which required some small code reorganization in lib.rs and changing the crate features a little to expose the sdk and engine logic, but not the public entry points (if these are imported also then there is a linker error about state_migration begin defined twice because it is also defined in the test contract).

…te can be imported into a test contract without importing all the public exports in lib.rs that are hidden by the contract compile-time feature

birchmd · 2021-05-20T18:39:42Z

src/sdk.rs

@@ -4,6 +4,7 @@ use borsh::{BorshDeserialize, BorshSerialize};

 const READ_STORAGE_REGISTER_ID: u64 = 0;
 const INPUT_REGISTER_ID: u64 = 0;
+const GAS_FOR_STATE_MIGRATION: u64 = 100_000_000_000_000;


I'm not 100% sure on the value for this constant. 100 Tgas seems like it should be enough any sort of state migration we want to do, but we could push it higher if we think this might not be enough.

We could also leave it as is and update the constant only if we actually write a migration that requires more gas (I assume we'll test all upgrades before we do them, so we'll know if we need more gas).

I think it makes sense to set it as a max value (300 Tgas). As this is not user's function I don't think there's any problem that we will be required to pay some more gas for the state migration (which hopefully won't happen too frequently). So I don't think we need to put any max value constrain here.

Well, we can't set it to 300 Tgas because that the way promises work is they take gas from the original transaction. So if the original deploy_upgrade transaction was given 300 Tgas then we could only give the promise it creates an amount of gas equal to 300 minus whatever amount is used in creating the promise.

We could set it higher, e.g. I think 250 Tgas would work, but I don't know if there is much value in figuring out exactly how high it can be when we don't know that 100 isn't enough.

Oh, yeah sorry I forgot that we are talking about the inner promise. Then sure 250 Tgas could be an option.

Done in 208de2d

sept-en

LGTM!

joshuajbouw

Just a small nitpick about the authors, but generally, this is fine. Good work.

etc/state-migration-test/Cargo.toml

src/lib.rs

artob

LGTM 👍

* Document `aurora encode-address` usage. * Cache cargo artifacts between CI runs. (#92) * Address comments from audit. (#86) * Validate register length in `read_input_arr20()` * Only read register length in `Engine::get_code_size` * Add `read_input_borsh()` * Ensure `method.args.len() == args_decoded.len()` * Ensure register size is 8 in `read_u64` * Use constant to specify the register ID used in `read_input()` * Reduce size of `cargo cache` in CI. (#95) * Define a `Wei` newtype for balances. (#96) * Fix evm-bully builds after recent refactoring. (#100) * Refactor `Engine::get_state` to return a `Result`. (#99) * Ensure that `Cargo.lock` in the repo is valid. (#101) * Remove unneeded nightly feature. (#102) * Dispatch precompiles on the full address. (#107) * Support state migration on upgrade. (#103) * Remove resolved TODOs * Fix state migration test * Conditional compilation minor improvements. Co-authored-by: Arto Bendiken <arto@aurora.dev> Co-authored-by: Michael Birch <michael@near.org> Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

* Base precompile code between connectors (#73) * Base precompile code between connectors * Handle errors and validate input * Use proper result * Document `aurora encode-address` usage. * Cache cargo artifacts between CI runs. (#92) * Address comments from audit. (#86) * Validate register length in `read_input_arr20()` * Only read register length in `Engine::get_code_size` * Add `read_input_borsh()` * Ensure `method.args.len() == args_decoded.len()` * Ensure register size is 8 in `read_u64` * Use constant to specify the register ID used in `read_input()` * Reduce size of `cargo cache` in CI. (#95) * Define a `Wei` newtype for balances. (#96) * Fix evm-bully builds after recent refactoring. (#100) * Refactor `Engine::get_state` to return a `Result`. (#99) * Ensure that `Cargo.lock` in the repo is valid. (#101) * Remove unneeded nightly feature. (#102) * Implement BC generational storage. * fix address input * remove note * put key on the end of the storage key * remove pub from methods * Dispatch precompiles on the full address. (#107) * Support state migration on upgrade. (#103) * Implement the ETH connector. (#59) * Move when to call `set_generation` * Fix arg Co-authored-by: Marcelo Fornet <mfornet94@gmail.com> Co-authored-by: Arto Bendiken <arto@aurora.dev> Co-authored-by: Michael Birch <michael@near.org> Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com> Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru>

* Add tests for state check after selfdestruct * Aurora runner tracks storage usage to avoid underflow when storage is released in future transactions (#85) * Implement generational storage (#87) * Base precompile code between connectors (#73) * Base precompile code between connectors * Handle errors and validate input * Use proper result * Document `aurora encode-address` usage. * Cache cargo artifacts between CI runs. (#92) * Address comments from audit. (#86) * Validate register length in `read_input_arr20()` * Only read register length in `Engine::get_code_size` * Add `read_input_borsh()` * Ensure `method.args.len() == args_decoded.len()` * Ensure register size is 8 in `read_u64` * Use constant to specify the register ID used in `read_input()` * Reduce size of `cargo cache` in CI. (#95) * Define a `Wei` newtype for balances. (#96) * Fix evm-bully builds after recent refactoring. (#100) * Refactor `Engine::get_state` to return a `Result`. (#99) * Ensure that `Cargo.lock` in the repo is valid. (#101) * Remove unneeded nightly feature. (#102) * Implement BC generational storage. * fix address input * remove note * put key on the end of the storage key * remove pub from methods * Dispatch precompiles on the full address. (#107) * Support state migration on upgrade. (#103) * Implement the ETH connector. (#59) * Move when to call `set_generation` * Fix arg Co-authored-by: Marcelo Fornet <mfornet94@gmail.com> Co-authored-by: Arto Bendiken <arto@aurora.dev> Co-authored-by: Michael Birch <michael@near.org> Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com> Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru> * Fix layout of the key * Fix all tests (don't wipe the storage all the time) * Use correct generation in writing storage * Remove unnecessary references Co-authored-by: Michael Birch <michael@near.org> Co-authored-by: Joshua J. Bouw <dev@joshuajbouw.com> Co-authored-by: Arto Bendiken <arto@aurora.dev> Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com> Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru> Co-authored-by: Michael Birch <michael.birch@aurora.dev>

birchmd added 4 commits May 20, 2021 18:24

Add state_migration function and call it during upgrade

eaeafe5

Use a separate compile-time feature for engine code so that EngineSta…

c18e0d9

…te can be imported into a test contract without importing all the public exports in lib.rs that are hidden by the contract compile-time feature

Add contract to use for testing state migrations

841c92e

Add test for state migration

db606ef

birchmd requested review from artob, sept-en and joshuajbouw May 20, 2021 18:36

birchmd commented May 20, 2021

View reviewed changes

birchmd added the C-enhancement Category: New feature or request label May 20, 2021

Merge branch 'develop' into state-migration

9f5e7c4

sept-en approved these changes May 21, 2021

View reviewed changes

birchmd mentioned this pull request May 21, 2021

Remedy action items from #74 #76

Closed

joshuajbouw approved these changes May 24, 2021

View reviewed changes

etc/state-migration-test/Cargo.toml Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

birchmd added 2 commits May 25, 2021 13:54

Increase GAS_FOR_STATE_MIGRATION to 256 Tgas

208de2d

Move some code

d14fe51

artob self-assigned this May 28, 2021

artob approved these changes May 28, 2021

View reviewed changes

artob merged commit b09b530 into develop May 28, 2021

artob deleted the state-migration branch May 28, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support state migration on upgrade #103

Support state migration on upgrade #103

birchmd commented May 20, 2021

birchmd May 20, 2021

sept-en May 20, 2021

birchmd May 20, 2021

sept-en May 20, 2021

birchmd May 25, 2021

sept-en left a comment

joshuajbouw left a comment

artob left a comment

Support state migration on upgrade #103

Support state migration on upgrade #103

Conversation

birchmd commented May 20, 2021

birchmd May 20, 2021

Choose a reason for hiding this comment

sept-en May 20, 2021

Choose a reason for hiding this comment

birchmd May 20, 2021

Choose a reason for hiding this comment

sept-en May 20, 2021

Choose a reason for hiding this comment

birchmd May 25, 2021

Choose a reason for hiding this comment

sept-en left a comment

Choose a reason for hiding this comment

joshuajbouw left a comment

Choose a reason for hiding this comment

artob left a comment

Choose a reason for hiding this comment