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

PullRequest to upstream #21

Open
JavaJens opened this issue Jul 22, 2015 · 33 comments
Open

PullRequest to upstream #21

JavaJens opened this issue Jul 22, 2015 · 33 comments

Comments

@JavaJens
Copy link
Owner

I'd like to use this issue to discuss any changes required before submitting a pull request to upstream.

@dev-mb-zz
Copy link

As far as I followed the discussion moxies biggest concern was battery drain when using websockets instead of gcm. To have some arguments it would be good to have numbers available to compare gcm and websockets versions in terms of relative battery drain in same usage scenarios.

Are there any reliable procedures for measuring battery drain in dependence on idle/typing/sending/recieving activity? Sadly this doesn't seem to be trivial to me.

@Isgar
Copy link

Isgar commented Jul 22, 2015

In my experience having gapps installed is a huge drain on battery of itself. Apps using GCM thus don't have additional drain. Websocket TextSecure added a big drain on my gapps-free device mostly since there was nothing waking the device up before.

So to properly compare it, you'd have to have a clean install with and without gapps/websocket. Even then it is questionable how these numbers compare to real life scenarios.

@dev-mb-zz
Copy link

Thats a good point, I observed that Gapps used a lot of battery as well. No this consumption moved to Textsecure. But in general battery lifetime seems to be in same orders of magnitude (2-3 days depending on usage - MotoG fist generation 2years old).

@relyt29
Copy link

relyt29 commented Jul 22, 2015

Regarding battery life, lets gather some statistics, in an organized fashion. The problem with effective statistics is that you need to be able to effectively control for your environment.

As for presenting the pull request, probably what we will need to do is as you you mention in #15, although again, the problem is staying current with upstream. What I would suggest is maintaining the status quo, doing the code review on websocket-reborn, and then when a specific goal has been achieved (but I don't know what that is) we take all the diffs of websocket-reborn, rebase, and apply them in one commit, and submit the PR. Then inevitably, the PR will sit there for a week and a half until somebody upstream gets a chance to look at it, by which point it will be behind again. But at least at that point, we'll have feedback to incorporate back in, and do it all again until its finally "good enough."

@schachmat
Copy link

So did we do enough testing to try another PR to upstream? The changeset is not too complex at least.

@relyt29
Copy link

relyt29 commented Aug 12, 2015

EDIT: I didn't see someone did do a battery test for #22. How many of those are good enough before we have enough data? I think #22 needs to be resolved first, so that we have something to bring to the table with regards to performance stats, better than "I've been using this for 2 months with both data and wifi, and my battery life has been fine, everything works great, I haven't experienced any bugs" even if its true (which it is) its still only personal anecdote.

Also, some more code review would be nice, to make sure its up to spec. Moxie has really strict code quality requirements, and this is maybe a few hundred lines of code.
Also there would need to be a PR made in tandem with libtextsecure-java for the fetchesMessages support.

If we could get a list of how many people have actually read all of the changes and checked that they're sane that could be helpful to get an idea whether or not we think we're ready to merge. like a Google spreadsheet of github users who have read and checked off on it. @JavaJens what do you think of that idea?

Also javajens probably will have to rebase off master before submitting, and get rid of all my "merging upstream 2.2blah" commits and whatever else noise

@JavaJens
Copy link
Owner Author

Rebasing is no problem. It works without merge conflicts and I've removed the build flavor and config stuff, so it is even less code.

However, I agree with @f41c0r that some more code review would be good. We could use the wiki in the Repo for that?

Regarding libtextsecure: I once opened a PR where moxie said it will come: signalapp/libsignal-service-java#2
Now there is another PR with the same changes, but no comment signalapp/libsignal-service-java#5

I will try to get my old phone running again for some battery tests. Do you think we can use espresso for automating the testing?

@schachmat
Copy link

The last two points are clear of course. And the current changes are actually really not that many ("85 additions and 27 deletions") and if you leave out the applicationId from the Manifest and the build.gradle stuff, which will not be needed upstream, it's even less.

