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

feat: new backup transfer method #3489

Closed
wants to merge 29 commits into from
Closed

feat: new backup transfer method #3489

wants to merge 29 commits into from

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Jul 7, 2022

⚠️ There be dragons 🐉
There be exciting things

Idea: use p2p based transfer methods to move backups from one device to the other.

Implementation: Use iroh a new implementation of IPFS to build iroh-share, a simple protocol that moves data from device a to device b.

Local Testing

Using the new commands send-backup and receive-backup <Ticket> you can test this on a local machine or between two machines running the repl example.

Current Restrictions

  • Both devices must be in the same network to discover each other

TODOs

Before Merging

  • finish iroh-share
  • test on very large files
  • add progress bar
  • add indicator for finishing
  • review current security properties
  • fixup todos
  • add documentation
  • finish C-api

Future

  • improve iroh-shares hole punching so it does not need to be on the same network anymore
  • add python bindings
  • add node bindings

@@ -222,7 +222,7 @@ impl<'a> BlobObject<'a> {
/// to be lowercase.
pub fn suffix(&self) -> Option<&str> {
let ext = self.name.rsplit('.').next();
if ext == Some(&self.name) {
if ext == Some(self.name.as_str()) {
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 had to make these changes because things would suddenly not compile anymore otherwise, not sure what happened there, seems like a compiler issue 🤷

@Simon-Laux
Copy link
Member

Simon-Laux commented Jul 10, 2022

In which direction does this go? only "scan to import" for now?
Though I guess it's probably easy to switch directions once the connection works at all (to also allow scan code from computer (when computer has no camera) to export from phone to that computer)

@link2xt
Copy link
Collaborator

link2xt commented Sep 10, 2022

librocksdb takes forever to build, why is it needed as a dependency? Is it possible to disable it as a feature somehow?
I see iroh supports different stores: https://github.com/n0-computer/iroh/tree/main/stores

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
deltachat-ffi/Cargo.toml Show resolved Hide resolved
Comment on lines +2612 to +2652
* The path must exist and being accessible at least until the other device has received all data.
* The folder is not cleaned up by the backup send;
* a temporary directory seems to be good place therefore.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a UI developer, but this seems a bit awkward and something that the core could all handle internally?

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 don't have strong opinions in either direction @r10s what do you think?

Copy link
Member

@r10s r10s Sep 23, 2022

Choose a reason for hiding this comment

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

in general, it is nicer if the UI would not need to take care about such details.

but not sure how easy it really is for the core to really fulfill the requirements. so, as a pragmatic approach, i am also fine with leaving things up to the UIs for now, where we can more safely pass a directly that is really handled by the OS.

* The path must exist and being accessible at least until the other device has received all data.
* The folder is not cleaned up by the backup send;
* a temporary directory seems to be good place therefore.
* @param passphrase Used to at-rest-encrypt the backuped database in `folder` before sending,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param passphrase Used to at-rest-encrypt the backuped database in `folder` before sending,
* @param passphrase Used to at-rest-encrypt the backup in `folder` before sending,

Copy link
Member Author

Choose a reason for hiding this comment

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

no this is correct, only the database is encrypted

* similar to dc_imex().
* The same passphrase must be given to dc_receive_backup() on the other device.
* If NULL or empty string is given, the sent backup is not encrypted at rest.
* The transport of the backup is always encrypted additionally.
Copy link
Member

Choose a reason for hiding this comment

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

If the transport is encrypted why is this passphrase still needed? Would be nice if this described what it is protecting against as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

This passphrase is there to encrypt the sql database. It is the same as the existing other backup methods. I personally would love to drop it, but didn't want to open that can of worms.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @r10s

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i'd also better leave that can of worms closed, at least for now :)



/**
* Waits for the sending to finish and frees the backup sender object.
Copy link
Member

Choose a reason for hiding this comment

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

Is thee a way to abort the sending?

Copy link

Choose a reason for hiding this comment

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

Or resume any time later (incl. never)?

* but on a device that has scanned the QR code generated by dc_backup_sender_qr().
*
* While dc_receive_backup() returns immediately, the started job may take a while;
* you can stop it using dc_stop_ongoing_process().
Copy link
Member

Choose a reason for hiding this comment

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

An ongoing process is IIRC a global modal that can not be dismissed. I guess this is normally done from the "create account" screen or similar so that is what is desired?

examples/repl/cmdline.rs Outdated Show resolved Hide resolved
src/imex.rs Show resolved Hide resolved
src/imex.rs Show resolved Hide resolved
src/imex.rs Outdated Show resolved Hide resolved
src/imex.rs Outdated Show resolved Hide resolved
src/imex.rs Show resolved Hide resolved
src/imex.rs Show resolved Hide resolved
src/imex.rs Show resolved Hide resolved
src/imex.rs Outdated Show resolved Hide resolved
src/imex.rs Show resolved Hide resolved
@dignifiedquire
Copy link
Member Author

librocksdb takes forever to build, why is it needed as a dependency? Is it possible to disable it as a feature somehow?
I see iroh supports different stores: https://github.com/n0-computer/iroh/tree/main/stores

@link2xt no it is not optional, those stores are just for experimentation. RocksDB is the only supported DB in iroh atm

Copy link
Member

@flub flub left a comment

Choose a reason for hiding this comment

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

foo-shm and foo-wal are checked in accidentally i think.

CI isn't happy yet, but i think this is probably mostly fine

@dignifiedquire
Copy link
Member Author

foo-shm and foo-wal are checked in accidentally i think.

ups, fixed

@dignifiedquire
Copy link
Member Author

iroh requires rust1.63, so this bumps the msrv for dc

@r10s
Copy link
Member

r10s commented Mar 6, 2023

closing, this is superseded by #4007

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.

7 participants