Skip to content

fix(backup)_: do not show the backup notifi backup during normal fetch #6550

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 13, 2025
Merged

fix(backup)_: do not show the backup notifi backup during normal fetch #6550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,13 @@ func TestLoginAccount(t *testing.T) {
acc, err := b.CreateAccountAndLogin(createAccountRequest)
require.NoError(t, err)
require.Equal(t, nameserver, b.config.WakuV2Config.Nameserver)

accountsDB, err := b.accountsDB()
require.NoError(t, err)
backupFecthed, err := accountsDB.BackupFetched()
require.NoError(t, err)
require.True(t, backupFecthed)

require.True(t, acc.HasAcceptedTerms)

waitForLogin(c)
Expand Down
5 changes: 5 additions & 0 deletions api/geth_backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1773,6 +1773,11 @@ func (b *GethStatusBackend) prepareSettings(request *requests.CreateAccount, inp
//settings.MnemonicWasNotShown = true
}

if !input.fetchBackup {
// This is a an account created from scratch, we can mark the BackupFetched as true
settings.BackupFetched = true
}

if request.WakuV2Fleet != "" {
settings.Fleet = &request.WakuV2Fleet
}
Expand Down
6 changes: 4 additions & 2 deletions multiaccounts/settings/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,11 @@ INSERT INTO settings (
test_networks_enabled,
fleet,
auto_refresh_tokens_enabled,
news_feed_last_fetched_timestamp
news_feed_last_fetched_timestamp,
backup_fetched
) VALUES (
?,?,?,?,?,?,?,?,?,?,?,?,?,?,
?,?,?,?,?,?,?,?,?,'id',?,?,?,?,?,?,?,?,?,?,?,?)`,
?,?,?,?,?,?,?,?,?,'id',?,?,?,?,?,?,?,?,?,?,?,?,?)`,
s.Address,
s.Currency,
s.CurrentNetwork,
Expand Down Expand Up @@ -188,6 +189,7 @@ INSERT INTO settings (
s.AutoRefreshTokensEnabled,
// Default the news feed last fetched timestamp to now
time.Now().Unix(),
s.BackupFetched,
)
if err != nil {
return err
Expand Down
1 change: 1 addition & 0 deletions multiaccounts/settings/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ type Settings struct {
TelemetrySendPeriodMs int `json:"telemetry-send-period-ms,omitempty"`
LastBackup uint64 `json:"last-backup,omitempty"`
BackupEnabled bool `json:"backup-enabled?,omitempty"`
BackupFetched bool `json:"backup-fetched?,omitempty"`
AutoMessageEnabled bool `json:"auto-message-enabled?,omitempty"`
GifAPIKey string `json:"gifs/api-key"`
TestNetworksEnabled bool `json:"test-networks-enabled?,omitempty"`
Expand Down
4 changes: 4 additions & 0 deletions protocol/messenger_backup_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ func (m *Messenger) handleBackup(state *ReceivedMessageState, message *protobuf.
}

func (m *Messenger) updateBackupFetchProgress(message *protobuf.Backup, response *wakusync.WakuBackedUpDataResponse, state *ReceivedMessageState) error {
if m.backedUpFetchingStatus == nil {
return nil
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure this is needed. I don't think we had any crashes, but I just realized that on normal app runs (way after the actual restore), this code could still run and it's better to guard against any crashes.


m.backedUpFetchingStatus.fetchingCompletedMutex.Lock()
defer m.backedUpFetchingStatus.fetchingCompletedMutex.Unlock()

Expand Down
31 changes: 31 additions & 0 deletions protocol/messenger_backup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,37 @@ func (s *MessengerBackupSuite) TestFetchingDuringBackup() {
s.Require().Equal(ActivityCenterNotificationTypeBackupSyncingSuccess, state.Response.ActivityCenterNotifications()[0].Type)
}

func (s *MessengerBackupSuite) TestBackupWithoutStatusFetching() {
bob1 := s.m
// Remove the status fetching
bob1.backedUpFetchingStatus = nil
bob1.config.messengerSignalsHandler = &MessengerSignalsHandlerMock{
wakuBackedUpDataResponseChan: make(chan *wakusync.WakuBackedUpDataResponse, 1000),
}

state := ReceivedMessageState{
Response: &MessengerResponse{},
}

backup := &protobuf.Backup{
Clock: 1,
ContactsDetails: &protobuf.FetchingBackedUpDataDetails{
DataNumber: uint32(0),
TotalNumber: uint32(1),
},
}

err := bob1.HandleBackup(
&state,
backup,
&v1protocol.StatusMessage{},
)
s.Require().NoError(err)
// The backup was handled but nothing was sent to the AC
s.Require().Len(state.Response.ActivityCenterNotifications(), 0)
s.Require().True(state.Response.BackupHandled)
}

func (s *MessengerBackupSuite) TestBackupSettings() {
s.T().Skip("flaky test")
const (
Expand Down
5 changes: 0 additions & 5 deletions protocol/messenger_mailserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,11 +320,6 @@ func (m *Messenger) RequestAllHistoricMessages(forceFetchingBackup, withRetries

allResponses := &MessengerResponse{}
if forceFetchingBackup || !backupFetched {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should add the if m.processBackedupMessages here too. What happens here is that even though we are not restoring the account, we end up doing a full backup fetch anyway for no reason?

I did not touch it right now just in case it would cause a regression, but I don't see the point of doing a full (30 days) fetch of backups if we created an account from scratch. Let me know if I'm missing something. The person that added that code was Andrea 4 years ago 😅

Also, if processBackedupMessages is false, we don,t even handle the backups:

func (m *Messenger) HandleBackup(state *ReceivedMessageState, message *protobuf.Backup, statusMessage *v1protocol.StatusMessage) error {
	if !m.processBackedupMessages {
		return nil
	}

cc @status-im/status-go-guild

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes much sense to fetch backups for newly created accounts. Adding this condition seems reasonable to me and definitely better than removing backup fetching tracking.

Copy link
Collaborator

@igor-sirotin igor-sirotin May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, the messages will be skipped in HandleBackup. But to me the issue is actually HandleBackup, not here.

If I would call RequestAllHistoricMessages with forceFetchingBackup = true, I'd expect backup to be fetched and the messages to be processed.

I'd keep as is for now, we should carefully refactor the way backup is handled.

Copy link
Collaborator

@igor-sirotin igor-sirotin May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think:

  1. either the condition should be forceFetchingBackup || (!backupFetched && m.processBackedupMessages
    both here and in HandleBackup (which will bring some weird dependencies between those functions)

  2. or we remove forceFetchingBackup option completely
    I think it's only called from desktop in asyncFetchWakuBackupMessagesTask. Which I think is bound to the explicit button in settings and is only used (if used) for testing.

I'd prefer (2), as it would remove the code, not add it 😄

cc @osmaczko @jrainville

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's only called from desktop in asyncFetchWakuBackupMessagesTask. Which I think is bound to the explicit button in settings and is only used (if used) for testing.

It is indeed only used in Desktop, in the backupSyncingComponent in the ActivityCenterPopup of all places.

I'll dig some more to understand why we added this force fetch, but I'm very tempted to just remove it as you proposed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source of the code

Ok I found it. It's used to force the re-fecth of everything in the onboarding.

So when re-importing from the seed phrase, we fetch everything. In the old onboarding, we waited on the page until everything was fetched, now we do it in the background.
In both cases, the user can click Retry if something didn't get fetched.

It at least confirms that it's only used in the case were were are re-importing an old account, ie processBackedupMessages is gonna be set to true.

Remaining problem

However, I just realized that removing the forceFetchingBackup param won't help us in this case, because RequestAllHistoricMessages gets called during the normal flow of the app too (like going back from sleep).

So at somepoint, we'll end up doing the full history fetch anyway.

Possible solutions

I guess we have two options:
a. we could set BackupFetched to true for accounts created from scratch
b. just add the processBackedupMessages like in @igor-sirotin 's option 1.

I think option b is the simpler one, but it does feel weird that BackupFetched will stay false for the whole life of that account.
We could also rename it to fetchAndProcessBackedupMessages so that it's clearer that it's not just about not parsing them, because it's weird to think that we might want to fetch backups but never process them (useless request and computation)

cc @igor-sirotin @osmaczko

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, setting BackupFetched to true for new accounts sounds good to me for now.
@osmaczko wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, setting BackupFetched to true for new accounts sounds good to me for now. @osmaczko wdyt?

Not ideal, because it is untrue, the backup will never be fetched for new accounts.. but it is good enough for now indeed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code to do as you guys proposed. I put it in the same commit by accident, but it's still pretty small so it shouldn't be an issue

err = m.startBackupFetchingTracking(allResponses)
if err != nil {
return nil, err
}

m.logger.Info("fetching backup")
err := m.syncBackup()
if err != nil {
Expand Down