feat: Merge the remainder of the new groupchats implementation

[JFreegman]

Commit history: https://github.com/jfreegman/toxcore/tree/ngc_jf
Spec: https://toktok.ltd/spec.html#dht-group-chats

---

This change is 

[JFreegman]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

---

other/analysis/run-cppcheck, line 24 at r1 (raw file):

Previously, iphydf wrote…

No TODO for me please :).

Done.

---

toxcore/LAN_discovery.c, line 343 at r1 (raw file):

Previously, iphydf wrote…

Maybe you can make a PR with all the formatting changes (run astyle; make PR) first?

Probably a bad idea since my astyle formats differently from whatever the latest version is

[sudden6]

AFAICT at least the new auto_tests are missing a license and copyright info.

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

---

auto_tests/group_invite_test.c, line 50 at r2 (raw file):

        return false;
    }

whitespace issue

---

toxcore/Messenger.c, line 368 at r2 (raw file):

/** @brief the friend connection and onion connection for a groupchat.
 *
 * @retval 0 if success.

Documentation doesn't match implementation type

[JFreegman]

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @iphydf and @sudden6)

---

auto_tests/group_invite_test.c, line 50 at r2 (raw file):

Previously, sudden6 wrote…

whitespace issue

Done.

---

toxcore/Messenger.h, line 40 at r2 (raw file):

Previously, iphydf wrote…

What needs to be done to remove this?

I'm not sure to be honest.

---

toxcore/Messenger.h, line 372 at r2 (raw file):

Previously, iphydf wrote…

Change to /** style comment. Same below.

Done.

---

toxcore/Messenger.c, line 368 at r2 (raw file):

Previously, sudden6 wrote…

Documentation doesn't match implementation type

Done.

---

toxcore/Messenger.c, line 405 at r2 (raw file):

Previously, iphydf wrote…

/**

Done.

[sudden6]

Reviewed 15 of 60 files at r1, 4 of 14 files at r2, 2 of 4 files at r3, all commit messages.
Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @iphydf, @JFreegman, and @sudden6)

---

auto_tests/group_moderation_test.c, line 50 at r4 (raw file):

    size_t user_event_count;

    bool kick_check;  // moderater gets kicked

typo, moderator

---

auto_tests/group_moderation_test.c, line 493 at r4 (raw file):

    do {
        iterate_all_wait(autotoxes, NUM_GROUP_TOXES, ITERATION_INTERVAL);
    } while (!all_peers_connected(autotoxes));

should this loop have a timeout?

---

auto_tests/group_topic_test.c, line 98 at r4 (raw file):

 * Return -1 on failure.
 */
static int set_topic(Tox *tox, uint32_t groupnumber, const char *topic, size_t length)

probably better to use bool here

---

other/analysis/variants.sh, line 4 at r4 (raw file):

run "$@"
run -DVANILLA_NACL -I/usr/include/sodium "$@"

why is that removed, is Vanilla NaCl not supported anymore?

---

toxcore/group_chats.h, line 62 at r4 (raw file):

/** Group broadcast packet types */
typedef enum Group_Broadcast_Type {
    GM_STATUS          = 0x00,  // Peer changed their status

nit: why the GM_ prefix, doesn't fit Group_Broadcats_Type IMHO

---

toxcore/group_chats.h, line 150 at r4 (raw file):

 * `pakcket_type` must be either NET_PACKET_GC_LOSSY or NET_PACKET_GC_LOSSLESS.
 */
uint16_t gc_get_wrapped_packet_size(uint16_t length, uint8_t packet_type);

can packet_type be turned into an enum if it has only two named values that work? Is there a possible error condition and how is it signaled? I suspect at least length has an upper bound.

---

toxcore/group_chats.h, line 201 at r4 (raw file):

 */
non_null()
int gc_toggle_ignore(const GC_Chat *chat, uint32_t peer_id, bool ignore);

nit: IMHO it's less of a toggle but more a setter function, because of the ignore parameter

---

toxcore/group_chats.h, line 407 at r4 (raw file):

/** @brief Returns the group role of peer designated by `peer_id`.
 * Returns (uint8_t)-1 on failure.

How about UINT8_MAX here?

---

toxcore/group_chats.h, line 710 at r4 (raw file):

 * Note: This function may modify the peer list and change peer numbers.
 *
 * Return 0 if packet is successfully handled.

how about returning bool here?

---

toxcore/group_chats.h, line 719 at r4 (raw file):

/** @brief Handles an invite accept packet.
 *
 * Return 0 on success.

same

---

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

/* Maximum number of bytes to pad packets with */
#define GC_MAX_PACKET_PADDING 8

Why is this padded? and why 8 bytes?

---

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

/* How often we try to handshake with an unconfirmed peer */
#define GC_SEND_HANDSHAKE_INTERVAL 3

please include the unit of time in the comment or define name (for subsecond units).

---

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

/* How often we rotate session encryption keys with a peer */
#define GC_KEY_ROTATION_TIMEOUT (5 * 60)

same

---

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

/* How often we try to reconnect to peers that recently timed out */
#define GC_TIMED_OUT_RECONN_TIMEOUT (GC_UNCONFIRMED_PEER_TIMEOUT * 3)

same

---

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

#define GC_TIMED_OUT_RECONN_TIMEOUT (GC_UNCONFIRMED_PEER_TIMEOUT * 3)

/* How long before we stop trying to reconnect with a timed out peer */

same

---

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

non_null()
static bool saved_peer_is_valid(const GC_SavedPeerInfo *saved_peer);

maybe move that one to the others on top?

---

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

    }

    uint32_t index = random_u32(chat->rng) % chat->numpeers;

