NC-2147 Implement Petersburg hardfork #601
Conversation
* create a new fork definition * make it constantinople without EIP-1283 * update tests where appropriate
There was a problem hiding this comment.
While we'll likely change the milestone name from Petersburg to something agreed upon with geth, I think it may be worth merging this - it's technically good and will guarantee that our next release has the newly scheduled block number. What name is used for it has no impact on how it behaves.
| @@ -42,6 +42,7 @@ | |||
| @Setup | |||
| public void prepare() throws Exception { | |||
| operationBenchmarkHelper = OperationBenchmarkHelper.create(); | |||
| // ?? do we create another benchmark ?? | |||
There was a problem hiding this comment.
No, I think the Constantinople benchmark is all we need here as the operation of block hash is unaffected (technically we could use a much earlier milestone) and the gas calculator isn't what we're trying to benchmark anyway.
There was a problem hiding this comment.
Now that I added a PetersburgGasCalculator I moved it over to that one, just to keep current.
| public void petersburg() { | ||
| milestone("Petersburg"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Since the transaction tests haven't been updated for "Petersburg" we shouldn't add this. I doubt there will be additional transaction tests added since they're just about parsing transactions, not about executing them and that's unaffected.
| @@ -193,6 +193,12 @@ private MainnetProtocolSpecs() {} | |||
| .name("Constantinople"); | |||
| } | |||
|
|
|||
| public static ProtocolSpecBuilder<Void> petersburgDefinition(final int chainId) { | |||
| return constantinopleDefinition(chainId) | |||
| .gasCalculator(TangerineWhistleGasCalculator::new) | |||
There was a problem hiding this comment.
Don't we need a new GasCalculator ?
There was a problem hiding this comment.
Good spot - we will because Constantinople gas calculator also provides the gas cost for EXTCODEHASH and CREATE2 which aren't available in tangerine whistle. It should omit the calculateStorageCost and calculateStorageRefundAmount overrides that are in ConstantinopleGasCalculator.
There was a problem hiding this comment.
New gas calculator added. I extended Constantinople and copied the code from Frontier since that was the last time that particular method was updated.
ajsutton
dismissed
their stale review
Missed the gas calculator detail, so just to avoid any risk of it slipping through retracting approval.
* Add PetersbergGasCalculator
|
We'll want to stay compatible with Geth's genesis config. The PR for Geth is ethereum/go-ethereum#18486 and I think it's using 'constantinopleFixBlock" as the genesis config option but it's not entirely clear - "PetersburgBlock" is used a lot through the code as well. The other detail is that if "constantinopleFixBlock" is omitted, it defaults to the same block as "constantinopleBlock". This makes a lot of sense for MainNet but feels slightly weird and unexpected. But if we don't do the same it could be a major source of confusion in the future because it's a subtle but very important difference in genesis config meaning. |
* Add rinkeby Constantinople block * Petersburg -> ConstantinopleFix
* add Ropsten constantinopleFix block * run reference tests * remove ConstantinopleFix from TransactionTestCases (there are no tests)
|
Ready for another review.
|
# Conflicts: # ethereum/core/src/test-support/java/tech/pegasys/pantheon/ethereum/core/ExecutionContextTestFixture.java
PR description
Fixed Issue(s)
NC-2147