Make conferences persist across restarts #263

isotoxin · 2016-11-11T11:59:43Z

This is not new groupchats. This is just upgrade to old groupchats with some advatages:

Groupchats are now saved into tox_save
Client can get groupchat unique id to save message log
Auto restore groupchats after restart even your friend uses non-upgraded version

API compatibility: almost 100%
see changes at tox_conference_invite_cb and TOX_CONFERENCE_STATE_CHANGE

Protocol compatibility: 100%, except rare incorrect behaviour of non-upgraded clients due bug in old groupchats code.

group.c totally refactored

This change is

This is not new groupchats. This is just upgrade to old groupchats with some advatages: - Groupchats are now saved into tox_save - Client can get groupchat unique id to save message log - Auto restore groupchats after restart even your friend uses non-upgraded version

SkyzohKey · 2016-11-11T14:36:48Z

@isotoxin As pointed out by @iphydf on the #toktok chan, reviewing a 3000 lines diff will be hard and long. Maybe you'd be better opening 1 pull-request per bullet-point, would make it easier to review and merge rapidly ;)

SkyzohKey · 2016-11-11T14:55:10Z

From Travis Builds:

c-toxcore/toxcore/group.c:551:18: error: unused variable 'comp_d' [-Werror,-Wunused-variable]
uint64_t comp_d = 0;

c-toxcore/toxcore/group.c:909:10: error: unused variable 'index' [-Werror,-Wunused-variable]
aint index = -1;

c-toxcore/toxcore/group.c:551:18: error: unused variable 'comp_d' [-Werror,-Wunused-variable]
uint64_t comp_d = 0;

c-toxcore/toxcore/group.c:909:10: error: unused variable 'index' [-Werror,-Wunused-variable]
aint index = -1;

c-toxcore/toxcore/group.c:2940:18: error: unused variable 'peer_index' [-Werror,-Wunused-variable]
aint peer_index = addpeer(g, groupnumber, real_pk, temp_pk, new_peer_gid);

@isotoxin If they are unused, shouldn't them only be deleted ? Anyway this is required for the travis check to pass.

isotoxin · 2016-11-11T15:29:58Z

So, I just rewrite whole group chats code. I cant split it to small pieces because it works only as organic whole. Of cource, you should understand how old groupchats works, to fully and adequately review this code. As I understand I am currently the only person who understands how old groupchats works (Irg can no longer be taken into account). So, it mean this PR will never be approved. Ok, I created PR and my conscience is clear.
New Isotoxin just released. You can test how upgraded groupchats work.

dvor · 2016-11-11T15:35:34Z

I'm sorry for off topic

So, it mean this PR will never be approved. Ok, I created PR and my conscience is clear.

There is no need to be that angry and pessimistic. People won't treat you nice when you tell them "I'm clever, you all are stupid, my work is done here".

SkyzohKey · 2016-11-11T15:41:45Z

@isotoxin I was just pointing that reviewing a 3000 lines diff is hard/long, I'm sorry if you were offended, that wasn't my goal.

Anyway I'm sure this will end merged once review is done, but you know developing FOSS is a community work and if you are working without accepting criticism and advice I'm sure people won't be willing to help.

Flame war isn't really needed if we want Toxcore to dominate the world a time, let's works together instead of shooting ourselves the legs.

isotoxin · 2016-11-11T16:29:06Z

I apologize if my answer seemed rude. And I'm open for criticism and advice. I received lot of criticism and advice about Isotoxin and I accept everything. I just don't want to do something that I can't understand why I should it do. Anyway, if you have questions about my code (why I done it that way or something like), you can always ask me, and I will answer. Also I ready to fix this code, if you found bugs or you think how to make it better. But don't tell me about tests. I will not write or modify any tests. Sorry.

iphydf · 2016-11-11T22:13:48Z

Reviewed 9 of 10 files at r1.
Review status: 9 of 10 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

