feat(ethportal-api): add `BeaconState` type #1150

ogenev · 2024-02-08T16:11:13Z

What was wrong?

To add support for Era files, we need to deserialize the beacon state. No BeaconState type is available in ethportal-api.

How was it fixed?

Implement BeaconState type

To-Do

Clean up commit history and use conventional commits.

KolbyML · 2024-02-15T08:57:34Z

I think these test files should go in portal-test-specs.

ogenev · 2024-02-19T08:03:44Z

I think these test files should go in portal-test-specs.

Hmm, not sure, those are test assets for ethereum beacon network, not related to portal network specs.

KolbyML · 2024-02-19T08:24:00Z

I think these test files should go in portal-test-specs.

Hmm, not sure, those are test assets for ethereum beacon network, not related to portal network specs.

The repo is just has spec in the name because of convention (execution-spec-tests and consensus-spec-tests) the repo is just for test data. Expessially with how big these test data files are. This is exactly why the portal-spec-test repo was made so we didn't need to have big test files in ethereum/trin and so other portal implementations could reuse the same test data. I am assuming since we are using these test data file other portal beacon implementations would also find value in it as well. Those reasons don't have to be mutually exlusive though to put test data there.

So the general direction should be a migration from test_assets -> portal-spec-tests for where we host our test data.

ogenev · 2024-02-19T09:05:40Z

The repo is just has spec in the name because of convention (execution-spec-tests and consensus-spec-tests) the repo is just for test data. Expessially with how big these test data files are. This is exactly why the portal-spec-test repo was made so we didn't need to have big test files in ethereum/trin and so other portal implementations could reuse the same test data. I am assuming since we are using these test data file other portal beacon implementations would also find value in it as well. Those reasons don't have to be mutually exlusive though to put test data there.

So the general direction should be a migration from test_assets -> portal-spec-tests for where we host our test data.

Those test files are extracted from consensus-spec-tests repo because I don't want to add this repo as a submodule - it is very huge, we want to use a few folders and use Git Large File Storage which will complicate our workflow.

Those files here are used for testing serialization/deserialization for the BeaconState which is part of the consensus specs and is used for deserializing Era files. I don't think that we want to migrate all test_assets to portal-spec-tests because most of those files are used to test Trin-specific implementations and not all of them are part of portal specs. I think portal-test-specs should be kept only for test assets for the portal network implementation which should not include consensus and execution layer test files.

The repo is just has spec in the name because of convention (execution-spec-tests and consensus-spec-tests) the repo is just for test data.

I don't think this is true. If we just want to have a repo for storing large files for testing in trin, it should be called trin-tests. Using the same convention as execution-spec-tests and consensus-spec-tests is not a coincidence, it is because we want to accomplish the same thing for the portal - store test assets and test vectors that are used for testing portal network specs.

@njgheorghita, @morph-dev what do you think?

morph-dev · 2024-02-19T09:34:48Z

I'm not sure I have enough knowledge of beacon network and these files to provide opinion on this specific case. I'm also missing broader context of what portal-spec-tests should be (and if it should be follow the same principles as execution-spec-tests and consensus-spec-tests). With that being said, I will still give my opinion.

My feeling is that portal-spec-tests should contain test files that can/should be used by all clients in order to test their implementation of the Portal Network. Are all clients expected to deserialize Era files?!
Even if it is not required by portal network spec, but if it is expected or useful in general context (e.g. in order to verify data, build bridge, etc.), I would say they can go there (e.g. under special directory, with README file as explanation).

I also believe that we probably have some files in test_assets that we don't want to move to portal-spec-tests because they are trin specific.

njgheorghita

A few questions about automatically deriving traits, just to double check that there's a good reason for manually implementing them, rather than automatically deriving them.

With regards to the test assets location...
I also don't have a clear understanding of the spec repo's defined purpose.

Is it strictly for client-agnostic json test vectors for clients to test against?

or...

Is it a dump for all large test files b/c we don't want to increase the size of trin?

This is probably something that needs defining, and inscribing in the readme's, so we don't have to repeat this debate for every future test vector. For the purpose of this pr, I think it's ok to move ahead as-is (eg. test files inside trin). Let's discuss this in the next sync, figure out how we want to define what test assets go where. And then if we do decide that these test vectors are better suited in the specs repo, then we can make that change later.

njgheorghita · 2024-02-19T14:57:01Z

ethportal-api/src/types/consensus/participation_flags.rs

+use tree_hash::{Hash256, PackedEncoding, TreeHash, TreeHashType};
+
+#[derive(Debug, Default, Clone, Copy, PartialEq, Deserialize, Serialize)]
+#[serde(transparent)]


