Set PetersburgBlock as optional when checking Fork order

[vdamle]

PetersburgBlock, if not explicitly set, is assumed to be active depending on whether ConstantinopleBlock is active. However, when checking fork ordering, PetersburgBlock is treated mandatory. In private chains which are running 1.8.27 with ConstantinopleBlock active, migration to 1.9 fails because PetersburgBlock is not explicitly set (even the gas calculation works as though Petersburg block is set). This PR sets PetersburgBlock to optional to reflect the treatment of the fork across the rest of the code. The main benefit of this change is that private chains which set the Constantinople fork and are running with Geth 1.8.27 can upgrade to Geth 1.9.x and apply the Istanbul fork after upgrade.

I discussed this with @holiman on Discord, creating a PR as follow up to the discussion.

[holiman]

I am not sure this is correct.

Some context about the whole situation:

• Constantinople adds, among other things, eip 1283, which is the initial version of net-sstore-gas-metering. The code you point to re gas calculation checks "Are we after Petersburg, or before Constantinople, then skip 1283"
• petersburg, aka ConstatntopleFix removes 1283 functionality again.
• on testnets, 1283 existed for some blocks. On mainnet, the two (Constantinople + Petersburg) were set to be on the same block, effectively never having 1283 at all.

Now, if you make petersburg optional, I think you get the wrong semantics, making it possible for istanbul to happen if petersburg is nil. I am not 100% conviced that this PR is correct, but will need to think about it some more..

[vdamle]

Thanks for your input @holiman . The "Are we after Petersburg" question is determined using the following two criteria:

• If Petersburg is explicitly set, is the block number higher than what's set? If yes, we are after Petersburg
• If Petersburg is nil, are we after Constantinople? If yes, we are after Petersburg

Given the above, (ref:

go-ethereum/params/config.go

Lines 419 to 424 in 54add42

	// IsPetersburg returns whether num is either
	// - equal to or greater than the PetersburgBlock fork block,
	// - OR is nil, and Constantinople is active
	func (c *ChainConfig) IsPetersburg(num *big.Int) bool {
	return isForked(c.PetersburgBlock, num) || c.PetersburgBlock == nil && isForked(c.ConstantinopleBlock, num)
	}

), chains on 1.8.27 which do not have Petersburg set, but are mining blocks higher than what is set for Constantinople return true for IsPetersburg() -- in effect, they do not apply 1283 functionality. Such chains cannot upgrade to 1.9.x and apply Istanbul fork with the current constraints even though the client code behavior is exactly as though PetersburgBlock is set.

[karalabe]

This is an oddball issue. I raised a concern in the original Petersburg PR that auto-enabling seems wrong. Now, the messy part is that the chain will auto-enable it with constantinople, BUT the forkid and other code will not, since they only look at the numbers.

[karalabe]

I was wrong. Fork ID dedups forks at the same block number, so that should still work ok.

[karalabe]

@vdamle Can't you just reinit your private chain's fork config with Petersburg enabled at the Constantinople block number? Wondering if Geth would accept that or complain that the new fork config is incompatible with the chain (could happen). I'd be leaning towards special casing the init logic to permit backwards setting Petersburg than to make it optional and continue the sharade.

[vdamle]

Thanks @karalabe. I thought of that as an alternative and spent time yesterday testing that. It works without issues for upgrade and also when adding nodes after upgrade (all blocks sync fine). Here's the proposed change, I created a separate PR (i'm pretty sure this is the special casing in init you refer to)

#21473

[holiman]

Closing in favour of #21473