-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: implement BeaconBlocksByRoot libp2p handler #471
feat: implement BeaconBlocksByRoot libp2p handler #471
Conversation
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. We should add the request to the list on LambdaEthereumConsensus.P2P.IncomingRequests.Receiver
, so peers can use it.
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.
Could you add a "unit" test that makes an interaction between this function and https://github.com/lambdaclass/lambda_ethereum_consensus/blob/main/lib/lambda_ethereum_consensus/p2p/block_downloader.ex#L84 ?
You could patch Libp2pPort.send_request
and make that one call the function you implemented. And then you could also patch Libp2pPort.send_response(message_id, response_chunk)
.
@h3lio5 is it clear what's being asked? We can have a call if not clear |
test/unit/p2p_blocks_test.exs
Outdated
BlockStore.store_block(signed_block_input) | ||
|
||
# ssz serialize and snappy compress the block root | ||
with {:ok, ssz_serialized} <- Ssz.to_ssz(BeaconBlocksByRootRequest{body: [block_root]}), |
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.
with {:ok, ssz_serialized} <- Ssz.to_ssz(BeaconBlocksByRootRequest{body: [block_root]}), | |
with {:ok, ssz_serialized} <- Ssz.to_ssz(%BeaconBlocksByRootRequest{body: [block_root]}), |
test/unit/p2p_blocks_test.exs
Outdated
alias LambdaEthereumConsensus.Store.Db | ||
|
||
setup do | ||
expose(Handler, :all) |
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.
We don't need to expose the Handler
functions anymore.
You need to add the new types to the |
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.
As talked in the daily, we should split this into two PRs: one with the new feature, and another with the tests
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.
Summarizing what's left to do in this PR:
- remove
test/unit/p2p_blocks_test.exs
(it should be re-added in another PR) - merge
main
- address comments
Co-authored-by: Tomás Grüner <47506558+MegaRedHand@users.noreply.github.com>
Co-authored-by: Tomás Grüner <47506558+MegaRedHand@users.noreply.github.com>
Co-authored-by: Martin Paulucci <mpaulucci@Martins-MacBook-Pro.local>
Lints are failing. Could you run |
Motivation
Implements the BeaconBlocksByRoot p2p handler.
Closes #446