Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simulation network broadcast fix #262

Merged
merged 4 commits into from
Jul 18, 2023
Merged

Simulation network broadcast fix #262

merged 4 commits into from
Jul 18, 2023

Conversation

bacv
Copy link
Member

@bacv bacv commented Jul 17, 2023

  1. Removed different types of network messages that were used to separate adhoc and broadcast messages. That was causing broadcasted but delayed messages be dropped.
  2. Broadcast functionality was added to Output::BroadcastTimeoutQc message type.
  3. Updated default value of tally to 2.

@bacv bacv added this to the Simulation App milestone Jul 17, 2023
@bacv bacv requested review from danielSanchezQ and al8n July 17, 2023 16:58
@bacv bacv self-assigned this Jul 17, 2023
al8n
al8n approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@al8n al8n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

danielSanchezQ
danielSanchezQ reviewed Jul 18, 2023
View reviewed changes
simulations/src/network/mod.rs
Comment on lines 108 to +109
from_node_receivers: HashMap<NodeId, Receiver<NetworkMessage<M>>>,
from_node_broadcast_receivers: HashMap<NodeId, Receiver<NetworkMessage<M>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? A broadcasting can be just treated as sending a message to each node in the network. There should be no duplication of handlers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Network interface communicates with the Network via the channels. To implement broadcasting two options comes to mind: Use enum to distinguish adhoc and broadcast message (this was done in a first implementation), or use dedicated channel. I feel that later solution plays nice with how network collects messages after each step and dispatches it (taking message delays into account).

@bacv bacv requested a review from danielSanchezQ July 18, 2023 10:31
@bacv bacv force-pushed the simulation-network-broadcast-fix branch from b5d562a to a4efb91 Compare July 18, 2023 11:52
@bacv bacv merged commit 9f71dbb into master Jul 18, 2023
2 checks passed
@bacv bacv deleted the simulation-network-broadcast-fix branch July 18, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielSanchezQ danielSanchezQ danielSanchezQ approved these changes

@al8n al8n al8n approved these changes

Assignees

@bacv bacv

Labels
simulation
Projects
None yet
Milestone
Simulation App
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bacv @danielSanchezQ @al8n