For comparing the battery drain, I cannot help unfortunately, since I do not have and do not want gapps on my phone. However I have a proposal for people who have a >=Lollipop phone with gapps and can afford to not use it for a while:

  1. fully charge the phone
  2. leave it at your desk untouched for 24 hours
  3. during that period make sure the phone gets 20 textsecure messages with at least 5 minute gaps in between (either from a second phone or just ask a friend)
  4. after the 24 hours go to the battery settings, select TextSecure and write down the values or take a screenshot
  5. Also take the dump with Battery Historian

Do this once for the version with gapps/GCM and once only with websockets. Then we have some data which is actually comparable and since the process in which it was collected is specified, the data can actually be discussed. Of course the two values of 24 hours and 20 messages are arbitrarily choosen and can be changed.

@ghost
Copy link

ghost commented Aug 25, 2015

Perhaps a dumb commented, but I'm assuming ios doesn't rely on gcm. So how does signal handle this?

@paride
Copy link

paride commented Aug 25, 2015

I would say using the Apple Push Notification Service:

https://en.wikipedia.org/wiki/Apple_Push_Notification_Service

@ghost
Copy link

ghost commented Aug 25, 2015

@legovini What about Telegram, Whatsapp, etc on Android?

@relyt29
Copy link

relyt29 commented Aug 25, 2015

WhatsApp is closed source, probably also uses GCM, since its better for battery and whatnot
Telegram is open source, but has a dependency on Google Play Services so is almost certaintly using GCM.

edit: I stand corrected see @legovini's post

@paride
Copy link

paride commented Aug 25, 2015

Telegram is able to work without GCM, and the version on f-droid supports only this way of operation (as far as I understand from the description of the package). It has no major drawbacks. I'm not sure about how it works, but for sure websockets are the right way to implement this functionality.

@relyt29
Copy link

relyt29 commented Aug 25, 2015

@ghost
Copy link

ghost commented Aug 26, 2015

I'm currently on CM 12.1 without Gapps. Both Whatsapp and Telegram work fine :) why can't we do the same? :p

@ghost
Copy link

ghost commented Aug 26, 2015

Telegram is Open-Source so I suppose we could have a look at it?

@ghost
Copy link

ghost commented Aug 26, 2015

@f41c0r Indeed, telegram does store messages so that one can acces then on multiple devices. If you prefer end to end encryption you can use the secret chat feature which won't store the messages on telegrams servers and it's encrypted end to end.
There's been a lot of negativity about telegram but really, It's really not as bad as people think. We could definitely check out their source and see how they handle things.

@h-2
Copy link

h-2 commented Sep 20, 2015

I use Conversations, Kontalk and Telegram, all without Push and I get 2 days of battery life with my OnePlus One. Kontalk is even in "always-on" mode where it permanently keeps a connection.
The main issue with not having push is that multiple applications poll or keep-alive at different intervals. If all of the mentioned applications where to do so every 6 minutes of the hour exactly (for example) the phone could at least sleep in between.
That having been said Web-sockets allow for "always-on" with much lower keep-alives (right?). So JavaJens's approach is already much better.

@ghost
Copy link

ghost commented Sep 20, 2015

Perhaps that would be a nice feature for Android. Allowing users to set a refresh interval in android itself. That way it could sleep said interval and allow all apps to refresh at once.
This interval can then also increase/decrease for battery saving.

@schachmat
Copy link

Just use http://developer.android.com/reference/android/app/AlarmManager.html#setWindow(int, long, long, android.app.PendingIntent) for that. Enforcing a global refresh interval would restrict apps needing a more frequent wakeup.

@h-2
Copy link

h-2 commented Sep 20, 2015

In any case thanks for still working on this. It is relevant to a lot of people I know!

@ghost
Copy link

ghost commented Sep 21, 2015

So I came across a few articles that somehow suggest governments can't read whatsapp as it's end-to-end encrypted. They did indeed work with open whisper systems but they never documented this.

http://uk.businessinsider.com/whatsapp-snapchat-encryption-isis-lord-king-2015-1?op=1

http://readwrite.com/2015/01/13/david-cameron-encryption-messaging-apps-imessage-whatsapp-snapchat

I'm assuming whatsapp's encryption isn't an obstacle for the NSA is it? There's no way they have an actual good implementation of end-to-end encryption and the feds be ok with that.

P.S sorry if this is completely off topic

