Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: allow specifying eip-7840 blobSchedule via geth genesis #3835

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion config/cspell-md.json
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@
"beaconroot",
"Grandine",
"EVMBN",
"kaust"
"kaust",
"blobschedule"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in cspell-ts.json

]
}
7 changes: 4 additions & 3 deletions packages/common/src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ export class Common {
// Assign hardfork changes in the sequence of the applied hardforks
this.HARDFORK_CHANGES = this.hardforks().map((hf) => [
hf.name,
hardforksDict[hf.name] ??
(this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]),
// Allow to even override an existing hardfork specification
(this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]) ??
hardforksDict[hf.name],
Comment on lines +69 to +71
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

important change here cc @jochem-brouwer @acolytec3 @holgerd77

(please see comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

])
this._hardfork = this.DEFAULT_HARDFORK
this._params = opts.params ? JSON.parse(JSON.stringify(opts.params)) : {} // copy
Expand Down Expand Up @@ -322,7 +323,7 @@ export class Common {
this._mergeWithParamsCache(this._params[eip] ?? {})
}
}
// Parameter-inlining HF config (e.g. for istanbul)
// Parameter-inlining HF config (e.g. for istanbul or custom blobSchedule)
this._mergeWithParamsCache(hfChanges[1].params ?? {})
if (hfChanges[0] === hardfork) break
}
Expand Down
59 changes: 45 additions & 14 deletions packages/common/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

import { Goerli, Holesky, Kaustinen6, Mainnet, Sepolia } from './chains.js'
import { Hardfork } from './enums.js'
import { hardforksDict } from './hardforks.js'

import type { HardforksDict } from './types.js'
import type { PrefixedHexString } from '@ethereumjs/util'


type ConfigHardfork =
| { name: string; block: null; timestamp: number }
| { name: string; block: number; timestamp?: number }
Expand Down Expand Up @@ -93,6 +96,33 @@
throw new Error('nonzero terminal total difficulty is not supported')
}

let customHardforks: HardforksDict | undefined = undefined;
if (config.blobSchedule !== undefined) {
customHardforks = {};
const blobGasPerBlob = 131072;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add this case to tests?

}

Check warning on line 107 in packages/common/src/utils.ts

View check run for this annotation

Codecov / codecov/patch

packages/common/src/utils.ts#L106-L107

Added lines #L106 - L107 were not covered by tests
const { target, max, baseFeeUpdateFraction: blobGasPriceUpdateFraction } = hfSchedule as { target?: number, max?: number, baseFeeUpdateFraction?: undefined };
if (target === undefined || max === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is blobGasPriceUpdateFraction not mandatory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new Error(`undefined target or max specified in blobSchedule for hardfork=${hfKey}`);
}

Check warning on line 111 in packages/common/src/utils.ts

View check run for this annotation

Codecov / codecov/patch

packages/common/src/utils.ts#L110-L111

Added lines #L110 - L111 were not covered by tests


// copy current hardfork info to custom and add blob config
const customHfConfig = JSON.parse(JSON.stringify(hfConfig));
customHfConfig.params = {
...customHardforks.params,
// removes blobGasPriceUpdateFraction key to prevent undefined overriding if undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works, if blobGasPriceUpdateFraction is undefined then it will set the param to undefined also.

...{ targetBlobGasPerBlock: blobGasPerBlob * target, maxBlobGasPerBlock: blobGasPerBlob * max, blobGasPriceUpdateFraction }
}

customHardforks[hfKey] = customHfConfig;
}
}

