feat: allow the large custom NGC packets to be handled also by the client

[zoff99]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.74%. Comparing base (d6d67d5) to head (130c5a9).
Report is 139 commits behind head on master.

Files with missing lines	Patch %	Lines
toxcore/group_chats.c	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
- Coverage   73.77%   73.74%   -0.03%     
==========================================
  Files         148      148              
  Lines       30366    30371       +5     
==========================================
- Hits        22401    22396       -5     
- Misses       7965     7975      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nurupo]

Just left some general comments about the code. Have no idea what problem this feature solves and if it makes any sense to add the feature to group chats, so will leave that judging and the PR approval to @JFreegman.

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf and @zoff99)

---

toxcore/group_chats.c line 4997 at r2 (raw file):

/** @brief Returns false if a custom incoming (non private) packet is too large. */
static bool custom_gc_incoming_non_private_packet_length_is_valid(uint16_t length, bool lossless)

Is non_private the proper naming convention in here? I'm not familiar with group chats, so I'm not sure. From what I see, nothing else in group_chats.c uses "non_private", and from the code around, it looks like just packet refers only to public packets, while things related to private packets are explicitly named private_packet. If this is correct, then the function name should be changed to custom_gc_incoming_packet_length_is_valid to match the naming convention.

---

toxcore/group_chats.c line 5010 at r2 (raw file):

    return true;
}

Wow, that's a mouthful. Something like this is way easier to read:

static bool custom_gc_incoming_non_private_packet_length_is_valid(uint16_t length, bool lossless)
{
    return lossless ? length <= MAX_GC_CUSTOM_LOSSLESS_INCOMING_ASSEMBLED_PACKET_SIZE
                    : length <= MAX_GC_CUSTOM_LOSSY_PACKET_SIZE;
}

---

toxcore/group_common.h line 50 at r2 (raw file):

/* allow incoming NGC custom packets that are non private to be up to the total max size of MAX_GC_PACKET_SIZE
 * which is 50000 bytes. the data itself can only be less than that because of NGC header overhead
 */

The rest of the comments are capitalized.

Does it make sense to mention the number 50000? If the constants change (although unlikely), the comment would be incorrect.

---

toxcore/group_common.h line 52 at r2 (raw file):

 */
#define MAX_GC_CUSTOM_LOSSLESS_INCOMING_ASSEMBLED_PACKET_SIZE   MAX_GC_PACKET_SIZE

#define MAX_GC_CUSTOM_LOSSLESS_INCOMING_ASSEMBLED_PACKET_SIZE MAX_GC_PACKET_SIZE

Just one space is enough, unless you are trying to align it with somethin I don't see?

[zoff99]

Reviewable status: 1 change requests, 0 of 3 approvals obtained (waiting on @Green-Sky, @iphydf, @JFreegman, and @nurupo)

---

toxcore/group_chats.c line 5010 at r2 (raw file):

Previously, nurupo wrote…

Wow, that's a mouthful. Something like this is way easier to read:

static bool custom_gc_incoming_non_private_packet_length_is_valid(uint16_t length, bool lossless)
{
    return lossless ? length <= MAX_GC_CUSTOM_LOSSLESS_INCOMING_ASSEMBLED_PACKET_SIZE
                    : length <= MAX_GC_CUSTOM_LOSSY_PACKET_SIZE;
}

Done.

---

toxcore/group_common.h line 50 at r2 (raw file):

Previously, nurupo wrote…

The rest of the comments are capitalized.

Does it make sense to mention the number 50000? If the constants change (although unlikely), the comment would be incorrect.

Done.

---

toxcore/group_common.h line 52 at r2 (raw file):

Previously, nurupo wrote…

#define MAX_GC_CUSTOM_LOSSLESS_INCOMING_ASSEMBLED_PACKET_SIZE MAX_GC_PACKET_SIZE

Just one space is enough, unless you are trying to align it with somethin I don't see?

Done.

[nurupo]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 1 change requests, 0 of 3 approvals obtained (waiting on @Green-Sky, @iphydf, @JFreegman, and @zoff99)

