Skip to content
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

Remove generic E from RequestId #6462

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Conversation

jxs
Copy link
Member

@jxs jxs commented Oct 3, 2024

Issue Addressed

Continue cleaning up rpc code by removing unrequired generic E from request id, and passing MAX_BLOBS_PER_BLOCK when constructing a BlobsByRange request

@jxs jxs force-pushed the remove-e-from-requestid branch 2 times, most recently from 96ef419 to a1f7761 Compare October 3, 2024 22:39
@@ -323,11 +314,14 @@ pub struct BlobsByRangeRequest {

/// The number of slots from the start slot.
pub count: u64,

/// maximum number of blobs in a single block.
pub max_blobs_per_block: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is the same on all requests, does it need to be appended to every request? Or can it be part of the handler struct

Copy link
Member Author

@jxs jxs Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Lion, good question, the thing is:

self.network_send
.send(NetworkMessage::SendRequest {
peer_id,
request: RequestType::BlobsByRange(BlobsByRangeRequest {
start_slot: *request.start_slot(),
count: *request.count(),
}),
request_id: AppRequestId::Sync(SyncRequestId::RangeBlockAndBlobs { id }),
})
.map_err(|_| RpcRequestSendError::NetworkSendError)?;

the request is created here for example, it's outside of the Rpc NetworkBehaviour.
Also BlobsByRangeRequest::max_blobs_requested(&self) requires it, how would you envision storing max_blobs_per_block?
My objective is for Rpc to be completely unaware of E: EthSpec so that we can uncouple it from the rest of lighthouse and all its config values are passed dynamically.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I take that back, if we increase the blob count we will have different requests with different limits at runtime. So okay with me to keep it on the request

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have added this field to BlobsByRangeRequest. It breaks the SSZ representation.

@AgeManning
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Oct 16, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 772929f

mergify bot added a commit that referenced this pull request Oct 16, 2024
@mergify mergify bot merged commit 772929f into sigp:unstable Oct 16, 2024
28 checks passed
michaelsproul added a commit to michaelsproul/lighthouse that referenced this pull request Oct 21, 2024
mergify bot pushed a commit that referenced this pull request Oct 21, 2024
* Revert "Remove generic E from RequestId (#6462)"

This reverts commit 772929f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants