common: allow specifying eip-7840 blobSchedule via geth genesis #3835
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| // Allow to even override an existing hardfork specification | ||
| (this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]) ?? | ||
| hardforksDict[hf.name], |
There was a problem hiding this comment.
important change here cc @jochem-brouwer @acolytec3 @holgerd77
(please see comment)
There was a problem hiding this comment.
Ok, this works, but the problem is that if you want to correctly override it, you first have to import all the params from the original hardfork and then set that as custom hardfork
There was a problem hiding this comment.
My initial thoughts here are: load the original hardforksDict[hf.name] and then merge the overridden changes in it? (via copy so via JSON.stringify / JSON.parse)
There was a problem hiding this comment.
Ok rather lengthy review 😅
This change:
// Allow to even override an existing hardfork specification
(this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]) ??
hardforksDict[hf.name],Is rather big, and I think we should test this more, because I think currently "wrong" inputs can throw away all hardfork params (for that common)?
| // Allow to even override an existing hardfork specification | ||
| (this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]) ?? | ||
| hardforksDict[hf.name], |
There was a problem hiding this comment.
Ok, this works, but the problem is that if you want to correctly override it, you first have to import all the params from the original hardfork and then set that as custom hardfork
| // Allow to even override an existing hardfork specification | ||
| (this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]) ?? | ||
| hardforksDict[hf.name], |
There was a problem hiding this comment.
My initial thoughts here are: load the original hardforksDict[hf.name] and then merge the overridden changes in it? (via copy so via JSON.stringify / JSON.parse)
| let customHardforks: HardforksDict | undefined = undefined; | ||
| if (config.blobSchedule !== undefined) { | ||
| customHardforks = {}; | ||
| const blobGasPerBlob = 131072; |
There was a problem hiding this comment.
Ideally, we do not hard-code this constant and read it from blobGasPerBlob (= 2**17 / 131072)
| for (const [hfKey, hfSchedule] of Object.entries(config.blobSchedule)) { | ||
| const hfConfig = hardforksDict[hfKey]; | ||
| if (hfConfig === undefined) { | ||
| throw new Error(`unknown hardfork=${hfKey} specified in blobSchedule`); |
There was a problem hiding this comment.
Could you add this case to tests?
| throw new Error(`unknown hardfork=${hfKey} specified in blobSchedule`); | ||
| } | ||
| const { target, max, baseFeeUpdateFraction: blobGasPriceUpdateFraction } = hfSchedule as { target?: number, max?: number, baseFeeUpdateFraction?: undefined }; | ||
| if (target === undefined || max === undefined) { |
There was a problem hiding this comment.
Why is blobGasPriceUpdateFraction not mandatory?
There was a problem hiding this comment.
The "fallback" logic now works different from target/max and blobGasPriceUpdateFraction, for blobGasPriceUpdateFraction it will do the fallback, but for max/target it has to be explicitly defined.
| const customHfConfig = JSON.parse(JSON.stringify(hfConfig)); | ||
| customHfConfig.params = { | ||
| ...customHardforks.params, | ||
| // removes blobGasPriceUpdateFraction key to prevent undefined overriding if undefined |
There was a problem hiding this comment.
I don't think this works, if blobGasPriceUpdateFraction is undefined then it will set the param to undefined also.
| bootstrapNodes: [], | ||
| consensus: | ||
| config.clique !== undefined | ||
| ? { | ||
| type: 'poa', |
There was a problem hiding this comment.
Why is this code block moved?
| "prague": { | ||
| "target": 61, | ||
| "max": 91, | ||
| "baseFeeUpdateFraction": 13338477 |
There was a problem hiding this comment.
Could you also add a test where baseFeeUpdateFraction is not set (also not explicitly to undefined)? I think this will override and set the param in the fork to undefined instead of leaving that key alone.
| @@ -321,6 +321,7 @@ | |||
| "beaconroot", | |||
| "Grandine", | |||
| "EVMBN", | |||
| "kaust" | |||
| "kaust", | |||
| "blobschedule" | |||
There was a problem hiding this comment.
This should be in cspell-ts.json
|
I have updated this PR description to mark this issue #3822 to be closed if it gets merged |
allow gethgenesis to specify eip 7840 blobschedule + optional update fraction: ethereum/EIPs#9240
Closes #3822