-
Notifications
You must be signed in to change notification settings - Fork 957
Revise logging in BlobsByRoot requests #8296
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
Conversation
| request: BlobsByRootRequest, | ||
| ) -> Result<(), (RpcErrorResponse, &'static str)> { | ||
| let requested_roots: Vec<Hash256> = | ||
| request.blob_ids.iter().map(|id| id.block_root).collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note that the behaviour before #7756 is to always return the first block_root
| let Some(requested_root) = request.blob_ids.as_slice().first().map(|id| id.block_root) |
A (possibly) desirable outcome from this is, we can deduce to know that if we are rejecting BlobsByRoot request after Fulu slots, for example, if returned = 0, then we can look for the block_root(s) and check if they are all Fulu, and if they are, then we know we don't return anything
| inbound_request_id: InboundRequestId, | ||
| request: BlobsByRootRequest, | ||
| ) -> Result<(), (RpcErrorResponse, &'static str)> { | ||
| let requested_roots: Vec<Hash256> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use HashSet here to deduplicate block roots?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, change in b5d7afd
jimmygchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Issue Addressed
#7756 introduces a logging issue, where the relevant log:
lighthouse/beacon_node/network/src/network_beacon_processor/rpc_methods.rs
Lines 380 to 385 in da5b231
obtains the
block_rootfromslots_by_block_root.keys(). If theblock_rootis empty (block not found in the data availability checker), then the log will not show any block root:DEBUG BlobsByRoot outgoing response processed peer_id: 16Uiu2HAmCBxs1ZFfsbAfhSA98rUUL8Q1egLPb6WpGdKZxX6HqQYX, block_root: [], returned: 4This PR revises to return the
block_rootin the request as a vector of block root