[JFreegman]

Can you give a summary of the changes and why they're necessary?

[zoff99]

icoming (from the network) packets (when they are reassembled) for NGC packets can be up to about 50k bytes.
this is already used interally in toxcore. this PR just changes a size check so that those larger custom packets can be passed through to the client via the api. the api is not changed.
it is used for NGC group images in some clients.

this applies only to lossless custom packets and also does not apply to custom private packets.
so it only applies to "non private" custom lossless packets.

a stub for toxic can be found here: TokTok/toxic#269

[JFreegman]

toxcore/group_chats.c line 4998 at r4 (raw file):

/** @brief Returns false if a custom incoming (non private) packet is too large. */
static bool custom_gc_incoming_non_private_packet_length_is_valid(uint16_t length, bool lossless)

I'm also not a fan of non_private. We already assume anything not marked as private is non-private.

[zoff99]

Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, and @JFreegman)

[zoff99]

Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, @JFreegman, and @nurupo)

---

toxcore/group_chats.c line 4997 at r2 (raw file):

Previously, nurupo wrote…

Is non_private the proper naming convention in here? I'm not familiar with group chats, so I'm not sure. From what I see, nothing else in group_chats.c uses "non_private", and from the code around, it looks like just packet refers only to public packets, while things related to private packets are explicitly named private_packet. If this is correct, then the function name should be changed to custom_gc_incoming_packet_length_is_valid to match the naming convention.

Done.

[zoff99]

Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, @JFreegman, and @nurupo)

---

toxcore/group_chats.c line 4998 at r4 (raw file):

Previously, JFreegman wrote…

I'm also not a fan of non_private. We already assume anything not marked as private is non-private.

Done.

[JFreegman]

Is there a reason we're not updating TOX_GROUP_MAX_CUSTOM_LOSSLESS_PACKET_LENGTH in tox.h?

Edit: fyi all across toxcore we assume that the max value is the same for both inbound and outbound packets. This is the sole exception.

Reviewed 1 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: 2 change requests, 0 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, and @nurupo)

[nurupo]

Just left some general comments about the code. Have no idea what problem this feature solves and if it makes any sense to add the feature to group chats, so will leave that judging and the PR approval to @JFreegman.

0 of 2 approvals obtained
nurupo requested changes

Looks like I actually need to approve the PR because I have requested the changes, as otherwise the PR would be blocked on me even if @JFreegman approves it.

[Green-Sky]

Is there a reason we're not updating TOX_GROUP_MAX_CUSTOM_LOSSLESS_PACKET_LENGTH in tox.h?

Edit: fyi all across toxcore we assume that the max value is the same for both inbound and outbound packets. This is the sole exception.

@zoff99 why is this receive only? + add notes about actual sizes to the tox.h (with warnings for using large values might causing congestion)

[zoff99]

Is there a reason we're not updating TOX_GROUP_MAX_CUSTOM_LOSSLESS_PACKET_LENGTH in tox.h?
Edit: fyi all across toxcore we assume that the max value is the same for both inbound and outbound packets. This is the sole exception.

@zoff99 why is this receive only? + add notes about actual sizes to the tox.h (with warnings for using large values might causing congestion)

because we did not talk about sending larger custom packets.
we can do the sending part in another PR.
but is this PR now approved?

[JFreegman]

Is there a reason we're not updating TOX_GROUP_MAX_CUSTOM_LOSSLESS_PACKET_LENGTH in tox.h?
Edit: fyi all across toxcore we assume that the max value is the same for both inbound and outbound packets. This is the sole exception.

@zoff99 why is this receive only? + add notes about actual sizes to the tox.h (with warnings for using large values might causing congestion)

because we did not talk about sending larger custom packets. we can do the sending part in another PR. but is this PR now approved?

Those defines are used for both sending and receiving. The API shouldn't be lying to clients about the max possible size of an inbound custom lossless packet, and clients should know about the real limits of both inbound and outbound packets, which should be the same value. This PR is incoherent. The only client developers who will understand what's actually going on after this PR are you and the people who reviewed your PR.