Make bootstrap daemon use toxcore's version

[nurupo]

I have wanted to make the daemon report toxcore's version for quite a bit now, and here I finally I got around to doing this. The main motivation for this was the onion vulnerability #873. To fix the vulnerability, all DHT nodes should be updated to the new upcoming toxcore version. Reporting toxcore version in the bootstrap daemon would enable us to see at the very least if the bootstrap nodes are updated, my main interest being the the nodes that are listed on the https://nodes.tox.chat page which displays the daemon version of the nodes.

The daemon version is just a uint32_t, there is not much you can encode in 4 bytes. I have been thinking of packing the toxcore version into that uint32_t, 1 byte for major, 1 byte for minor, 1 byte for patch and 1 byte for misc: bit indicating that this is a new version scheme, a bit indicating that the daemon was build using a release checkpoint of toxcore rather than an in-development (there might be big differences between 2.x.y release and the same 2.x.y right before the version bump, containing all the changes for the next version). Such version encoding would be inconsistent with the previous one of YYYYMMDD, which you could print out as an number to show the version to the user, which is what nodes.tox.chat and a few other scrips do. With the packed version encoding the version would need to be decoded first, if you print the uint32_t as it is, it won't make sense. So to keep the compatibility with the way the daemon version is being printed by others I settled on a 1AAABBBCCC format, where AAA is the major version, BBB is the minor version and CCC is the patch version, all capped at 999. The leading 1 is just so that the leading zeros wouldn't get removed when printing it as a number, e.g. instead of 0.2.1 being printed as 2001, it would be printed as 1000002001, i.e. 000.002.001.

---

This change is 

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[kpp]

You are overcomplicating the issue.

Previously it used to be define DAEMON_VERSION_NUMBER 2016010100UL. Nothing stops you from updating it to define DAEMON_VERSION_NUMBER 2018041700UL .

Don't forget you are not the only one who is implementing toxcore. These changes are changes in protocol and must be discussed with all implementors of tox.

[iphydf]

The bootstrap Daemon version response isn't part of the specified protocol. It should be, but until then the implementation is the definition. I think we can make this change and then write it down so others can implement it.

[kpp]

The bootstrap Daemon version response isn't part of the specified protocol.

That is not the reason to make your implementation become the part of the specified protocol since there are a lot of programs that check the version of tox with the given packet. You are going to break them. Why?

My suggestion is to write down a document that standardize the current situation. If 4 bytes is not enough for you we may keep 128 bytes of version info.

Please stop juggling with bits and bytes. You have done enough which led to #873

[iphydf]

Yes, agree. We should make sure those programs support the new version format before releasing this change. Do you know of other programs besides the node list?

[kpp]

Do you know of other programs besides the node list?

https://github.com/irungentoo/toxcore/blob/master/other/fun/bootstrap_node_info.py
https://github.com/TokTok/c-toxcore/blob/master/other/fun/bootstrap_node_info.py
https://github.com/tox-rs/tox/blob/master/src/toxcore/dht/packet.rs#L1095

The vulnerability was found in tox-rs/tox, as it was described by Evgeniy in #873. You are not the only one implementors of tox in the world.

[nurupo]

Previously it used to be define DAEMON_VERSION_NUMBER 2016010100UL. Nothing stops you from updating it to define DAEMON_VERSION_NUMBER 2018041700UL.

Right, I could update it like that, but it would be more meaningful if the version reflected the toxcore version instead. The daemon usually doesn't change much at all, it can stay for a year without a single change aside from refactoring, yet toxcore changes a lot faster and changes the daemon's behavior, like the recent DHT changes in toxcore aimed to reduce the bandwidth consumption.

Don't forget you are not the only one who is implementing toxcore. These changes are changes in protocol and must be discussed with all implementors of tox.

It would make sense to extend the daemon's version information to include daemon name and version and the name and version of the toxcore implementation it uses then. Don't think this is going to fit into 4 bytes though, we'd likely need some string for that, so some spec change is needed.

Please stop juggling with bits and bytes. You have done enough which led to #873

Are you asking everyone to stop coding? Now that's rude and inappropriate of you.

[kpp]

Right, I could update it like that

Would you please do it!

It would make sense to extend the daemon's version information to include daemon name and version and the name and version of the toxcore implementation it uses then.

Then let's do it! Make 2 new packets, call them REQ_VERSION, RESP_VERSION, let them be in semver syntax (128 bytes) + date + 128 byte comment. What stops you from making things like that?

[nurupo]

That is not the reason to make your implementation become the part of the specified protocol since there are a lot of programs that check the version of tox with the given packet. You are going to break them. Why?

The bootstrap version is defined just as a 32 bit word in the spec, the spec doesn't specify versioning scheme, it's up to an implementation, it could be anything, there could be another bootstrap daemon implementation somewhere that doesn't follow tox-boostrapd's versioning scheme of YYYYMMDDVV. Also, all programs that check the bootstrap version that I know of: bootstrap_node_info and Tox Status treat it as an integer without any regards to its scheme just as the spec defines it. What I have proposed doesn't break the spec nor those programs printing the version, it's still a 32 bit word as defined in the spec. So, what breakage are you talking about here exactly?

[kpp]

The bootstrap version is defined just as a 32 bit word in the spec

So will you update docs according to your PR or no? If you will not then it is fine for you to make any changes. Have fun!

[sudden6]

This field seems to specify only informational stuff and not something code should depend on (yet), so I think @nurupo is right that it should be mostly human readable. In a future version I'd like to have a better specified versioning scheme + a comment/implementation name string.

[nurupo]

Then let's do it! Make 2 new packets, call them REQ_VERSION, RESP_VERSION, let them be in semver syntax (128 bytes) + date + 128 byte comment. What stops you from making things like that?

I don't think that there is anything that stops anyone from proposing changes to the spec. Is there?

So will you update docs according to your PR or no? If you will not then it is fine for you to make any changes. Have fun!

If what you want is to improve the spec by making the definition of a bootstrap version more detailed than a 32 bit word, i.e. define the exact versioning scheme that all bootstrap daemons need to follow, instead of leaving it up to an implementation, or define a new bootstrap version packet, you should open a discussion/proposal thread against the spec repository and discuss it in there, instead of discussing it in a PR that makes a spec compatible and non-breaking change to an implementation of a bootstrap daemon. What stops you from doing that?

[sudden6]

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[iphydf]

This is compliant with the spec as written.

---

Reviewed 1 of 1 files at r1.
Review status: complete! 2 of 1 LGTMs obtained

---

Comments from Reviewable

[iphydf]

Please rebase onto master and optionally fast-forward master onto it.

[nurupo]

@iphydf rebased.