Add Changes to BlobSidecars Endpoint #4455

Gua00va · 2023-07-01T07:28:06Z

Issue Addressed

Addresses Issue #4317
Update the getBlobSidecars endpoint as of ethereum/beacon-APIs#286.

path changed from beacon/blobs to beacon/blob_sidecars
new indices query parameter

realbigsean · 2023-07-05T16:03:28Z

Hey @Gua00va , I cherry-picked the renaming changes into here because it sounds like there's a team relying on the rename (blobscan?). Thank you for the fix!

Gua00va · 2023-07-14T19:29:54Z

Hey @Gua00va , I cherry-picked the renaming changes into here because it sounds like there's a team relying on the rename (blobscan?). Thank you for the fix!

Hey @realbigsean, only the rename has been merged, right? The query part is yet to be added.

Gua00va · 2023-07-20T04:32:59Z

Hey! @jimmygchen, I have added the required changes. But I think my work lacks code quality. Do you have any suggestions to improve it?

jimmygchen · 2023-07-20T05:48:41Z

beacon_node/http_api/src/lib.rs

+                            let mut list = Vec::new();
+                            for i in vec.iter() {
+                                list.push(blob_sidecar_list.get(*i as usize).unwrap().clone());
+                            }


A few suggestions:
1 . Blobs are quite big so we'd want to avoid cloning as much as possible. Instead of iterating using iter, you could use .into_iter (which moves and consumes the original values), and then use .filter to retain the requested indices, and finally collect the blob sidecar(s) into a vec using .collect.
2. Always avoid using unwrap in production code, as it can panic and cause Lighthouse to crash. Generally we handle the error by either logging or returning an error.

With this current logic, if user sends 1,1,1 as inputs, it would result in the endpoint returning 3 blob sidecars. But I think it you go with using the above approach and iterate using blob_sidecar_list.into_iter(), this shouldn't be an issue.

A small bonus would be to move this filtering logic into a function in block_id.rs, to keep this already huge file from growing further.

Thanks for the feedback !

…into blob-endpoint

jimmygchen · 2023-07-21T01:37:25Z

beacon_node/http_api/src/lib.rs

+                    //     None => {
+                    //         blob_sidecar_list
+                    //     },
+                    // };    


Please remove commented code.

Just a minor comment on naming of blob_sidecar_list_indexed, I think it's slightly confusing because it's just a list (potentially filtered), maybe blob_sidecar_list_filtered or something similar would be clearer?

jimmygchen · 2023-07-21T01:46:28Z

beacon_node/http_api/src/block_id.rs

+            Some(vec) => {
+                let list = blob_sidecar_list.into_iter()
+                .enumerate()
+                .filter(|(index, _)| vec.contains(index))


I think it would be more accurate to filter using the blob_sidecar.index instead, in case if it's somehow not the same order in the list. This means we can also get rid of .enumerate and map.

jimmygchen · 2023-07-21T01:49:39Z

beacon_node/http_api/src/block_id.rs

+                .map(|(_, blob)| blob)
+                .collect();
+                let indexed_list = BlobSidecarList::new(list);
+                indexed_list.unwrap()


Although this is an unexpected scenario, we should always avoid using unwrap in production code. In this case it's probably best to return a server error, something like:

BlobSidecarList::new(list) .map_err(|e| warp_utils::reject::custom_server_error(format!("{e:?}")))?

The map_err function maps the ssz Error type into the required warp::Rejection error type, and the ? allow the error to be automatically propagated / returned.

jimmygchen · 2023-07-21T01:53:47Z

beacon_node/http_api/src/block_id.rs

+        let blob_sidecar_list_indexed = match indices.indices {
+            Some(vec) => {
+                let list = blob_sidecar_list.into_iter()
+                .enumerate()


The code formatting seems a bit off here and will fail our CI tests, you can fix it by running

cargo fmt --all

You may also want to run clippy to ensure the changes passes our CI. Some guidelines here in case you haven't seen it:
https://lighthouse-book.sigmaprime.io/contributing.html?highlight=fmt#rust

We'll kick off a CI workflow once this is fixed!

jimmygchen · 2023-07-21T01:55:06Z

Hey @Gua00va, thanks for the quick fixes! 🙏 I've left a few more comments.

jimmygchen · 2023-07-21T06:14:53Z

beacon_node/http_api/src/block_id.rs

+                let filtered_list = BlobSidecarList::new(list)
+                    .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)));
+                filtered_list.unwrap()


