Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

core/block_validator.go#calcDifficultyAtlantis is wrong #67

Closed
meowsbits opened this issue Jun 17, 2019 · 7 comments
Closed

core/block_validator.go#calcDifficultyAtlantis is wrong #67

meowsbits opened this issue Jun 17, 2019 · 7 comments

Comments

@meowsbits
Copy link

meowsbits commented Jun 17, 2019

https://github.com/eth-classic/go-ethereum/blob/development/core/block_validator.go#L362

At first review this appears to use a hardcoded bomb delay value according to EIP1234, which is not relevant to ETC nor Atlantis.

Atlantis occurs canonically after ECIP1010 (difficulty bomb defuse), which makes this spec and logic inapplicable.

func calcDifficultyAtlantis(time uint64, parent *types.Header) *big.Int {
	bombDelayFromParent := new(big.Int).Sub(big.NewInt(3000000), big1)
	// https://github.com/ethereum/EIPs/issues/100.
	// algorithm:
	// diff = (parent_diff +
	//         (parent_diff / 2048 * max((2 if len(parent.uncles) else 1) - ((timestamp - parent.timestamp) // 9), -99))
	//        ) + 2^(periodCount - 2)

	bigTime := new(big.Int).SetUint64(time)
	bigParentTime := parent.Time

	// holds intermediate values to make the algo easier to read & audit
	x := new(big.Int)
	y := new(big.Int)

	// (2 if len(parent_uncles) else 1) - (block_timestamp - parent_timestamp) // 9
	x.Sub(bigTime, bigParentTime)
	x.Div(x, big9)
	if parent.UncleHash == types.EmptyUncleHash {
		x.Sub(big1, x)
	} else {
		x.Sub(big2, x)
	}
	// max((2 if len(parent_uncles) else 1) - (block_timestamp - parent_timestamp) // 9, -99)
	if x.Cmp(bigMinus99) < 0 {
		x.Set(bigMinus99)
	}
	// parent_diff + (parent_diff / 2048 * max((2 if len(parent.uncles) else 1) - ((timestamp - parent.timestamp) // 9), -99))
	y.Div(parent.Difficulty, params.DifficultyBoundDivisor)
	x.Mul(y, x)
	x.Add(parent.Difficulty, x)

	// minimum difficulty can ever be (before exponential factor)
	if x.Cmp(params.MinimumDifficulty) < 0 {
		x.Set(params.MinimumDifficulty)
	}
	// calculate a fake block number for the ice-age delay
	// Specification: https://eips.ethereum.org/EIPS/eip-1234
	fakeBlockNumber := new(big.Int)
	if parent.Number.Cmp(bombDelayFromParent) >= 0 {
		fakeBlockNumber = fakeBlockNumber.Sub(parent.Number, bombDelayFromParent)
	}
	// for the exponential factor
	periodCount := fakeBlockNumber
	periodCount.Div(periodCount, expDiffPeriod)

	// the exponential factor, commonly referred to as "the bomb"
	// diff = diff + 2^(periodCount - 2)
	if periodCount.Cmp(big1) > 0 {
		y.Sub(periodCount, big2)
		y.Exp(big2, y, nil)
		x.Add(x, y)
	}
	return x
}
@meowsbits
Copy link
Author

Only EIP100 should be introduced.

@austinabell
Copy link
Contributor

yeah adding the bomb delay segment was because I wasn't aware ETC defused the difficulty bomb when adding that in (ETC has an equivalent of the EIPs you mentioned) and I wanted to make sure the client stayed in sync with other clients (as other clients had this value in) Very minor change as it doesn't affect anything really but I'll have a PR in soon

@meowsbits
Copy link
Author

meowsbits commented Jun 17, 2019

Very minor change as it doesn't affect anything really

At block 11,500,000 (or fork number + 3000000) the network would have forked if this client were in play. Block difficulty is a consensus issue.

Although yes, the fix is simple.

@austinabell
Copy link
Contributor

austinabell commented Jun 17, 2019

Yeah another case of not choosing my words carefully, I meant to say it wasn't an urgent PR since those blocks are so incredibly far in the future (In reference to the testnet we have running and the timeline of the PR).

@meowsbits
Copy link
Author

meowsbits commented Jun 17, 2019

>>> 3000000 * 14

  3000000 × 14

   = 42000000

>>> ans / 60 / 60 / 24

  ((ans / 60) / 60) / 24

   = 486.111

where 14 is approximate average block time seconds

**~ 486 days**

incredibly far in the future

is more like "the next few decades", eg.

@GregTheGreek
Copy link

>>> 3000000 * 14

  3000000 × 14

   = 42000000

>>> ans / 60 / 60 / 24

  ((ans / 60) / 60) / 24

   = 486.111

where 14 is approximate average block time seconds

**~ 486 days**

incredibly far in the future

is more like "the next few decades", eg.

May we please revert back to testing and less on semantics. Thank you for identifying the bug, we're working on it 😄

@soc1c
Copy link
Contributor

soc1c commented Jun 18, 2019

Done #69

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants