-
Notifications
You must be signed in to change notification settings - Fork 386
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
Fixes some hive test related to invalid hash #4102
Conversation
b0d5443
to
561c5c9
Compare
src/Nethermind/Nethermind.Merge.Plugin/Handlers/V1/NewPayloadV1Handler.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/InvalidChainTracker/InvalidChainTracker.cs
Show resolved
Hide resolved
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.
Potentially missing tests that InvalidChainTracker
is used correctly in all those places.
Despite quite a few comments on details I really like this approach and code!
src/Nethermind/Nethermind.Consensus/Processing/BlockchainProcessor.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Consensus/Processing/BlockchainProcessor.cs
Outdated
Show resolved
Hide resolved
|
||
_api.HeaderValidator = new InvalidHeaderInterceptor( | ||
new MergeHeaderValidator(_poSSwitcher, _api.BlockTree, _api.SpecProvider, _api.SealValidator, | ||
_api.LogManager), | ||
_invalidChainTracker, | ||
_api.LogManager); | ||
_api.UnclesValidator = new MergeUnclesValidator(_poSSwitcher, _api.UnclesValidator); | ||
_api.BlockValidator = new BlockValidator(_api.TxValidator, _api.HeaderValidator, _api.UnclesValidator, | ||
_api.SpecProvider, _api.LogManager); | ||
_api.BlockValidator = new InvalidBlockInterceptor( | ||
new BlockValidator(_api.TxValidator, _api.HeaderValidator, _api.UnclesValidator, | ||
_api.SpecProvider, _api.LogManager), | ||
_invalidChainTracker, | ||
_api.LogManager); | ||
_api.HealthHintService = | ||
new MergeHealthHintService(_api.HealthHintService, _poSSwitcher); | ||
_mergeBlockProductionPolicy = new MergeBlockProductionPolicy(_api.BlockProductionPolicy); | ||
_api.BlockProductionPolicy = _mergeBlockProductionPolicy; | ||
|
||
_api.FinalizationManager = new MergeFinalizationManager(_blockFinalizationManager, _api.FinalizationManager, _poSSwitcher); | ||
|
||
// Need to do it here because blockprocessor is not available in init | ||
_invalidChainTracker.SetupBlockchainProcessorInterceptor(_api.BlockchainProcessor!); |
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.
Maybe we should add InitBlockchain to plugins that is after InitBlockchain step? @MarekM25 WDYT?
src/Nethermind/Nethermind.Merge.Plugin/InvalidChainTracker/InvalidChainTracker.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/InvalidChainTracker/InvalidChainTracker.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/InvalidChainTracker/InvalidChainTracker.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/InvalidChainTracker/InvalidChainTracker.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/InvalidChainTracker/InvalidChainTracker.cs
Show resolved
Hide resolved
} | ||
|
||
Keccak effectiveParent = parent; | ||
BlockHeader? parentHeader = TryGetBlockHeader(parent); |
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.
just take in Headers in first place, they should be available in all scenarios: OnInvalidBlock(BlockHeader failedBlock, BlockHeader parent)
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.
Ok
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.
Problem, sometime it does not have the parent header. Eg, in case of newPayload where it's parent is not synced or missed.
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.
ok, but maybe lets try resolve parent upstream then?
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.
Upstream?
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.
Upstream - the code that is calling this code. Now with OnInvalidBlockArg I think it's not a problem to get ParentHeader from caller.
src/Nethermind/Nethermind.Merge.Plugin.Test/InvalidChainTrackerTest.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin.Test/InvalidChainTrackerTest.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Merge.Plugin.Test/InvalidChainTrackerTest.cs
Outdated
Show resolved
Hide resolved
561c5c9
to
d130647
Compare
Changes:
Types of changes