Update Dependencies & Go #35
Conversation
chore: update dependencies
|
Well, the tests seem to fail, but the exact same query fails on the live version: 
I don't think the current tests are very reproducible as they rely on having Kubo installed and running, as well as an active Internet connection, and specific expectations from the network. |
daemon.go
Outdated
| func providerRecordInDHT(ctx context.Context, d kademlia, c cid.Cid, p peer.ID) bool { | ||
| func providerRecordInDHT(ctx context.Context, d kademlia, c cid.Cid) bool { |
There was a problem hiding this comment.
What's the idea behind the change here? This version seems to answer the question "has anyone advertised the record" which is different from "did node X that is hosting the data advertise it has the data".
Unless this is part of bigger changes towards #24 or #6, seems like we'd rather answer the second question. Otherwise, someone might see:
✔ Successfully connected to multiaddr
✔ Found multiaddr with 2 dht peers
✔ Found the multihash in the dht
✔ Successfully downloaded the CID from the peer
and yet the data would still not be retrievable because a different peer (who is unreachable) had advertised themselves in the DHT but this one had not
There was a problem hiding this comment.
Ah, okay. I will revert this. I would also like to actually incorporate the features from #24. I don't mind tackling that in this PR.
Regarding the tests:
- Locally only one test is failing: checking if the bootstrap peer has the empty directory.
- In the CI there's more tests failing.
I still have to take a better look at it.
TODO: rewrite initial comment with the goals of this PR.
- Update dependencies and Go version (closes Upgrade go-libp2p for quic-v1 multiaddrs to work #34)
- Port features from
pl-diagnose(closes Port features from pl-diagnose #24) - Allow testing using CID only (closes Allow for testing using only a CID #6, likely addressed by above)
- Try getting the UCI going (maybe another PR)
- Improve Dockerfile (based on someguy/rainbow)
|
I'm closing this as it's unlikely that I'll get back to this until EOM. Feel free to reuse any of the commits in the future. |
|
Reopening this, as the PR seems to get most of the job done and we can continue from here (thanks @hacdias). I'm not planning on bringing in the pl-diagnose features in this PR though as it'll make it easier to land this one. |
| @@ -133,9 +154,9 @@ func (d *daemon) runCheck(writer http.ResponseWriter, uristr string) error { | |||
| } | |||
| } | |||
|
|
|||
| testHost, err := libp2p.New(libp2p.ConnectionGater(&privateAddrFilterConnectionGater{})) | |||
| testHost, err := d.createTestHost() | |||
There was a problem hiding this comment.
@aschmahmann Why do we create a new libp2p host for testing if Vole also creates a host?
There's a case here where the host created in this scope successfully connects to one of the maddrs of the passed peerID (using the results from peerAddrsInDHT), but then checkBitswapCID fails with the maddr that was passed in.
There was a problem hiding this comment.
There's a case here where the host created in this scope successfully connects to one of the maddrs of the passed peerID (using the results from peerAddrsInDHT), but then checkBitswapCID fails with the maddr that was passed in.
This might not be handled as cleanly as it could (particularly in handling things like if the users only wants to pass a /p2p/<peerID> multiaddr), but that failure is IMO a good one to have. We want separate checks for:
- Is the CID (really multihash) advertised in the DHT (or later IPNI)?
- Are the peer's addresses discoverable (particularly useful if the announcements are DHT based, but also independently useful)
- Is the peer contactable with the address the user gave us?
- Is the address the user gave us present in the DHT?
- Does the peer say they have at least that one CID (doesn't say anything about the rest of any associated DAG but it a quick test and start) when asked using a protocol most clients speak (i.e. currently Bitswap, later also trustless gateways, etc.)?
If we allowed the test to succeed instead of fail we wouldn't have a check on just the passed multiaddr working. We can do something to tease out each test better, but maybe better to just follow that up in another PR
There was a problem hiding this comment.
Yeah I agree It's very useful to have all those 5 separate checks.
I think we just need to make sure we structure the results better. Will follow up in a separate PR
Closes #34.