Make EIP-4844 parameters configurable #6192
Conversation
src/Nethermind/Nethermind.Specs.Test/ChainSpecStyle/ChainSpecBasedSpecProviderTests.cs
Show resolved
Hide resolved
| @@ -40,11 +40,14 @@ | |||
| } | |||
| }, | |||
| "params": { | |||
| "blobGasPriceUpdateFraction": "0x10fafa", | |||
There was a problem hiding this comment.
very minor: maybe keep cancun values close to each other rather than alphabetic?
There was a problem hiding this comment.
I don't think it brings much value, as not everyone knows what parameter belongs to EIP-4844 or Cancun while it's always easy to check visually whether a particular parameter is defined.
| @@ -41,11 +41,14 @@ | |||
| } | |||
| }, | |||
| "params": { | |||
| "blobGasPriceUpdateFraction": "0x10fafa", | |||
There was a problem hiding this comment.
should we put into gnosis when we are not doing hardfork yet?
There was a problem hiding this comment.
What's your concern exactly? How can this be a problem?
| @@ -258,6 +259,14 @@ public void Chiado_loads_properly() | |||
| VerifyGnosisShanghaiExceptions(preShanghaiSpec, postShanghaiSpec); | |||
| GetTransitionTimestamps(chainSpec.Parameters).Should().AllSatisfy( | |||
| t => ValidateSlotByTimestamp(t, ChiadoSpecProvider.BeaconChainGenesisTimestamp, GnosisBlockTime).Should().BeTrue()); | |||
|
|
|||
| Assert.Multiple(() => | |||
There was a problem hiding this comment.
This test can still be Parallelizable?
There was a problem hiding this comment.
Well, nothing in NUnit docs indicates otherwise.
| public static void OverrideBlobGasPriceUpdateFraction(UInt256 value) => BlobGasPriceUpdateFraction = value; | ||
|
|
||
| public static void OverrideMaxBlobGasPerBlock(ulong value) => MaxBlobGasPerBlock = value; | ||
|
|
||
| public static void OverrideMinBlobGasPrice(UInt256 value) => MinBlobGasPrice = value; | ||
|
|
||
| public static readonly UInt256 BlobGasUpdateFraction = 3338477; | ||
| public static readonly UInt256 MinBlobGasPrice = 1; | ||
| public static void OverrideTargetBlobGasPerBlock(ulong value) => TargetBlobGasPerBlock = value; |
There was a problem hiding this comment.
Does it make sense for the mutators to be granular or maybe we should have just one taking all parameters?
There was a problem hiding this comment.
I also was thinking about this. If we move all mutators to a single method, we also move all null checks to that method and always call, say, Eip4844Constants.OverrideValues(...) in the chain spec loader. For a developer, it looks like we always override those values (until they dig into the method to see those checks), while I find it more explanatory when null checks are visible in the chain spec loader. So it's immediately obvious we override the values only if they are defined in the chain spec.
@LukaszRozmej As already has been discussed, I chose the same solution as we have for |
Changes
Made the following EIP-4844 parameters configurable with chain spec:
BLOB_GASPRICE_UPDATE_FRACTIONMAX_BLOB_GAS_PER_BLOCKMIN_BLOB_GASPRICETARGET_BLOB_GAS_PER_BLOCKThe default EIP-4844 parameters can be overridden by defining them in a chain spec file (e.g.,
gnosis.json). Otherwise, the hardcoded defaults are used.Refactored some parameter names to match the ones in EIP-4844.
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes