TokTok / c-toxcore

The future of online communications.
https://tox.chat
GNU General Public License v3.0
2.21k stars 280 forks source link

feat: Add option to disable DNS lookups in toxcore. #2694

Open iphydf opened 4 months ago

iphydf commented 4 months ago

Allows clients to prevent leaking IP addresses through DNS lookups. This option, together with disabling Tox UDP, entirely prevents any UDP packets being sent by toxcore.


This change is Reviewable

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Project coverage is 73.10%. Comparing base (fa20168) to head (df6d87e). Report is 5 commits behind head on master.

Files Patch % Lines
toxcore/network.c 55.55% 4 Missing :warning:
toxcore/DHT.c 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2694 +/- ## ========================================== + Coverage 73.03% 73.10% +0.07% ========================================== Files 149 149 Lines 30531 30537 +6 ========================================== + Hits 22298 22325 +27 + Misses 8233 8212 -21 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emdee-is commented 4 months ago

This should be labeled with api-break as changes it the fields in toxOptions.

Green-Sky commented 4 months ago

This should be labeled with api-break as changes it the fields in toxOptions.

maybe, direct usage of the options struct is deprecated.

iphydf commented 4 months ago

https://github.com/TokTok/c-toxcore/blob/master/toxcore/tox.h#L526-L527

The memory layout of this struct (size, alignment, and field order) is not part of the ABI.

emdee-is commented 4 months ago

https://github.com/TokTok/c-toxcore/blob/master/toxcore/tox.h#L526-L527

The memory layout of this struct (size, alignment, and field order) is not part of the ABI.

Adding getters and setters is a part of the ABI in semantic versionsing: https://github.com/TokTok/c-toxcore/issues/2702

iphydf commented 4 months ago

https://github.com/TokTok/c-toxcore/blob/master/toxcore/tox.h#L526-L527

The memory layout of this struct (size, alignment, and field order) is not part of the ABI.

Adding getters and setters is a part of the ABI in semantic versionsing: https://github.com/TokTok/c-toxcore/issues/2702

Can you quote and link to a spec of semantic versioning that confirms this?

emdee-is commented 4 months ago

Adding getters and setters is a part of the ABI in semantic versionsing: #2702

Can you quote and link to a spec of semantic versioning that confirms this?

I've QAed Python for 2 of the biggest companies in the world, and they have coding standards that specify this sort of thing (in one of them I wrote the first draft for Python), but I can't cite their internal documents. In brief:

Adding getters and setters is adding new funtionality, so must be in a new minor. I also know this is the case for the Python project from having submitted a module into the Python standard library: their developers all know the rules and follow them strictly.

You're "releasing" big aglomerations rarely (years) and features go in without notice or planning or discussion. I assume that this may be the reason the project has lost so many developers and GUIs.

iphydf commented 4 months ago

I understand now where the confusion is. Let me clarify: in toxcore, we've decided to use semver without patch, and shifted the version right by 1 position: 0.major.minor, and we have no patch level releases. This is documented in tox.h.

That is, until we release 1.0.0, at which point it'll be semver with patch.

emdee-is commented 4 months ago

You're right - I missed that comment.

iphydf commented 3 months ago

I approve the code changes, but why is the behavoir explained inverted. Why is dns_enabled explained by the not state?

Because it's enabled by default, and has been true forever, so I figured it would be clearer to existing client devs if we explain what happens if you disable it explicitly.

sh3ll4rpw commented 1 week ago

When this will be merged? I'm waiting for a long time