feat(backup)_: local user data backups #6722
Conversation
Jenkins BuildsClick to see older builds (194)
|
| if rowsAffected == 0 { | ||
| // If no rows were affected, it means the setting does not exist | ||
| return errors.New("settings not initalized, please call CreateSettings first") | ||
| } |
There was a problem hiding this comment.
"Bug" we found with @osmaczko . If the settings are not created at the start (of a test for example), then saveSetting still "works", ie doesn't send an error. It makes debugging the failing test very annoying.
| backup := &protobuf.Backup{} // TODO: fill me in more efficient way than waku backup | ||
|
|
||
| // TODO is using this for the clock ok? | ||
| settings, err := s.prepareSyncSettingsMessages(uint64(time.Now().UnixMilli()), true) |
There was a problem hiding this comment.
The old code used the clock from clock, chat := m.getLastClockWithRelatedChat().
I think using the time for Now should be ok, since we're exporting Now.
|
|
||
| for _, setting := range backup.Settings { | ||
| // TODO is it ok to use the messenger here? Otherwise, I have to duplicate a lot of code | ||
| err = s.messenger.HandleBackedUpSettings(setting) |
There was a problem hiding this comment.
I'm using the messenger's HandleBackedUpSettings here because I'd need to duplicate a lot of code.
In theory HandleBackedUpSettings I could just move, because once we remove the old backup code, only the local backup will use it.
However, HandleBackedUpSettings calls extractAndSaveSyncSetting, which is used fro Sync messages too (paired devices), so the Messenger needs it too.
Thankfully, extractAndSaveSyncSetting doesn't rely on anything Messenger related, so maybe we could extract it to a common place where we only have to pass the needed function to save the setting?
Finally, I'll also be lacking the messengerSignalsHandler. There is probably another way to do it?
There was a problem hiding this comment.
I'll create a new issue to modify the code to not use the messenger here.
I'll also address the duplication there
| } | ||
|
|
||
| // TODO this is a duplicate of the code in messenger_handler. Should it be moved to a common place? | ||
| func (s *Service) handleSyncWatchOnlyAccount(message *protobuf.SyncAccount) (*accounts.Account, error) { |
There was a problem hiding this comment.
Similar to the Account Service, I had the issue of having to duplicate code. This Service doesn't have access to the Messenger, so I guess creating a share function would do the trick?
Let me know where and how is the best way to do it
There was a problem hiding this comment.
I'll create a separate issue to remove the duplication
|
I think it will be simpler to review the full final PR, though it will be a lot indeed. |
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
4 times, most recently
from
a86c42c to
6cc985b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6722 +/- ##
============================================
- Coverage 59.06% 28.79% -30.28%
============================================
Files 828 796 -32
Lines 101562 98874 -2688
============================================
- Hits 59991 28469 -31522
- Misses 33998 66047 +32049
+ Partials 7573 4358 -3215
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
6cc985b to
b54e6a5
Compare
jrainville
changed the title
|
@igor-sirotin @osmaczko ready for review. One PR to rule them all 😅 |
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
b54e6a5 to
96899be
Compare
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
4 times, most recently
from
5bacf9d to
c16b32a
Compare
| } else { | ||
| backupDir = filepath.Join(n.config.RootDataDir, "backups") | ||
| } | ||
| fullPath := filepath.Join(backupDir, fmt.Sprintf("%x_user_data.bkp", accountIdentifier[:4])) |
There was a problem hiding this comment.
Is overwriting the backup file each time intentional?
There was a problem hiding this comment.
Yes. Old ones don't serve any purpose and only clutter. Like the community description, only the latest is useful
| continue | ||
| } | ||
| if err := fn(raw); err != nil { | ||
| return fmt.Errorf("load %q failed: %w", name, err) |
There was a problem hiding this comment.
Something to reconsider: should the entire process fail if one of the modules cannot be restored?
| PrivateKey: crypto.Keccak256(crypto.FromECDSA(privateKey)), | ||
| FileNameGetter: filenameGetter, | ||
| // TODO set to true to enable the local backup | ||
| BackupEnabled: false, |
There was a problem hiding this comment.
Looking at it now, this can probably be removed. The condition for running the loop should be based on the Interval, if it's provided, the loop should run, otherwise, it shouldn't. In other words, BackupEnabled is redundant and doesn’t sound right in retrospect (I know it’s my own code, lol).
Re: original comment // TODO: check from database when last saved, this still holds, if it is decided to do backup once per week or so.
There was a problem hiding this comment.
Re: original comment // TODO: check from database when last saved, this still holds, if it is decided to do backup once per week or so.
I don't think it matters to know the last time it was saved. This process is way smaller than the current Waku backup. I currently put it at every 30 minutes even.
We used to backup very rarely because it polluted the network, but now it's local and in the background, so it doesn't affect much. So backing up as often as is realistic is a positive thing
node/get_status_node.go
Outdated
| n.localBackup.Register("settings", | ||
| func() ([]byte, error) { return n.accountsSrvc.ExportBackup() }, | ||
| func(data []byte) error { return n.accountsSrvc.ImportBackup(data) }) | ||
|
|
||
| n.localBackup.Register("wallet", | ||
| func() ([]byte, error) { return n.walletSrvc.LocalBackup().ExportBackup() }, | ||
| func(data []byte) error { return n.walletSrvc.LocalBackup().ImportBackup(data) }) | ||
|
|
||
| n.localBackup.Register("messenger", | ||
| func() ([]byte, error) { return n.wakuV2ExtSrvc.API().Messenger().ExportBackup() }, | ||
| func(data []byte) error { return n.wakuV2ExtSrvc.API().Messenger().ImportBackup(data) }) |
protocol/messenger.go
Outdated
| @@ -223,6 +223,10 @@ func (m *Messenger) GetOwnPrimaryName() (string, error) { | |||
| return m.settings.DisplayName() | |||
| } | |||
|
|
|||
| func (m *Messenger) GetBackupPath() (string, error) { | |||
There was a problem hiding this comment.
Should not belong to messenger.
There was a problem hiding this comment.
I put it in the Accounts Service instead. Is that ok?
| @@ -580,6 +580,7 @@ func (m *Messenger) handleSyncChats(messageState *ReceivedMessageState, chats [] | |||
| if err != nil { | |||
| return err | |||
| } | |||
| messageState.AllChats.Store(chat.ID, chat) | |||
There was a problem hiding this comment.
It is already done in m.saveChat.
There was a problem hiding this comment.
This is the messageState, not the m (messenger). I need the messageState to be updated for the response to contains the chat correctly.
I don,t recall the exact use case, but if I didn't add this, I think the chat didn't appear after the backup, I had to reload the app.
| chat.LastClockValue = clock | ||
| err = m.saveChat(chat) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Why ExportBackup is modifying chat's clock?
There was a problem hiding this comment.
I copied the code from BackupData.
As I can see, that clock is used to be put inside the backup protobufs of the communities and chats.
So it should help to not import an old backup.
However, I'm pretty sure if the clock is the same as an old one, we also ignore, so I can probably safely delete it. What do you think?
There was a problem hiding this comment.
Thanks for the comments @osmaczko I addressed some of them. I asked a couple of further questions too and I'll start working on the backwards compatibility test
| } else { | ||
| backupDir = filepath.Join(n.config.RootDataDir, "backups") | ||
| } | ||
| fullPath := filepath.Join(backupDir, fmt.Sprintf("%x_user_data.bkp", accountIdentifier[:4])) |
There was a problem hiding this comment.
Yes. Old ones don't serve any purpose and only clutter. Like the community description, only the latest is useful
| PrivateKey: crypto.Keccak256(crypto.FromECDSA(privateKey)), | ||
| FileNameGetter: filenameGetter, | ||
| // TODO set to true to enable the local backup | ||
| BackupEnabled: false, |
There was a problem hiding this comment.
Re: original comment // TODO: check from database when last saved, this still holds, if it is decided to do backup once per week or so.
I don't think it matters to know the last time it was saved. This process is way smaller than the current Waku backup. I currently put it at every 30 minutes even.
We used to backup very rarely because it polluted the network, but now it's local and in the background, so it doesn't affect much. So backing up as often as is realistic is a positive thing
protocol/messenger.go
Outdated
| @@ -223,6 +223,10 @@ func (m *Messenger) GetOwnPrimaryName() (string, error) { | |||
| return m.settings.DisplayName() | |||
| } | |||
|
|
|||
| func (m *Messenger) GetBackupPath() (string, error) { | |||
There was a problem hiding this comment.
I put it in the Accounts Service instead. Is that ok?
| @@ -580,6 +580,7 @@ func (m *Messenger) handleSyncChats(messageState *ReceivedMessageState, chats [] | |||
| if err != nil { | |||
| return err | |||
| } | |||
| messageState.AllChats.Store(chat.ID, chat) | |||
There was a problem hiding this comment.
This is the messageState, not the m (messenger). I need the messageState to be updated for the response to contains the chat correctly.
I don,t recall the exact use case, but if I didn't add this, I think the chat didn't appear after the backup, I had to reload the app.
| chat.LastClockValue = clock | ||
| err = m.saveChat(chat) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I copied the code from BackupData.
As I can see, that clock is used to be put inside the backup protobufs of the communities and chats.
So it should help to not import an old backup.
However, I'm pretty sure if the clock is the same as an old one, we also ignore, so I can probably safely delete it. What do you think?
| ) | ||
|
|
||
| // Service is a wallet local backup service. | ||
| type Service struct { |
There was a problem hiding this comment.
Maybe manager would be better? or just backuper even if it's not a real word? 😄
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
2013e7d to
45cf37a
Compare
Fixes #18248 #18107 #18120 #18122 #18141 #18121 Implements all the backend for the local user data backups. Uses the scaffold @osmaczko created to make it more extendable. Instead of using the old Messenger code for backups, we now use a new controller that let's other modules register to be backed up. This includes the messenger for chats, contacts and communities, the accounts service for settings and the wallet service for watch-only accounts. I also created new Protobufs for those backups, so that we no longer user the super generic Waku Backup protobuf. Finally, I added some integration tests and a functional test that does the whole flow. A lot of cleanups can still be done to reduce duplication, but they can be done in another commit/issue, since the internal of the services can be modified without the backup process being affected, since the protobufs are in place already.
jrainville
force-pushed
the
feat/local-backup-implement-controller
branch
from
45cf37a to
4f23465
Compare
Fixes status-im/status-desktop#18248 status-im/status-desktop#18107 status-im/status-desktop#18120 status-im/status-desktop#18122 status-im/status-desktop#18141 status-im/status-desktop#18121
This is the big PR replacing all the smaller ones, since a lot of the early PRs are now semi-obsolete.
Implements all the backend for the local user data backups.
Uses the scaffold @osmaczko created to make it more extendable. Instead of using the old Messenger code for backups, we now use a new controller that let's other modules register to be backed up. This includes the messenger for chats, contacts and communities, the accounts service for settings and the wallet service for watch-only accounts.
I also created new Protobufs for those backups, so that we no longer user the super generic Waku Backup protobuf.
Finally, I added some integration tests and a functional test that does the whole flow.
A lot of cleanups can still be done to reduce duplication, but they can be done in another commit/issue, since the internal of the services can be modified without the backup process being affected, since the protobufs are in place already.