Conversation
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
Seems fine, I'm just not sure how we can be certain that bitswap records will only ever start with this varint.
| result := make([]peer.AddrInfo, 0, len(parsedResponse.MultihashResults[0].ProviderResults)) | ||
| for _, m := range parsedResponse.MultihashResults[0].ProviderResults { | ||
| result = append(result, m.Provider) | ||
| if bytes.HasPrefix(m.Metadata, bitswapPrefix) { |
There was a problem hiding this comment.
How do you know that the metadata will have this prefix?
There was a problem hiding this comment.
The spec we settled on says that supported protocols should be uvarints in sorted order. bitswap is the smallest defined transport, so if it's supported it'll be at the beginning.
There was a problem hiding this comment.
Interesting so the protocols are not in user-specified order? Is this mostly because the current indexer advertisements are not for lists of transports but instead separate advertisements per transport? If so is the a short or longer lived expectation (not that this is relevant to merging this PR specifically, but trying to understand how it'll operate with other systems that allow suggested prioritization/ordering).
There was a problem hiding this comment.
this was a constraint on extending from 1 protocol to an array of them in a backwards compatible way with the current wire protocol.
Once we switch to something compatible with the records being discussed in reframe we'll make a switch once in what we ask from providers and will remove this expectation.
| return nil, fmt.Errorf("unexpected number of responses") | ||
| } | ||
| result := make([]peer.AddrInfo, len(parsedResponse.MultihashResults[0].ProviderResults)) | ||
| result := make([]peer.AddrInfo, 0, len(parsedResponse.MultihashResults[0].ProviderResults)) |
There was a problem hiding this comment.
@guseggert it feels like we should've been getting errors due to this previously no? Any idea why not.
There was a problem hiding this comment.
i think nil results are dropped without error in the combiner
There was a problem hiding this comment.
Yeah they are dropped. What kind of error would you have expected, @aschmahmann ?
There was a problem hiding this comment.
Not sure what's best here and obviously nothing bad happened, but if one of the systems started returning nil provider records to us that'd be a bug we'd potentially want to investigate. Probably just logging would be sufficient though, and ofc not a big rush given we fixed the above issue.
4 participants
Currently, all provider records coming back from storetheindex are being returned to clients.
We should filter that implicitly to only providers advertising bitswap while clients are implicitly expecting that behavior.