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

Run fork function after cron for null block safety #4114

Merged
merged 2 commits into from
Oct 3, 2020

Conversation

ZenGround0
Copy link
Contributor

Actors state migrations will take place within a fork function called from handleStateForks. In general migrations need the epoch the transition is run at to work correctly. We are assuming the epoch passed to any migration function will be the same epoch passed to handleStateForks, i.

i is the fork epoch. In the common case of no null blocks it is the "previous epoch" in the sense that epoch i's cron has already run as part of processing the parent tipset and state.

If there are null blocks then for i > parentEpoch epoch i's cron will not have run before the fork function because of the current ordering of fork function before cron. We can't distinguish these two cases from within the migration function so the epoch we are passed in won't be well defined with respect to our state.

The fix is to run cron before the fork function.

@magik6k
Copy link
Contributor

magik6k commented Sep 30, 2020

(TODO: Make sure this doesn't break consensus on the already performed forks (should be fairly easy with just lotus state --tipset @[epoch] compute-state))

@ZenGround0
Copy link
Contributor Author

@magik6k this does not break consensus at breeze, smoke and ignition epochs

Before change

$ ./lotus version
Daemon:  0.8.1+git.efc1b24f.dirty+api0.16.0
Local: lotus version 0.8.1+git.efc1b24f.dirty
$ ./lotus state --tipset=@41281 compute-state
computed state cid:  bafy2bzaceajkwiwjw5hdwgp73u7f4nl6ayihxaxashcmtaqgxamww6xvufhpi
$ ./lotus state --tipset=@51001 compute-state
computed state cid:  bafy2bzaceaolfm5uaald2skcsfzamspm357hsaqyaq7mmlwlpl4oup5svaucu
$ ./lotus state --tipset=@94001 compute-state
computed state cid:  bafy2bzacecyce7bcu6yazuwwa5wn6vo5ksaztv73idmiw4w5rrpzi6shk4hu4

After change

$ ./lotus version
Daemon:  0.8.0+git.8091bb31.dirty+api0.16.0
Local: lotus version 0.8.0+git.8091bb31.dirty
$ ./lotus state --tipset=@41281 compute-state
computed state cid:  bafy2bzaceajkwiwjw5hdwgp73u7f4nl6ayihxaxashcmtaqgxamww6xvufhpi
$ ./lotus state --tipset=@51001 compute-state
computed state cid:  bafy2bzaceaolfm5uaald2skcsfzamspm357hsaqyaq7mmlwlpl4oup5svaucu
$ ./lotus state --tipset=@94001 compute-state
computed state cid:  bafy2bzacecyce7bcu6yazuwwa5wn6vo5ksaztv73idmiw4w5rrpzi6shk4hu4

Double checked that these match the next header to confirm compute-state is including cron/fork functions. I also checked 41820, 51000 and 94000 in case I was off by one and found matches

@magik6k magik6k requested review from arajasek and Stebalien October 2, 2020 19:52
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

This makes sense.

if i > parentEpoch {
// run cron for null rounds if any
if err := runCron(); err != nil {
return cid.Cid{}, cid.Cid{}, err
Copy link
Member

Choose a reason for hiding this comment

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

nit cid.Undef.

@ZenGround0
Copy link
Contributor Author

In case its not clear I don't have merge permissions so someone else should push the button.

@arajasek arajasek merged commit 4631cee into master Oct 3, 2020
@arajasek arajasek deleted the fix/fork-after-cron branch October 3, 2020 03:05
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.

4 participants