The .unwrap call can be removed and return the error to the caller using ?

Suggested change

let filtered_list = BlobSidecarList::new(list)

.map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)));

filtered_list.unwrap()

BlobSidecarList::new(list)

.map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))?

jimmygchen · 2023-07-21T06:19:00Z

common/eth2/src/types.rs

@@ -648,6 +648,12 @@ pub struct ValidatorBalancesQuery {
    pub id: Option<Vec<ValidatorId>>,
 }

+#[derive(Clone, Deserialize)]


Might be worth adding a deny_unknown_fields attribute here for consistency with other endpoints and return error if user provided some unknown parameters.

Suggested change

#[derive(Clone, Deserialize)]

#[derive(Clone, Deserialize)]

#[serde(deny_unknown_fields)]

jimmygchen

Looks great, we're close! Just two small comments and we should be good 👍

realbigsean

Nice!

changed name

5c03546

Gua00va marked this pull request as draft July 1, 2023 07:29

Fix sidecars

752ad83

Added query type and parrameter

de02a55

Gua00va marked this pull request as ready for review July 14, 2023 19:26

Gua00va added 2 commits July 15, 2023 22:55

added query struct and function

95ad1a9

added method

c076093

Gua00va changed the title ~~[WIP] Add Changes to BlobSidecars Endpoint~~ Add Changes to BlobSidecars Endpoint Jul 18, 2023

Merge branch 'deneb-free-blobs' into blob-endpoint

5a4e18f

jimmygchen reviewed Jul 20, 2023

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. HTTP-API deneb labels Jul 20, 2023

Gua00va added 3 commits July 20, 2023 19:49

improved filtering method

8cbf6c9

Merge branch 'blob-endpoint' of https://github.com/Gua00va/lighthouse …

0a1c1e5

…into blob-endpoint

added blob_sidecar_list_indexed to block_id

0e3edda

Gua00va requested a review from jimmygchen July 20, 2023 16:31

minor blobqueryindex fix

57f6157

jimmygchen reviewed Jul 21, 2023

View reviewed changes

Gua00va added 2 commits July 21, 2023 09:53

function and formatting fix

ba2b58f

minor function and naming fix

35327e0

Gua00va requested a review from jimmygchen July 21, 2023 04:39

jimmygchen reviewed Jul 21, 2023

View reviewed changes

jimmygchen requested changes Jul 21, 2023

View reviewed changes

minor changes

6dd9af6

jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 21, 2023

realbigsean approved these changes Jul 21, 2023

View reviewed changes

realbigsean merged commit f1f04bc into sigp:deneb-free-blobs Jul 21, 2023

realbigsean mentioned this pull request Jul 21, 2023

Update getBlobSidecars endpoint path and add indices query parameter #4317

Closed

protolambda mentioned this pull request Jan 23, 2024

Blob sidecar indices-filter does not work #5107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Changes to BlobSidecars Endpoint #4455

Add Changes to BlobSidecars Endpoint #4455

Gua00va commented Jul 1, 2023 •

edited

Loading

realbigsean commented Jul 5, 2023

Gua00va commented Jul 14, 2023 •

edited

Loading

Gua00va commented Jul 20, 2023

jimmygchen Jul 20, 2023

jimmygchen Jul 20, 2023

Gua00va Jul 20, 2023

jimmygchen Jul 21, 2023

jimmygchen Jul 21, 2023 •

edited

Loading

jimmygchen Jul 21, 2023

jimmygchen Jul 21, 2023

jimmygchen commented Jul 21, 2023

jimmygchen Jul 21, 2023

jimmygchen Jul 21, 2023

jimmygchen left a comment

realbigsean left a comment

	#[derive(Clone, Deserialize)]
	#[derive(Clone, Deserialize)]
	#[serde(deny_unknown_fields)]

Add Changes to BlobSidecars Endpoint #4455

Add Changes to BlobSidecars Endpoint #4455

Conversation

Gua00va commented Jul 1, 2023 • edited Loading

Issue Addressed

realbigsean commented Jul 5, 2023

Gua00va commented Jul 14, 2023 • edited Loading

Gua00va commented Jul 20, 2023

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

jimmygchen Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmygchen commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmygchen left a comment

Choose a reason for hiding this comment

realbigsean left a comment

Choose a reason for hiding this comment

Gua00va commented Jul 1, 2023 •

edited

Loading

Gua00va commented Jul 14, 2023 •

edited

Loading

jimmygchen Jul 21, 2023 •

edited

Loading