Block: add methods to calculate difficulty of a consecutive header/block

[jochem-brouwer]

Cherry-picked from #895

It is (recently) not allowed anymore to edit params like difficulty in Header. In order to currently set the (right) difficulty in header (i.e. the canonical difficulty) then one has to create a mock Header and then call canonicalDifficulty on this mock Header. Then, one can create a new Header with the difficulty as returned by this canonicalDifficulty call.

It doesn't make a lot of sense to create a mock header. We want to "just set it to the correct value".

This PR:

• Adds calcDifficultyFromHeader as a BlockOption. If this is defined, then this Header will be used to calculate the difficulty; any existing difficulty will be overwritten to this new, correct, value. Suggestion by @holgerd77 👍
• Change tests in blockchain to use this new method.
• Make canonicalDifficulty method a bit more readable by instead using the corresponding gt/lt methods of BN instead of using cmp. Suggestion by @ryanio 👍

[codecov]

Codecov Report

Merging #929 into master will decrease coverage by 0.08%.
The diff coverage is 33.33%.

Flag	Coverage Δ
block	76.47% <33.33%> (-0.15%)	⬇️
blockchain	80.57% <ø> (-0.39%)	⬇️
common	91.79% <ø> (ø)
ethash	82.08% <ø> (ø)
tx	88.37% <ø> (ø)
vm	87.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[ryanio]

Looks great! Should we update the usage of canonicalDifficulty to getConsecutiveCanonicalDifficulty in blockchain/test/index.ts?

[jochem-brouwer]

Yes, good point @ryanio, I will alter the tests so that we do not use mock headers anymore.

[ryanio]

lgtm!

