Add ability to disable UDP hole punching

[GrayHatter]

This change is

[GrayHatter]

closes #254

[jhert0]

---

Review status: 0 of 14 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Review status: 0 of 14 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

auto_tests/dht_test.c, line 306 at r1 (raw file):

    ck_assert_msg(net != 0, "Failed to create Networking_Core");

    DHT *dht = new_DHT(NULL, net. 2);

2 is not a member of net. You meant ,.

---

auto_tests/dht_test.c, line 443 at r1 (raw file):

        ip_init(&ip, 1);

        dhts[i] = new_DHT(NULL, new_networking(NULL, ip, DHT_DEFAULT_PORT + i ), 2);

Did this run through astyle?

---

other/bootstrap_daemon/src/tox-bootstrapd.c, line 246 at r1 (raw file):

    }

    DHT *dht = new_DHT(NULL, net, 2);

You can include tox.h and use the symbolic constant for NORMAL here and in other places where you added a literal 2.

---

toxcore/DHT.c, line 2093 at r2 (raw file):

    }

    if (!dht->nat_hole_punching_level) {

I suggest using == 0 explicitly, because later we'll have == 1 and others (or named constants).

---

toxcore/DHT.h, line 241 at r1 (raw file):

    Networking_Core *net;

    uint8_t     nat_hole_punching_level;

Align with other members or remove alignment altogether.

---

toxcore/tox.api.h, line 367 at r1 (raw file):

  /**
   * Only try small port ranges, and wait a long time between attempts.
   * (This Level Currently Unsupported; defaults to NORMAL)

No need to capitalise. I suggest: This level is currently unsupported and is equivalent to $NORMAL. without parentheses.

---

toxcore/tox.api.h, line 371 at r1 (raw file):

  POLITE,
  /**
   * Try to establing a direct UDP connection through any supported NAT device.

• "establish"
• This should probably mention again that it's doing that using UDP hole-punching.

---

toxcore/tox.api.h, line 376 at r1 (raw file):

  NORMAL,
  /**
   * Agressivly try to establish a direct connection to all friends.

"Aggressively"

---

toxcore/tox.api.h, line 377 at r1 (raw file):

  /**
   * Agressivly try to establish a direct connection to all friends.
   * USE WITH CAUTION, this may interferr with some poorly configured NAT devices

"interfere"

---

toxcore/tox.api.h, line 379 at r1 (raw file):

   * USE WITH CAUTION, this may interferr with some poorly configured NAT devices
   * leading to a denial of service effect to all devices behind the NAT.
   * (This Level Currently Unsupported; defaults to NORMAL)

See above.

---

toxcore/tox.api.h, line 381 at r1 (raw file):

   * (This Level Currently Unsupported; defaults to NORMAL)
   */
  AGRESSIVE,

AGGRESSIVE

---

toxcore/tox.api.h, line 527 at r1 (raw file):

       * and end_port were swapped.
       */
      uint16_t start_port;

Is this only DHT? I thought this port is also used for all other communications (net_crypto).

---

toxcore/tox.api.h, line 548 at r1 (raw file):

      /**
       * How agressive toxcore will be when trying to establish a direct UDP

I suggest: "How aggressively toxcore will try to [...]". On the first read I got a bit confused. If others disagree, fix the typo: "aggressive".

---

toxcore/tox.api.h, line 551 at r1 (raw file):

       * connection with friends through a Network Address Translation device.
       *
       * Care must be used when changing this from the default as too low a level

"Care must be taken"

However, this sentence is too fancy for technical documentation. I don't think you need to tell people to take care if you tell them the effects of low/high levels. They will know what the effects are, so they will know that they need to take care.

---

toxcore/tox.api.h, line 553 at r1 (raw file):

       * Care must be used when changing this from the default as too low a level
       * could stop toxcore from creating a direct connection to friends, and too
       * high a level could cause a DOS effect, and disrupt ALL network traffic for

"DoS"

---

Comments from Reviewable

[iphydf]

Reviewed 13 of 14 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[GrayHatter]

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

auto_tests/dht_test.c, line 306 at r1 (raw file):

Previously, iphydf wrote…

2 is not a member of net. You meant ,.

Done.

---

auto_tests/dht_test.c, line 443 at r1 (raw file):

Previously, iphydf wrote…

Did this run through astyle?

Done.

---

other/bootstrap_daemon/src/tox-bootstrapd.c, line 246 at r1 (raw file):

Previously, iphydf wrote…

You can include tox.h and use the symbolic constant for NORMAL here and in other places where you added a literal 2.

Done.

---

toxcore/DHT.c, line 2093 at r2 (raw file):

Previously, iphydf wrote…

I suggest using == 0 explicitly, because later we'll have == 1 and others (or named constants).

Done.

---

toxcore/DHT.h, line 241 at r1 (raw file):

Previously, iphydf wrote…

Align with other members or remove alignment altogether.

Done.

---

toxcore/tox.api.h, line 367 at r1 (raw file):

Previously, iphydf wrote…

No need to capitalise. I suggest: This level is currently unsupported and is equivalent to $NORMAL. without parentheses.

Done.

---

toxcore/tox.api.h, line 371 at r1 (raw file):

Previously, iphydf wrote…

• "establish"
• This should probably mention again that it's doing that using UDP hole-punching.

Done.

---

toxcore/tox.api.h, line 376 at r1 (raw file):

Previously, iphydf wrote…

"Aggressively"

Done.

---

toxcore/tox.api.h, line 377 at r1 (raw file):

Previously, iphydf wrote…

"interfere"

Done.

---

toxcore/tox.api.h, line 379 at r1 (raw file):

Previously, iphydf wrote…

See above.

Done.

---

toxcore/tox.api.h, line 381 at r1 (raw file):

Previously, iphydf wrote…

AGGRESSIVE

Done.

---

toxcore/tox.api.h, line 527 at r1 (raw file):

Previously, iphydf wrote…

Is this only DHT? I thought this port is also used for all other communications (net_crypto).

It COULD be both, but the other is IMO misleading, as everything having to do with ports is started, or handled by the DHT. Yes Networking_Core is technically lower, but this way, if we change something later, we don't have to break the api to add something that uses separate ports from the DHT.

---

toxcore/tox.api.h, line 548 at r1 (raw file):

Previously, iphydf wrote…

I suggest: "How aggressively toxcore will try to [...]". On the first read I got a bit confused. If others disagree, fix the typo: "aggressive".

Done.

---

toxcore/tox.api.h, line 551 at r1 (raw file):

Previously, iphydf wrote…

"Care must be taken"

However, this sentence is too fancy for technical documentation. I don't think you need to tell people to take care if you tell them the effects of low/high levels. They will know what the effects are, so they will know that they need to take care.

rewirtten

---

toxcore/tox.api.h, line 553 at r1 (raw file):

Previously, iphydf wrote…

"DoS"

Done.

---

Comments from Reviewable

[iphydf]

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

auto_tests/dht_test.c, line 306 at r3 (raw file):

    ck_assert_msg(net != 0, "Failed to create Networking_Core");

    DHT *dht = new_DHT(NULL, net, 2);

Tests can include tox.h. Use it to get the NORMAL constant here (and in other places).

---

toxcore/DHT.c, line 2093 at r3 (raw file):

    }

    // TODO(toktok/c-toxcore#214)

I'd like a short statement about what is to do here. Given that nat_hole_punching_level is a uint8_t, it's not immediately clear what should be done here.

---

toxcore/DHT.c, line 2094 at r3 (raw file):

    // TODO(toktok/c-toxcore#214)
    switch (dht->nat_hole_punching_level) {

Isn't this check a bit too late? do_NAT executes and (maybe?) sends NAT pings and does other stuff that it doesn't need to do when hole punching is disabled. You may want an additional == 0 check in there to completely disable any do_NAT activity.

Do check whether this suggestion makes sense. I haven't checked it thoroughly.

---

toxcore/tox.api.h, line 551 at r3 (raw file):

      /**
       * Sets how aggressive toxcore will be when trying to establish a direct UDP
       * connection "hole punching" with friends when behind a NAT device.

I think some sentence merge conflict happened here.

---

toxcore/tox.api.h, line 553 at r3 (raw file):

       * connection "hole punching" with friends when behind a NAT device.
       *
       * Clients should be aware of the implications of changing this from it's default.

"its"

---

toxcore/tox.api.h, line 554 at r3 (raw file):

       *
       * Clients should be aware of the implications of changing this from it's default.
       * Too low a level could stop toxcore from creating a direct connections, and too

"Setting it too low could [...]".

---

Comments from Reviewable

[GrayHatter]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

auto_tests/dht_test.c, line 306 at r3 (raw file):

Previously, iphydf wrote…

Tests can include tox.h. Use it to get the NORMAL constant here (and in other places).

Done.

---

toxcore/DHT.c, line 2093 at r3 (raw file):

Previously, iphydf wrote…

I'd like a short statement about what is to do here. Given that nat_hole_punching_level is a uint8_t, it's not immediately clear what should be done here.

Done.

---

toxcore/DHT.c, line 2094 at r3 (raw file):

Previously, iphydf wrote…

Isn't this check a bit too late? do_NAT executes and (maybe?) sends NAT pings and does other stuff that it doesn't need to do when hole punching is disabled. You may want an additional == 0 check in there to completely disable any do_NAT activity.

Do check whether this suggestion makes sense. I haven't checked it thoroughly.

I could, and it would make sense in this change, but I don't think I should. Toxcore will first try to connect on the port suggested by other nodes who say they're already connected to the target peer.

This change won't stop toxcore from trying to connect via UDP, it only stops it from punching extra holes. Which is what we want.

If you want to disable ANY kind of DHT connections, you can just disable UDP which will give the effect in your first comment.

---

toxcore/tox.api.h, line 551 at r3 (raw file):

Previously, iphydf wrote…

I think some sentence merge conflict happened here.

Done.

---

toxcore/tox.api.h, line 553 at r3 (raw file):

Previously, iphydf wrote…

"its"

Done.

---

toxcore/tox.api.h, line 554 at r3 (raw file):

Previously, iphydf wrote…

"Setting it too low could [...]".

Done.

---

Comments from Reviewable

[GrayHatter]

This pull is reliably eating toxcore profiles. in the uTox development branch.

Can someone else confirm?

Also, backup any profile you care about before trying

[nurupo]

So, I assume this won't get merged because of profile eating?

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 2 files at r5.
Review status: 13 of 14 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Pushing to v0.0.5 because this exceeds the scope of the bug, which was to add an option to disable hole punching, while this PR introduces aggressiveness levels and breaks the API by renaming fields in Tox_Options.

[GrayHatter]

I reverted a lot of the changes, this pull in simply an addition of the bool, hole_punching enabled, disabled

---

Review status: 1 of 13 files reviewed at latest revision, 9 unresolved discussions.

---

toxcore/tox.api.h, line 527 at r1 (raw file):

Previously, nurupo wrote…

It makes more sense to rename it to udp_start_port then. That would require renaming the tcp_port to relay_server_tcp_port, because keeping it as tcp_port might make people think it used for the same thing as the udp_start_port/udp_end_port but just using different protocol, which would be incorrect.

reverted to a simple pull

---

toxcore/tox.h, line 586 at r4 (raw file):

Previously, nurupo wrote…

Why this rename? This port is used not only for DHT.

reverted

---

toxcore/tox.h, line 592 at r4 (raw file):

Previously, nurupo wrote…

Why this rename? This port is used not only for DHT.

reverted

---

Comments from Reviewable

[zetok]

Review status: 1 of 12 files reviewed at latest revision, 10 unresolved discussions.

---

toxcore/tox.api.h, line 524 at r6 (raw file):

     * Enables or disables UDP hole-punching in toxcore, (Default: enabled).
     */
    bool nat_level;

It's not clear what the bool is about just from the bool name. Shouldn't it be renamed to something else?

---

Comments from Reviewable

[nurupo]

Reviewed 11 of 13 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

toxcore/tox.api.h, line 522 at r6 (raw file):

    /**
     * Enables or disables UDP hole-punching in toxcore, (Default: enabled).

Should we be more explicit about how the true/false maps to "enables"/"disables"? Reading this I could guess that setting nat_level = true would enable hole-punching, but I don't like to guess.

---

toxcore/tox.api.h, line 524 at r6 (raw file):

Previously, zetok wrote…

It's not clear what the bool is about just from the bool name. Shouldn't it be renamed to something else?

`nat_level` is a weird name for disabling/enabling hole punching. What is a "nat level" anyway?`nat_holepunching_enabled` or `nat_holepunching` might be better.

Also, are we sure that we won't ever want to extend this to hold more than 2 values (true/false)? Using an enum might be more preferable API-compatibility-wise if we decide to extend this in the future. As a bonus, even if we never get to extend it, it makes the client code more readable, e.g. new_DHT(NULL, net, TOX_NAT_HOLEPUNCHING_NORMAL); vs new_DHT(NULL, net, true);.

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

toxcore/tox.api.h, line 522 at r6 (raw file):

    /**
     * Enables or disables UDP hole-punching in toxcore, (Default: enabled).

Replace comma with period.

---

toxcore/tox.api.h, line 524 at r6 (raw file):

Previously, nurupo wrote…

nat_level is a weird name for disabling/enabling hole punching. What is a "nat level" anyway?nat_holepunching_enabled or nat_holepunching might be better.

Also, are we sure that we won't ever want to extend this to hold more than 2 values (true/false)? Using an enum might be more preferable API-compatibility-wise if we decide to extend this in the future. As a bonus, even if we never get to extend it, it makes the client code more readable, e.g. new_DHT(NULL, net, TOX_NAT_HOLEPUNCHING_NORMAL); vs new_DHT(NULL, net, true);.

For now, we're sure, but anyway this bool is going away very soon, when it becomes part of the "nat traversal technologies" enum by @Ansa89.

---

Comments from Reviewable

[GrayHatter]

Review status: 8 of 12 files reviewed at latest revision, 4 unresolved discussions.

---

toxcore/tox.api.h, line 522 at r6 (raw file):

Previously, iphydf wrote…

> Replace comma with period.

Done.

---

toxcore/tox.api.h, line 522 at r6 (raw file):

Previously, nurupo wrote…

> Should we be more explicit about how the true/false maps to "enables"/"disables"? Reading this I could guess that setting `nat_level = true` would enable hole-punching, but I don't like to guess.

Done.

---

toxcore/tox.api.h, line 524 at r6 (raw file):

Previously, iphydf wrote…

> For now, we're sure, but anyway this bool is going away very soon, when it becomes part of the "nat traversal technologies" enum by @Ansa89.

renamed, and per IRC conversation, it will eventually be a typedef enum, but not this time.

---

Comments from Reviewable

[iphydf]

---

Reviewed 7 of 13 files at r6, 2 of 4 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[iphydf]

Review status: 10 of 12 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

toxcore/Messenger.c, line 1937 at r9 (raw file):

    }

    m->dht = new_DHT(m->log, m->net, options->nat_level);

hole_punching_enabled

---

Comments from Reviewable

[nurupo]

Do we need some kind of auto-test for this new functionality?

Otherwise it looks good.

---

Reviewed 2 of 2 files at r8, 2 of 2 files at r9, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[iphydf]

I don't think it's possible to auto-test it.