Make chainspec extendable by plugins #7540

deffrian · 2024-10-02T13:32:14Z

Fixes Closes Resolves #
#6222
Right now our ChainSpec is a mess. This pr tries to fix some issues.

Changes

Adds new interface IChainSpecEngineParameters. This works similar to IConfig.

In runtime we search for all inheritors of this interface and create instances. We set corresponding fields from chainspec file.
During ReleaseSpec creation we call IChainSpecEngineParameters.AddTransitions. Plugin devs write implementation of this function if they want to add transitions.
After all ReleaseSpec created we call IChainSpecEngineParameters.AdjustReleaseSpec. Plugin devs write implementation of this function if they want to change any parameters in ReleaseSpec.

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

LukaszRozmej

I think the direction is good, but there is some details to iron out.

LukaszRozmej · 2024-10-03T08:11:13Z

src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

+                        // hex encoded byte array
+                        string hex = valueString.Trim().RemoveStart('0').RemoveStart('x').TrimEnd();
+                        value = Enumerable.Range(0, hex.Length)
+                            .Where(x => x % 2 == 0)
+                            .Select(x => Convert.ToByte(hex.Substring(x, 2), 16))
+                            .ToArray();


Bytes.FromHexString
AsSpan().Trim() if needed

src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

LukaszRozmej · 2024-10-03T08:19:43Z

src/Nethermind/Nethermind.Optimism/OPConfigHelper.cs

+    private readonly long? _bedrockBlockNumber = parameters.BedrockBlockNumber;
+    private readonly ulong? _regolithTimestamp = parameters.RegolithTimestamp;


Why do we have to make stuff nullable?

There might be no value in config, so better to make it nullable imo

@deffrian shouldn't we always have a value for these hardforks?

On op-sepolia which actually does not have a bedrock block we're using 0x0, for example.

I'm in favor of letting it default to 0 rather than making it null

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParametersProvider.cs

src/Nethermind/Nethermind.Specs/ChainSpecStyle/IChainSpecEngineParameters.cs

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParametersProvider.cs

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>

emlautarom1

Some questions regarding nullability and discovery, but overall LGTM.

emlautarom1 · 2024-10-03T14:47:08Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParametersProvider.cs

