Previously, low-trust SignalRecipients wouldn’t be marked as registered
unless they were newly-created. Now, they are marked as registered in
places where they should be (eg, successfully sending a message or
fetching a pre key means that the device exists).
As part of this change, SignalRecipients are unregistered by default,
though most call sites still mark them as registered immediately, so the
behavior in practice will be identical. There are a few places (such as
ensureAccountId) which will no longer mark new values as registered.
Finally, creating a new SignalRecipient would also update Storage
Service, even if that recipient was created in response to a storage
service update. Now, recipients updated as part of storage service
operations won’t immediately trigger another storage service update.
RATE LIMITS & IS CRITICAL PRIORITY
Before this change, there was a notion of “critical” discovery tasks
that had their own rate limit, and that rate limit was used for
UUIDBackfillTask. In this change, that logic has been generalized to
consider a separate rate limit for additional request types.
QUEUE PRIORITIES
All of the discovery operations were updated to use `.userInitiated` as
their priority. Before this change, the `UUIDBackfillTask` and message
sending flow used lower priorities. However, both of those should use
higher priorities -- the former blocks receiving messages and the latter
blocks outgoing messages. This change allows the code to be simplified.
TESTS
This removes most of the existing `UUIDBackfillTaskTest` test cases
(they were commented out) and all of the `ContactDiscoveryTaskTest` test
cases (which were for the rate limiter, which has been moved elsewhere).
It introduces a few new tests. These aren’t meant to be exhausive;
instead, they’re meant to hit most of the code in the new classes and a
few specific edge cases that aren’t likely to be hit in normal use.
Change license to AGPL
This commit:
- Updates the `LICENSE` file
- Start every file with something like:
// Copyright YEAR_FIRST_PUBLISHED Signal Messenger, LLC
// SPDX-License-Identifier: AGPL-3.0-only
---
First, I removed existing license headers with this Ruby 3.1.2 script:
require 'set'
EXTENSIONS_TO_CHECK = Set['.h', '.hpp', '.cpp', '.m', '.mm', '.pch', '.swift']
same = 0
different = 0
all_files = `git ls-files`.lines.map { |line| line.strip }
all_files.each do |relative_path|
if relative_path == 'Pods'
next
end
unless EXTENSIONS_TO_CHECK.include? File.extname(relative_path)
next
end
path = File.expand_path(relative_path)
contents = File.read(path)
new_contents = contents.sub(/\/\/\n\/\/ Copyright .*\n\/\/\n\n/, '')
if contents == new_contents
same += 1
else
different += 1
end
File.write(path, new_contents)
end
puts "updated #{different} file(s), left #{same} untouched"
I'm sure this script could be improved, but it worked well enough.
Then, I created `Scripts/lint/lint-license-headers` and ran it to auto-
fix a lot of files. This changed the mode of some files, but I think
that's actually desirable. For example,
`SignalServiceKit/src/Util/AppContext.m` previously had a mode of
`0755/-rwxr-xr-x`, and it's now `0644/-rw-r--r--`.
Then I fixed some stragglers and updated the precommit script.
See [a similar change in the Desktop app][0].
[0]: 8bfaf598af
- Adjust how clients provide QoS information to ContactDiscoveryTask
- Fix an issue where legacy CDS was reporting a generic server failure
as a result of network failures.
- ContactDiscoveryTask will no longer error on an empty set
- Remove CDSFeedback operation
After some discussion, we're okay considering that if CDS is down, all
of Signal is down. This will help protect session state and reduce the
risk of divergence.
Also changed the FeatureFlag guarding this code to
useOnlyModernContactDiscovery.
- Removed static UUIDBackfillTask initializer and removed the
registration from AppDelegate. Kicking off the task will be handled
with IOS-631.
- Moved the async perform work to its own function
- Some minor syntactic changes
- Updated database fetch to retrieve empty string UUIDs
- Handles non-e164 numbers fetched from database
- Leverages the MockSSKEnvironment to deal with the shared
SignalServiceAddressCache. Removes added test support.
- Add tests to exercise new functionality
This adds a whole bunch of tests for UUIDBackfillTask. This required a
small change to be made in SignalServiceAddress. Currently,
SignalServiceAddress accesses a singleton cache to force updates to any
existing addresses after a UUID is discovered. This breaks a whole bunch
of test verification.
To workaround this, I've added a testing_ flag to modify the singleton
SignalServiceAddressCache to temporarily disable it. This is going to
cause problems if we ever enable test parallelization, but it's all we
can do for now.
This also adds in incremental backoff logic, changes some of the log
statements and disables the task on production.
Also, adopted some minor feedback Matthew provided in the draft PR,
including:
- Rename: {signal -> registered}RecipientsWithoutUUID
- Move to owsFailDebug instead of assertionFailure
- Adopt `IsNetworkConnectivityFailure` for discerning network failures
- Add some extra assertions around our registered/unregistered sets
