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

miner: retrieve mining state from live database #25139

Merged
merged 2 commits into from
Jun 22, 2022
Merged

miner: retrieve mining state from live database #25139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea665cb
miner: retrieve mining state from live database
rjl493456442 Jun 21, 2022
7216338
eth/catalyst: ignore stale fcu events from cl
rjl493456442 Jun 21, 2022
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
18 changes: 11 additions & 7 deletions eth/catalyst/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,26 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa
return beacon.ForkChoiceResponse{PayloadStatus: beacon.INVALID_TERMINAL_BLOCK, PayloadID: nil}, nil
}
}

valid := func(id *beacon.PayloadID) beacon.ForkChoiceResponse {
return beacon.ForkChoiceResponse{
PayloadStatus: beacon.PayloadStatusV1{Status: beacon.VALID, LatestValidHash: &update.HeadBlockHash},
PayloadID: id,
}
}
if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) != update.HeadBlockHash {
// Block is not canonical, set head.
if latestValid, err := api.eth.BlockChain().SetCanonical(block); err != nil {
return beacon.ForkChoiceResponse{PayloadStatus: beacon.PayloadStatusV1{Status: beacon.INVALID, LatestValidHash: &latestValid}}, err
}
} else if api.eth.BlockChain().CurrentBlock().Hash() == update.HeadBlockHash {
// If the specified head matches with our local head, do nothing and keep
// generating the payload. It's a special corner case that a few slots are
// missing and we are requested to generate the payload in slot.
} else {
// If the head block is already in our canonical chain, the beacon client is
// probably resyncing. Ignore the update.
log.Info("Ignoring beacon update to old head", "number", block.NumberU64(), "hash", update.HeadBlockHash, "age", common.PrettyAge(time.Unix(int64(block.Time()), 0)), "have", api.eth.BlockChain().CurrentBlock().NumberU64())
return valid(nil), nil
Copy link
Member

Choose a reason for hiding this comment

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

This may be totally wrong, I haven't spent as much time with this code as any of you have, but it seems to me that this is semantically a fine request for the CL to make. The CL is just setting the head to a block we've previously executed. I believe the fork choice rules today would not allow this to happen, but it could be possible in the future as the fork choice rules change.

Maybe an alternative would be to only ignore requests which set a head that before the best known finalization point?

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Jun 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that was what we discussed today at stabby, I think this behavior is the correct one.

Regardless I would like to make no decisions based on the finalization point, so that geth will work fine in periods of non-finalization too

Copy link
Member

Choose a reason for hiding this comment

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

re: making no decisions based on finalization -- I don't think my comment would affect how geth behaves in the case of non-finalization? It would just stop the CL from reorging the chain to before a previously finalized head.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, right now I only define the behavior when the provided head is exactly matched with local head.
Apart from this, there are two main scenarios we need to pay attention to:

  • CL reorgs chain to ancestor
  • CL keeps sending us FCUs while syncing

For the former one, I am not sure if it's possible, perhaps @djrtwo can answer it
For the latter one, I think it's totally fine to ignore the event

}
api.eth.SetSynced()

Expand Down Expand Up @@ -183,12 +193,6 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa
return beacon.STATUS_INVALID, beacon.InvalidForkChoiceState.With(errors.New("safe block not in canonical chain"))
}
}
valid := func(id *beacon.PayloadID) beacon.ForkChoiceResponse {
return beacon.ForkChoiceResponse{
PayloadStatus: beacon.PayloadStatusV1{Status: beacon.VALID, LatestValidHash: &update.HeadBlockHash},
PayloadID: id,
}
}
// If payload generation was requested, create a new block to be potentially
// sealed by the beacon client. The payload will be requested later, and we
// might replace it arbitrarily many times in between.
Expand Down
1 change: 0 additions & 1 deletion miner/miner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import (
type Backend interface {
BlockChain() *core.BlockChain
TxPool() *core.TxPool
StateAtBlock(block *types.Block, reexec uint64, base *state.StateDB, checkLive bool, preferDisk bool) (statedb *state.StateDB, err error)
}

// Config is the configuration parameters of mining.
Expand Down
10 changes: 0 additions & 10 deletions miner/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,16 +756,6 @@ func (w *worker) makeEnv(parent *types.Block, header *types.Header, coinbase com
// Retrieve the parent state to execute on top and start a prefetcher for
// the miner to speed block sealing up a bit.
state, err := w.chain.StateAt(parent.Root())
if err != nil {
// Note since the sealing block can be created upon the arbitrary parent
// block, but the state of parent block may already be pruned, so the necessary
// state recovery is needed here in the future.
//
// The maximum acceptable reorg depth can be limited by the finalised block
// somehow. TODO(rjl493456442) fix the hard-coded number here later.
state, err = w.eth.StateAtBlock(parent, 1024, nil, false, false)
log.Warn("Recovered mining state", "root", parent.Root(), "err", err)
}
if err != nil {
return nil, err
}
Expand Down