+        IEnumerable<Type> types = TypeDiscovery.FindNethermindBasedTypes(type).Where(x => x.IsClass);
+        foreach (Type @class in types)
+        {
+            string engineName = @class.Name.Remove(@class.Name.Length - EngineParamsSuffix.Length);


Could we do the discovery based on implementing IChainSpecEngineParameters rather than class name? Even if we go with the manual registration route we could perform this check.

Not sure what you mean. It already looks for all inheritors of IChainSpecEngineParameters. I didn't make manual registration because ChainSpec is more similar to IConfig. So I thought we should behave the same way here

My question was more in line with replacing the class name logic with something more "typesafe": right now it seems like I need to follow a specific naming convention but it's not enforced statically, and to figure it out I need to take a look into this implementation.

If I were to add another IChainSpecEngineParameters class, what would the requirements be and how would I figure them out?

I think what Lautaro means is that we should have a property IChainSpecEngineParamerters.Name or something similar, instead of using the name of the class. I understand @deffrian's point that we're being consistent with how IConfig works. But maybe we should start improving both.

I think what Lautaro means is that we should have a property IChainSpecEngineParamerters.Name or something similar, instead of using the name of the class.

Precisely, I just didn't want to push for any particular approach.

we're being consistent with how IConfig works

Didn't know that we were using the class name approach there. If it is consistent with the existing style then let's move forward, but we might want to add a TODO to eventually improve (that is, move to the type system) these kind of requirements.

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParametersProvider.cs

emlautarom1 · 2024-10-03T15:09:48Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParametersProvider.cs

+    private readonly Dictionary<string, JsonElement> _chainSpecParameters =
+        new(StringComparer.InvariantCultureIgnoreCase);
+
+    private readonly ConcurrentDictionary<Type, IChainSpecEngineParameters> _instances = new();


Why does this have to be concurrent? We cannot add elements through AllChainSpecParameters and there is no other accessor.

emlautarom1 · 2024-10-03T15:13:19Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/IChainSpecParametersProvider.cs

+public interface IChainSpecParametersProvider
+{
+    string SealEngineType { get; }
+    ICollection<IChainSpecEngineParameters> AllChainSpecParameters { get; }


If we don't intend to allow modification (which I think we don't) prefer IEnumerable to ICollection

emlautarom1 · 2024-10-03T15:16:47Z

src/Nethermind/Nethermind.Optimism/OptimismChainSpecEngineParameters.cs

+    {
+        ArgumentNullException.ThrowIfNull(RegolithTimestamp);
+        ArgumentNullException.ThrowIfNull(BedrockBlockNumber);
+        ArgumentNullException.ThrowIfNull(CanyonTimestamp);


Some of these fields (like CanyonTimestamp) used to be nullable and still are here, but others have changed (like RegolithTimestamp).
Which fields are actually required and which are not?

src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

…tersProvider.cs Co-authored-by: Lautaro Emanuel <31224949+emlautarom1@users.noreply.github.com>

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>

…tersProvider.cs Co-authored-by: Lautaro Emanuel <31224949+emlautarom1@users.noreply.github.com>

…ctor/chainspec-v2

# Conflicts: # src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs

# Conflicts: # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

# Conflicts: # src/Nethermind/Nethermind.Core/SealEngineType.cs # src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs # src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecLoader.cs # src/Nethermind/Nethermind.Specs/ChainSpecStyle/Json/ChainSpecJson.cs

emlautarom1

Some comments before approving

src/Nethermind/Nethermind.Specs/ChainSpecStyle/IChainSpecEngineParameters.cs

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpec.cs

emlautarom1 · 2024-10-28T16:17:04Z

src/Nethermind/Nethermind.Optimism/OptimismChainSpecEngineParameters.cs

+    {
+        if (CanyonTimestamp <= startTimestamp)
+        {
+            spec.BaseFeeMaxChangeDenominator = CanyonBaseFeeChangeDenominator!.Value;


Possible risk of null dereference?

src/Nethermind/Nethermind.Optimism/OptimismSynchronizerModule.cs

emlautarom1 · 2024-10-28T16:23:51Z

src/Nethermind/Nethermind.Optimism/OPConfigHelper.cs

+    private readonly long? _bedrockBlockNumber = parameters.BedrockBlockNumber;
+    private readonly ulong? _regolithTimestamp = parameters.RegolithTimestamp;


On op-sepolia which actually does not have a bedrock block we're using 0x0, for example.

I'm in favor of letting it default to 0 rather than making it null

emlautarom1 · 2024-10-28T17:49:16Z

src/Nethermind/Nethermind.Consensus.Ethash/EthashChainSpecEngineParameters.cs

+using System;
+using System.Buffers;
+using System.Collections.Generic;
+using System.Globalization;


Unused import

For comment on top: This is chainSpec, so better to force people to set 0 explicitly so they don't missconfigure it

src/Nethermind/Nethermind.Consensus.AuRa/Validators/AuRaParameters.cs

src/Nethermind/Nethermind.Consensus.AuRa/InitializationSteps/StartBlockProducerAuRa.cs

src/Nethermind/Nethermind.Consensus.AuRa/Config/AuRaChainSpecEngineParameters.cs

emlautarom1 · 2024-10-28T18:33:23Z

src/Nethermind/Nethermind.AuRa.Test/ChainSpecLoaderTest.cs

+    [Test]
+    public void Can_load_posdao_with_rewriteBytecode()
+    {
+        // TODO: modexp 2565


Context on this TODO?

Don't know. This is from the original test

# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.HelperFunctions.cs # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

emlautarom1

LGTM

LukaszRozmej

Almost done, can you check if we need those custom dictionary converters?

src/Nethermind/Nethermind.Consensus.AuRa/Config/AuRaChainSpecEngineParameters.cs

src/Nethermind/Nethermind.Specs/ChainSpecStyle/Json/LongUInt256DictionaryConverter.cs

src/Nethermind/Nethermind.Consensus.Ethash/EthashChainSpecEngineParameters.cs

LukaszRozmej · 2024-10-31T12:07:01Z

src/Nethermind/Nethermind.Core/SealEngineType.cs

-        public const string NethDev = nameof(NethDev);
        public const string Ethash = nameof(Ethash);
        public const string BeaconChain = nameof(BeaconChain);
-        public const string Optimism = nameof(Optimism);


I think we could keep them there for simplicity.

src/Nethermind/Nethermind.Optimism/OptimismSynchronizerModule.cs

LukaszRozmej · 2024-10-31T12:09:31Z

src/Nethermind/Nethermind.Runner.Test/EthereumRunnerTests.cs

+        // we need this to discover ChainSpecEngineParameters
+        new CliqueConfig();
+        new OptimismConfig();
+        new TaikoPlugin();


Can we do it in some more elegant way?

This is the only way I know

what are we trying to achieve here?

My assumption is that .net is not loading plugins if we don't use them. If you remove this it will not discover ChainSpecEngineParameters

Can we use TypeDiscovery class for that?

# Conflicts: # src/Nethermind/Nethermind.JsonRpc.Test/Modules/TraceRpcModuleTests.cs # src/Nethermind/Nethermind.Runner.Test/EthereumRunnerTests.cs

deffrian added 2 commits October 1, 2024 17:42

Custom chainspec engine

2ada5d8

Optimism & add more types to decoder

d9d4f2a

deffrian changed the title ~~Make chainspec extendable by plugins~~ [WIP]Make chainspec extendable by plugins Oct 2, 2024

jmederosalvarado self-requested a review October 2, 2024 16:42

LukaszRozmej reviewed Oct 3, 2024

View reviewed changes

LukaszRozmej requested a review from emlautarom1 October 3, 2024 08:34

deffrian and others added 2 commits October 3, 2024 12:19

Update src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

d61acc3

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>

Update src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

6ea1a10

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>

emlautarom1 reviewed Oct 3, 2024

View reviewed changes

deffrian and others added 10 commits October 5, 2024 13:00

Small fixes

33f8a6c

Update src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParame…

f7b8e32

…tersProvider.cs Co-authored-by: Lautaro Emanuel <31224949+emlautarom1@users.noreply.github.com>

Small fixes

32cbfa4

Custom chainspec engine

70d2227

Optimism & add more types to decoder

da4313b

Update src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

3c39505

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>

Update src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs

c44f8e8

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>

Small fixes

7f8ae03

Update src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParame…

be56036

…tersProvider.cs Co-authored-by: Lautaro Emanuel <31224949+emlautarom1@users.noreply.github.com>

Small fixes

b6f1a53

deffrian force-pushed the refactor/chainspec-v2 branch from 32cbfa4 to b6f1a53 Compare October 6, 2024 12:22

deffrian added 10 commits October 6, 2024 16:01

Merge remote-tracking branch 'origin/refactor/chainspec-v2' into refa…

bed5240

…ctor/chainspec-v2

NethDev

4b5d291

Clique

777c49a

Fix tests

4261b1e

Runner tests

2946160

Merge branch 'master' into refactor/chainspec-v2

6d0ece3

# Conflicts: # src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs

Fix build

ddbcc2d

Ethash & switch to json serialization

15cb899

Fix tests

a9dd55e

Merge branch 'master' into refactor/chainspec-v2

c3d296f

# Conflicts: # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

deffrian added 8 commits October 23, 2024 17:26

AuraParameters

300dd01

Remove comments

d488037

TODOs

5c40e24

Fix spec tests

24c91c1

Fix Taiko

407de4f

Formatting & tests

e39085b

EngineName

728ee3a

deffrian changed the title ~~[WIP]Make chainspec extendable by plugins~~ Make chainspec extendable by plugins Oct 27, 2024

deffrian marked this pull request as ready for review October 27, 2024 15:29

TODO

7e3e146

emlautarom1 reviewed Oct 28, 2024

View reviewed changes

deffrian added 2 commits October 30, 2024 13:30

Merge branch 'master' into refactor/chainspec-v2

3a004be

# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.HelperFunctions.cs # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

Fix suggestions

5030e93

emlautarom1 approved these changes Oct 30, 2024

View reviewed changes

LukaszRozmej added 2 commits October 31, 2024 12:43

refactors

92d7253

more refactors

c7b4d63

LukaszRozmej reviewed Oct 31, 2024

View reviewed changes

LukaszRozmej and others added 8 commits October 31, 2024 13:33

fix for strings

fed3fdd

Remove TestSealEngineType.cs and JsonConverter

bbff932

Merge branch 'master' into refactor/chainspec-v2

27de2f5

# Conflicts: # src/Nethermind/Nethermind.JsonRpc.Test/Modules/TraceRpcModuleTests.cs # src/Nethermind/Nethermind.Runner.Test/EthereumRunnerTests.cs

Rollback JsonConverter removal

d0b5b08

Handle null

9ea7527

Remove DifficultyBombDelaysConverter

c9c38bf

Merge branch 'master' into refactor/chainspec-v2

69d8c21

Make discovery of ChainSpecEngineParameters explicit

98c8178

LukaszRozmej approved these changes Nov 4, 2024

View reviewed changes

Merge branch 'master' into refactor/chainspec-v2

18a3619

deffrian merged commit c91e0b2 into master Nov 4, 2024
77 checks passed

deffrian deleted the refactor/chainspec-v2 branch November 4, 2024 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make chainspec extendable by plugins #7540

Make chainspec extendable by plugins #7540

deffrian commented Oct 2, 2024 •

edited

Loading

LukaszRozmej left a comment

LukaszRozmej Oct 3, 2024

LukaszRozmej Oct 3, 2024

deffrian Oct 3, 2024

jmederosalvarado Oct 9, 2024

emlautarom1 Oct 28, 2024

emlautarom1 left a comment

emlautarom1 Oct 3, 2024

deffrian Oct 6, 2024

emlautarom1 Oct 7, 2024

jmederosalvarado Oct 9, 2024

emlautarom1 Oct 9, 2024 •

edited

Loading

emlautarom1 Oct 3, 2024

emlautarom1 Oct 3, 2024

emlautarom1 Oct 3, 2024

emlautarom1 left a comment

emlautarom1 Oct 28, 2024

emlautarom1 Oct 28, 2024

emlautarom1 Oct 28, 2024

deffrian Oct 30, 2024

emlautarom1 Oct 28, 2024

deffrian Oct 30, 2024

emlautarom1 left a comment

LukaszRozmej left a comment

LukaszRozmej Oct 31, 2024

LukaszRozmej Oct 31, 2024

deffrian Nov 1, 2024

LukaszRozmej Nov 1, 2024

deffrian Nov 1, 2024

LukaszRozmej Nov 1, 2024

		private readonly long? _bedrockBlockNumber = parameters.BedrockBlockNumber;
		private readonly ulong? _regolithTimestamp = parameters.RegolithTimestamp;

Make chainspec extendable by plugins #7540

Make chainspec extendable by plugins #7540

Conversation

deffrian commented Oct 2, 2024 • edited Loading

Changes

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

LukaszRozmej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 left a comment

Choose a reason for hiding this comment

LukaszRozmej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deffrian commented Oct 2, 2024 •

edited

Loading

emlautarom1 Oct 9, 2024 •

edited

Loading