-
Notifications
You must be signed in to change notification settings - Fork 434
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
[WIP]Make chainspec extendable by plugins #7540
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the direction is good, but there is some details to iron out.
// 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bytes.FromHexString
AsSpan().Trim() if needed
value = null; | ||
return false; | ||
} | ||
switch (Type.GetTypeCode(type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert.ChangeType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont understand what you mean
private readonly long? _bedrockBlockNumber = parameters.BedrockBlockNumber; | ||
private readonly ulong? _regolithTimestamp = parameters.RegolithTimestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to make stuff nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be no value in config, so better to make it nullable imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deffrian shouldn't we always have a value for these hardforks?
JsonProperty? jsonProperty = _chainSpecParameters[engineName].EnumerateObject().FirstOrDefault(p => | ||
string.Compare(p.Name, property.Name, StringComparison.InvariantCultureIgnoreCase) == 0); | ||
|
||
if (jsonProperty is null) | ||
{ | ||
throw new FormatException($"ChainSpec property {property.Name} is not set"); | ||
} | ||
|
||
object value = ConfigSourceHelper.ParseValue(property.PropertyType, jsonProperty.Value.Value.ToString(), "chainspec", property.Name); | ||
property.SetValue(instance, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use json deserialization here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought best idea is to do th same way we do in config. Just extending capabilities for chainspec
private void InitializeInstances() | ||
{ | ||
Type type = typeof(IChainSpecEngineParameters); | ||
IEnumerable<Type> types = TypeDiscovery.FindNethermindBasedTypes(type).Where(x => x.IsClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want auto-discovery or explicit registration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Transaction type refactoring we've been working with explicit registration. This should make transitioning to a DI approach easier but makes the immediate code harder since you need to register at the appropriate time, a lot of times directly into static
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same argument, do as in config. But I don't mind both tbh
{ | ||
Type type = typeof(IChainSpecEngineParameters); | ||
IEnumerable<Type> types = TypeDiscovery.FindNethermindBasedTypes(type).Where(x => x.IsClass); | ||
foreach (Type @class in types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instances should be constructed lazily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Construction is done on setup, so performance is not a concern(it takes small amount of time). I prefer to catch any miss-configuration errors at this point.
src/Nethermind/Nethermind.Specs/ChainSpecStyle/IChainSpecEngineParameters.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecParametersProvider.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Lukasz Rozmej <[email protected]>
Co-authored-by: Lukasz Rozmej <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions regarding nullability and discovery, but overall LGTM.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
private readonly Dictionary<string, JsonElement> _chainSpecParameters = | ||
new(StringComparer.InvariantCultureIgnoreCase); | ||
|
||
private readonly ConcurrentDictionary<Type, IChainSpecEngineParameters> _instances = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this have to be concurrent? We cannot add elements through AllChainSpecParameters
and there is no other accessor.
public interface IChainSpecParametersProvider | ||
{ | ||
string SealEngineType { get; } | ||
ICollection<IChainSpecEngineParameters> AllChainSpecParameters { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't intend to allow modification (which I think we don't) prefer IEnumerable
to ICollection
{ | ||
ArgumentNullException.ThrowIfNull(RegolithTimestamp); | ||
ArgumentNullException.ThrowIfNull(BedrockBlockNumber); | ||
ArgumentNullException.ThrowIfNull(CanyonTimestamp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
…tersProvider.cs Co-authored-by: Lautaro Emanuel <[email protected]>
Co-authored-by: Lukasz Rozmej <[email protected]>
Co-authored-by: Lukasz Rozmej <[email protected]>
…tersProvider.cs Co-authored-by: Lautaro Emanuel <[email protected]>
32cbfa4
to
b6f1a53
Compare
…ctor/chainspec-v2
# Conflicts: # src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs
# Conflicts: # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs
# Conflicts: # src/Nethermind/Nethermind.Merge.Plugin/MergePlugin.cs
# Conflicts: # src/Nethermind/Nethermind.Config/ConfigSourceHelper.cs
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 toIConfig
.ReleaseSpec
creation we callIChainSpecEngineParameters.AddTransitions
. Plugin devs write implementation of this function if they want to add transitions.ReleaseSpec
created we callIChainSpecEngineParameters.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?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs
. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.