remove backfill from eip 2935 and serve hashes from state when eip 7709 enabled #7140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -47,28 +39,12 @@ public void ApplyHistoryBlockHashes(BlockHeader blockHeader) | |||
| return null; | |||
| } | |||
| var blockIndex = new UInt256((ulong)(requiredBlockNumber % Eip2935Constants.RingBufferSize)); | |||
| Address? eip2935Account = spec.Eip2935ContractAddress ?? Eip2935Constants.BlockHashHistoryAddress; | |||
| Address eip2935Account = spec.Eip2935ContractAddress; | |||
There was a problem hiding this comment.
this should not be removed
|
NOTE: though 7709 is included here but the implementation is not complete yet - we dont charge the appropiate gas as its dependent on verkle. |
tanishqjasoria
changed the title
tanishqjasoria
requested changes
Jun 12, 2024
| @@ -28,7 +28,7 @@ public override void Setup() | |||
|
|
|||
|
|
|||
| [TestCase(MainnetSpecProvider.CancunBlockTimestamp, false)] | |||
| [TestCase(MainnetSpecProvider.PragueBlockTimestamp, true)] | |||
| [TestCase(MainnetSpecProvider.PragueBlockTimestamp, false)] | |||
| public void CorrectBlockhashBeingUsed(ulong timestamp, bool eipEnabled) | |||
There was a problem hiding this comment.
here eipEnabled=false does not make sense
eip2935 is enabled - just the expectation changed
i think this test can be removed or modify it for using 7709
| { | ||
| Hash256? result = provider.GetBlockhash(currentHeader, 0); | ||
| if (currentHeader.Number > Eip2935Constants.RingBufferSize) | ||
| if (currentHeader.Number > Eip2935Constants.RingBufferSize || spec.IsEip7709Enabled || currentHeader.Number > 256) |
There was a problem hiding this comment.
this condition does not make sense - i think this should be
(spec.IsEip7709Enabled && currentHeader.Number > Eip2935Constants.RingBufferSize) || currentHeader.Number > 256
tanishqjasoria
approved these changes
Jun 18, 2024
# Conflicts: # src/Nethermind/Nethermind.Consensus.Clique/CliquePlugin.cs # src/Nethermind/Nethermind.Consensus.Ethash/NethDevPlugin.cs # src/Nethermind/Nethermind.Consensus/Processing/ReadOnlyChainProcessingEnv.cs # src/Nethermind/Nethermind.Consensus/Producers/BlockProducerEnvFactory.cs # src/Nethermind/Nethermind.Optimism/OptimismBlockProducerEnvFactory.cs # src/Nethermind/Nethermind.Optimism/ReadOnlyChainProcessingEnv.cs
rjnrohit
approved these changes
Jun 21, 2024
LukaszRozmej
approved these changes
Jun 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Eip2935 is going to collect block hashes in the state, when Eip7709 gets enabled it will start serving collected block hashes from the state.
Before: if eip2935IsEnabled we stored the last 8192 hashes in the state and served last 256 block hashes from state on request
In this PR: if eip2935IsEnabled we store parent block hash in the state and if eip7709IsEnabled serve last 256 block hashes from state
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
There is no tests for Eip7709, tests that confirm blockhashes being served from the state
Eip2935 tests cannot test that blockhashes stored in the state
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.