Skip to content

[WIP] feat: migrate to iroh 0.5 #4546

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

Closed
wants to merge 19 commits into from
Closed

[WIP] feat: migrate to iroh 0.5 #4546

wants to merge 19 commits into from

Conversation

dignifiedquire
Copy link
Collaborator

@dignifiedquire dignifiedquire commented Jul 14, 2023

Context: We will be releasing the new version of iroh (0.5.0) with support for holepunching next week.
This is a WIP PR to track that we can upgrade deltachat easily and test things out.

TODOs

Sorry, something went wrong.

@link2xt
Copy link
Collaborator

link2xt commented Jul 14, 2023

Is it possible to disable holepunching at first? So we have a stable release with updated iroh without quinn patches, and then enable holepunching in a later version.

@dignifiedquire
Copy link
Collaborator Author

Is it possible to disable holepunching at first?

Not entirely, we can disable usage of the relays, but not all of hole punching.

};

// get the hash of the next blob, or finish if there are no more
let Some(blob) = blobs.next() else {
Copy link

Choose a reason for hiding this comment

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

this assumes that the request is complete without gaps. Which I guess is fine, but maybe there should be an assert for the assumption somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how would you check for that?

Copy link

Choose a reason for hiding this comment

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

You could do an assert on the request. But then again you are creating the request a few lines earlier, so maybe just have a comment where the request is created? Otherwise check that child_index is consecutive.

@dignifiedquire
Copy link
Collaborator Author

@link2xt is this a known issue?

error: higher-ranked lifetime error
   --> deltachat-jsonrpc/src/api/mod.rs:145:1
    |
145 | / #[rpc(
146 | |     all_positional,
147 | |     ts_outdir = "typescript/generated",
148 | |     openrpc_outdir = "openrpc"
149 | | )]
    | |__^
    |
    = note: could not prove `Pin<Box<[async block@deltachat-jsonrpc/src/api/mod.rs:145:1: 149:3]>>: CoerceUnsized<Pin<Box<(dyn Future<Output = Result<Value, yerpc::Error>> + Send + 'd)>>>`
    = note: this error originates in the attribute macro `rpc` (in Nightly builds, run with -Z macro-backtrace for more info)

@link2xt
Copy link
Collaborator

link2xt commented Jul 24, 2023

@link2xt is this a known issue?

error: higher-ranked lifetime error
   --> deltachat-jsonrpc/src/api/mod.rs:145:1
    |
145 | / #[rpc(
146 | |     all_positional,
147 | |     ts_outdir = "typescript/generated",
148 | |     openrpc_outdir = "openrpc"
149 | | )]
    | |__^
    |
    = note: could not prove `Pin<Box<[async block@deltachat-jsonrpc/src/api/mod.rs:145:1: 149:3]>>: CoerceUnsized<Pin<Box<(dyn Future<Output = Result<Value, yerpc::Error>> + Send + 'd)>>>`
    = note: this error originates in the attribute macro `rpc` (in Nightly builds, run with -Z macro-backtrace for more info)

No, master and stable branches build successfully currently.

@dignifiedquire dignifiedquire marked this pull request as ready for review July 25, 2023 13:32
@dignifiedquire
Copy link
Collaborator Author

I am confused by the latest failures

  • cargo deny check passes locally for me
  • the async python test failure seems to be unrelated

@link2xt
Copy link
Collaborator

link2xt commented Jul 25, 2023

  • cargo deny check passes locally for me

run scripts/deny.sh, it updates the index.

@link2xt
Copy link
Collaborator

link2xt commented Jul 28, 2023

I updated Desktop to use this branch and tried to scan the QR code generated with iroh 0.5, but Android failed with an error "invalid DCBACKUP payload". Is this incompatible with previous version of iroh?

EDIT: I have now compiled Android with iroh 0.5 and backup transfer from Desktop to Android worked.

So the problem is that QR code format changed and only iroh 0.5 can parse the tickets generated by iroh 0.5.

@r10s
Copy link
Contributor

r10s commented Aug 2, 2023

So the problem is that QR code format changed and only iroh 0.5 can parse the tickets generated by iroh 0.5.

ftr, as discussed in n0 chat, we cannot merge this pr in this form for that reason (we would break add-second-device for at least some weeks until all delta chat versions are updated)

@link2xt link2xt deleted the branch main October 25, 2023 21:22
@link2xt link2xt closed this Oct 25, 2023
@link2xt link2xt reopened this Oct 25, 2023
@link2xt link2xt changed the base branch from master to stable October 25, 2023 21:30
@link2xt
Copy link
Collaborator

link2xt commented Oct 26, 2023

As a temporary solution I forked 0.4.1 and updated dependencies: #4881

@link2xt
Copy link
Collaborator

link2xt commented Nov 28, 2023

Current plan is to keep using iroh 0.4 (we are currently at iroh 0.4.2 pre-release) to keep backwards compatibility. We may eventually switch backup transfer to using latest iroh-net directly to setup a connection and use it to stream the .tar file. This should also allow to stream the backup in any direction, so would be a user-visible improvement.

@link2xt link2xt closed this Nov 28, 2023
@dignifiedquire
Copy link
Collaborator Author

In light of #5041 this becomes more relevant to avoid pulling in two versions of iroh. The biggest blocker to this was backwards compatibility, before proposing any solutions @link2xt @hpk42 could I get an overview of what the stability you need/want for this is?

@link2xt
Copy link
Collaborator

link2xt commented Jan 26, 2024

@dignifiedquire We are ok with breaking compatibility once, but then it makes sense to switch from using full iroh stack to just setting up a connection and allow backup transfer in both directions (see my previous comment) without agreeing in advance on the content. But then I expect that establishing connections given IP addresses and peer ID should be backwards-compatible in future versions of iroh.

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.

None yet

4 participants