diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java index c1622e69a..033d780c1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java @@ -2737,7 +2737,7 @@ public class RecipientDatabase extends Database { } } - public void clearDirtyStateForRecords(@NonNull List records) { + public void clearDirtyStateForStorageIds(@NonNull Collection ids) { SQLiteDatabase db = databaseHelper.getWritableDatabase(); Preconditions.checkArgument(db.inTransaction(), "Database should already be in a transaction."); @@ -2747,8 +2747,8 @@ public class RecipientDatabase extends Database { String query = STORAGE_SERVICE_ID + " = ?"; - for (SignalRecord record : records) { - String[] args = SqlUtil.buildArgs(Base64.encodeBytes(record.getId().getRaw())); + for (StorageId id : ids) { + String[] args = SqlUtil.buildArgs(Base64.encodeBytes(id.getRaw())); db.update(TABLE_NAME, values, query, args); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java index 294d4b194..5a7f455d4 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/StorageSyncJobV2.java @@ -34,6 +34,7 @@ import org.thoughtcrime.securesms.storage.StorageSyncHelper.WriteOperationResult import org.thoughtcrime.securesms.storage.StorageSyncModels; import org.thoughtcrime.securesms.storage.StorageSyncValidations; import org.thoughtcrime.securesms.transport.RetryLaterException; +import org.thoughtcrime.securesms.util.SetUtil; import org.thoughtcrime.securesms.util.TextSecurePreferences; import org.thoughtcrime.securesms.util.Util; import org.whispersystems.libsignal.InvalidKeyException; @@ -54,10 +55,12 @@ import org.whispersystems.signalservice.internal.storage.protos.ManifestRecord; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.Locale; +import java.util.Set; import java.util.concurrent.TimeUnit; /** @@ -222,7 +225,6 @@ public class StorageSyncJobV2 extends BaseJob { List localStorageIdsBeforeMerge = getAllLocalStorageIds(context, Recipient.self().fresh()); KeyDifferenceResult keyDifference = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), localStorageIdsBeforeMerge); - if (keyDifference.hasTypeMismatches()) { Log.w(TAG, "[Remote Sync] Found type mismatches in the key sets! Scheduling a force push after this sync completes."); needsForcePush = true; @@ -232,7 +234,6 @@ public class StorageSyncJobV2 extends BaseJob { Log.i(TAG, "[Remote Sync] Retrieving records for key difference: " + keyDifference); List remoteOnly = accountManager.readStorageRecords(storageServiceKey, keyDifference.getRemoteOnlyKeys()); - List localOnly = buildLocalStorageRecords(context, self, keyDifference.getLocalOnlyKeys()); if (remoteOnly.size() != keyDifference.getRemoteOnlyKeys().size()) { Log.w(TAG, "[Remote Sync] Could not find all remote-only records! Requested: " + keyDifference.getRemoteOnlyKeys().size() + ", Found: " + remoteOnly.size() + ". Scheduling a force push after this sync completes."); @@ -290,17 +291,26 @@ public class StorageSyncJobV2 extends BaseJob { Log.i(TAG, "IDs : " + localStorageIdsBeforeMerge.size() + " IDs before merge, " + localStorageIdsAfterMerge.size() + " IDs after merge"); } - localOnly.removeAll(contactResult.getLocalMatches()); - localOnly.removeAll(gv1Result.getLocalMatches()); - localOnly.removeAll(gv2Result.getLocalMatches()); - localOnly.removeAll(accountResult.getLocalMatches()); - - recipientDatabase.clearDirtyStateForRecords(localOnly); - - Log.i(TAG, "[Remote Sync] After the conflict resolution, there are " + localOnly.size() + " local-only records remaining."); - //noinspection unchecked Stop yelling at my beautiful method signatures - mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, localOnly, contactResult, gv1Result, gv2Result, accountResult); + mergeWriteOperation = StorageSyncHelper.createWriteOperation(remoteManifest.get().getVersion(), localStorageIdsAfterMerge, contactResult, gv1Result, gv2Result, accountResult); + + List postMergeLocalOnlyIds = StorageSyncHelper.findKeyDifference(remoteManifest.get().getStorageIds(), mergeWriteOperation.getManifest().getStorageIds()).getLocalOnlyKeys(); + List remoteInsertIds = Stream.of(mergeWriteOperation.getInserts()).map(SignalStorageRecord::getId).toList(); + Set unhandledLocalOnlyIds = SetUtil.difference(postMergeLocalOnlyIds, remoteInsertIds); + + if (unhandledLocalOnlyIds.size() > 0) { + Log.i(TAG, "[Remote Sync] After the conflict resolution, there are " + unhandledLocalOnlyIds.size() + " local-only records remaining that weren't otherwise inserted. Adding them as inserts."); + + List localOnly = buildLocalStorageRecords(context, self, unhandledLocalOnlyIds); + + mergeWriteOperation = new WriteOperationResult(mergeWriteOperation.getManifest(), + Util.concatenatedList(mergeWriteOperation.getInserts(), localOnly), + mergeWriteOperation.getDeletes()); + + recipientDatabase.clearDirtyStateForStorageIds(unhandledLocalOnlyIds); + } else { + Log.i(TAG, "[Remote Sync] After the conflict resolution, there are no local-only records remaining."); + } StorageSyncValidations.validate(mergeWriteOperation, remoteManifest, needsForcePush); db.setTransactionSuccessful(); @@ -409,7 +419,11 @@ public class StorageSyncJobV2 extends BaseJob { DatabaseFactory.getStorageKeyDatabase(context).getAllKeys()); } - private static @NonNull List buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull List ids) { + private static @NonNull List buildLocalStorageRecords(@NonNull Context context, @NonNull Recipient self, @NonNull Collection ids) { + if (ids.isEmpty()) { + return Collections.emptyList(); + } + RecipientDatabase recipientDatabase = DatabaseFactory.getRecipientDatabase(context); StorageKeyDatabase storageKeyDatabase = DatabaseFactory.getStorageKeyDatabase(context); @@ -423,12 +437,12 @@ public class StorageSyncJobV2 extends BaseJob { RecipientSettings settings = recipientDatabase.getByStorageId(id.getRaw()); if (settings != null) { if (settings.getGroupType() == RecipientDatabase.GroupType.SIGNAL_V2 && settings.getSyncExtras().getGroupMasterKey() == null) { - Log.w(TAG, "Missing master key on gv2 recipient"); + throw new MissingGv2MasterKeyError(); } else { records.add(StorageSyncModels.localToRemoteRecord(settings)); } } else { - Log.w(TAG, "Missing local recipient model! Type: " + id.getType()); + throw new MissingRecipientModelError("Missing local recipient model! Type: " + id.getType()); } break; case ManifestRecord.Identifier.Type.ACCOUNT_VALUE: @@ -442,7 +456,7 @@ public class StorageSyncJobV2 extends BaseJob { if (unknown != null) { records.add(unknown); } else { - Log.w(TAG, "Missing local unknown model! Type: " + id.getType()); + throw new MissingUnknownModelError("Missing local unknown model! Type: " + id.getType()); } break; } @@ -451,6 +465,20 @@ public class StorageSyncJobV2 extends BaseJob { return records; } + private static final class MissingGv2MasterKeyError extends Error {} + + private static final class MissingRecipientModelError extends Error { + public MissingRecipientModelError(String message) { + super(message); + } + } + + private static final class MissingUnknownModelError extends Error { + public MissingUnknownModelError(String message) { + super(message); + } + } + public static final class Factory implements Job.Factory { @Override public @NonNull StorageSyncJobV2 create(@NonNull Parameters parameters, @NonNull Data data) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java index c4e0f2d94..71c73eacf 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java @@ -141,7 +141,7 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor implements public @NonNull Result process(@NonNull Collection remoteRecords, @NonNull StorageKeyGenerator keyGenerator) throws IOException { List remoteDeletes = new LinkedList<>(); List> remoteUpdates = new LinkedList<>(); - List localMatches = new LinkedList<>(); Set matchedRecords = new TreeSet<>(this); for (E remote : remoteRecords) { @@ -57,8 +56,6 @@ abstract class DefaultStorageRecordProcessor implements if (local.isPresent()) { E merged = merge(remote, local.get(), keyGenerator); - localMatches.add(local.get()); - if (matchedRecords.contains(local.get())) { Log.w(TAG, "Multiple remote records map to the same local record! Marking this one for deletion. (Type: " + local.get().getClass().getSimpleName() + ")"); remoteDeletes.add(remote); @@ -79,7 +76,7 @@ abstract class DefaultStorageRecordProcessor implements } } - return new Result<>(remoteUpdates, remoteDeletes, localMatches); + return new Result<>(remoteUpdates, remoteDeletes); } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java index 2f0f66672..a3f99104e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV1RecordProcessor.java @@ -117,7 +117,7 @@ public final class GroupV1RecordProcessor extends DefaultStorageRecordProcessor< if (Arrays.equals(lhs.getGroupId(), rhs.getGroupId())) { return 0; } else { - return lhs.getGroupId()[0] - rhs.getGroupId()[0]; + return 1; } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java index 19fbf8f78..8b65dbcb1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/GroupV2RecordProcessor.java @@ -117,7 +117,7 @@ public final class GroupV2RecordProcessor extends DefaultStorageRecordProcessor< if (Arrays.equals(lhs.getMasterKeyBytes(), rhs.getMasterKeyBytes())) { return 0; } else { - return lhs.getMasterKeyBytes()[0] - rhs.getMasterKeyBytes()[0]; + return 1; } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java index 054ffabdb..33876ed73 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageRecordProcessor.java @@ -23,12 +23,10 @@ public interface StorageRecordProcessor { final class Result { private final Collection> remoteUpdates; private final Collection remoteDeletes; - private final Collection localMatches; - Result(@NonNull Collection> remoteUpdates, @NonNull Collection remoteDeletes, @NonNull Collection localMatches) { + Result(@NonNull Collection> remoteUpdates, @NonNull Collection remoteDeletes) { this.remoteDeletes = remoteDeletes; this.remoteUpdates = remoteUpdates; - this.localMatches = Stream.of(localMatches).map(SignalRecord::asStorageRecord).toList(); } public @NonNull Collection getRemoteDeletes() { @@ -39,10 +37,6 @@ public interface StorageRecordProcessor { return remoteUpdates; } - public @NonNull Collection getLocalMatches() { - return localMatches; - } - public boolean isLocalOnly() { return remoteUpdates.isEmpty() && remoteDeletes.isEmpty(); } @@ -61,7 +55,7 @@ public interface StorageRecordProcessor { builder.append("- ").append(update.toString()).append("\n"); } - return super.toString(); + return builder.toString(); } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java index 5678e6d38..5e820ca62 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncHelper.java @@ -355,10 +355,9 @@ public final class StorageSyncHelper { */ public static @NonNull WriteOperationResult createWriteOperation(long currentManifestVersion, @NonNull List allStorageKeys, - @NonNull List localOnlyRecords, @NonNull StorageRecordProcessor.Result... results) { - Set inserts = new LinkedHashSet<>(localOnlyRecords); + Set inserts = new LinkedHashSet<>(); Set deletes = new LinkedHashSet<>(); for (StorageRecordProcessor.Result result : results) { @@ -682,9 +681,9 @@ public final class StorageSyncHelper { private final List inserts; private final List deletes; - private WriteOperationResult(@NonNull SignalStorageManifest manifest, - @NonNull List inserts, - @NonNull List deletes) + public WriteOperationResult(@NonNull SignalStorageManifest manifest, + @NonNull List inserts, + @NonNull List deletes) { this.manifest = manifest; this.inserts = inserts; diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java index 713d744dd..fca159833 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncValidations.java @@ -7,6 +7,7 @@ import com.annimon.stream.Stream; import org.signal.core.util.logging.Log; import org.thoughtcrime.securesms.recipients.Recipient; +import org.thoughtcrime.securesms.transport.RetryLaterException; import org.thoughtcrime.securesms.util.Base64; import org.thoughtcrime.securesms.util.SetUtil; import org.whispersystems.libsignal.util.guava.Optional; @@ -66,10 +67,12 @@ public final class StorageSyncValidations { Set writeDeletes = Stream.of(result.getDeletes()).map(ByteBuffer::wrap).collect(Collectors.toSet()); if (writeInserts.size() > insertedIds.size()) { + Log.w(TAG, "WriteInserts: " + writeInserts.size() + ", InsertedIds: " + insertedIds.size()); throw new MoreInsertsThanExpectedError(); } if (writeInserts.size() < insertedIds.size()) { + Log.w(TAG, "WriteInserts: " + writeInserts.size() + ", InsertedIds: " + insertedIds.size()); throw new LessInsertsThanExpectedError(); } @@ -78,10 +81,12 @@ public final class StorageSyncValidations { } if (writeDeletes.size() > deletedIds.size()) { + Log.w(TAG, "WriteDeletes: " + writeDeletes.size() + ", DeletedIds: " + deletedIds.size()); throw new MoreDeletesThanExpectedError(); } if (writeDeletes.size() < deletedIds.size()) { + Log.w(TAG, "WriteDeletes: " + writeDeletes.size() + ", DeletedIds: " + deletedIds.size()); throw new LessDeletesThanExpectedError(); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index 49b44b2ec..05408416a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -74,7 +74,7 @@ public final class FeatureFlags { private static final String ANIMATED_STICKER_MIN_TOTAL_MEMORY = "android.animatedStickerMinTotalMemory"; private static final String MESSAGE_PROCESSOR_ALARM_INTERVAL = "android.messageProcessor.alarmIntervalMins"; private static final String MESSAGE_PROCESSOR_DELAY = "android.messageProcessor.foregroundDelayMs"; - private static final String STORAGE_SYNC_V2 = "android.storageSyncV2"; + private static final String STORAGE_SYNC_V2 = "android.storageSyncV2.2"; /** * We will only store remote values for flags in this set. If you want a flag to be controllable diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java index 4d4dfa2e9..034512c61 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalAccountRecord.java @@ -12,6 +12,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil; import org.whispersystems.signalservice.internal.storage.protos.AccountRecord; import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -64,6 +65,10 @@ public final class SignalAccountRecord implements SignalRecord { SignalAccountRecord that = (SignalAccountRecord) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Objects.equals(this.givenName, that.givenName)) { diff.add("GivenName"); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java index d86ebe03e..3fa3586aa 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalContactRecord.java @@ -10,6 +10,7 @@ import org.whispersystems.signalservice.api.util.UuidUtil; import org.whispersystems.signalservice.internal.storage.protos.ContactRecord; import org.whispersystems.signalservice.internal.storage.protos.ContactRecord.IdentityState; +import java.util.Arrays; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -56,6 +57,10 @@ public final class SignalContactRecord implements SignalRecord { SignalContactRecord that = (SignalContactRecord) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Objects.equals(this.getAddress().getNumber(), that.getAddress().getNumber())) { diff.add("E164"); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java index 3c9bb4a4c..15e92cb42 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV1Record.java @@ -40,6 +40,10 @@ public final class SignalGroupV1Record implements SignalRecord { SignalGroupV1Record that = (SignalGroupV1Record) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Arrays.equals(this.groupId, that.groupId)) { diff.add("MasterKey"); } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java index 32cd6a2bf..91adb9aa8 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/storage/SignalGroupV2Record.java @@ -42,6 +42,10 @@ public final class SignalGroupV2Record implements SignalRecord { SignalGroupV2Record that = (SignalGroupV2Record) other; List diff = new LinkedList<>(); + if (!Arrays.equals(this.id.getRaw(), that.id.getRaw())) { + diff.add("ID"); + } + if (!Arrays.equals(this.getMasterKeyBytes(), that.getMasterKeyBytes())) { diff.add("MasterKey"); }