fix: DHTBootstrap should always respond to version packets with toxcore version

[Tha14]

The current implementation does not allow responding to bootstrap node packets which results in some of the nodes in nodes.tox.chat to not have a version. This also means that we do not know which of those nodes are actually up-to-date and which are not, making things complicated when it comes to contacting the server owners.

---

This change is 

[nurupo]

I'd say that it's good that it reports "1" as the version. While DHTBootstrap is fine for local testing purposes, we don't want people use DHTBootstrap for hosting real bootstrap nodes, as it doesn't even offer TCP relay server service. With the version being "1" it's easy to detect someone running DHTBootstrap whenever someone submits a node to nodes.tox.chat and ask them to switch to using tox-bootstrapd instead, as that is what we want everyone to run.

[nurupo]

Oh, I see it does support the relay server now. I guess it has been a while since I have looked into DHTBootstrap's code.

[Tha14]

@nurupo I have people asking me frequently on how to host the bootstrap node on windows. I'd assume we can support the windows version since it works properly with the correct arguments passed. And yes, it does support all the basic stuff now. I'm writing a doc for toxcore on the proper steps to take in order to setup a bootstrap node and telling people that they can't use windows is a bit weird. If you think there is no reason to do any of it then let me know.

[nurupo]

Yep, the only reason to use DHTBootstrap is either for testing or if you are on Windows, as tox-bootstrapd is UNIX-only.

I wonder if there is a similar issue with the maximum number of sockets open on Windows that tox-bootstrapd resolves by increasing the file descriptor limit. DHTBootstrap is not very battle-tested, especially on Windows, we treated it mostly as a toy bootstrap node for testing purposes, so I wouldn't be surprised if there are issues like these that pop up.

I'm writing a doc for toxcore on the proper steps to take in order to setup a bootstrap node

Something like this? https://wiki.tox.chat/users/runningnodes

Also, note that there is already a document describing how to setup tox-bootstrapd, so just pointing to that document should be enough, no need to duplicate the same information in multiple places that might get out of sync over time.

[nurupo]

other/DHT_bootstrap.c line 161 at r1 (raw file):

#ifdef DHT_NODE_EXTRA_PACKETS
    bootstrap_set_callbacks(dht_get_net(dht), (uint32_t)DAEMON_VERSION_NUMBER, (const uint8_t *) motd, strlen(motd));

Should be strlen(motd)+1, so that we also transmit the null terminator. sizeof() that was here before returned the entire size of the string, including the null terminator, while strlen() returns one less,as it doesn't count the null terminator.

[Green-Sky]

I wonder if there is a similar issue with the maximum number of sockets open on Windows that tox-bootstrapd resolves by increasing the file descriptor limit. DHTBootstrap is not very battle-tested, especially on Windows, we treated it mostly as a toy bootstrap node for testing purposes, so I wouldn't be surprised if there are issues like these that pop up.

excerpt form stackoverflow:

However for a single process that uses the default C run-time libraries then the default limit is 512

this means yes, you can only serve 512 simultaneously. or worse, with the "handshake done, no data sent" /attack/ it would be 512-256=256

edit: but it might not be as bad?

For network connection the maximum number of open files per session is 16 384. This can be checked with the net config server command

[nurupo]

Just to reiterate, I'm not opposed to this PR, it's blocked only by this off-by-one error and GitHub checks / CI build failures. Well, it is not technically an error, as sending a 0 length string should be allowed, it's that everything: DHTBootstrap and tox-bootstrapd, always sent a 1-length null string for an empty string, so I'm just wary of unnecessary breaking anything that might rely on this behavior when reading the packet.

[emdee-is]

Tha can you fix #2330 while you're in there; only a couple of lines in the README (not INSTALL). Thanks.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a1d5518) 74.46% compared to head (9d88c8f) 74.48%.
Report is 1 commits behind head on master.

❗ Current head 9d88c8f differs from pull request most recent head 06d949a. Consider uploading reports for the commit 06d949a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2354      +/-   ##
==========================================
+ Coverage   74.46%   74.48%   +0.02%     
==========================================
  Files          87       87              
  Lines       26211    26211              
==========================================
+ Hits        19517    19523       +6     
+ Misses       6694     6688       -6     

see 11 files with indirect coverage changes

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

[nurupo]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @Tha14)

[iphydf]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @Tha14)