Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 17, 2024

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

@iphydf iphydf added this to the v0.2.20 milestone Feb 17, 2024
@iphydf iphydf force-pushed the disable-dns branch 6 times, most recently from deae4d9 to f1045da Compare February 17, 2024 20:32
Copy link

codecov bot commented Feb 17, 2024

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 19 commits behind head on master.

Files with missing lines Patch % Lines
toxcore/network.c 55.55% 4 Missing ⚠️
toxcore/DHT.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emdee-is
Copy link

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

@Green-Sky
Copy link
Member

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

maybe, direct usage of the options struct is deprecated.

@iphydf
Copy link
Member Author

iphydf commented Feb 17, 2024

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.

toxcore/tox.h Outdated Show resolved Hide resolved
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 13 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@emdee-is
Copy link

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: #2702

@iphydf
Copy link
Member Author

iphydf commented Feb 21, 2024

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: #2702

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

@emdee-is
Copy link

emdee-is commented Feb 22, 2024

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:

    1. MAJOR version when you make incompatible API changes,
    1. MINOR version when you add functionality in a backwards compatible manner, and
    1. PATCH version when you make backwards compatible bug fixes

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
Copy link
Member Author

iphydf commented Feb 22, 2024

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
Copy link

You're right - I missed that comment.

@iphydf iphydf force-pushed the disable-dns branch 2 times, most recently from f4cbc97 to df6d87e Compare March 28, 2024 16:19
@iphydf iphydf marked this pull request as ready for review March 28, 2024 16:19
Copy link
Member

@Green-Sky Green-Sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@iphydf
Copy link
Member Author

iphydf commented Mar 28, 2024

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.

nurupo
nurupo previously requested changes Mar 29, 2024
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @iphydf)


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

     *
     * Default: true. May become false in the future (0.3.0).
     */

This comment block is exceeding the 80-column width.

@sh3ll4rpw
Copy link

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

@iphydf iphydf modified the milestones: v0.2.20, v0.2.21 Nov 8, 2024
@github-actions github-actions bot added the enhancement New feature for the user, not a new feature for build script label Nov 8, 2024
@iphydf iphydf force-pushed the disable-dns branch 2 times, most recently from 8063f4d to 9922a0d Compare November 12, 2024 21:07
@iphydf iphydf force-pushed the disable-dns branch 3 times, most recently from b78ea61 to fea022b Compare November 27, 2024 11:59
Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @nurupo)


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

Previously, nurupo wrote…

This comment block is exceeding the 80-column width.

Done.

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.
@iphydf iphydf dismissed nurupo’s stale review November 27, 2024 19:15

I've addressed the comment.

@iphydf iphydf merged commit 819aa2b into TokTok:master Nov 27, 2024
62 checks passed
@iphydf iphydf deleted the disable-dns branch November 27, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature for the user, not a new feature for build script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants