accounts, consensus, core, eth: make chain maker consensus agnostic#15497
Conversation
| b.header.Difficulty = ethash.CalcDifficulty(b.config, b.header.Time.Uint64(), b.parent.Header()) | ||
| if pow, ok := b.engine.(consensus.PoW); ok { | ||
| b.header.Difficulty = pow.CalcDifficulty(b.config, b.header.Time.Uint64(), b.parent.Header()) | ||
| } |
There was a problem hiding this comment.
Take care, the difficulty should work for all consensus engines, not just for PoW based ones. Clique has its own notion of difficulty too.
| // AccumulateRewards credits the coinbase of the given block with the mining | ||
| // reward. The total reward consists of the static block reward and rewards for | ||
| // included uncles. The coinbase of each uncle block is also rewarded. | ||
| AccumulateRewards(*params.ChainConfig, *state.StateDB, *types.Header, []*types.Header) |
There was a problem hiding this comment.
I think consensus.Engine.Finalize already contains the reward accumulation within. I'm not sure you need an external method for it.
|
|
||
| // CalcDifficulty returns the difficulty that a new block should have when created at time | ||
| // given the parent block's time and difficulty. | ||
| CalcDifficulty(*params.ChainConfig, uint64, *types.Header) *big.Int |
There was a problem hiding this comment.
Could we perhaps similarly use consensus.Engine.Prepare to calculate the difficulty? Maybe some code needs the raw difficulty, just asking. Ether way, it should probably land in the Engine interface, not in PoW only.
rjl493456442
force-pushed
the
improve_chain_marker
branch
2 times, most recently
from
86c8ccc to
b05355e
Compare
|
@karalabe Updated |
karalabe
left a comment
karalabe
left a comment
There was a problem hiding this comment.
Almost perfect :) I've written up some suggestions, plus 1 open question remains.
There was a problem hiding this comment.
This became a bit convoluted. I think it's fine to retain the global method ethash.CalcDifficulty, and just wrap it with the engine.CalcDifficulty.
There was a problem hiding this comment.
If we completely move CalcDifficulty inside the ethash engine, then outside code that wants to work with ethash will always need an engine. I think the best would be to leave this method as a plain global, and also to wrap it within the engine. That way we have an engine agnostic way to calculate a difficulty, but it can be done without requiring an engine object too.
There was a problem hiding this comment.
If you retain the global ethash.CalcDifficulty method too, you won't need either the NewFaker, nor the emphemeral chain here.
There was a problem hiding this comment.
Calling CalcDifficulty here is a bit expensive as it needs to regenerate the snapsot internally. Imho it would be better to leave this code as is and calculate the difficulty directly.
Alternatively, you can add a global clique.CalcDifficulty method the same was as in ethash.CalcDifficulty, that would contain just the essence and operate on a snapshot:
func CalcDifficulty(snap *Snapshot, signer common.Address) *big.Int {
if snap.inturn(snap.Number+1, c.signer) {
return new(big.Int).Set(diffInTurn)
}
return new(big.Int).Set(diffNoTurn)
}And you can call this function directly here, and also wrap it with engine.CalcDifficulty.
There was a problem hiding this comment.
Bonus point if we can figure out a way to get rid of this ephemeral blockchain here. I'm also unsure if it is correct in the case of Clique (i.e. does this chain contain any data, or will it just blow up when invoked for clique)?
There was a problem hiding this comment.
The reason to construct an ephemeral blockchain here is we need to use it to calculate the new difficulty for the new block.
As for clique, chainReader is necessary for difficulty calculation. So we'd better keep it.
rjl493456442
force-pushed
the
improve_chain_marker
branch
from
b05355e to
5993d19
Compare
rjl493456442
force-pushed
the
improve_chain_marker
branch
from
5993d19 to
597d5cc
Compare
rjl493456442
force-pushed
the
improve_chain_marker
branch
from
597d5cc to
fdffaf3
Compare
| return CalcDifficulty(snap, c.signer) | ||
| } | ||
|
|
||
| func CalcDifficulty(snap *Snapshot, signer common.Address) *big.Int { |
There was a problem hiding this comment.
Don't forget to have a doc here ;)
| return CalcDifficulty(chain.Config(), time, parent) | ||
| } | ||
|
|
||
| func CalcDifficulty(config *params.ChainConfig, time uint64, parent *types.Header) *big.Int { |
There was a problem hiding this comment.
Don't forget to have a doc here ;)
rjl493456442
force-pushed
the
improve_chain_marker
branch
from
998538a to
993267d
Compare
…thereum#15497) * accounts, consensus, core, eth: make chain maker consensus agnostic * consensus, core: move CalcDifficulty to Engine interface * consensus: add docs for calcDifficulty function * consensus, core: minor comment fixups
2 participants
Implements #15488