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

Use the correct lookback for the worker key when creating blocks #4539

Merged
merged 3 commits into from
Oct 23, 2020

Conversation

Stebalien
Copy link
Member

Previously, we'd use the current worker key when signing blocks and computing the VRF. Now, we use the worker key from the correct lookback epoch. This now matches the behavior on the validation side.

Required for (and tested by) #4513.

We need to compute the ticket based on our worker key from the lookback epoch,
not the current epoch.
return nil, xerrors.Errorf("getting lookback miner actor state: %w", err)
}

lbst, _, err := sm.TipSetState(ctx, lbts)
Copy link
Contributor

Choose a reason for hiding this comment

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

This computes the state, we could just load it (we may also be doing this in GetMiningBase)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I should be walking back one fewer blocks and looking at the parent state?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I can do this reliably given null blocks. Currently, given:

  1. TS_A
  2. TS_B (parent: state at height 1)
  3. NULL <-- lookback height
  4. NULL
  5. TS_C (parent: state at height 4)

Currently, given a lookback height of 3, we'll use the state computed at height 2 (from TS_B). However, we don't actually record this state CID anywhere on chain. The next recorded state CID is for the state computed at height 4 (after processing two null rounds).

So, I could try to do my best to try to use parent tipset states from the chain, but that's going to be complicated.

Instead, what if we just improved our caching? We already maintain a tipset state cache, we could populate this cache with all tipsets between now and finality. (in a different PR)

Copy link
Member

Choose a reason for hiding this comment

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

just do GetTipsetByHeight for the lookback height, and ensure prev is false, then take that tipsets epoch, add one, and call GetTipsetByHeight again on that with prev also equal to false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the null blocks, I'm pretty sure you'd end up with the exact same result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but I need the state computed by executing TS_B. If I lookup lookback+1 and take the parent state, I'll get the parent state of TS_B, not the state resulting from the evaluation of TS_B. The fundamental issue here is that the state root produced by executing TS_B isn't recorded anywhere on chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

(alternatively, we could have looked at the parent state of the lookback tipset, but that needs a network upgrade)

Copy link
Member

Choose a reason for hiding this comment

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

What? if a.Parents() == b then TipSetState(b) == a.ParentStateRoot.

Copy link
Member

Choose a reason for hiding this comment

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

The fundamental issue here is that the state root produced by executing TS_B isn't recorded anywhere on chain.

Yes, its TS_C.ParentStateRoot

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently I need to pay more attention when reading code. @whyrusleeping is correct.

miner/miner.go Outdated Show resolved Hide resolved
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Change LGTM, can optimize now, or later.

@Stebalien Stebalien merged commit 2cf770c into master Oct 23, 2020
@Stebalien Stebalien deleted the steb/fix-worker-key-lookback branch October 23, 2020 04:10
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.

3 participants