[holgerd77]

  /**
   * Returns the canonical difficulty of any consecutive new header, given the timestamp of the consecutive block and (optionally) the header number and the common
   * @param timestamp - The `BN` timestamp of the consecutive header which we should calculate the difficulty for
   * @param number - The `BN` header number of the consecutive header. Defaults to the current headers' number + 1
   * @param common - The `Common` to use: defaults to the current headers' Common
   */

  public getConsecutiveCanonicalDifficulty(timestamp: BN, number?: BN, common?: Common): BN {
    const blockTs = timestamp.clone()
    const { timestamp: parentTs, difficulty: parentDif } = this
    common = common || this._common
    let num = number?.clone() || this.number.addn(1)
    const hardfork = common.hardfork() || common.activeHardfork(num.toNumber())
    const minimumDifficulty = new BN(common.paramByHardfork('pow', 'minimumDifficulty', hardfork))
    const offset = parentDif.div(
      new BN(common.paramByHardfork('pow', 'difficultyBoundDivisor', hardfork))
    )

For some strange reasons GitHub doesn't let me post on the code directly right now, so I'll just copy over here. TBH I am still a bit confused here on the changes. Does the "difficulty of any consecutive new header" mean that this will also give the difficulty e.g. 5 headers in advance? If so, can we have a test for this? If not, why do we need number as a parameter beside the number + 1 thing? 🤔 Same question a bit for Common, why not always use the Common instance from the block together with the activeHardfork() request, since this will likely contain the correct information?

const hardfork = common.hardfork() || common.activeHardfork(num.toNumber())

This line doesn't seem correct to me since common.hardfork() will always return some hardfork, so common.activeHardfork(num.toNumber()) will never get into play if I am not missing something.

* @param block  - The block to get the canonical difficulty for

This has been left in the block method version comment.

[jochem-brouwer]

Hi @holgerd77, thanks for these comments.

I agree that this number parameter looks weird - why would you need this? But this is actually already a requirement in the current tests. Note that in the current code, there is never an assumption that parentHeader is actually the parent header of this block (i.e. it has header.number - 1 as block number). We are actually testing this feature in the tests: difficulty.test.spec calls canonicalDifficulty where these block header numbers differ a lot.

const hardfork = common.hardfork() || common.activeHardfork(num.toNumber())

Note that originally, we called into _getHardfork(), which is the same code as above. By the type signature of common.hardfork() it seems that this can also return null. So the logic here should be the same as the original logic. The use case of providing this specific Common instance is to get the difficulty if you want, for some reason, not take into account the difficulty bomb.

I will update the docs when I rebase this one.

[jochem-brouwer]

Actually upon inspecting this Common code it seems that the type signature of hardfork() is wrong; it states that it can also return null but I see no reason why it would return null; it should always return a string. I think this was introduced when we introduced default hardforks to Common?

Will need to investigate this one a bit more. Looks like this is feature (?) is not tested thoroughly.

[holgerd77]

I gave the initial argumentation from the opening post a re-read and TBH I am not really convinced by this whole construction. If the whole problem is the now-existing immutability of the header parameters, wouldn't it be cleaner to directly trigger difficulty calculation in the constructor, e.g. by adding a calculateDifficulty flag, and then operate on the to be created block?

I think this whole combination of Common - yes/no - number - yes/no - is unfortunate and will be very difficult to get under control regarding the hardfork to choose.

[holgerd77]

@jochem-brouwer Side note on the Common.hardfork() null question: yes, this is outdated (somewhat: wrong, not more wrong than an any type though 😜 ), this is still from the "old" days before the DEFAULT_HARDFORK param was introduced and it was still allowed to not pass a HF. Will add a note within the tiny-changes-issue to udate.

[jochem-brouwer]

Hi @holgerd77 thanks for these suggestions. I agree with your concerns and upon some more thought the current approach might not be the best approach.

In practice in your feature where we let the constructor calculate the difficulty would only make sense in fromHeaderData, right? In the other ways to create the block it does not make much sense. This would also require we pass the parent block header to the constructor, because we have to figure out the differences in timestamps.

I'd have to check if this would lead to any problems in the tests but I think this is possible. It does mean that we allow the parentHeader to be a parentHeader which has a lower block number than the one we expect - but since we explicitly let the constructor calculate the difficulty I think this is OK.

RE: the hardfork() - makes a lot of sense, should fix this at some point but not really of an issue currently 👍

I will change this so we will only support this in the constructor, which indeed makes more sense!

[holgerd77]

Maybe pass in a calcDifficultyFromParentBlockHeader?: BlockHeader option as BlockOption, calculate at the end of the header constructor before the freeze and leave the canonicalDifficulty() logic "as is"? Then this can be triggered from all the constructors.

[jochem-brouwer]

Hi @holgerd77, I rolled back this PR and 🍒 -picked one commit from the original PR (c672b07) and instead wrote your approach. This is much simpler - thanks so much for this suggestion!

I have instead named this parameter calcDifficultyFromHeader: it is shorter and is also a bit more correct (it does not strictly have to be a parentHeader). Will update the description of this PR. Ready for re-review! 😄

[holgerd77]

Oh yeah, this really makes it a lot cleaner, think this was worth the exchange! 😄

Thanks @jochem-brouwer for the update on this, will merge.

[holgerd77]

Yeah, that's nice to have these comparisons replaced, for me this BN.js cmp function remained pretty unreadable over the time I am dealing with it.

[holgerd77]

If a BlockHeader is given, then this header is used to calculate the difficulty upon creating the Block Header.

This was to prone yet for misunderstanding, have updated the comment with an additional commit. The double Block Header reference was too confusing - one refering to the header provided and one to the header created.

Some general note: I think the usage of "this" in these kind of option comments often don't work, since it is always a bit misleading in the sense that it becomes not 100% clear if "this" refers to "this" option provided or to "this" instance created, I now stumbled upon this (haha 😄 ) a couple of times. Think it works better to in doubt be a bit more redundant and e.g. repeat "the header provided" instead of using "this header".

[jochem-brouwer]

Ah yes I'm somewhat used to write this in these descriptions, will have to self-review those sometimes 👍

[holgerd77]

I think it should rather be avoided to write test descriptions out of the self perspective 😄 , won't let this be a block though of course, just some note.

[jochem-brouwer]

Ah yes you are right 😅 Will consider this too.