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

bug: new kubo/ipfs/libp2p protocols break JS ecosystem #310

Open
SgtPooki opened this issue Feb 14, 2023 · 2 comments
Open

bug: new kubo/ipfs/libp2p protocols break JS ecosystem #310

SgtPooki opened this issue Feb 14, 2023 · 2 comments
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization

Comments

@SgtPooki
Copy link
Contributor

SgtPooki commented Feb 14, 2023

Describe the bug
Multiple issues across the JS ecosystem when new protocols are added.

export const names: Record<string, Protocol> = {}
export const codes: Record<number, Protocol> = {}
export const table: Array<[number, number, string, boolean?, boolean?]> = [
[4, 32, 'ip4'],
[6, 16, 'tcp'],
[33, 16, 'dccp'],
[41, 128, 'ip6'],
[42, V, 'ip6zone'],
[43, 8, 'ipcidr'],
[53, V, 'dns', true],
[54, V, 'dns4', true],
[55, V, 'dns6', true],
[56, V, 'dnsaddr', true],
[132, 16, 'sctp'],
[273, 16, 'udp'],
[275, 0, 'p2p-webrtc-star'],
[276, 0, 'p2p-webrtc-direct'],
[277, 0, 'p2p-stardust'],
[280, 0, 'webrtc'],
[290, 0, 'p2p-circuit'],
[301, 0, 'udt'],
[302, 0, 'utp'],
[400, V, 'unix', false, true],
// `ipfs` is added before `p2p` for legacy support.
// All text representations will default to `p2p`, but `ipfs` will
// still be supported
[421, V, 'ipfs'],
// `p2p` is the preferred name for 421, and is now the default
[421, V, 'p2p'],
[443, 0, 'https'],
[444, 96, 'onion'],
[445, 296, 'onion3'],
[446, V, 'garlic64'],
[448, 0, 'tls'],
[460, 0, 'quic'],
[461, 0, 'quic-v1'],
[465, 0, 'webtransport'],
[466, V, 'certhash'],
[477, 0, 'ws'],
[478, 0, 'wss'],
[479, 0, 'p2p-websocket-star'],
[480, 0, 'http'],
[777, V, 'memory']
]
// populate tables
table.forEach(row => {
const proto = createProtocol(...row)
codes[proto.code] = proto
names[proto.name] = proto
})

To Reproduce
Steps to reproduce the behavior:

  1. pull down repo ipfs/ipfs-webui
  2. git checkout ea89e7f
  3. rm patches/multiaddr+8.1.2.patch
  4. npm install
  5. start ipfs daemon using kubo 0.18.1 or later in another terminal
    • Note the port number in the line API server listening on /ip4/127.0.0.1/tcp/<kuboPort>
  6. run KUBO_PORT_2033_TEST=<kuboPort> npm run test:unit -- src/bundles/identity.test.js
  7. Note the error "no protocol with name: quic-v1"

Expected behavior
Looking up unknown protocols does not propagate errors, nor cause requests to fail. We should fallback to supported/known protocols

Additional context

Potential Solutions

  1. Pull out protocols-table.js into it's own package. Keep it at v1.0.x, there are NO breaking changes, only protocols added. Any JS package stays up to date by depending directly on the table
  2. Stop throwing errors from
    export function getProtocol (proto: number | string): Protocol {
    if (typeof proto === 'number') {
    if (codes[proto] != null) {
    return codes[proto]
    }
    throw new Error(`no protocol with code: ${proto}`)
    } else if (typeof proto === 'string') {
    if (names[proto] != null) {
    return names[proto]
    }
    throw new Error(`no protocol with name: ${proto}`)
    }
    throw new Error(`invalid protocol id type: ${typeof proto}`)
    }
    , because different packages in the ecosystem are not handling this properly.
  3. backport any protocol-table changes for all multiaddr versions (No one wants to do this)
@SgtPooki SgtPooki changed the title bug: pull out multiaddr/src/protocols-table.js to it's own package bug: new kubo/ipfs/libp2p protocols break JS ecosystem Feb 14, 2023
@SgtPooki SgtPooki added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Feb 16, 2023
@2color
Copy link

2color commented Mar 11, 2023

Is this problem showing up in packages using an older version of js-multiaddr? If so, what's stopping an upgrade to the latest version of js-multiaddr?

@SgtPooki
Copy link
Contributor Author

SgtPooki commented Apr 2, 2023

Is this problem showing up in packages using an older version of js-multiaddr? If so, what's stopping an upgrade to the latest version of js-multiaddr?

I’ll have to investigate this to confirm. Maybe this problem is solved in the latest versions.

However, I imagine it would still be a problem if anyone is out of date on any package required, which can be really confusing.

This issue was intended to bring attention to this error and that the only viable solutions are:

  1. Update all packages
  2. Patch your old version of the package
  3. Ensure all packages in the lib-space (the ones we own) guard against new and unexpected failures.

However, all of those mean someone(consumers, consumers, owners; respectively) is responsible for work that we shouldn't require of them.

Number 1 is improbable to expect all consumers to be able to do.
Number 2 is a localized fix for a global problem.
Number 3 is a not-DRY and highly error-prone solution thats not practical nor maintainable.

Ideally, we could solve this problem in a future proof way where

  1. newly added protocols don’t need guarded against in every lib consuming this one
  2. Consumers with old deps arent forced between a rock and a hard place
  3. Devs don’t need to solve this error everytime we utilize this package.

I’m not 100% sure what the fix for this should be right now but I know we need to a better solution. My gut instinct for a solution is converting the fatal error to a logged but passable one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

2 participants