-
Notifications
You must be signed in to change notification settings - Fork 41
Fix WebRTC to LibP2P propagation #1033
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
Changes from all commits
60d28a9
b5c83e8
144afec
d8c3cb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,7 @@ pub enum P2pChannelsSnarkAction { | |
| Libp2pBroadcast { | ||
| snark: Snark, | ||
| nonce: u32, | ||
| is_local: bool, | ||
| }, | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,18 +210,27 @@ impl P2pChannelsSnarkState { | |
| } | ||
| Ok(()) | ||
| } | ||
| #[cfg(not(feature = "p2p-libp2p"))] | ||
| P2pChannelsSnarkAction::Libp2pBroadcast { .. } => Ok(()), | ||
| #[cfg(feature = "p2p-libp2p")] | ||
| P2pChannelsSnarkAction::Libp2pBroadcast { snark, nonce } => { | ||
| P2pChannelsSnarkAction::Libp2pBroadcast { | ||
| snark, | ||
| nonce, | ||
| is_local, | ||
| } => { | ||
| let dispatcher = state_context.into_dispatcher(); | ||
| let message = Box::new((snark.statement(), (&snark).into())); | ||
| let message = v2::NetworkPoolSnarkPoolDiffVersionedStableV2::AddSolvedWork(message); | ||
| let nonce = nonce.into(); | ||
| let message = GossipNetMessageV2::SnarkPoolDiff { message, nonce }; | ||
| dispatcher.push(P2pNetworkPubsubAction::Broadcast { message }); | ||
| if is_local { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @binier I am confused by this, is this enough to differentiate non-webrtc from webrtc sourced messages? What happens with messages received from libp2p? aren't they going to follow the same path and be considered as webrtc-sourced by this condition here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My assumption is that if msg was received from libp2p, it should still be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, but if that is the case please add a comment clarifying that there, because right now it is quite confusing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also may break accidentally if for some reason that caching logic changes somehow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It will "break" anyways if that happens. Though right now maybe not, coz block rebroadcast won't wait for block application right? It seems to do only precheck and rebroadcast in that case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @binier block rebroadcast now waits for the application to be successful, Grga implemented that in #1001 (but just for blocks at the moment, this doesn't apply to transactions or snarks yet). My point is that if for whatever reason this code needs to be changed (or somebody is trying to debug something semi-related), it would be better to not depend on our own memories or to have to re-understand the code to understand the assumptions made here (which depend on code from somewhere else). The comment makes the (non-obvious) assumption very explicit and will save time (both in that case, or any other kind of refactor affecting this). |
||
| dispatcher.push(P2pNetworkPubsubAction::Broadcast { message }); | ||
| } else { | ||
| // rebroadcast snark if received from webrtc network, otherwise noop. | ||
| dispatcher.push(P2pNetworkPubsubAction::WebRtcRebroadcast { message }); | ||
| } | ||
| Ok(()) | ||
| } | ||
| #[cfg(not(feature = "p2p-libp2p"))] | ||
| P2pChannelsSnarkAction::Libp2pBroadcast { .. } => Ok(()), | ||
| P2pChannelsSnarkAction::Libp2pReceived { peer_id, snark, .. } => { | ||
| let (dispatcher, state) = state_context.into_dispatcher_and_state(); | ||
| let p2p_state: &P2pState = state.substate()?; | ||
|
|
||
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.
what would happen if we dispatched this one unconditionally? is the conditional check what avoids the duplication of the message? (I ask because we can do this here because the transition frontier still doesn't use queued actions, but once that is implemented we will have to figure out something else here)
Uh oh!
There was an error while loading. Please reload this page.
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.
yes and yes