Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: migrations: function for migrating builtin actors with only code changes #8815

Merged
merged 8 commits into from
Jun 10, 2022

Conversation

geoff-vball
Copy link
Contributor

@geoff-vball geoff-vball commented Jun 7, 2022

Related Issues

Closes #8780

Proposed Changes

Adds LiteMigration() to upgrades.go which should allow for easy upgrades if actors code needs to change but state does not. Example provided above the function to perform all the migration duties. Check actors_version_checklist.md for the rest of the steps.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@geoff-vball geoff-vball requested a review from a team as a code owner June 7, 2022 03:09
@geoff-vball geoff-vball changed the title create function for migrating builtin actors with only code changes feat: migrations: function for migrating builtin actors with only code changes Jun 7, 2022
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch 2 times, most recently from b4577fb to 24a2f4c Compare June 7, 2022 03:19
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch from 24a2f4c to b28cb2a Compare June 7, 2022 03:23
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Testy test, pls.

chain/consensus/filcns/upgrades.go Show resolved Hide resolved
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch 8 times, most recently from 6e1a894 to fdb3547 Compare June 9, 2022 17:54
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch 6 times, most recently from 87c7fd4 to 526acd7 Compare June 10, 2022 14:56
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch from 526acd7 to 3c4792d Compare June 10, 2022 15:35
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Generally looks good, but some things need to change.

Can you also please update actors_version_checklist to reflect this new flow under the Migration instructions.

@@ -38,6 +39,8 @@ import (
type ChainIO interface {
ChainReadObj(context.Context, cid.Cid) ([]byte, error)
ChainHasObj(context.Context, cid.Cid) (bool, error)
ChainPutObj(context.Context, blocks.Block) error
Copy link
Contributor

Choose a reason for hiding this comment

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

@magik6k We had to do this just so we could use the itest framework for a migration test, which is not really what it's intended for, but is better than any of the existing frameworks we have (I think). Questions for you:

  • Do you absolutely hate it and refuse to merge this PR?
  • Is there a way to not expose this method over the JSON-RPC, and have it just for things like this test?
  • Do you have a better way to write this test?

api/api_full.go Outdated Show resolved Hide resolved
gateway/node.go Outdated Show resolved Hide resolved
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
chain/stmgr/actors.go Outdated Show resolved Hide resolved
chain/stmgr/actors.go Outdated Show resolved Hide resolved
itests/lite_migration_test.go Show resolved Hide resolved
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch from 09cb375 to 801c670 Compare June 10, 2022 18:09
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch from 10638a3 to e84a778 Compare June 10, 2022 18:59
@geoff-vball geoff-vball force-pushed the gstuart/lite-migration branch from e84a778 to 3f712c3 Compare June 10, 2022 19:07
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Landing lest it rot, but we do need Magik answer re: #8815 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants