-
Notifications
You must be signed in to change notification settings - Fork 441
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
Add OP Holocene fork #7761
Add OP Holocene fork #7761
Conversation
- WriteTo - IsZero
- Source tests from op-geth
- Would always result in validation errors
- Base and Optimism
/// </remarks> | ||
public sealed class OptimismBaseFeeCalculator( | ||
IBaseFeeCalculator baseFeeCalculator, | ||
ISpecProvider specProvider) : IBaseFeeCalculator |
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 don't see much point in having IOptimismSpecHelper
for starters, but could replace this ISpecProvider
with IOptimismSpecHelper
, and replace releaseSpec.GetSpec(parent).IsOpHoloceneEnabled
with helper.IsHolocene(parent)
.
If we don't use IOptimismSpecHelper
then its additions related to Holocene are essentially dead code (no usages).
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 still don't like the hidden dependency and side effect with BaseFeeCalculator.
I would change exceptions handling.
src/Nethermind/Nethermind.Optimism/OptimismBaseFeeCalculator.cs
Outdated
Show resolved
Hide resolved
var spec = _specProvider.GetSpec(currentBestBlock.Header); | ||
if (spec.IsOpHoloceneEnabled) | ||
{ | ||
EIP1559Parameters eip1559Parameters = optimismPayload.DecodeEIP1559Parameters(); |
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.
This could fail and throw exception
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 that at that point we've already validated the attributes so there should not be a risk of exceptions (unless I'm missing something):
- We validate attributes in
nethermind/src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs
Lines 89 to 95 in 4013c7a
public async Task<ResultWrapper<ForkchoiceUpdatedV1Result>> Handle(ForkchoiceStateV1 forkchoiceState, PayloadAttributes? payloadAttributes, int version) | |
{ | |
Block? newHeadBlock = GetBlock(forkchoiceState.HeadBlockHash); | |
return await ApplyForkchoiceUpdate(newHeadBlock, forkchoiceState, payloadAttributes) | |
?? ValidateAttributes(payloadAttributes, version) | |
?? StartBuildingPayload(newHeadBlock!, forkchoiceState, payloadAttributes); | |
} |
- Then we use the
IPreparationService
inStartBuildingPayload
nethermind/src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs
Line 327 in 4013c7a
payloadId = _payloadPreparationService.StartPreparingPayload(newHeadBlock.Header, payloadAttributes); |
- Eventually,
OptimismPayloadPreparationService.ImproveBlock
is called:
nethermind/src/Nethermind/Nethermind.Merge.Plugin/BlockProduction/PayloadPreparationService.cs
Lines 74 to 85 in 4013c7a
public string StartPreparingPayload(BlockHeader parentHeader, PayloadAttributes payloadAttributes) | |
{ | |
string payloadId = payloadAttributes.GetPayloadId(parentHeader); | |
if (!_payloadStorage.ContainsKey(payloadId)) | |
{ | |
Block emptyBlock = ProduceEmptyBlock(payloadId, parentHeader, payloadAttributes); | |
ImproveBlock(payloadId, parentHeader, payloadAttributes, emptyBlock, DateTimeOffset.UtcNow); | |
} | |
else if (_logger.IsInfo) _logger.Info($"Payload with the same parameters has already started. PayloadId: {payloadId}"); | |
return payloadId; | |
} |
nethermind/src/Nethermind/Nethermind.Optimism/OptimismPayloadPreparationService.cs
Lines 45 to 46 in 4013c7a
protected override void ImproveBlock(string payloadId, BlockHeader parentHeader, | |
PayloadAttributes payloadAttributes, Block currentBestBlock, DateTimeOffset startDateTime) |
} | ||
} | ||
|
||
public static class BaseFeeCalculator |
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 we could remove this static class altogether by leveraging extension methods but for now I'll leave it as it is to avoid more diffs.
src/Nethermind/Nethermind.Optimism/Rpc/OptimismPayloadAttributes.cs
Outdated
Show resolved
Hide resolved
This reverts commit cc96168.
Resolves #7760
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added tests that cover all new LOCs. Manual testing is still required.
Documentation
Requires documentation update
I don't think it's a requirement to update the docs.
Requires explanation in Release Notes
We should communicate that Nethermind is ready to support the new OP fork.
Remarks
Initially, only Sepolia timestamps are included for both Optimism and Base since those are the only ones currently known at this time.