feat(primitives): Add versioning to Account #4089
Conversation
- Adding migration script to run on dump - Adding test to make sure dump is parsable
EgorKulikov
requested review from
ailisp,
evgenykuzyakov,
frol,
MaksymZavershynskyi and
SkidanovAlex
as code owners
| use near_chain_configs::Genesis; | ||
|
|
||
| #[test] | ||
| fn test_load_genesis() { |
There was a problem hiding this comment.
wouldn't this test fail if we enable protocol_feature_add_account_versions?
There was a problem hiding this comment.
Why will it? And I think it should've ran as part of CI, no?
There was a problem hiding this comment.
I misread and thought we are using the same file in both cases :)
There was a problem hiding this comment.
Should the node with the feature enabled be able to parse the existing genesis file? (it is not the case now as far as I can tell)
| @@ -0,0 +1,23 @@ | |||
| """ | |||
There was a problem hiding this comment.
@chefsale @mhalambek do you want this script to be part of nearcore or part of near-ops? Also @EgorKulikov the naming of this file is a bit confusing -- the number is used to mean the new protocol version. We can either drop it or change it to 107
There was a problem hiding this comment.
Not really sure what is the use case of this script. I see its a one time migration, but not sure what else would we use it in near-ops. If its just a one off migration it can stay in nearcore
|
@chefsale please make sure that you run the migration script provided in this file when doing hard fork on betanet |
| @@ -0,0 +1,23 @@ | |||
| """ | |||
There was a problem hiding this comment.
@bowenwang1996 does this mean that everyone (every RPC node, every Bisontrails instance, every indexer node, etc) will be required to run this script or re-download the genesis file and records once we enable this feature? It feels like quite a cumbersome process. It seems that I had the wrong impression that the genesis file never changes. Do nodes need the file only on their first start and it is never used/loaded beyond that point?
There was a problem hiding this comment.
This only happens when we do hard fork. Also unless it is mainnet, there is no reason for other people to run the migration themselves and they can wait for us to provide the new genesis. If it is mainnet, then first of all we do not expect to do hardforks there and also even if we do it requires much more careful coordination
core/primitives-core/src/account.rs
Outdated
| #[cfg(feature = "protocol_feature_add_account_versions")] | ||
| impl BorshDeserialize for Account { | ||
| fn deserialize(buf: &mut &[u8]) -> Result<Self, Error> { | ||
| if buf.len() == std::mem::size_of::<AccountV1>() { | ||
| // This should only ever happen if we have pre-transition account serialized in state | ||
| // See test_account_size | ||
| Ok(Account::AccountV1(<AccountV1 as BorshDeserialize>::deserialize(buf)?)) | ||
| } else { | ||
| #[derive(BorshDeserialize)] | ||
| enum DeserializableAccount { | ||
| AccountV1(AccountV1), | ||
| }; | ||
| let deserialized_account = DeserializableAccount::deserialize(buf)?; | ||
| match deserialized_account { | ||
| DeserializableAccount::AccountV1(account) => Ok(Account::AccountV1(account)), | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It worth moving it up to line 46, so you don't forget to add a field to DeserializableAccount.
There was a problem hiding this comment.
I guess adding new version will lead to changing new, and that will fail serialization/deserialization tests
Renamed json for test
Just like we have different versions of Block or BlockHeader types we'll now have same for accounts. As accounts are saved in state we have to be able to deserialize old Account structure as well
Migration
Script scripts/migrations/107-add-account-versioning.py added to migrate genesis file for betanet
Test plan
Tests added/updated to check serialization/deserialization, including from older version. Also for this to work new version should always serialize into longer sequence of bytes, such check added as well
Tests that current genesis is deserializable