Add: pass upload date to OpenTTD client

[perezdidac]

Implementing logic to pass the "upload-date" field to OpenTTD so we could explore the ability of displaying it in the NewGRF window as well as sorting by upload date in the client as well.

I have tested it by making the corresponding changes in OpenTTD to receive the value, and here it is:

In particular, I've focused on making sure this doesn't break older versions. Here is what I have successfully tested:

1. Bananas returning the new field, OpenTTD not knowing about it -> OpenTTD ignores it as it has read the latest field it knows about it (tags) and moves on to the next NewGRF. No memory leak or anything out of normal.
2. Bananas returning the new field, OpenTTD knowing about it -> OpenTTD reads it successfully.
3. Bananas not returning the new field, OpenTTD expecting it -> OpenTTD tries to read it but does not actually read it. I used CanReadFromPacket() to verify and make it safe in case we have to rollback Bananas and newer OpenTTD clients still want the value.

[TrueBrain]

Note: I have no idea if this will work or it will blow everything up since I haven't been able to successfully run bananas-server locally, but at least I can get some attention from @TrueBrain on this :)

You make it sound like I am impossible to reach! You can always poke me on Discord (or IRC) or whatever .. just try TrueBrain indeed; I don't normally respond to TrueNorth :D :P

You haven't mentioned what issue you run into starting this locally; it should "just work", so I am curious what doesn't work. But possibly best to talk this over on Discord, I think :) So poke me there with where ever you are stuck!

As for this PR, the short answer: it is complicated. The longer:

Where we made all the other parts of the protocol contain a "protocol version", for some reason we completely forgot to add this in the BaNaNaS part of the protocol. In other words: making changes to the protocol like you propose here, would break all older clients instantly. And as we like that even 1.4 (or 0.7) can still use the Online Content Service, that is not ideal :)

This means before we can extend the protocol, we somehow need a method that older clients can still connect while newer can too. There are two approaches to this problem:

1. find a way to add a version to the protocol. This can be done by a new packet, or for example using some bits from the "content_type" field (to name something hacky). I have not given it any real thought, honestly.
2. rework the BaNaNaS protocol to, for example, be JSON based. We have been wondering about this for a while now. Additionally, we might even want to make it completely HTTPS based (by adding libcurl to the client), and ditch the custom protocol here. That would instantly fix so many issues. Of course older clients still use the custom protocol, just newer no longer won't.

Before we can extend the protocol, either of the above should be implemented. 1) is relative lightweight, just needs a clever design. 2) takes a lot more effort, but is more future-proof. Nobody is working on either.

Otherwise, if we would have something like above in place, your PR seems fine.

That all said, if you just want to experiment a bit, your PR will be just fine. It allows you to get the info in the client so you can see how it looks and feels, and find out if this is a good idea at all :) It just ... can't go to production for now.

As I said: complicated :D But we have to tackle this problem sooner or later, so if you are up for it, I am here to help!

[perezdidac]

Thanks for your lovely response. That's the reason I added the new field at the end of the packet, since I looked at the response debugging OpenTTD and yes it looks like field come in a sequence one after the other. Are you sure the extra packet of data at the end will hurt clients?
The option (1) you propose is more short term and seems doable. Option (2) is the way to go IMO, but yeah it will require more work.
BTW the issue I am facing when running it locally is: https://pastebin.com/raw/xNU0kySa ValueError: reuse_port not supported by socket module
It seems that reuse_port does not work on Windows maybe

[TrueBrain]

You are correct that old clients will be very forgiving .. which is a bug on its own, I would say :D That does mean this is pretty hacky, but .. the effort the other solutions take is much greater.

So, I do not have a problem with this PR. The PR in OpenTTD I leave to another dev to evaluate what he thinks of it etc. If that is approved, we will make this happen. We just have to be careful in what order we merge what :) But that will be fine!

[TrueBrain]

We need to check if there is any production BaNaNaS entry now that exceeds packet size. I believe --validate does this, but I am not really sure, I have to admit. Just something we need to take care of before merging :)

[perezdidac]

I agree, is this something you could try running in the server? or I can do somehow? since I don't have the files it only loads 4 of them (the ones you sent :-))

[TrueBrain]

Tnx for the reminder, I forgot; all current content checks out fine, so that is good :)

That does remind me, we also need to change this in bananas-api:
https://github.com/OpenTTD/bananas-api/blob/7e9f4cf8c79cd6954607c8e43595d3c4d27e516e/bananas_api/new_upload/session_validation.py#L98

It ensures nobody can accidentally upload a blob bigger than what is allowed :)

[perezdidac]

