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.

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

iroh-net transport #105

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

iroh-net transport #105

wants to merge 9 commits into from

Conversation

fogodev
Copy link
Collaborator

@fogodev fogodev commented Oct 29, 2024

This is a first draft on iroh-net transport implementation for quic-rpc. It's heavily based on quinn transport. It has a bunch of duplicated code from quinn transport too, that I can abstract away between these two transports if needed.

Leaving it as a draft for now because it's failing the server_away_and_back test for some reason and iroh_net_channel_bench test performance seem to be much worse than respective quinn transport test. I know that communicating through iroh-net will have extra overheads when comparing to communication over quinn with proper direct addresses. But I'm not 100% sure that this totally explains the extra time it takes compared to quinn transport.

@flub flub requested a review from rklaehn October 30, 2024 09:20
let allowed_node_ids = match access_control {
AccessControl::Unrestricted => BTreeSet::new(),
AccessControl::Allowed(list) if list.is_empty() => {
tracing::warn!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think empty should mean nobody has access. E.g. you got a config file where you define allowed node ids, and you set this to [] accidentally or because you want to remove access. This should not make it unrestricted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, makes sense indeed, I will remove this warn! and return an error instead, better fail early than being unresponsive for all requests

@rklaehn
Copy link
Collaborator

rklaehn commented Nov 6, 2024

I merged the new naming and the change to how mapping works. So there will be quite some conflicts. Should not be anything complex though, since neither the transports themselves nor the interaction patterns stuff has changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants