[Libp2p] - Refactor VID Delayed Requester To Use Message Passing #3688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukeiannucci
commented
Sep 20, 2024
lukeiannucci
commented
Sep 20, 2024
bfish713
reviewed
Sep 23, 2024
jparr721
reviewed
Sep 23, 2024
jparr721
reviewed
Sep 23, 2024
jparr721
reviewed
Sep 23, 2024
| Ok(serialized) => serialized, | ||
| Err(e) => { | ||
| tracing::error!("Failed to serialize outgoing message: this should never happen. Error: {e}"); | ||
| match receiver.recv_direct().await { |
This was
linked to
issues
Sep 24, 2024
lukeiannucci
changed the title
bfish713
reviewed
Sep 26, 2024
| .await; | ||
| } | ||
| } | ||
| SequencingMessage::General(_) => {} |
There was a problem hiding this comment.
Should this also include responses for proposal? Or is that handled by a different message type?
There was a problem hiding this comment.
we have those GeneralConsensusMessage. Im not sure how easy it would be to refactor them into here. I need to think a little more on this
There was a problem hiding this comment.
If we want to move them we can probably make a separate issue
lukaszrzasik
approved these changes
Sep 27, 2024
lukeiannucci
force-pushed
the
li/req-res-vid
branch
from
c41de18
to
4b59c24
Compare
This was referenced Sep 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3617 and #3599
This PR:
Implements the HotShotEvent Request / Response framework for VID shares. We recently did this to retrieve QCs and now we are making it a standard. In the Request and Response task we now emit/receive new
HotShotEvents, those being:VidRequestSend: This is when a node sends a request to the DA committee to retrieve a VID share for a specified view.VidRequestRecv: This event is for members of the DA committee, they will receive the request and respond with the VID share for the view.VidResponseSend: This event emits the VID share to the network task so we can send it back to the requesting node.VidResponseRecv: The requesting node will wait in a background task for this response and then validate the VID share.Previously we used a separate channel to request / receive VID shares. We also cleanup the old logic in this PR.
Another thing we do is remove the loop where we iterate over all DA committee members for a given view
VidShareRecvevents to verify . We now add the sender public key and use that so no need to check every member.This PR does not:
Key places to review:
I do most of the refactoring in
request.rsandresponse.rs. I am also not using the other channel anymore and relying on the main network task to send these messages now.Also on HS Event
VidShareRecvmake sure the logic looks correct where I remove the loopHow to test this PR:
The
combined_networktests will test this new logic