Sounds good! I'll propose those changes as well, keep u posted

[perezdidac]

There you go. OpenTTD/bananas-api#85

[LordAro]

Given this is (primarily) meant to be read by the OTTD client, could simplify parsing stuff at the other end to just make it a unix timestamp, rather than a pre-formatted date? Adding a full ISO8601 parser to OTTD is non-trivial...

[TrueBrain]

For master server protocol we use days since year 0, for which OpenTTD has code to make that into readable dates. We have no Python equivalent of it as far as I know, but shouldn't be hard to implement.

Might be an alternative?

[perezdidac]

Thanks for the comments you all!
Changes made, upload date now getting passed as a 4 bytes UNIX epoch timestamp.

[TrueBrain]

My only issue with 4byte unix timestamp is that it expires in 2038. You could argue how likely it is OpenTTD still exists then, but give we are midway between when OpenTTD was created (2004) and 2038, it is something to consider. One could hope we rewrote the protocol by then, or fix it when the date approaches, but looking at our 17 year history, not sure that is likely :p

Software is slowly moving to 8byte unix timestamps, but no clue if that is word the effort. This is btw why I mentioned days since year 0, as we know that works a lot longer :D hell, we could even fit that in a 16bit value if we offset it to start-year-of-bananas (2007). Just no clue if that is a good idea or not :D most likely it is not :p

I am +0 on this, but I did want to mention it so at least it is considered :) I have no problem making this a problem for future-us, honestly :D

[perezdidac]

My only issue with 4byte unix timestamp is that it expires in 2038. You could argue how likely it is OpenTTD still exists then, but give we are midway between when OpenTTD was created (2004) and 2038, it is something to consider. One could hope we rewrote the protocol by then, or fix it when the date approaches, but looking at our 17 year history, not sure that is likely :p

Software is slowly moving to 8byte unix timestamps, but no clue if that is word the effort. This is btw why I mentioned days since year 0, as we know that works a lot longer :D hell, we could even fit that in a 16bit value if we offset it to start-year-of-bananas (2007). Just no clue if that is a good idea or not :D most likely it is not :p

I am +0 on this, but I did want to mention it so at least it is considered :) I have no problem making this a problem for future-us, honestly :D

First of all, OpenTTD WILL exist in 2038, I love making that assumption :)
BUT I am sure the BaNaNas API will be on a structured data messaging format like JSON or proto or obviously something more modern (it's 2038... maybe telepathy?). So yeah, I agree that we could assume we will have a better protocol by then. If not, then I personally commit to address this issue by then. Or maybe my kid does it if they are ever into OpenTTD (I'll try to encourage but not too hard).

I am OK going with either solution, the thing is that my PRs are normally controversial in a sense that I normally get mixed opinions from developers on how to do things, so I want to stick to this approach for now unless I get lots of push back, since I am afraid that if I end up passing it differently someone else might be completely opposed. What do you think? Again, I am happy with anything, I just want to get the feature in in a minimal clean way like this one ensuring long term and not breaking anything. Happy to chat over IRC or Discord live if you want, I am on PST.

And again, THANK YOU for your help and suggestions :)

[LordAro]

I realise we're planning on replacing the network protocol in the "near future", but I'd still feel more comfortable if this were 8 bytes rather than 4. No need to be introducing more Y2k38 issues at this point :)

[perezdidac]

I realise we're planning on replacing the network protocol in the "near future", but I'd still feel more comfortable if this were 8 bytes rather than 4. No need to be introducing more Y2k38 issues at this point :)

Thanks for the comment. @TrueBrain didn't seem to have a strong opinion on 4 vs 8 bytes, and I don't have it either. I gotta say in 2038 we'll definitely have a new network protocol, it seems to me there is no question here. I haven't explored how to pass a 8 bytes timestamp, doing a quick Google search I couldn't find much.

I see this feature as a temporary approach to pass upload-date until the new network protocol is done. In fact, I am interested in how the new network protocol would be interested as I've worked on many projects like this before and distributed systems, I'd love to help.

[TrueBrain]

I know this doesn't deserve the "award for best handling contributions", but as OpenTTD/OpenTTD#8902 has stalled (and it has been 3 years), I am going to close up these PRs.

As I mentioned in OpenTTD/OpenTTD#8902, it is not because I disagree with this implementation. In fact, I think it wouldn't hurt to add. But I also don't see any developer picking it up and bringing it home.

Again, sorry for the long delay, and I do appreciate the effort you put into this. If you ever feel like picking this up again, please start at OpenTTD/OpenTTD#8902, and once we are okay that, we can resurrect these PRs :)