test: move 4844 tests to sim tests #6622
Merged
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6622 +/- ##
=========================================
Coverage 61.69% 61.69%
=========================================
Files 556 556
Lines 58820 58820
Branches 1887 1887
=========================================
Hits 36287 36287
Misses 22492 22492
Partials 41 41 |
Performance Report✔️ no performance regression detected Full benchmark results
|
|
Decided to split this PR to multiple, so temporally converting to draft. |
nazarhussain
force-pushed
the
nh/sim-4844
branch
from
309d8a0
to
4a7fd0f
Compare
matthewkeil
reviewed
Apr 9, 2024
There was a problem hiding this comment.
Silly comment but can we add JS to the Web3 portion of the file and function names, or perhaps go back to Eth1Provider naming so its a bit more clear what is happening in here. Makes sense after opening it but looking through the file structure in vscode it would be a bit less clear unless someone understood what it was. While i love file names to match the class they export we dont do that anywhere else in the codebase so we should probably match that and call the file web3JsPluginProvider or web3JsProvider or something similar. Up to you how you want to handle that though.
Comment on lines
+27
to
+32
| const blob = new Uint8Array(FIELD_ELEMENTS_PER_BLOB * BYTES_PER_FIELD_ELEMENT); | ||
| const dv = new DataView(blob.buffer, blob.byteOffset, blob.byteLength); | ||
| for (let i = 0; i < FIELD_ELEMENTS_PER_BLOB; i++) { | ||
| dv.setUint32(i * BYTES_PER_FIELD_ELEMENT, i); | ||
| } | ||
| return blob; |
There was a problem hiding this comment.
Why not just use return Uint8Array.from(crypto.randomBytes(FIELD_ELEMENTS_PER_BLOB * BYTES_PER_FIELD_ELEMENT)) here?
There was a problem hiding this comment.
I believe because that will be bigendian and we want the blob to be little-endian.
| @@ -18,9 +18,9 @@ export function createExecutionHeadAssertion({ | |||
| return AssertionMatch.None; | |||
| }, | |||
| async capture({node}) { | |||
| const blockNumber = await node.execution.provider?.getBlockNumber(); | |||
| const blockNumber = await node.execution.provider?.eth.getBlockNumber(); | |||
There was a problem hiding this comment.
I didn't notice the eth namespace in Web3Plugins.ts. Is that the default?
There was a problem hiding this comment.
That comes as default with the web3.js
| return blobSideCars.response.data.length; | ||
| }, | ||
|
|
||
| assert: async ({store}) => { |
There was a problem hiding this comment.
I know you said there is no reason to assert the blobs are correct but part of me thinks that seems incorrect. How do you know they are not getting included in the wrong block or that the commitments match?
matthewkeil
approved these changes
Apr 9, 2024
|
Had a partial review from before you broke out the other PR’s from this one. Some of those comments are still relevant though |
I want to cover few more scenarios as well, will do in separate PR. This PR is just converting existing assertions from old tests to sim tests, so that's fine. |
|
@matthewkeil I am merging this PR and open small PRs to address remaining comments. |
nflaig
pushed a commit
that referenced
this pull request
Apr 11, 2024
* Remove old 4844 sim test * Add support for 4844 tests in sim tests * Fix file path * Fix merge conflict issue
|
🎉 This PR is included in v1.18.0 🎉 |
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.
Motivation
Cover more scenarios in the sim tests.
Description
Steps to test or reproduce