toxav/groupav.c, line 494 at r2 (raw file):

    if (groupchat_enable_av(log, g_c, groupnumber, audio_callback, userdata) == -1) {
        del_groupchat(g_c, groupnumber);
        return -10;

The documentation said -1 .. -6. Perhaps make this -7 and document what it means.

toxav/groupav.c, line 514 at r2 (raw file):

    uint8_t data[MAX_CRYPTO_DATA_SIZE]; // according to caller, length is maximum 1024 bytes (see group_send_audio)
    data[0] = GROUP_AUDIO_PACKET_ID;
    size_t plen = 1 + sizeof(uint16_t) + length;

Add an assert to check that this length doesn't exceed the data buffer size. Even if you can statically argue that it can't happen, future code changes could make it happen and then an assertion failure is better than undefined behaviour.

toxav/groupav.c, line 520 at r2 (raw file):

    memcpy(data + 1 + sizeof(sequnum), packet, length);

    if (send_group_lossy_packet(g_c, groupnumber, data, (uint16_t)plen) == -1) {

Why the explicit cast?

toxcore/group.h, line 191 at r2 (raw file):

enum {
    CHAT_CHANGE_OCCURRED,
    __UNUSED__CHAT_CHANGE_PEER_DEL,

This identifier is reserved in ISO C (7.1.3p1).

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

#define MESSENGER_STATE_TYPE_TCP_RELAY     10
#define MESSENGER_STATE_TYPE_PATH_NODE     11
#define MESSENGER_STATE_TYPE_OLDGROUPCHATS 100

Old group chats are now called conference. I suggest using CONFERENCE here and in other places where you use "old groups" names.

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

        case MESSENGER_STATE_TYPE_OLDGROUPCHATS: {
            oldgroups_load(m, data, length);

Is the error intentionally ignored here?

toxcore/tox.api.h, line 2051 at r2 (raw file):

     * @param friend_number The friend who invited us.
     *  if friend_number == UINT32_MAX then autojoin
     *  if client do not call tox_conference_join or toxav_join_av_groupchat immediately

What does "immediately" mean? When exactly is the conference deleted? Is it the next time tox_iterate is called?
Use $join for the first function. The second one needs to stay as is, because we can't (yet) refer to things in other files.

toxcore/tox.api.h, line 2157 at r2 (raw file):

   *
   * @param conference_number The conference number of the conference to be entered.
   * conference_number can be obtained by tox_conference_by_uid

Replace tox_conference_by_uid with $by_uid. That way, the documentation stays correct for other apidsl backends, and you can be sure that if by_uid changes, you'll know which places in the documentation to update.

toxcore/tox.api.h, line 2158 at r2 (raw file):

   * @param conference_number The conference number of the conference to be entered.
   * conference_number can be obtained by tox_conference_by_uid
   * Call this function only if you leave conference using tox_conference_leave.

$leave

Also, move this section (starting with "Call") up to before the parameter list. Have an empty line between the one-line description and this additional information section.

toxcore/tox.api.h, line 2180 at r2 (raw file):

   *
   * @param conference_number The conference number of the conference to be disconnected.
   * conference_number can be obtained by tox_conference_by_uid.

$by_uid

toxcore/tox.api.h, line 2185 at r2 (raw file):

   * No one can invite you to this conference after you leave it with keep_leave is true.
   * Also keep_leave == true means conference will not try to connect to other peers after restart.
   * Call tox_conference_enter to enable auto connect and invite.

$enter

toxcore/tox.api.h, line 2450 at r2 (raw file):

   * @return true on success.
   */
  bool get_uid(uint32_t conference_number, uint8_t[CONFERENCE_UID_SIZE] uid);

This function has the same errors as by_uid: uid can be NULL and conference_number can be invalid.
This function can be const, because it doesn't mutate the Tox instance.

toxcore/tox.api.h, line 2456 at r2 (raw file):

   *
   * @return the conference number on success, UINT32_MAX on failure.
   * @param uid A byte array containing the conference id (TOX_CONFERENCE_UID_SIZE).

$CONFERENCE_UID_SIZE

toxcore/tox.h, line 249 at r2 (raw file):

#define TOX_CONFERENCE_UID_SIZE        32

uint32_t tox_conference_uid_size(void);

At the top of tox.c, there is a list of constant-functions. Add this one to the list.

Comments from Reviewable

isotoxin · 2016-11-12T13:21:34Z

I fixed some @iphydf issues and connection issues (according to the results of Isotoxin usage)

Why the explicit cast?

Because Visual Studio warning. Visual Studio shows lot of warning for almost every toxcore source file. I suppress all warnings of group.c and groupav.c

This function has the same errors as by_uid: uid can be NULL and conference_number can be invalid.

There is no reason to complicate api in this case. Non-NULL uid - is requrement of tox_conference_get_uid call, same as requirement for size of buffer. So, the only one way this function can be failed - invalid conference_number. Return bool value is enough.

zetok · 2016-11-12T14:20:35Z

Groupchats are now saved into tox_save

If state format is modified, add that change to the spec.

Now groupchats are processed as one list of members. It helps to avoid false join to group, when 2 or more groups are requested from same friend.

GrayHatter · 2016-11-18T21:36:42Z

Anyone following this willing to write tests for this?

Otherwise in can't be merged...

iphydf · 2016-11-18T21:52:28Z

I'm mainly concerned with the fact that it doesn't build and some testing/ code no longer compiles. Would be nice to at least get that fixed.

isotoxin · 2016-11-19T08:25:16Z

Just for the sake of my interest, what kind of tests are you expected? Isotoxin uses this code right now and it works fine. You can build m... qTox, for example, with this code and make sure that it works. So, what tests?

GrayHatter · 2016-11-19T08:36:18Z

@isotoxin just the group chat/conference tests that used to work, and the toxcore apps that are bundled.

Like nTox https://travis-ci.org/TokTok/c-toxcore/jobs/176506450#L1227

or the auto_tests https://github.com/TokTok/c-toxcore/blob/master/auto_tests/conference_test.c

uToxingInTox · 2016-11-20T18:37:05Z

Anyone following this willing to write tests for this?
Otherwise in can't be merged...

Writing tests is one of toktok goals, isn't it? So what are you waiting for?

iphydf · 2016-11-28T23:01:30Z

@uToxingInTox I would like to remind you that one of the toktok goals is to have a friendly, professional, and welcoming environment for people to collaborate on common or overlapping goals. Aggression and sneer can't be tolerated in such an environment. Please try to behave in a more appropriate way.

Regarding your premise statement: writing tests is not a goal for toktok. Having a well-tested codebase is. Accepting code that actively counteracts our goals seems like a bad idea. I don't expect new tests, mainly because I think the current auto tests are not a good way to do reliable hermetic testing. They are a good way to do integration tests, but for those, they are insufficient. Breaking the tests and/or breaking the build when adding a feature or cleaning up code doesn't make sense.

And regarding your question: what we are waiting for regarding tests is mainly for us to have time and for that to be prioritised. We have several priorities, and getting a 0.1 release ready in time for the next qTox release (end of December) is currently the highest one. We can have all the tests we want, but if nobody is using the code, there is only so much confidence we can have in that it actually works.

iphydf

Took a first look at the actual group code changes. I didn't have time to go through all of it, so here are some questions and suggestions. I'll take another look once those are addressed.

I'm sorry this takes a long time. I have very limited time, and I can only keep a limited amount of code in working set memory at a time, which is why I usually ask people to make ~300 line changes. This is a 3500 line change, so bear with me while I slowly work my way through it.

iphydf · 2016-11-30T20:36:28Z

toxcore/group.c

- * return 0 if the groupnumber is valid.
- */
-static uint8_t groupnumber_not_valid(const Group_Chats *g_c, int groupnumber)
+typedef ptrdiff_t aint;


What does "aint" stand for, and why does it exist? The rest of the code tends to use explicitly sized ints (int32_t etc.).

Because toxcore can be compiled as 32 and 64 bits. aint (auto int) - just best-for-current-architecture integer. This is just optimization trick. I know, toxcore is not performance-related lib, but this trick can save some bytes of code size.

If you think this optimization is absolutely necessary, I'd prefer if you used the C99 type int_fast32_t intended to make this optimization, else just use int32_t.

iphydf · 2016-11-30T20:36:41Z

toxcore/group.c

+    UNS_TEMP,
+    UNS_FOREVER,
+
+} Unsubscribe;


Unsubscribe is a verb. Types are usually nouns.

iphydf · 2016-11-30T20:37:18Z

toxcore/group.c

+#define GROUP_MESSAGE_NEW_PEER_ID 16
+#define GROUP_MESSAGE_KILL_PEER_ID 17
+#define GROUP_MESSAGE_NAME_ID 48
+#define GROUP_MESSAGE_TITLE_ID 49


Would it make sense for these to be enums as well? Also INVITE_* and PEER_*.

Look to original code. I just moved these macros to one place. Ok, next level is to convert them to enum values.

iphydf · 2016-11-30T20:40:21Z

toxcore/group.c

 {
-    uint32_t i;
+    aint i;


num_chats is uint16_t, so you're comparing a signed int to an unsigned below. It used to be unsigned < unsigned. Please change the type of i to uint16_t or another unsigned type.

It is save to compare 32/64-bit signed index variable with something 16-bit unsigned, because second op automatically cast to type of first op and full range of 16 bit unsigned can be placed into 32 bit signed without loss of bits. Even "i" will be negative, this comparison works correctly and without compiler warnings. Also, no need to force compiler use 16-bit variable for index. Compiler should use whole 32/64 bit register.

iphydf · 2016-11-30T20:42:04Z

toxcore/group.c

    }

-    return &g_c->chats[groupnumber];
+    return g_c->chats + groupnumber;


Why this change? I don't think the different syntax for pointer arithmetic improves clarity here.

Because I do not like addition symbols like &, [, ]

iphydf · 2016-11-30T20:52:21Z

toxcore/group.c

-static uint8_t groupnumber_not_valid(const Group_Chats *g_c, int groupnumber)
+typedef ptrdiff_t aint;
+
+#define CLOSEST( i ) ((g->closest_peers_entry & (1ull << i)) != 0)


Pass g to the macro to make it independent of the current lexical environment, and to clarify at the call site which values are used in the operation.

Technically, you're right. However, in this case not need to be independent of current lexical environment, because it is not common macros. Well, if it's so important, I will add "g" letter to the macro parameters

iphydf · 2016-11-30T20:52:47Z

toxcore/group.c

+typedef ptrdiff_t aint;
+
+#define CLOSEST( i ) ((g->closest_peers_entry & (1ull << i)) != 0)
+#define CLOSESTSET( i ) { g->closest_peers_entry |= (1ull << i); }


Add an underscore somewhere in this macro name to clarify which words it is composed of.

iphydf · 2016-11-30T20:55:02Z

toxcore/group.c

-static int send_packet_online(Friend_Connections *fr_c, int friendcon_id, uint16_t group_num, uint8_t *identifier);
-
-static int connect_to_closest(Group_Chats *g_c, int groupnumber, void *userdata)
+static void apply_changes_in_peers(Group_Chats *g_c, aint groupnumber, void *userdata)


Can you break this function into logical pieces? It's quite long.

Logical pieces are separated by comments. I can add more comments for you

iphydf · 2016-11-30T20:55:44Z

toxcore/group.c

-    } else {
-        if (g->numpeers != (uint32_t)peer_index) {
-            memcpy(&g->group[peer_index], &g->group[g->numpeers], sizeof(Group_Peer));
+            if (peer->friendcon_id == -2 || peer->auto_join || id_equal(g->real_pk, peer->real_pk)) {


Looks like -2 means something. When you add/use a constant, make sure it's used throughout the code.

iphydf · 2016-11-30T20:56:45Z

toxcore/Messenger.c

@@ -2731,6 +2732,11 @@ static int friends_list_load(Messenger *m, const uint8_t *data, uint32_t length)
    return num;
 }

+uint32_t saved_conferences_size(const Messenger *m);


Don't put extern declarations into .c files. These belong in a .h file.

Tell me, what .h file? group.h? Nope. Messenger.c do not include group.h. Messenger.h? Nope. These functions defined in group.c and you will be first who will told me about my mistake. So, what .h file?

GrayHatter · 2016-11-30T21:18:14Z

Against the better judgement of the others on my team. I have taken the liberety to delete posts than are off topic, with the sole goal of making this pull very easy for developers to read parse and communicate. I'd very much like to keep this pull request free of off topip comments or complaints. Because that the only way the outstanding issues can be fixed n a reasonable time so we can merge this.

If it becomes a problem again I can lock this pull to collaborators only.

isotoxin · 2016-12-01T12:22:02Z

Btw, why do you carp to code style? This groupchat upgade is temporary. Yes, temporary. Current groupchat protocol has ugly architecture and hard to improve. It should be totally rewritten. Oh, wait... New groupchats are almost done (sarcasm).
Seriously, I planned to write new groupchats (really new), or make new groupchats of jfr working (unlikely). Anyway, this code is just temporary improvement. It was done only to make Isotoxin better. Better then other lazy-slow-update clients.

sudden6 · 2016-12-01T12:30:34Z

You know the saying "Nothing last longer than a temporary fix." ? ;)

If some code has a completely different style than the rest and is not easy to understand nobody will ever dare to touch it to fix bugs or improve something. It will just rot away. IMO it's the right decision for toxcore to keep the coding standards high, even if this works slightly against fast integration of new features. In the long run it'll probably pay off and will help to keep the security high.

isotoxin · 2016-12-01T12:48:31Z

You know the saying "Nothing last longer than a temporary fix." ? ;)

It's true, especially when you consider your approach to development. You will never do anything new in the code of the toxcore, because you will always be dissatisfied with the code style or lack of understanding of what the code is doing or absence of test or something else.
I'm tired. Let's divide the responsibilities. You will fix the code style as you like, write tests as you like, but I'm going to write code that does something useful.
Sorry for this rude post. Ah. Simple question: does qTox use toktok? No? Why is all this fuss?

sudden6 · 2016-12-01T13:26:34Z

Simple question: does qTox use toktok?
No qTox doesn't yet use toktok, but the plan is to switch on the next release. Btw I wasn't speaking for qTox, just for myself.

Why is all this fuss?
Because toxcore is about a stable and compatible basis between all clients. Again speaking only for myself.

iphydf · 2016-12-01T15:54:35Z

@isotoxin ok, dividing responsibilities this way sounds reasonable. @robinlinden would you be willing to take this code, make a new PR, and go through the review process with it? We can fall back on @isotoxin when in doubt about a particular change.

Regarding your question: no, qTox is waiting for the 0.1 release, which is why that is currently the highest priority. We can have all the tests we want, but without anybody actually using it, it's both useless and hard to gain confidence in the code actually doing what our users want.

iphydf · 2016-12-01T16:04:20Z

@GrayHatter or you?

GrayHatter · 2016-12-01T18:26:27Z

@iphydf no. At this point i'm not able to take on additional jobs I have too many out standing.

That said per our IRC conversation I dont want to sink any more time into old style group chats when we have a seemingly better implementation waiting for the net_crypto fix that's needed for more than just new group chats.

If @isotoxin Is unwilling or unable to put the time into HIS pull request. I dont think we should bother to spend our already limited time either.

Because I dont have the time to monitor this thread I'm going to lock it to collaborators only. @isotoxin if you'd like to make further comments/fixes on this pull find me on IRC and I or anyone can unlock it. But as you've said twice now, you dont want to put in the work to bring the code quality up I'd rather not let this sink into a flame war ntreod.

Any toktok devs if you'd like to take over the work on this. I'd suggest you open a new pull

iphydf · 2016-12-01T19:50:07Z

Thanks @isotoxin for contributing this change. We'll try to proceed without bothering you too much with questions. I've invited you to join as toktok collaborator so you can continue contributing to the discussion here if you wish to do so. We are proceeding this way following your recommendation to divide responsibilities. I hope we understood your intention correctly.

Thanks @robinlinden for taking on the review process on behalf of @isotoxin.

I suggest @isotoxin closes this PR if nothing remains to be done here. I would also like to ask everybody involved in future discussions to stay on the topic of the contribution. We can discuss development processes and prioritisation elsewhere.

GrayHatter · 2016-12-12T09:55:50Z

with over 10 days, and no updates I'm going to close this as replaced by #295

SkyzohKey added CAT:code_cleanup enhancement New feature for the user, not a new feature for build script security Security messenger Messenger toxav Audio/video P2 Medium priority and removed P2 Medium priority labels Nov 11, 2016

SkyzohKey added this to the v0.0.5 milestone Nov 11, 2016

SkyzohKey assigned GrayHatter, dvor, JFreegman, tux3 and iphydf Nov 11, 2016

SkyzohKey added PR:changes_needed and removed PR:review_needed enhancement New feature for the user, not a new feature for build script labels Nov 11, 2016

remove unused variables

3990406

iphydf changed the title ~~upgrade groupchats~~ Make conferences persist across restarts Nov 11, 2016

connection and other issues fixed

4317571

GrayHatter removed this from the v0.0.5 milestone Nov 14, 2016

Restore algorithm has been changed

7e75346

Now groupchats are processed as one list of members. It helps to avoid false join to group, when 2 or more groups are requested from same friend.

iphydf requested changes Nov 30, 2016

View reviewed changes

isotoxin added 2 commits December 1, 2016 15:02

cosmetics

fd25759

spaces

59dc77c

TokTok locked and limited conversation to collaborators Dec 1, 2016

GrayHatter closed this Dec 12, 2016

iphydf modified the milestones: v0.2.0, meta Mar 26, 2017

iphydf unassigned GrayHatter Mar 26, 2017

iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics and removed code cleanup labels May 4, 2020

Make conferences persist across restarts #263

Make conferences persist across restarts #263

Conversation

isotoxin commented Nov 11, 2016 • edited Loading

SkyzohKey commented Nov 11, 2016 • edited Loading

SkyzohKey commented Nov 11, 2016

isotoxin commented Nov 11, 2016

dvor commented Nov 11, 2016

SkyzohKey commented Nov 11, 2016

isotoxin commented Nov 11, 2016

iphydf commented Nov 11, 2016

isotoxin commented Nov 12, 2016

zetok commented Nov 12, 2016

GrayHatter commented Nov 18, 2016

iphydf commented Nov 18, 2016

isotoxin commented Nov 19, 2016

GrayHatter commented Nov 19, 2016

uToxingInTox commented Nov 20, 2016

iphydf commented Nov 28, 2016

iphydf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrayHatter commented Nov 30, 2016

isotoxin commented Dec 1, 2016 • edited Loading

sudden6 commented Dec 1, 2016

isotoxin commented Dec 1, 2016

sudden6 commented Dec 1, 2016

iphydf commented Dec 1, 2016

iphydf commented Dec 1, 2016

GrayHatter commented Dec 1, 2016

iphydf commented Dec 1, 2016 • edited Loading

GrayHatter commented Dec 12, 2016

isotoxin commented Nov 11, 2016 •

edited

Loading

SkyzohKey commented Nov 11, 2016 •

edited

Loading

isotoxin commented Dec 1, 2016 •

edited

Loading

iphydf commented Dec 1, 2016 •

edited

Loading