Fix WebRTC to LibP2P propagation #1033
Conversation
| 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.
@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.
My assumption is that if msg was received from libp2p, it should still be in mcache so the action won't be enabled because this will be false: https://github.com/openmina/openmina/blob/b5c83e8fb6276306d730a627a0cb79da9af301a4/p2p/src/network/pubsub/p2p_network_pubsub_actions.rs#L206
There was a problem hiding this comment.
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.
also may break accidentally if for some reason that caching logic changes somehow
There was a problem hiding this comment.
also may break accidentally if for some reason that caching logic changes somehow
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.
@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).
binier
force-pushed
the
fix/p2p/webrtc_to_libp2p_propagation
branch
from
a75f4ac to
144afec
Compare
…nders for libp2p received blocks Same issue affects transactions and snarks too, so needs to be fixed there as well.
| }, | ||
| }) { | ||
| // Otherwise block was received from WebRTC so inject it in libp2p. | ||
| store.dispatch(P2pNetworkPubsubAction::WebRtcRebroadcast { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
is the conditional check what avoids the duplication of the message?
but once that is implemented we will have to figure out something else here)
yes and yes
2 participants
No description provided.