const params = {
name,
chainId,
Expand All @@ -111,25 +141,26 @@
},
hardfork: undefined as string | undefined,
hardforks: [] as ConfigHardfork[],
customHardforks,
bootstrapNodes: [],
consensus:
config.clique !== undefined
? {
type: 'poa',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this code block moved?

algorithm: 'clique',
clique: {
// The recent geth genesis seems to be using blockperiodseconds // cspell:disable-line
// and epochlength for clique specification
// see: https://hackmd.io/PqZgMpnkSWCWv5joJoFymQ
period: config.clique.period ?? config.clique.blockperiodseconds, // cspell:disable-line
epoch: config.clique.epoch ?? config.clique.epochlength,
},
}
: {
type: 'pow',
algorithm: 'ethash',
ethash: {},
type: 'poa',
algorithm: 'clique',
clique: {
// The recent geth genesis seems to be using blockperiodseconds // cspell:disable-line
// and epochlength for clique specification
// see: https://hackmd.io/PqZgMpnkSWCWv5joJoFymQ
period: config.clique.period ?? config.clique.blockperiodseconds, // cspell:disable-line
epoch: config.clique.epoch ?? config.clique.epochlength,
},
}
: {
type: 'pow',
algorithm: 'ethash',
ethash: {},
},
}

const forkMap: { [key: string]: { name: string; postMerge?: boolean; isTimestamp?: boolean } } = {
Expand Down
81 changes: 80 additions & 1 deletion packages/common/test/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe('[Utils/Parse]', () => {

it('should correctly parse deposit contract address', async () => {
// clone json out to not have side effects
const customData = postMergeHardforkData
const customData = JSON.parse(JSON.stringify(postMergeHardforkData))
Object.assign(customData.config, {
depositContractAddress: '0x4242424242424242424242424242424242424242',
})
Expand All @@ -108,4 +108,83 @@ describe('[Utils/Parse]', () => {
'should parse correct address',
)
})

it('should assign correct blob schedule', async () => {
// clone json out to not have side effects
const customData = JSON.parse(JSON.stringify(postMergeHardforkData))
Object.assign(customData.config, {
"chainId": 3151908,
"homesteadBlock": 0,
"eip150Block": 0,
"eip155Block": 0,
"eip158Block": 0,
"byzantiumBlock": 0,
"constantinopleBlock": 0,
"petersburgBlock": 0,
"istanbulBlock": 0,
"berlinBlock": 0,
"londonBlock": 0,
"mergeNetsplitBlock": 0,
"depositContractAddress": "0x4242424242424242424242424242424242424242",
"terminalTotalDifficulty": 0,
"terminalTotalDifficultyPassed": true,
"shanghaiTime": 0,
"cancunTime": 0,
"blobSchedule": {
"prague": {
"target": 61,
"max": 91,
"baseFeeUpdateFraction": 13338477
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
},
"pragueTime": 1736942378,
});

const common = createCommonFromGethGenesis(customData, {
chain: 'customChain',
});
const paramsTx = {
4844: {
blobCommitmentVersionKzg: 1, // The number indicated a versioned hash is a KZG commitment
blobGasPerBlob: 131072, // The base fee for blob gas per blob
maxBlobGasPerBlock: 786432, // The max blob gas allowable per block
blobGasPriceUpdateFraction: 3338477,
targetBlobGasPerBlock: 393216,
},
7691: {
maxBlobGasPerBlock: 1179648, // The max blob gas allowable per block
},
}
common.updateParams(paramsTx);

const testCases = [
// should be picked from eip params
[Hardfork.Cancun, 393216n, 786432n, 3338477n],
// from the genesis blobschedule
[Hardfork.Prague, 7995392, 11927552, 13338477],
]
for (const [testHf, testTarget, testMax, testUpdateFraction] of testCases) {
common.setHardfork(testHf as Hardfork);

const targetBlobGasPerBlock = common.param("targetBlobGasPerBlock");
const maxBlobGasPerBlock = common.param("maxBlobGasPerBlock");
const blobGasPriceUpdateFraction = common.param("blobGasPriceUpdateFraction");

assert.equal(
targetBlobGasPerBlock,
testTarget,
'target blob gas should match',
)
assert.equal(
maxBlobGasPerBlock,
testMax,
'max blob gas should match',
)
assert.equal(
blobGasPriceUpdateFraction,
testUpdateFraction,
'update fraction should match',
)
}
})
})
Loading