@JavaJens
Copy link
Owner Author

IIRC correctly this is only on the Android version and it is not transparent to the user if the encryption is used or not: http://www.heise.de/ct/artikel/Keeping-Tabs-on-WhatsApp-s-Encryption-2630361.html
See this blog post for the OWS side of things: https://whispersystems.org/blog/whatsapp/

But please, bring it back to topic 😉 Maybe the (un)official mailing list is a better place.

@ghost
Copy link

ghost commented Sep 21, 2015

Plus the fact that there's no fingerprint checking so there's no way to tell if there is a mitm attack.

Where exactly can I find the mailing list?

@h-2
Copy link

h-2 commented Oct 6, 2015

I see that we now get LibreSignal builds, but that RedPhone registration is still disabled. Is this something that is planned to work with Websockets, as well? Probably a unified patch will be necessary to be accepted upstream, now that the apps are merged, right?

@ghost
Copy link

ghost commented Oct 9, 2015

Wow, this endeavor has been going on quite a while. I admire the determination all around, thank you.

Do we think this has a chance of accepted by @moxie0? And 3rd party distribution would be accepted. It seems he's been doing everything possible to stop that. It'd be a shame if all this hard work were to go to waste.

@iexos
Copy link

iexos commented Oct 10, 2015

They can't really do anything about 3rd party distribution as long as it doesn't violate trademark.

But if this fork really is not going to be merged, it is worth considering setting up a seperate server with federation to Open Whisper Systems. Apart from facilitating goodwill upstream, this prevents WS Signal to break should they decide to close/change the websocket API.

@h-2
Copy link

h-2 commented Oct 10, 2015

I think distribution and websockets are two different issues and should be treated as such. Let's try to tackle websockets first and get that upstream. Whether there will official TextSecure builds for F-Droid? I doubt it, but making a lot of commotion about third party builds now won't help us to get websocket-support merged...

@ghost
Copy link

ghost commented Oct 10, 2015

I think distribution and websockets are two different issues and should be treated as such.

According to Whisper System's statement they're directly related to the issue of supporting devices without Google services. That's the whole point of adding websocket functionality, correct?

it is worth considering setting up a seperate server with federation to Open Whisper Systems

@iexos I think this is the most realistic option. It's closing in on 3 years and still no reliable solution.

@h-2
Copy link

h-2 commented Oct 10, 2015

According to Whisper System's statement they're directly related to the issue of supporting devices without Google services. That's the whole point of adding websocket functionality, correct?

Yes and No. The point of websockets is supporting non-GCM platforms, but that doesn't mean they support F-Droid as distributions model or anything like that. I think they might accept the patch just as a basis for other clients (Chrome-Plugin...) and not offer any other Android packages. Or they might offer self-updating, self-signed APKs. My point is that since I know Moxie has a particular dislike for F-Droid, I don't think we should associate Websockets too strongly with F-Droid-distribution, because it will just lower the chances of the patch being accepted.

it is worth considering setting up a seperate server with federation to Open Whisper Systems

@iexos I think this is the most realistic option. It's closing in on 3 years and still no reliable solution.

maybe @JavaJens can comment on this? I thought the plan was still to try and get the patch upstream, or not? Also note that OWS Server does not federate "just like that", but that they have to explicitly whitelist the other server which means they are not going to do it if you make them angry... Also it was reported that federation (with the only other server by CM) does actually not work too well, but I don't have up-to-date information on that.

@iexos
Copy link

iexos commented Oct 11, 2015

Also note that OWS Server does not federate "just like that", but that they have to explicitly whitelist the other server which means they are not going to do it if you make them angry...

moxie said he prefers we don't use their server, so I would guess he would at least be more open to federation? If it is technically viable, of course. In any way, splitting the userbase is not an option.

@ddast
Copy link

ddast commented Nov 13, 2016

@JavaJens Is there still interest in trying to make a pull request? I recently had a look at your implementation and made some minor changes, such as changing the registration process in a way upstream wants it and deactivating voice since it does not work withough GCM. To keep the diff minimal I removed things like the websocket flavor. Do you want me to make a pull request for your repo such that we can discuss what is still missing?

@mimi89999
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants