RPC RequestId Cleanup (#7238)

I've been working at updating another library to latest Lighthouse and got very confused with RPC request Ids.

There were types that had fields called `request_id` and `id`. And interchangeably could have types `PeerRequestId`, `rpc::RequestId`, `AppRequestId`, `api_types::RequestId` or even `Request.id`.

I couldn't keep track of which Id was linked to what and what each type meant.

So this PR mainly does a few things:
- Changes the field naming to match the actual type. So any field that has an  `AppRequestId` will be named `app_request_id` rather than `id` or `request_id` for example.
- I simplified the types. I removed the two different `RequestId` types (one in Lighthouse_network the other in the rpc) and grouped them into one. It has one downside tho. I had to add a few unreachable lines of code in the beacon processor, which the extra type would prevent, but I feel like it might be worth it. Happy to add an extra type to avoid those few lines.
- I also removed the concept of `PeerRequestId` which sometimes went alongside a `request_id`. There were times were had a `PeerRequest` and a `Request` being returned, both of which contain a `RequestId` so we had redundant information. I've simplified the logic by removing `PeerRequestId` and made a `ResponseId`. I think if you look at the code changes, it simplifies things a bit and removes the redundant extra info.

I think with this PR things are a little bit easier to reasonable about what is going on with all these RPC Ids.

NOTE: I did this with the help of AI, so probably should be checked
This commit is contained in:
Age Manning
2025-04-03 21:10:15 +11:00
committed by GitHub
parent 80626e58d2
commit d6cd049a45
16 changed files with 438 additions and 739 deletions

View File

@@ -108,7 +108,7 @@ pub enum SyncMessage<E: EthSpec> {
/// A block has been received from the RPC.
RpcBlock {
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
peer_id: PeerId,
beacon_block: Option<Arc<SignedBeaconBlock<E>>>,
seen_timestamp: Duration,
@@ -116,7 +116,7 @@ pub enum SyncMessage<E: EthSpec> {
/// A blob has been received from the RPC.
RpcBlob {
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
peer_id: PeerId,
blob_sidecar: Option<Arc<BlobSidecar<E>>>,
seen_timestamp: Duration,
@@ -124,7 +124,7 @@ pub enum SyncMessage<E: EthSpec> {
/// A data columns has been received from the RPC
RpcDataColumn {
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
peer_id: PeerId,
data_column: Option<Arc<DataColumnSidecar<E>>>,
seen_timestamp: Duration,
@@ -153,7 +153,7 @@ pub enum SyncMessage<E: EthSpec> {
/// An RPC Error has occurred on a request.
RpcError {
peer_id: PeerId,
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
error: RPCError,
},
@@ -477,9 +477,9 @@ impl<T: BeaconChainTypes> SyncManager<T> {
}
/// Handles RPC errors related to requests that were emitted from the sync manager.
fn inject_error(&mut self, peer_id: PeerId, request_id: SyncRequestId, error: RPCError) {
fn inject_error(&mut self, peer_id: PeerId, sync_request_id: SyncRequestId, error: RPCError) {
trace!("Sync manager received a failed RPC");
match request_id {
match sync_request_id {
SyncRequestId::SingleBlock { id } => {
self.on_single_block_response(id, peer_id, RpcEvent::RPCError(error))
}
@@ -509,8 +509,8 @@ impl<T: BeaconChainTypes> SyncManager<T> {
fn peer_disconnect(&mut self, peer_id: &PeerId) {
// Inject a Disconnected error on all requests associated with the disconnected peer
// to retry all batches/lookups
for request_id in self.network.peer_disconnected(peer_id) {
self.inject_error(*peer_id, request_id, RPCError::Disconnected);
for sync_request_id in self.network.peer_disconnected(peer_id) {
self.inject_error(*peer_id, sync_request_id, RPCError::Disconnected);
}
// Remove peer from all data structures
@@ -751,25 +751,27 @@ impl<T: BeaconChainTypes> SyncManager<T> {
self.add_peers_force_range_sync(&peers, head_root, head_slot);
}
SyncMessage::RpcBlock {
request_id,
sync_request_id,
peer_id,
beacon_block,
seen_timestamp,
} => {
self.rpc_block_received(request_id, peer_id, beacon_block, seen_timestamp);
self.rpc_block_received(sync_request_id, peer_id, beacon_block, seen_timestamp);
}
SyncMessage::RpcBlob {
request_id,
sync_request_id,
peer_id,
blob_sidecar,
seen_timestamp,
} => self.rpc_blob_received(request_id, peer_id, blob_sidecar, seen_timestamp),
} => self.rpc_blob_received(sync_request_id, peer_id, blob_sidecar, seen_timestamp),
SyncMessage::RpcDataColumn {
request_id,
sync_request_id,
peer_id,
data_column,
seen_timestamp,
} => self.rpc_data_column_received(request_id, peer_id, data_column, seen_timestamp),
} => {
self.rpc_data_column_received(sync_request_id, peer_id, data_column, seen_timestamp)
}
SyncMessage::UnknownParentBlock(peer_id, block, block_root) => {
let block_slot = block.slot();
let parent_root = block.parent_root();
@@ -845,9 +847,9 @@ impl<T: BeaconChainTypes> SyncManager<T> {
}
SyncMessage::RpcError {
peer_id,
request_id,
sync_request_id,
error,
} => self.inject_error(peer_id, request_id, error),
} => self.inject_error(peer_id, sync_request_id, error),
SyncMessage::BlockComponentProcessed {
process_type,
result,
@@ -1018,12 +1020,12 @@ impl<T: BeaconChainTypes> SyncManager<T> {
fn rpc_block_received(
&mut self,
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
peer_id: PeerId,
block: Option<Arc<SignedBeaconBlock<T::EthSpec>>>,
seen_timestamp: Duration,
) {
match request_id {
match sync_request_id {
SyncRequestId::SingleBlock { id } => self.on_single_block_response(
id,
peer_id,
@@ -1060,12 +1062,12 @@ impl<T: BeaconChainTypes> SyncManager<T> {
fn rpc_blob_received(
&mut self,
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
peer_id: PeerId,
blob: Option<Arc<BlobSidecar<T::EthSpec>>>,
seen_timestamp: Duration,
) {
match request_id {
match sync_request_id {
SyncRequestId::SingleBlob { id } => self.on_single_blob_response(
id,
peer_id,
@@ -1084,12 +1086,12 @@ impl<T: BeaconChainTypes> SyncManager<T> {
fn rpc_data_column_received(
&mut self,
request_id: SyncRequestId,
sync_request_id: SyncRequestId,
peer_id: PeerId,
data_column: Option<Arc<DataColumnSidecar<T::EthSpec>>>,
seen_timestamp: Duration,
) {
match request_id {
match sync_request_id {
SyncRequestId::DataColumnsByRoot(req_id) => {
self.on_data_columns_by_root_response(
req_id,