Can you add a docstring here

ethportal-api/src/types/consensus/participation_flags.rs

njgheorghita · 2024-02-19T14:59:39Z

ethportal-api/src/types/consensus/beacon_state.rs

@@ -0,0 +1,312 @@
+#[cfg(test)]


Same reasoning as this comment .

njgheorghita · 2024-02-19T15:00:05Z

ethportal-api/src/types/consensus/beacon_state.rs

+use std::sync::Arc;
+use superstruct::superstruct;
+use tree_hash::{Hash256, TreeHash};
+use tree_hash_derive::TreeHash;


Don't we have a clippy lint for ordering imports into groups? eg. std first, exterior second, internal third

Not sure what ordering we use but this can be changed in the config. I think now it is internal first, std next and external latest.

Based on the rustfmt documentation, the default is (what we use) to preserve the source file's import groups.

Alternative, which we could enable, is closer to what @njgheorghita said:

std, core and alloc,

external crates,

self, super and crate imports.

Yeah, I think that would be nice to enable in the lint, since it is what we use throughout the majority of the codebase

njgheorghita · 2024-02-19T15:03:22Z

ethportal-api/src/types/consensus/beacon_state.rs

+}
+
+impl BeaconState {
+    #[cfg(test)]


Really? Why do we only want to ssz decode the state inside our tests? Is this to avoid the dead_code lint? If so, then I'd prefer to use allow[clippy::dead_code]

yes, it is to avoid the dead_code lint. I prefer this because that way we will not forget to remove the dead code attribute if it is not used.

Anyways, in #1167 I'm removing this.

ethportal-api/src/types/consensus/beacon_state.rs

ogenev force-pushed the support-era-files branch from 89ea6ea to 73241e8 Compare February 8, 2024 16:12

ogenev self-assigned this Feb 8, 2024

ogenev force-pushed the support-era-files branch 3 times, most recently from cae5d3e to f104d1f Compare February 13, 2024 09:27

ogenev changed the title ~~feat(ethportal-api): Add BeaconState type~~ feat(ethportal-api): add BeaconState type Feb 13, 2024

ogenev marked this pull request as ready for review February 13, 2024 13:35

ogenev requested review from njgheorghita and KolbyML February 13, 2024 13:35

ogenev mentioned this pull request Feb 14, 2024

feat(e2store): add support for Era files #1167

Merged

1 task

ogenev force-pushed the support-era-files branch from f104d1f to 9dddd60 Compare February 15, 2024 08:51

njgheorghita approved these changes Feb 19, 2024

View reviewed changes

ogenev force-pushed the support-era-files branch from 9dddd60 to 8c1efb8 Compare February 20, 2024 08:47

feat(ethportal-api): add BeaconState type

7887212

ogenev force-pushed the support-era-files branch from 8c1efb8 to 7887212 Compare February 20, 2024 09:17

ogenev merged commit 4daeb49 into ethereum:master Feb 20, 2024
8 checks passed

ogenev deleted the support-era-files branch February 20, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ethportal-api): add `BeaconState` type #1150

feat(ethportal-api): add `BeaconState` type #1150

ogenev commented Feb 8, 2024 •

edited

Loading

KolbyML commented Feb 15, 2024

ogenev commented Feb 19, 2024

KolbyML commented Feb 19, 2024

ogenev commented Feb 19, 2024 •

edited

Loading

morph-dev commented Feb 19, 2024

njgheorghita left a comment

njgheorghita Feb 19, 2024

njgheorghita Feb 19, 2024

ogenev Feb 20, 2024

njgheorghita Feb 19, 2024

ogenev Feb 20, 2024

morph-dev Feb 20, 2024

njgheorghita Feb 20, 2024

njgheorghita Feb 19, 2024

ogenev Feb 20, 2024

feat(ethportal-api): add BeaconState type #1150

feat(ethportal-api): add BeaconState type #1150

Conversation

ogenev commented Feb 8, 2024 • edited Loading

What was wrong?

How was it fixed?

To-Do

KolbyML commented Feb 15, 2024

ogenev commented Feb 19, 2024

KolbyML commented Feb 19, 2024

ogenev commented Feb 19, 2024 • edited Loading

morph-dev commented Feb 19, 2024

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(ethportal-api): add `BeaconState` type #1150

feat(ethportal-api): add `BeaconState` type #1150

ogenev commented Feb 8, 2024 •

edited

Loading

ogenev commented Feb 19, 2024 •

edited

Loading