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

improvement: reduce workload or proxy #58

Closed
wants to merge 1 commit into from

Conversation

WofWca
Copy link

@WofWca WofWca commented Aug 22, 2024

...to be comparable to browser-based proxies.
Otherwise the proxy polls every 5 seconds, which is 12x as frequent as browser-based proxies do.
See

Due to changes in the API, I think this is gonna be a breaking change.

Why WIP:

  • This requires updating to a (not yet released) Snowflake version.
    For reference, see de01c06
  • I am not sure whether it is OK to log.Fatalf inside the main function instead of in the goroutine.
  • Please verify that this will produce sane C binding outputs, and is good otherwise - I do not really understand this part.
  • Maybe there is a better way to write this? I used int, thinking that it's not gonna be available for the C version.

...to be comparable to browser-based proxies.
Otherwise the proxy polls every 5 seconds, which is 12x as frequent
as browser-based proxies do.
See
- https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/issues/40373
- guardianproject/orbot#1105
@tladesignz
Copy link
Owner

Let me know, when a Snowflake version will be released, which contains your changes! Then we'll move this forward.

@WofWca
Copy link
Author

WofWca commented Nov 18, 2024

Hey! Letting you know, as promised, that a new release has been published. It includes the change that is needed for this MR.

@tladesignz
Copy link
Owner

Do you mean Snowflake version 2.10.1?
I'm currently working on a complete rewrite with the help of Tor Project folks. See #60
That version is already in it: 768e1e3#diff-921d56f45ab720258a612811bbd4594084f19cd8a179ed28708deaf302e787a2R10

We're short of releasing, one more upstream bug needs to be resolved.

@WofWca
Copy link
Author

WofWca commented Nov 19, 2024

Yes, I meant 2.10.1. It includes the PollInterval config option.

@tladesignz
Copy link
Owner

Ok, is this helpful for you? 3e2825a

@WofWca
Copy link
Author

WofWca commented Dec 5, 2024

Yes, it's the right direction, but I'd say we need to default to a lower value, as in this MR.

Would you like me to rebase it?

@tladesignz
Copy link
Owner

The default comes from the upstream Snowflake library. Unless you give a good reason, why the default should be 2 seconds instead of 5, that is not going to change...

@tladesignz
Copy link
Owner

Oh, you actually mean the higher value of 120 seconds, which is a lower frequency? Well, still.

@WofWca
Copy link
Author

WofWca commented Dec 5, 2024

Oh, you actually mean the higher value of 120 seconds

Yes.
The reasons are laid out here guardianproject/orbot#1105
The upstream maintainers agree with me: https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/issues/40373:

orbot proxies poll very frequently, more frequently than browser proxies, because they use the Snowflake Go library which was historically only used by standalone clients with more resources. We should add a field to the SnowflakeProxy struct to allow callers of the library, like iptproxy, to modify the poll interval.

@tladesignz
Copy link
Owner

Sorry, but I don't see any argument in the mentioned documents, why this should default to 120 seconds in IPtProxy. It's definitely valuable to set it to something higher than 5 seconds in Orbot, true that. But why have a different default? Because mobile?

@n8fr8, what would you consider a sensible value in Orbot?

@WofWca
Copy link
Author

WofWca commented Dec 5, 2024

Yes, because mobile. The default for browsers is 60 seconds.

This project is currently only used in Orbot and in its iOS alternative, right?

@n8fr8
Copy link
Contributor

n8fr8 commented Dec 5, 2024

The SnowflakeProxy "kindness" mode feature is only on Orbot Android.

I don't have any special insight to what the value should be for this usage, but I think we want to lessen the burden on the user's device as we can.

@WofWca
Copy link
Author

WofWca commented Dec 5, 2024

I raised this question on today's Tor anti-censorship meeting. We agreed that 120s should be alright. I'll post a link later if needed.

Update: https://meetbot.debian.net/tor-meeting/2024/tor-meeting.2024-12-05-16.01.log.html

@tladesignz
Copy link
Owner

Well. Everybody agreed on that Orbot should move to a 120 second interval. That doesn't mean IPtProxy should.

Sorry, but the notion of changing the default of this in IPtProxy rubs me the wrong way. It'll be very different to what it was before. It will be very different to what the upstream Snowfake Proxy code uses.

For anybody using this other than us, that will create confusion. And no. I don't have an idea of who else is using this feature, and if there are users at all. After all, it's FOSS. But I know of different people using IPtProxy in general. So there might be users out there, who are using the Snowflake Proxy feature and they would be very surprised if I change this behind their back.

I will leave it at the default 5 seconds for now, in this lib.

Orbot then can, and should, increase this value.

I'll publish a release.

@tladesignz tladesignz closed this Dec 9, 2024
@WofWca
Copy link
Author

WofWca commented Dec 9, 2024

That's also fine. Thanks!

@tladesignz
Copy link
Owner

It's published! https://central.sonatype.com/artifact/com.netzarchitekten/IPtProxy

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

Successfully merging this pull request may close these issues.

3 participants