crypto_core.h has random_range_u32(...) which does exactly what's done here, add an offset of 1 and you even save some lines.

---

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

                                     uint16_t sync_flags)
{
    /** Do not change the order of these four send calls or else */

else what? Is this part of the protocol definition?

---

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

    }

    uint16_t peers_checksum;

how about a struct + pack/unpack for these? They seem to belong together to me

---

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

 *
 * If the privacy state has been set to private, we kill our group's connection to the DHT.
 * Otherwise, we create a new connection with the DHT and flat an announcement.

what does flat in this context mean? is there maybe a synonym that's more widespread?

[JFreegman]

None of the auto tests have license/copyright info

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @iphydf and @sudden6)

---

auto_tests/group_moderation_test.c, line 493 at r4 (raw file):

Previously, sudden6 wrote…

should this loop have a timeout?

Nope, see Iphy's answer in IRC

---

auto_tests/group_topic_test.c, line 98 at r4 (raw file):

Previously, sudden6 wrote…

probably better to use bool here

Done.

---

other/analysis/variants.sh, line 4 at r4 (raw file):

Previously, sudden6 wrote…

why is that removed, is Vanilla NaCl not supported anymore?

NaCl was building with a ton of unused function warnings which was blocking development.

---

toxcore/group_chats.h, line 62 at r4 (raw file):

Previously, sudden6 wrote…

nit: why the GM_ prefix, doesn't fit Group_Broadcats_Type IMHO

I'm reluctant to change any of the enum prefixes because we will probably end up changing them all again in the future to satisfy cimple

---

toxcore/group_chats.h, line 150 at r4 (raw file):

Previously, sudden6 wrote…

can packet_type be turned into an enum if it has only two named values that work? Is there a possible error condition and how is it signaled? I suspect at least length has an upper bound.

Done. Changed it to the Net_Packet_Type enum. Asserts are used to check length bounds.

---

toxcore/group_chats.h, line 201 at r4 (raw file):

Previously, sudden6 wrote…

nit: IMHO it's less of a toggle but more a setter function, because of the ignore parameter

Done.

---

toxcore/group_chats.h, line 407 at r4 (raw file):

Previously, sudden6 wrote…

How about UINT8_MAX here?

Done.

---

toxcore/group_chats.h, line 710 at r4 (raw file):

Previously, sudden6 wrote…

how about returning bool here?

Done.

---

toxcore/group_chats.h, line 719 at r4 (raw file):

Previously, sudden6 wrote…

same

Done.

---

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

Previously, sudden6 wrote…

Why is this padded? and why 8 bytes?

Done. Added a comment.

---

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

Previously, sudden6 wrote…

please include the unit of time in the comment or define name (for subsecond units).

Done.

---

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

Previously, sudden6 wrote…

same

Done.

---

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

Previously, sudden6 wrote…

same

Done.

---

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

Previously, sudden6 wrote…

same

Done.

---

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

Previously, sudden6 wrote…

maybe move that one to the others on top?

Done.

---

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

Previously, sudden6 wrote…

crypto_core.h has random_range_u32(...) which does exactly what's done here, add an offset of 1 and you even save some lines.

Done.

---

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

Previously, sudden6 wrote…

else what? Is this part of the protocol definition?

Validation won't work if they're sent in the wrong order. I'll add this info to the spec.

---

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

Previously, sudden6 wrote…

how about a struct + pack/unpack for these? They seem to belong together to me

This is the same style of packet packing/unpacking that's used for all other NGC packets. If we switch to a struct model it should be done uniformly (and should use cmp)

---

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

Previously, sudden6 wrote…

what does flat in this context mean? is there maybe a synonym that's more widespread?

Done. Should say flag.

[sudden6]

Ok, probably should discuss this outside this PR

Reviewed 9 of 60 files at r1, 1 of 14 files at r2, 1 of 4 files at r3, 1 of 1 files at r4, 5 of 12 files at r5.
Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @iphydf and @JFreegman)

---

auto_tests/group_moderation_test.c, line 493 at r4 (raw file):

Previously, JFreegman wrote…

Nope, see Iphy's answer in IRC

yeah, was being stupid here.

---

auto_tests/group_topic_test.c, line 106 at r5 (raw file):

    }

    return true;

can be simplified to a oneliner now

---

other/analysis/variants.sh, line 4 at r4 (raw file):

Previously, JFreegman wrote…

NaCl was building with a ton of unused function warnings which was blocking development.

But shouldn't it be added back now that development is done? Or is it gener

---

toxcore/group_chats.h, line 62 at r4 (raw file):

Previously, JFreegman wrote…

I'm reluctant to change any of the enum prefixes because we will probably end up changing them all again in the future to satisfy cimple

I see.

---

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

Previously, JFreegman wrote…

Done. Added a comment.

Ok, but how did you come up with exactly 8 bytes padding? why is it enough?

---

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

Previously, JFreegman wrote…

Validation won't work if they're sent in the wrong order. I'll add this info to the spec.

Can you also add a comment linking the spec? otherwise this comment is quite confusing to m.

---

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

Previously, JFreegman wrote…

This is the same style of packet packing/unpacking that's used for all other NGC packets. If we switch to a struct model it should be done uniformly (and should use cmp)

Ok then.

---

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

Previously, JFreegman wrote…

Done. Should say flag.

Makes sense now :)

---

toxcore/group_chats.c, line 1753 at r5 (raw file):

}

non_null() static int get_gc_peer_public_key(const GC_Chat *chat, uint32_t peer_number, uint8_t *public_key);

Move to the top with the rest? IDK if there's a style guide on where to put prototypes

---

toxcore/group_chats.c, line 1960 at r5 (raw file):

}

non_null() static void copy_self(const GC_Chat *chat, GC_Peer *peer);

same

---

toxcore/group_chats.c, line 2078 at r5 (raw file):

 */
non_null()
static int handle_gc_invite_response(GC_Chat *chat, GC_Connection *gconn)

not sure if handle_ means this will be used as a callback with a given signature, but otherwise it could return bool. There are multiple occurences of this

---

toxcore/group_chats.c, line 2080 at r5 (raw file):

static int handle_gc_invite_response(GC_Chat *chat, GC_Connection *gconn)
{
    if (!send_gc_sync_request(chat, gconn, 0xffff)) {

please define this magic value

---

toxcore/group_chats.c, line 3400 at r5 (raw file):

                          uint16_t length,  void *userdata)
{
    /** If this happens malicious behaviour is highly suspect */

should this really be a doxygen comment?

---

toxcore/group_chats.c, line 3635 at r5 (raw file):

    // TODO(jfreegman): improbable, but an overflow would break everything
    if (chat->topic_info.version == UINT32_MAX) {

break everything sounds bad, something we should do about this?

---

toxcore/group_chats.c, line 3755 at r5 (raw file):

        if (gc_get_enc_pk_from_sig_pk(chat, public_enc_key, topic_info->public_sig_key)) {
            if (sanctions_list_is_observer(&chat->moderation, public_enc_key)) {
                LOGGER_DEBUG(chat->log, "Invalid topic signature (sanctioned peeer attempted to change topic)");

typo peer

---

toxcore/group_chats.c, line 3856 at r5 (raw file):

    }

    const bool is_response = data[0] == 1;

please define this magic value

---

toxcore/group_chats.c, line 3897 at r5 (raw file):

    // save new keys and compute new shared key AFTER sending reponse packet with old key
    memcpy(gconn->session_public_key, new_session_pk, sizeof(gconn->session_public_key));
    memcpy(gconn->session_secret_key, new_session_sk, sizeof(gconn->session_secret_key));

gconn->session_secret_key should likely be memlocked too, I don't se that create_gc_session_keypair is doing it.

---

toxcore/group_chats.c, line 4064 at r5 (raw file):

    }

    const bool add_mod = data[0] != 0;

please define this magic value

---

toxcore/group_chats.c, line 4102 at r5 (raw file):

    }

    data[0] = add_mod ? 1 : 0;

same

---

toxcore/group_chats.c, line 4245 at r5 (raw file):

    }

    const bool add_obs = data[0] != 0;

same

---

toxcore/group_chats.c, line 4303 at r5 (raw file):

    }

    packet[0] = add_obs ? 1 : 0;

