Skip to content
This repository was archived by the owner on Jan 27, 2025. It is now read-only.

Conversation

@simsekgokhan
Copy link
Contributor

@simsekgokhan simsekgokhan commented Dec 3, 2022

This PR covers two related/dependent features seen below as Step 1, 2 and fixes for some Move errors (see commits).

Steps:

  1. v5 JSON export (new feature)
    cargo r -p ol-genesis-tools -- --recover /opt/rec.json --snapshot-path /opt/state_ver*
    state_ver: https://github.com/OLSF/epoch-archive/tree/main/359/state_ver_76353076.a0ff
    Resulting rec.json: https://drive.google.com/file/d/1jaumaKdaGjWExNdUVqNZDvIAQPmppt19/view

  2. Genesis v6 from JSON v5 (new feature)
    Create genesis blob using v6 code from JSON v5 export:
    cargo r -p ol-genesis-tools -- --fork --recovery-json-path /opt/rec.json --output-path /opt/genesis_from_recovery.blob
    Resulting genesis_from_recovery.blob: https://drive.google.com/file/d/1f2gfISy5AecZIXGQEOgP4yQm3ArpYRO1/view?usp=sharing

  3. Verify the validity of genesis blob by starting a v6 node in test mode with it:
    cargo r -p diem-node -- --test --genesis-modules /opt/genesis_from_recovery.blob
    Validator logs: https://drive.google.com/file/d/1DCuKe4hY0VwzYS3nMsQie4o5uyt25coo/view?usp=sharing

@simsekgokhan simsekgokhan marked this pull request as ready for review December 3, 2022 11:31
@corynthian
Copy link

corynthian commented Jan 12, 2023

Two questions spring to mind on this set of changes:

  1. Why is compiled bytecode included in the commit? Is this due to wanting to have binary compatibility across the board and if so, would it not be better instead to collate the files and produce a hash which can be verified, so that the codebase doesn't end up with binary artifacts?
  2. It is not immediately clear to me where the tests are for the move contracts or rust code in this case. How are you checking that the migration from v5 to v6 behaved as expected (e.g. checking for equality between the accounts from v5 to v6)?

@0o-de-lally
Copy link
Collaborator

0o-de-lally commented Jan 25, 2023

Two questions spring to mind on this set of changes:

  1. Why is compiled bytecode included in the commit? Is this due to wanting to have binary compatibility across the board and if so, would it not be better instead to collate the files and produce a hash which can be verified, so that the codebase doesn't end up with binary artifacts?

This is legacy from FB. It's a real PITA but many of the tests and builds depend on finding these files. Not something that can be easily fixed, we've tried a number of things.

  1. It is not immediately clear to me where the tests are for the move contracts or rust code in this case. How are you checking that the migration from v5 to v6 behaved as expected (e.g. checking for equality between the accounts from v5 to v6)?

@simsekgokhan is good about testing, so I'm guessing it's just not documented. Otherwise we need to ticket the e2e tests for this.

Edit: I see the steps to reproduce are here in the PR text. We likely just need to create some fixtures out of this and add the test.

@0o-de-lally
Copy link
Collaborator

0o-de-lally commented Jan 25, 2023

This PR looks correct to me. Although we are missing e2e tests for this feature. Ordinarily we wouldn't merge this, but since we need to do cleaning up on the v6 branches I'll approve for merge. IBut I see the steps to reproduce in the PR text. We need to clean up branches so, it's approved assuming we add the task for the e2e test.w

Copy link
Collaborator

@0o-de-lally 0o-de-lally left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK if we add tests

@sirouk
Copy link

sirouk commented Jan 25, 2023

LGTM

@sirouk sirouk merged commit 3163e80 into 0LNetworkCommunity:v6 Jan 25, 2023
@simsekgokhan simsekgokhan deleted the v6 branch March 14, 2023 09:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants