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: kubo 0.18 with /quic-v1 and /webtransport #2363

Merged
merged 8 commits into from
Jan 27, 2023
Merged

Conversation

galargh
Copy link
Contributor

@galargh galargh commented Dec 15, 2022

Related to ipfs/kubo#9417

This PR is to upgrade Kubo to v0.18.0-rc1.

@galargh
Copy link
Contributor Author

galargh commented Dec 15, 2022

@lidel Do you have any ideas on what might be going on here https://github.com/ipfs/ipfs-desktop/actions/runs/3705807808/jobs/6280210555?

@lidel
Copy link
Member

lidel commented Dec 16, 2022

Oof, yes, this is a fun one: Kubo 0.18 is listening on /quic-v1 multiaddrs by default.

IPFS Desktop uses an old version of ipfsd-ctl that depends on old version of multiaddr without multiformats/js-multiaddr#294, and that combination fails to start due to unknown protocol:

$ npm start
2022-12-16T00:42:40.895Z error: [ipfsd] start daemon Error: no protocol with name: quic-v1
...

Upgrading to latest libs is soft-blocked until we finish ipfs/kubo#9125 (subjective, but my gut feeling is it makes no sense to invest time in old library if we need to switch anyway).

For now, did the same thing we did in ipfs/ipfs-webui#2073 and patched-in new protocol support. This unblocks version bump to Kubo 0.18 :)

Long term, we want to switch to js-kubo-rpc-client and make it not fail when unknown multiaddr is returned – filled ipfs/js-kubo-rpc-client#103 to track the proper fix.

@lidel lidel changed the title Kubo Upgrade: v0.18 feat: kubo 0.18 with /quic-v1 and /webtransport Dec 16, 2022
@lidel
Copy link
Member

lidel commented Jan 23, 2023

Added config migrations to ensure Desktop and Brave (brave/brave-browser#27965) will run with the same settings.

CI need to be made green (works for me on 18.12.0, but fails on CI with 18.13.0) – if anyone has time to investigate, I can send a JPG with a prize.

@galargh
Copy link
Contributor Author

galargh commented Jan 25, 2023

I updated my local node to 18.13 and modified the tests so that I can see what the actual errors are:

Error: TypeError: RequestInit: duplex option is required when sending a body.

which looks like nodejs/node#46221.

I run out of time for now. I might try to give it another go in the evening.

@galargh
Copy link
Contributor Author

galargh commented Jan 25, 2023

I think updating ipfs-http-client might help. I'll try it out later.

@galargh
Copy link
Contributor Author

galargh commented Jan 25, 2023

e6528d3 Test fixed. I also found the underlying fix: ipfs/js-ipfs-utils#239

Now it's Test end-to-end that keeps acting up.

@galargh
Copy link
Contributor Author

galargh commented Jan 25, 2023

I think it's this -

const ipfsHttpModule = await import('ipfs-http-client')
- that's failing when running in electron.

@galargh
Copy link
Contributor Author

galargh commented Jan 26, 2023

Going for a more minimal update and updating only ipfs-utils to v9.10.0 instead of ipfs-http-client to v60.0.0 seem to have worked 🥳

@galargh galargh marked this pull request as ready for review January 26, 2023 10:14
@galargh galargh requested a review from a team as a code owner January 26, 2023 10:14
@galargh galargh requested a review from lidel January 26, 2023 10:14
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

questions about multiaddr changes that Alex and Nishant were doing, but I'm good with this otherwise.

Comment on lines +1 to +34
diff --git a/node_modules/multiaddr/src/convert.js b/node_modules/multiaddr/src/convert.js
index c315201..80170c7 100644
--- a/node_modules/multiaddr/src/convert.js
+++ b/node_modules/multiaddr/src/convert.js
@@ -51,6 +51,7 @@ Convert.toString = function convertToString (proto, buf) {
case 55: // dns6
case 56: // dnsaddr
case 400: // unix
+ case 466: // certhash
case 777: // memory
return bytes2str(buf)

@@ -84,6 +85,7 @@ Convert.toBytes = function convertToBytes (/** @type {string | number } */ proto
case 55: // dns6
case 56: // dnsaddr
case 400: // unix
+ case 466: // certhash
case 777: // memory
return str2bytes(str)

diff --git a/node_modules/multiaddr/src/protocols-table.js b/node_modules/multiaddr/src/protocols-table.js
index 3431af5..8939fb1 100644
--- a/node_modules/multiaddr/src/protocols-table.js
+++ b/node_modules/multiaddr/src/protocols-table.js
@@ -60,6 +60,9 @@ Protocols.table = [
[445, 296, 'onion3'],
[446, V, 'garlic64'],
[460, 0, 'quic'],
+ [461, 0, 'quic-v1'],
+ [465, 0, 'webtransport'],
+ [466, V, 'certhash'],
[477, 0, 'ws'],
[478, 0, 'wss'],
[479, 0, 'p2p-websocket-star'],
Copy link
Member

Choose a reason for hiding this comment

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

I thought we updated multiaddr to 11? @whizzzkid @achingbrain

Copy link
Member

Choose a reason for hiding this comment

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

This is all in multiaddr@11.4.x so you shouldn't need to monkey patch this.

Copy link
Member

@lidel lidel Feb 8, 2023

Choose a reason for hiding this comment

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

IIUC we have no bandwidth to update things to latest versions, and that was blocking shipping Kubo release to Desktop users. Many users are in similar situation.

Taking step back, Kubo update should not FORCE update of entire JS stack. These things should not be so tightly coupled. One should be able to get protocol list and have placeholder for unknown ones.

But the way things are today, ipfs.swarm.peers will crash when the backend suddenly gets updated to a version that returns new, unknown protocol. See ipfs/js-kubo-rpc-client#103

@lidel lidel added the P1 High: Likely tackled by core team if no one steps up label Jan 27, 2023
@lidel lidel merged commit 644ce7e into main Jan 27, 2023
@lidel lidel deleted the kubo-upgrade-v0.18 branch January 27, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High: Likely tackled by core team if no one steps up
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants