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

eth, miner: remove duplicated code #23256

Merged
merged 10 commits into from Jan 24, 2022
Merged

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Jul 23, 2021

This PR removes the duplicated code in the catalyst package, reuse the miner package code instead.

Highlight the critical changes in this PR:

  • Offer one more API in the miner package called GetSealingBlock which will assemble and return the sealing block based on the given parameters.
  • Sealing environment is abstracted out, all the functions will receive the sealing environment for further operations
  • Uncle blocks management is changed for sake of simplification

@rjl493456442 rjl493456442 requested a review from gballet as a code owner Jul 23, 2021
@rjl493456442 rjl493456442 changed the title [WIP] eth, miner: remove duplicated code eth, miner: remove duplicated code Jul 26, 2021
@holiman
Copy link
Contributor

holiman commented Jul 27, 2021

This touches some core logic in the miner, I think we should have a review call to discuss this one.

Copy link
Member

@gballet gballet left a comment

I agree with Martin, it seems correct (and I like that you broke some functions down into some with an explicit title) even though we should discuss this during a call. I have a couple nitpicks until then.

miner/worker_test.go Outdated Show resolved Hide resolved
miner/worker_test.go Outdated Show resolved Hide resolved
miner/worker.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member Author

rjl493456442 commented Jul 29, 2021

Yes @holiman and @gballet . I won't want to merge this PR now, definitely after the fork. And we can schedule a review call for it, especially for some tiny details in the miner refactoring.

miner/worker.go Outdated Show resolved Hide resolved
@badbitses
Copy link

badbitses commented Aug 22, 2021

I want to help

@badbitses
Copy link

badbitses commented Aug 22, 2021

Who left and why

miner/worker.go Outdated Show resolved Hide resolved
@jwasinger
Copy link
Member

jwasinger commented Aug 22, 2021

Who left and why

Hey @badbitses . This PR is mostly complete. If you are interested in contributing, I would advise to take a look through the open issues and see what piques your interest.

miner/worker.go Outdated Show resolved Hide resolved
miner/worker.go Outdated
func (w *worker) getSealingBlock(parent common.Hash, timestamp uint64, coinbase common.Address) (*types.Block, error) {
req := &getWorkReq{
params: &generateParams{
timestamp: timestamp,
forceTime: true,
parentHash: parent,
coinbase: coinbase,
noUncle: true,
noExtra: true,
},
result: make(chan *types.Block, 1),
}
Copy link
Contributor

@holiman holiman Nov 29, 2021

Choose a reason for hiding this comment

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

It seems that this method is only used for PoS blocks? Which makes sense, because it has noUncle and noExtra set to true, so it's not really abstract.
I find it a bit confusing to have this seemingly abstract block sealer mode for catalyst, whereas the ethash and clique engines use some other path to produce a block. Is the intent to later on align these three engines?

Copy link
Member Author

@rjl493456442 rjl493456442 Jan 5, 2022

Choose a reason for hiding this comment

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

The main reason for catalyst has different path to produce block is it's rpc-event-based engine. While for ethash and clique they are chain-event-based engine, which just create blocks non-stop.

It's still ugly for them to have different paths, but we can find some proper solution to merge these paths I guess.

miner/worker.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Show resolved Hide resolved
Copy link
Contributor

@holiman holiman left a comment

Looks ok, but I'm a bit confused by all the changes to the catalyst part.
Also, what I"m a bit scared about is coinbase - it would be bad if these changes make setting the coinbase not "stick".
So if a miner is mining, they need to be able to set the coinbase via,

  • startup param,
  • rpc,
    • before starting mining,
    • and during mining
    • and when mining is requested, but during sync, so mining is disabled

Do we have tests for these? Have you experimented anything with these scenarios?

//
// 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)
Copy link
Member

@karalabe karalabe Jan 5, 2022

Choose a reason for hiding this comment

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

Is this really so in practice? We're going to hold onto the latest 128 states either way for snap sync and reorgs. This clause would only trigger if someone requests mining on something super old. Is that a viable requirement?

Copy link
Member Author

@rjl493456442 rjl493456442 Jan 5, 2022

Choose a reason for hiding this comment

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

Not sure, but I guess so. Consensus layer might reorg to some old branch and require us to build a block there. If the chain is not finalized for very long time, so the deep reorg is expected?

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Jan 17, 2022

Choose a reason for hiding this comment

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

I think so too, since there is not really a guarantee that we won't reorg very deeply with non-finalization

Copy link
Member

@karalabe karalabe Jan 24, 2022

Choose a reason for hiding this comment

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

Sure, but we'd get a sethead first from the beacon chain before mining on top, wouldn't we? Which would do the reorg and reconstruction of the needed state? Maybe I'm wrong tho'

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Some small nits, looks pretty solid to me

eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Show resolved Hide resolved
miner/stress/beacon/main.go Show resolved Hide resolved
//
// 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)
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Jan 17, 2022

Choose a reason for hiding this comment

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

I think so too, since there is not really a guarantee that we won't reorg very deeply with non-finalization

miner/worker.go Outdated Show resolved Hide resolved
miner/worker.go Outdated Show resolved Hide resolved
@MariusVanDerWijden
Copy link
Member

MariusVanDerWijden commented Jan 17, 2022

I've asked Pari to run this code on a validator on kintsugi. Would be great to also put it on a clique miner maybe? cc @holiman

@holiman
Copy link
Contributor

holiman commented Jan 17, 2022

Sure

@holiman
Copy link
Contributor

holiman commented Jan 17, 2022

On rinkeby-aws-eu-west-3-001: Geth/v1.10.16-unstable-cc7f6269-20220112/linux-amd64/go1.17.6

@MariusVanDerWijden
Copy link
Member

MariusVanDerWijden commented Jan 21, 2022

eth/catalyst/api.go:80: File is not goimports-ed (goimports)
preparedBlocks *payloadQueue // preparedBlocks caches payloads (*ExecutableDataV1) by payload ID (PayloadID)

Copy link
Member

@karalabe karalabe left a comment

LGTM!

@karalabe karalabe added this to the 1.10.16 milestone Jan 24, 2022
@karalabe karalabe merged commit 78636ee into ethereum:master Jan 24, 2022
1 check was pending
ValentinTrinque pushed a commit to vegaprotocol/go-ethereum that referenced this issue Jan 25, 2022
* eth, miner: remove duplicated code

* eth/catalyst: remove unneeded code

* miner: keep update pending state even the Merge is happened

* eth, miner: rebase

* miner: fix tests

* eth, miner: address comments from marius

* miner: use empty zero randomness for pending blocks after the merge

* eth/catalyst: gofmt

* miner: add warning log for state recovery

* miner: ignore uncles for post-merge blocks

Co-authored-by: Péter Szilágyi <peterke@gmail.com>
MariusVanDerWijden pushed a commit to MariusVanDerWijden/go-ethereum that referenced this issue Jan 27, 2022
* eth, miner: remove duplicated code

* eth/catalyst: remove unneeded code

* miner: keep update pending state even the Merge is happened

* eth, miner: rebase

* miner: fix tests

* eth, miner: address comments from marius

* miner: use empty zero randomness for pending blocks after the merge

* eth/catalyst: gofmt

* miner: add warning log for state recovery

* miner: ignore uncles for post-merge blocks

Co-authored-by: Péter Szilágyi <peterke@gmail.com>
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this issue May 26, 2022
* eth, miner: remove duplicated code

* eth/catalyst: remove unneeded code

* miner: keep update pending state even the Merge is happened

* eth, miner: rebase

* miner: fix tests

* eth, miner: address comments from marius

* miner: use empty zero randomness for pending blocks after the merge

* eth/catalyst: gofmt

* miner: add warning log for state recovery

* miner: ignore uncles for post-merge blocks

Co-authored-by: Péter Szilágyi <peterke@gmail.com>
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.

None yet

9 participants