WIP - feat: add TCP and WS filtering with /ipfs fragment

[dryajov]

No description provided.

[coveralls]

Coverage increased (+0.7%) to 91.837% when pulling 94142af on dryajov:feat/ipfs-filter into 0101bd2 on whyrusleeping:master.

[daviddias]

What is the use case for TCP_IPFS, utility method? This might replicate faster, why not do

mafmt.IPFS(ma) && mafmt.TCP(ma)

[dryajov]

IPFS(ma) would match WS and WebRTC addrs as well, I believe?

[daviddias]

Well, then you do a match for those and negate their values, this is all booleans anyway, you can compose things

[dryajov]

mafmt.IPFS(ma) would match TCP, WebRTC and WS, even if I do something like

!mafmt.WS(ma) && !mafmt.<some other transport/proto>(ma) && mafmt.IPFS(ma)

mafmt.IPFS(ma) would still match them, since it matches everything that has /ipfs in it. I think a better way would be to allow mafmt.TCP(), mafmt.WS(), etc... to also match /ipfs fragments. The reason I did it in this way is to avoid breaking some other functionality, but I think I'm being overly cautious, don't think anything else would break.

[daviddias]

Yeah, sounds about right. A TCP multiaddr is still valid if it has a IPFS hash in it.

[dryajov]

@whyrusleeping @diasdavid I don't seem to have perms to push to this repo anymore... I have some changes sitting locally that resolve the merge conflicts.

[daviddias]

@dryajov I believe you never had, the PR is coming from your fork

[dryajov]

@diasdavid ugh, you're right 🥗 , somehow I repointed origin locally. Thanks!

[coveralls]

Coverage increased (+0.5%) to 91.837% when pulling 0673ac2 on dryajov:feat/ipfs-filter into 41a8a85 on whyrusleeping:master.

[coveralls]

Coverage increased (+0.5%) to 91.837% when pulling b1e01b9 on dryajov:feat/ipfs-filter into 41a8a85 on whyrusleeping:master.

[dryajov]

@diasdavid latest changes introduced in #18 don't seem to work with circuit addrs, i.e. /p2p-circuit/ip4/1.2.3.4/tcp/3456/ws/p2p-webrtc-star/ipfs/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSoooo4. I'm looking into what the issue is, but this is not ready to merge yet.

[daviddias]

@dryajov see: ipfs/js-ipfs#981 and https://github.com/ipfs/pm/pull/492/files#r136710447, you probably need to update your forks with master

[dryajov]

@diasdavid I pulled (rebase) latest master into it, so it should have all the changes...

[dryajov]

@diasdavid just want to get some feedback on the latest changes.

I've implemented what we discussed above (#16 (comment)), but this is a breaking change as most addrs would now match the /ipfs fragment. Could you please take a look an let me know? This would require changes to how we detect if an addr has the /ipfs fragment (multiaddr.getPeerId() should work well). So far I've identified libp2p and js-ipfs, but there might be others.

NOTE: WebRTC addrs are still breaking with this change, but I haven't spent time understanding what the issue might be.

[daviddias]

NOTE: WebRTC addrs are still breaking with this change, but I haven't spent time understanding what the issue might be.

Have you rebased master onto this branch and did an npm fresh install?

[daviddias]

Without wanting to go back and forth. Is there any reason why a transport code itself wouldn't be able to make two assertions .matchIPFS and .matchUDP so that if the first one happen, it decapsulates IPFS first and then does the matchUDP?

Note, that is how all the transports have been working:

• https://github.com/libp2p/js-libp2p-websockets/blob/master/src/index.js#L46-L57
• https://github.com/libp2p/js-libp2p-tcp/blob/master/src/index.js#L66-L76

There is a separation between what is a valid addr for a spec transport (i.e TCP was here before) and there is the addrs that our libp2p- accept.

libp2p-tcp accepts TCP multiaddrs or TCP + IPFS, but that assertion should happen on the filter function and not in mafmt, which is a utility to check capabilities/presence of certain addrs.

[dryajov]

Basically because of this - https://github.com/libp2p/js-libp2p-tcp/pull/80/files, we though at the time that having mafmt handle those special circuit cases such as /ip4/127.0.0.1/tcp/4001/ws/ipfs/QmRelay/p2p-circuit/ipfs/QmDst would be a better approach. I'm not so sure anymore, I think the initial approach of handling the special cases might be simpler and safer.

The two relevant PR that lead to this change are -

• feat: skip p2p-circuit addresses libp2p/js-libp2p-websockets#61
• feat: skip p2p-circuit addresses libp2p/js-libp2p-tcp#80

[dryajov]

Also, I think that since then, we've also scratched listening on circuit addresses and the more advanced circuit-relay features - we might not even need this anymore (for now) because we're not going to be using that kind of addressing right now. I would say we leave it alone and revisit when we actually need this feature.

Glad you asked the question tho!

[dryajov]

@diasdavid seems like I spoke too soon, we still need this as we are supporting specific relay addrs. We can go with this approach or reopen the PRs linked above (handle circuit as a special case in .filter). I'm not so sure of which one is better. This PR is definitely higher risk as it affects everything that uses mafmt, handling a special case in .filter is uglier, but might be safer for the time being?

Here is @vyzo response regarding Go's handling of specific circuit addrs.

dryajov: it accepts them
10:13 but unless it's an active relay it's pretty much a hint
10:13 if it's an active relay, then it adds the address to its peerstore for the dial attempt
10:13 so it will honor them
10:14 ah,the dialer
10:14 sorry, i was mislieading
10:14 it handles them just fine
10:14 it adds to the peerstore for the dial attempt
10:14 so it's a hint
10:14 may or may not be used by the dialer
10:14 and maybe there is already an existing connection

[dryajov]

After a conversation with @diasdavid we've decided to handle this in the filter function of each transport.