Schedule Goerli shanghaiTime and configure forkId for tests

[siladu]

Announced here: ethereum/execution-specs#724

Still waiting for confirmation of the forkId hash.

Marius confirmed:

I can confirm 0xf9843abf with 1678832736

Sepolia change for comparison: 5452159

I could update ForkIdsNetworkConfigTest but Goerli hasn't been kept up to date for that test since Istanbul. I checked code coverage and I don't believe that test is providing any extra value beyond what ForkIdTest is already doing. I would suggest we remove ForkIdsNetworkConfigTest and the unnecessary parts of ForkIdTestUtil. Would like some feedback from @jframe about that first though.

[jframe]

The ForkIdsNetworkConfigTest was last updated for London for Goerli so it has been kept up to date.

I think still makes sense to keep it. It tests that the configuration in the network json file is correct. Whereas the ForkIdTest is testing the forkId logic against milestones defined in the ForkIdTestUtil. So it's possible that without the ForkIdsNetworkConfigTest we wouldn't know if the values in the network json file are incorrect.

[jframe]

LGTM

[siladu]

The ForkIdsNetworkConfigTest was last updated for London for Goerli so it has been kept up to date.

I think still makes sense to keep it. It tests that the configuration in the network json file is correct. Whereas the ForkIdTest is testing the forkId logic against milestones defined in the ForkIdTestUtil. So it's possible that without the ForkIdsNetworkConfigTest we wouldn't know if the values in the network json file are incorrect.

@jframe Got my test names mixed up, switch ForkIdsNetworkConfigTest for ForkIdTest w.r.t not being updated since Istanbul...I updated ForkIdsNetworkConfigTest in this PR so that is the one I was suggesting we keep. But...I also mixed them up when I ran the coverage, so actually yes ForkIdTest is covering more of the code than ForkIdsNetworkConfigTest.

Since not convinced by the value of adding Goerli config to it though, is there any in your opinion?

[jframe]

The ForkIdsNetworkConfigTest was last updated for London for Goerli so it has been kept up to date.
I think still makes sense to keep it. It tests that the configuration in the network json file is correct. Whereas the ForkIdTest is testing the forkId logic against milestones defined in the ForkIdTestUtil. So it's possible that without the ForkIdsNetworkConfigTest we wouldn't know if the values in the network json file are incorrect.

@jframe Got my test names mixed up, switch ForkIdsNetworkConfigTest for ForkIdTest w.r.t not being updated since Istanbul...I updated ForkIdsNetworkConfigTest in this PR so that is the one I was suggesting we keep. But...I also mixed them up when I ran the coverage, so actually yes ForkIdTest is covering more of the code than ForkIdsNetworkConfigTest.

Since not convinced by the value of adding Goerli config to it though, is there any in your opinion?

ForkIdTest covers a lot more cases so we should definitely keep that. If we were to load the network configuration from the json files instead from the predefined values in ForkIdTestUtil then there would be no need for both test classes we could just have the ForkIdTest. Which would be a lot better. This would be better to do in another PR though.

As for the value of adding Goerli config to the ForkIdTest there isn't much value in that since we have already effectively tested that in ForkIdsNetworkConfigTest.