Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add autonat v2 spec #538
add autonat v2 spec #538
Changes from 10 commits
d663611
1db8613
0ff8ac6
f2a431c
62123df
0771bab
3e57202
f6def9a
b769d79
e4efaae
f28511c
d4da279
5b8d37d
05a0de2
8b52643
dd2750c
4e6ecaa
2af3309
6b1604b
f979fac
209b215
094089b
b4a856b
1c76613
03718ef
0195203
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think we should have a message for this too, otherwise implementing the decoder for these messages is going to be quite annoying because you need to attempt deserializing them and only if that fails, consider it bare "data".
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.
So the protocol is if the client accepts the DialDataRequest, it starts sending that amount of data. If the client doesn't accept the DialDataRequest, it resets the stream. You do not need to deserialise anything after you send DialDataRequest here. Just read that many bytes.
Wrapping the entire data in a protobuf will make the protobuf size very large.
Do you think adding a
DataRequestAccepted
message before sending that amount of data as raw bytes will make things cleaner?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 reason I am asking for this is because at least in
rust-libp2p
, we typically implement "codecs" which are modules that turn byte streams into typed streams (and sinks) of messages. Reading bare half-way through does not play that well with strong type systems.What do you mean it will make the protobuf large? Can't you just have a dynamically sized byte buffer in the protobuf? What is the difference?
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 guess we could unwrap the stream again and read a specified number of bytes.
Thoughts @mxinden?
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.
You'll need to bring the whole serialised protobuf into memory before decoding. This will take memory of the order of 100kb. Not prohibitive, but I'd like to avoid this if we can.
If I understand correctly, you would prefer it to be a protobuf so you can have a typed stream which says first object received from the stream is of type A and the second object received is of type B, right?
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 might work, but you're now hardcoding assumptions about the wire format of the Protobuf, outside of your Protobuf library. I'd like to avoid that.
Not sure I agree with this framing, but in the end, this boils down to a matter of taste.
How about repeatedly sending a smaller message (limited by our usual Protobuf size), until we've reached the required number of 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.
So in the current proposal because the number of bytes that you need to read in the second message is not fixed and is equal to the number you asked for in the
DialDataRequest
this prevents you from writing a generic reader, right?If that's the case we can prefix the bytes sent with a unsigned-varint just like we prefix other protobuf messages sent. This will also be consistent with the other messages exchanged everywhere which I'd argue is the source of inconsistency in the design as opposed to bytes not being sent as a TLV object.
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.
After doing a POC implementation, I think Marten's suggestion here is the most suited to implementation. The problem from a go perspective is buffering in the protobuf msg reader on the server side. This can happen if someone does a pipelined implementation on the client side which sends data as soon as it has sent the request. see documentation for NewDelimitedReader for another case where this happens. While this situation is unlikely, it is simpler to just use protobufs for transferring data.
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.
Yeah that makes sense. Would that message be a regular protobuf with a static array?
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'm using 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.
@mxinden mention that this design is takes inspiration from a different protocol but I forgot which one. Might be worth mentioning!
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.
E.g. related to the QUIC anti-amplification mechanism based on data size:
https://www.rfc-editor.org/rfc/rfc9000.html#section-8.1
I am not aware of an exact origin of the AutoNATv2 mechanism.
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.
QUIC is always a good source of inspiration :)
The specifics here is something that we came up with ourselves, in the discussion on the issue. If you're aware of any prior art (RFCs?), please let us know.
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.
should I mention QUIC?
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.
Instead of an index, why not send the address itself? In my eyes just a small complexity reduction. Feel free to ignore. Based on intuition, the additional bytes (index < address) doesn't have a performance impact.
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 think your suggestion is right. I'll resolve this while taking up the implementation. This will help us avoid sharing the slice between the thread that makes the dial request and the thread that receives the response. Similar effect can be had by sharing the idx to the requesting thread, but this seems simpler to me. I'll resolve this while implementing.
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.
Not really sure how to achieve this, but it would be nice if it could be made clear that this is happening on a new connection (and a different 5-tuple), whereas the rest of the exchange happens on the same 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.
This mentions
E_TRANSPORT_NOT_SUPPORTED
but that is missing from the protobufs?