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.

Sign up for GitHub

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

Jump to bottom

Implement network version 12 state migration / actors v4 migration #1101

Merged
merged 27 commits into from
Jun 21, 2021
Merged

Implement network version 12 state migration / actors v4 migration #1101

merged 27 commits into from
Jun 21, 2021

Conversation

creativcoder
Copy link
Contributor

@creativcoder creativcoder commented May 17, 2021 •
edited
Loading

Summary of changes
Changes introduced in this pull request:

  • Implements state migration scaffold in vm/state_migration crate.
  • Implements network version 12 / actors v4 migration.
  • Update: nv12 state migration takes around 13-15 secs.

Closes #1039

Other information and links

@creativcoder creativcoder requested a review from a user May 17, 2021 10:34
@creativcoder creativcoder changed the title Creativcoder/state migration Implement actors v4 migration May 17, 2021
@creativcoder creativcoder changed the title Implement actors v4 migration Implement state migration / actors v4 migration May 17, 2021
cryptoquick
cryptoquick reviewed May 17, 2021
View reviewed changes
Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Very cool to see more state migration work! Just one small comment, I can't comment on the code with what I know.

vm/interpreter/src/vm.rs Show resolved Hide resolved
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch 2 times, most recently from 8427ee1 to 0d973e9 Compare May 17, 2021 13:16
ec2
ec2 reviewed May 17, 2021
View reviewed changes
vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
vm/interpreter/src/vm.rs Show resolved Hide resolved
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch from ed470cc to afa5fed Compare May 17, 2021 15:30
ec2
ec2 approved these changes May 17, 2021
View reviewed changes
vm/state_migration/Cargo.toml Outdated Show resolved Hide resolved
vm/state_migration/Cargo.toml Outdated Show resolved Hide resolved
vm/state_migration/src/lib.rs Show resolved Hide resolved
vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
ghost
ghost suggested changes May 17, 2021
View reviewed changes
vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch from b000571 to 9497e47 Compare May 21, 2021 10:00
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch from 9497e47 to 378e23b Compare June 3, 2021 16:47
@creativcoder creativcoder changed the title Implement state migration / actors v4 migration Implement network version 12 state migration / actors v4 migration Jun 3, 2021
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch from 860e140 to 7c8dcb1 Compare June 3, 2021 18:20
ec2
ec2 reviewed Jun 4, 2021
View reviewed changes
ipld/blockstore/src/buffered.rs Outdated Show resolved Hide resolved
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch 6 times, most recently from ee0f7fd to 434ae51 Compare June 7, 2021 19:47
ec2
ec2 reviewed Jun 8, 2021
View reviewed changes
vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
vm/state_migration/src/nv12/miner.rs Show resolved Hide resolved
vm/state_migration/Cargo.toml Outdated Show resolved Hide resolved
vm/state_migration/src/lib.rs
};

let job_output = job.run(store_clone, prior_epoch).unwrap_or_else(|e| {
panic!(
Copy link
Member

Choose a reason for hiding this comment

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

is it okay to panic here?... i dont expect there be a good way to handle a state migration failure, but i wonder if we can do this more cleanly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, had thought about it, I could have created a global error variable outside and assign any Errs and could bail early from the spawned thread. But i guess it is safe to panic here as it's a child thread and not proceed with any further migration state change. Totally open if we have better error handling ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to ask whether is it safe for the migration to continue if any of the spawned thread fails, or we should discard the whole migration. If it's the later case, then yeah we can bail early with an Err. Thoughts?

vm/interpreter/src/vm.rs Outdated Show resolved Hide resolved
ec2
ec2 reviewed Jun 10, 2021
View reviewed changes
vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
ec2
ec2 requested changes Jun 10, 2021
View reviewed changes
vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
cryptoquick
cryptoquick suggested changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but I have just one suggestion.

vm/state_migration/src/lib.rs Outdated Show resolved Hide resolved
@creativcoder
simple root migration impl
a36027a
@creativcoder
fix borrow issues with blockstore
1be8b69
@creativcoder
/s/v2/v3 and /s/v3/v4
cfbe800
@creativcoder
migration works, but incorrect
b078bdc
@creativcoder
few nits
e9490cf
@creativcoder
try for_each_concurrent and a few refactors
4e9cd60
@creativcoder
revert to next.await and refactors
a634d89
@creativcoder
migration log
f3b0131
@creativcoder
address code review comments
d7d4506
@creativcoder
Box<dyn Error> -> String
3181b3e
@creativcoder
make blockstore Send + Sync
2d13662
@creativcoder
concurrent state migration
0b6e59b
@creativcoder
chore: cargo fmt
cbc561d
@creativcoder
replace Mutex<Map<_>> with DashMap
b2f7961
@creativcoder
move common migration code out of miner migrator
0412e3e
@creativcoder
clippy fixes
ace6d63
@creativcoder
cleanup unused
7f01cf1
@creativcoder
cargo fmt
fb28f60
@creativcoder
more clippy fixes
bfb9e20
@creativcoder
relocate a few comments
ea55641
@creativcoder
Address code review changes
007311e
@creativcoder
code review: rename arc_store to store
dc83e1b
@creativcoder
more compact in_state initialization in miner.rs
5082c6d
@creativcoder
migrate_state should return error when prev state not equal new state…
934d7db 
… post migration
@creativcoder
make clippy happy
e0e30c6
@creativcoder creativcoder force-pushed the creativcoder/state-migration branch from c9544da to e0e30c6 Compare June 21, 2021 17:46
ghost
ghost approved these changes Jun 21, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Great contribution, LGTM

@creativcoder creativcoder merged commit 02791b9 into main Jun 21, 2021
@creativcoder creativcoder deleted the creativcoder/state-migration branch June 21, 2021 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ec2 ec2 ec2 approved these changes

@cryptoquick cryptoquick cryptoquick approved these changes

@connormullett connormullett Awaiting requested review from connormullett

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement state migration / network version 12
3 participants
@creativcoder @cryptoquick @ec2