-
Notifications
You must be signed in to change notification settings - Fork 275
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
allow autonat v2 to dial all ips #542
Conversation
autonat/autonat-v2.md
Outdated
} | ||
|
||
message DialDataResponse { | ||
bool accepted = 1; |
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.
Why not put the bytes into this protobuf? Then we wouldn't need to break the framing to send these bytes.
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.
That would require memory of the order of 100kb for the protobufs on both ends, right?
The current version can be implemented using a smaller buffer.
I don't have good intuition here, do you think this is fine?
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.
@marten-seemann friendly ping
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.
The server is going to discard these bytes right away anyways. No point to allocate the space to put the bytes in the message. I agree with the initial design. (Sorry for the drive-by comment, I'll defer to whatever you all decide).
autonat/autonat-v2.md
Outdated
one can not validate the IP address of the requesting node. | ||
Upon receiving a `DialDataRequest` message the client responds with | ||
`DialDataResponse` message with a boolean `accepted` indicating if it accepted | ||
the request. If `accepted` is `false`, the `DialRequest` is considered complete |
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.
The client could also just reset the stream if it doesn't want to send that many bytes.
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.
I am accepting this. This looks simpler to me. Even if we add a different cost for dial in future, say some POW like mechanism, the client can just reject the charge by resetting the stream.
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.
LGTM.
Nit: I think it's spelled IP and not ip.
oops, thanks! |
Closes #536