feat: Merge the remainder of the new groupchats implementation #2269
Conversation
6ee1d3c to
77f6b54
Compare
4072339 to
7e2ec2c
Compare
There was a problem hiding this comment.
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
3cb443f to
7b52d02
Compare
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
96ce13f to
2904fbd
Compare
There was a problem hiding this comment.
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 statusnit: why the
GM_prefix, doesn't fitGroup_Broadcats_TypeIMHO
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_typebe 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 leastlengthhas 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
togglebut more a setter function, because of theignoreparameter
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_MAXhere?
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
boolhere?
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 8Why 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 3please 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.hhasrandom_range_u32(...)which does exactly what's done here, add an offset of1and 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
flatin this context mean? is there maybe a synonym that's more widespread?
There was a problem hiding this comment.
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 fitGroup_Broadcats_TypeIMHO
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_typebe 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 leastlengthhas 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
togglebut more a setter function, because of theignoreparameter
Done.
toxcore/group_chats.h, line 407 at r4 (raw file):
Previously, sudden6 wrote…
How about
UINT8_MAXhere?
Done.
toxcore/group_chats.h, line 710 at r4 (raw file):
Previously, sudden6 wrote…
how about returning
boolhere?
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.hhasrandom_range_u32(...)which does exactly what's done here, add an offset of1and 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
flatin this context mean? is there maybe a synonym that's more widespread?
Done. Should say flag.
d496c71 to
4ba058d
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
ee3236a to
3a0ad65
Compare
There was a problem hiding this comment.
Reviewed 1 of 4 files at r29, all commit messages.
Reviewable status:
I think I've addressed everything I can. No follow-ups in a long time.
87e0a26 to
2acb790
Compare
48efc81 to
2559a46
Compare
2559a46 to
eca315a
Compare
eca315a to
0a277b5
Compare
Commit history: https://github.com/jfreegman/toxcore/tree/ngc_jf
Spec: https://toktok.ltd/spec.html#dht-group-chats
This change is