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

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

Merged
merged 3 commits into from
Oct 23, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
defeba5
Fix lookback for worker key when computing ticket
Stebalien Oct 21, 2020
b056134
Sign blocks with the worker key from the correct lookback epoch
Stebalien Oct 21, 2020
59cfabf
don't resolve the worker key twice
Stebalien Oct 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion chain/gen/mining.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,17 @@ func MinerCreateBlock(ctx context.Context, sm *stmgr.StateManager, w api.WalletA
return nil, xerrors.Errorf("failed to load tipset state: %w", err)
}

worker, err := stmgr.GetMinerWorkerRaw(ctx, sm, st, bt.Miner)
lbts, err := stmgr.GetLookbackTipSetForRound(ctx, sm, pts, bt.Epoch)
if err != nil {
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.

if err != nil {
return nil, err
}

worker, err := stmgr.GetMinerWorkerRaw(ctx, sm, lbst, bt.Miner)
if err != nil {
return nil, xerrors.Errorf("failed to get miner worker: %w", err)
}
Expand Down
18 changes: 7 additions & 11 deletions miner/miner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ func (m *Miner) mineOne(ctx context.Context, base *MiningBase) (*types.BlockMsg,
rbase = bvals[len(bvals)-1]
}

ticket, err := m.computeTicket(ctx, &rbase, base)
ticket, err := m.computeTicket(ctx, &rbase, base, mbi)
if err != nil {
return nil, xerrors.Errorf("scratching ticket failed: %w", err)
}
Expand Down Expand Up @@ -456,16 +456,7 @@ func (m *Miner) mineOne(ctx context.Context, base *MiningBase) (*types.BlockMsg,
return b, nil
}

func (m *Miner) computeTicket(ctx context.Context, brand *types.BeaconEntry, base *MiningBase) (*types.Ticket, error) {
mi, err := m.api.StateMinerInfo(ctx, m.address, types.EmptyTSK)
if err != nil {
return nil, err
}
worker, err := m.api.StateAccountKey(ctx, mi.Worker, types.EmptyTSK)
if err != nil {
return nil, err
}

func (m *Miner) computeTicket(ctx context.Context, brand *types.BeaconEntry, base *MiningBase, mbi *api.MiningBaseInfo) (*types.Ticket, error) {
buf := new(bytes.Buffer)
if err := m.address.MarshalCBOR(buf); err != nil {
return nil, xerrors.Errorf("failed to marshal address to cbor: %w", err)
Expand All @@ -476,6 +467,11 @@ func (m *Miner) computeTicket(ctx context.Context, brand *types.BeaconEntry, bas
buf.Write(base.TipSet.MinTicket().VRFProof)
}

worker, err := m.api.StateAccountKey(ctx, mbi.WorkerKey, types.EmptyTSK)
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

input, err := store.DrawRandomness(brand.Data, crypto.DomainSeparationTag_TicketProduction, round-build.TicketRandomnessLookback, buf.Bytes())
if err != nil {
return nil, err
Expand Down