same

---

toxcore/group_chats.c, line 4513 at r5 (raw file):

static bool group_topic_lock_enabled(const GC_Chat *chat)
{
    return chat->shared_state.topic_lock == 0;

same

---

toxcore/group_chats.c, line 4578 at r5 (raw file):

    }

    // TODO(Jfreegman): handle this failure properly

How would it be handled properly?

---

toxcore/group_chats.c, line 5335 at r5 (raw file):

{
    if (packet_size != GC_MIN_ENCRYPTED_HS_PAYLOAD_SIZE + sizeof(Node_format)) {
        LOGGER_FATAL(chat->log, "invlaid packet size: %zu", packet_size);

typo invalid

---

toxcore/group_chats.c, line 5475 at r5 (raw file):

                                        uint16_t length)
{
    if (length < ENC_PUBLIC_KEY_SIZE + SIG_PUBLIC_KEY_SIZE + 1) {  // should be checked at lower level

If that's checked at lower level, how about turning it into an assert? Or do you want to do defense in depth?

---

toxcore/group_chats.c, line 5476 at r5 (raw file):

{
    if (length < ENC_PUBLIC_KEY_SIZE + SIG_PUBLIC_KEY_SIZE + 1) {  // should be checked at lower level
        LOGGER_FATAL(chat->log, "Invlaid handshake response size (%u)", length);

typo invalid you should probably grep for Invlaid as it occurred at least twice :)

---

toxcore/group_chats.c, line 5555 at r5 (raw file):

                                       const uint8_t *data, uint16_t length)
{
    if (length < ENC_PUBLIC_KEY_SIZE + SIG_PUBLIC_KEY_SIZE + 1) {  // should be checked at lower level

same about assert

---

toxcore/group_chats.c, line 6932 at r5 (raw file):

    }

    if (tcp_connections > 0) {  // TODO(Jfreegman): Remove this before merge

Time to remove?

---

toxcore/group_chats.c, line 7958 at r5 (raw file):

    }

    crypto_memunlock(chat->self_secret_key, sizeof(chat->self_secret_key));

I'm not quite happy with how secret keys are handled in this file. They are allocated/deallocated/updated in different places and it's hard to trace if they are memlocked/unlocked/cleared correctly. How about wrapping their access into dedicated functions that do the right things?

---

toxcore/group_connection.c, line 30 at r5 (raw file):

#define GCC_UDP_DIRECT_TIMEOUT (GC_PING_TIMEOUT + 4)

/** Returns true if ary entry does not contain an active packet. */

array?

---

toxcore/group_connection.c, line 41 at r5 (raw file):

 *
 * Return 0 on success.
 * Return -1 on failure.

It's a void function?

---

toxcore/group_connection.c, line 90 at r5 (raw file):

}

/** @brief Puts packet data in ary_entry.

array_entry

---

toxcore/group_connection.c, line 285 at r5 (raw file):

    }

    const uint32_t rand_idx = random_u32(rng) % gconn->tcp_relays_count;

use random_u32_range(...)

---

toxcore/group_onion_announce.c, line 71 at r5 (raw file):

#ifndef VANILLA_NACL

// TODO(Jfreegman): params - to struct

maybe do it now?

---

toxcore/tox.h, line 3290 at r5 (raw file):

 * Maximum length of a group topic.
 */
#define TOX_GROUP_MAX_TOPIC_LENGTH     512

IIRC @iphydf and me discussed the topic of defines in headers and came to the conclusion that only getter functions should be used, because they work better for FFI, so maybe don't introduce them with new code?

[sudden6]

Did another round of review, found nothing which should prevent a merge. As I said already I'm not happy with the plaintext passwords (Details: JFreegman#27), but this shouldn't block the initial merge.

Reviewed 7 of 10 files at r27, 13 of 13 files at r28, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @Green-Sky, @iphydf, and @zoff99)

[Green-Sky]

Reviewed 1 of 4 files at r29, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @iphydf and @zoff99)

I think I've addressed everything I can. No follow-ups in a long time.

[iphydf]

More work needs to be done on this, but the core code paths LGTM. We'll iterate on it after merge.