Add QUIC protocol implementation #1943
Conversation
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
| void start_up() override; | ||
| void tear_down() override; | ||
| void hangup() override; | ||
| void hangup_shared() override; | ||
| void wake_up() override; | ||
| void alarm() override; | ||
| void loop() override; |
There was a problem hiding this comment.
You don't have to implement these if you don't use them. The default implementation is reasonable enough.
There was a problem hiding this comment.
i'm not really familiar with td actor system, so i left them for debug logs in case of some unexpected signal from the scheduler
There was a problem hiding this comment.
I would define this in an anonymous namespace inside quic-connection.cpp unless there is a legitimate reason why we want to import the implementation from multiple places.
There was a problem hiding this comment.
that means you will need to declare all its ngtcp-insides in quic-connection.h, too
this is highly unwanted, as ngtcp exports some weird C symbols into global namespace -- they will contaminate the scope of every TU it's included into
see PImpl idiom
There was a problem hiding this comment.
Thanks, I know what pimpl is. I mostly don't like a separate header for implementation when this can be done in a single file, i. e. something resembling this: https://github.com/DanShaders/ton/blob/alpenglow/validator/consensus/block-producer.cpp#L11-L13.
There was a problem hiding this comment.
anyways, i plan to reuse this impl structure in server-side connections, so it will be useful to keep it in separate files
quic/quic-pimpl.cpp
Outdated
|
|
||
| std::string alpn_data(alpn.size() + 1, '\0'); | ||
| alpn_data[0] = static_cast<int8_t>(alpn.size()); | ||
| std::copy_n(alpn.c_str(), alpn.size(), alpn_data.begin()); |
There was a problem hiding this comment.
alpn_data.begin() + 1 as the last argument?
| params.initial_max_stream_data_bidi_local = 1 << 20; | ||
| params.initial_max_stream_data_bidi_remote = 1 << 20; | ||
| params.initial_max_data = 1 << 20; |
There was a problem hiding this comment.
Where is this 1MB(?) coming from?
| path.local.addr = const_cast<ngtcp2_sockaddr*>(local_address.get_sockaddr()); | ||
| path.local.addrlen = static_cast<ngtcp2_socklen>(local_address.get_sockaddr_len()); | ||
| path.remote.addr = const_cast<ngtcp2_sockaddr*>(remote_address.get_sockaddr()); | ||
| path.remote.addrlen = static_cast<ngtcp2_socklen>(remote_address.get_sockaddr_len()); |
There was a problem hiding this comment.
I don't like const casts here, is ngtcp2 guaranteed not to change passed sockaddrs?
| }; | ||
|
|
||
| constexpr static size_t QUIC_MTU = 1350; | ||
| constexpr static size_t UDP_MTU = 2048; |
There was a problem hiding this comment.
Why not proper 65507 bytes if this is already larger than ethernet MTU?
| virtual ~Callback() = default; | ||
| }; | ||
|
|
||
| constexpr static size_t QUIC_MTU = 1350; |
There was a problem hiding this comment.
Where does this come from? I think ngtcp2 does some form of dynamic MTU discovery, so we should probably query the value from it or use true maximum
|
|
||
| option(USE_QUIC "use ngtcp2 QUIC library" OFF) | ||
| if (USE_QUIC) | ||
| find_package(OpenSSL 3.2 REQUIRED COMPONENTS SSL Crypto) |
There was a problem hiding this comment.
We build with OpenSSL 3.1.4 in CI. Not sure how this passed, I think this might just never execute as it sees previously set OpenSSL_FOUND (or something).
|
Findings:
|
2 participants
No description provided.