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

Bitswap Retrieval, Sync version #86

Merged
merged 21 commits into from
Feb 16, 2023
Merged

Bitswap Retrieval, Sync version #86

merged 21 commits into from
Feb 16, 2023

Conversation

hannahhoward
Copy link
Collaborator

Goals

Get bitswap retrieval working

Implementation

more information coming (to be delivered sync for now)

add context key based blockstore and routing implementations that limit results for a bitswap
session to the retrieval in question
@@ -156,6 +160,8 @@ type RetrievalEvent interface {
// StorageProviderId returns the peer ID of the storage provider if this
// retrieval was requested via peer ID
StorageProviderId() peer.ID
// Protocol
Protocols() []multicodec.Code
Copy link
Member

Choose a reason for hiding this comment

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

We've gone plural for Protocols but singular for StorageProviderId. Some events (Candidates*) return an empty peerID for a call to StorageProviderId() because they have a plural form that makes sense for them, StorageProviderIds(). Aren't we going to end up in a situation where the majority of events have a single protocol in the slice returned from this? How many will it make sense to have the plural form?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

arguably, the right thing here is to just return retrieval candidates, IMHO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but actually, I think storage provider id indicates that an event is associated with a specific storage provider. protocols indicates all the protocols it applies to. Honestly? I'm not sure what the right thing is.

pkg/retriever/retriever.go Outdated Show resolved Hide resolved
Comment on lines +113 to +114
CandidateSplitter: NewProtocolSplitter([]multicodec.Code{multicodec.TransportGraphsyncFilecoinv1, multicodec.TransportBitswap}),
CandidateRetrievers: candidateRetrievers,
Copy link
Member

Choose a reason for hiding this comment

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

still not a fan of how these two arrays have to match up for this to work

Copy link
Member

Choose a reason for hiding this comment

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

and, what's going to happen when our candidateRetrievers only has graphsync but the splitter is considering both? Do we need a NullRetriever() to take its place or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no it's setup properly to just ignore

CoordinationKind: types.RaceCoordination,
CandidateSplitter: NewProtocolSplitter([]multicodec.Code{multicodec.TransportGraphsyncFilecoinv1, multicodec.TransportBitswap}),
CandidateRetrievers: candidateRetrievers,
CoordinationKind: types.RaceCoordination,
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 going to get weird if/when we have both strategies using the same linksystem.
The CAR(s) shouldn't double-write if both bitswap and graphsync both fetch the same block and try and put it, but we ought to test that this works cleanly and we don't end up with weird CARs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea I know we'll fix in the streaming PR

@hannahhoward hannahhoward marked this pull request as ready for review February 15, 2023 03:46
go.mod Outdated Show resolved Hide resolved
"github.com/stretchr/testify/require"
)

func TestBitswapRetriever(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

some nice tricks in here to make the test less verbose 👌

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2023

Codecov Report

Merging #86 (3ba9704) into main (1744b10) will increase coverage by 9.18%.
The diff coverage is 78.07%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   50.09%   59.27%   +9.18%     
==========================================
  Files          36       42       +6     
  Lines        2703     3202     +499     
==========================================
+ Hits         1354     1898     +544     
+ Misses       1288     1204      -84     
- Partials       61      100      +39     
Impacted Files Coverage Δ
cmd/lassie/daemon.go 0.00% <0.00%> (ø)
cmd/lassie/fetch.go 0.00% <0.00%> (ø)
cmd/lassie/main.go 0.00% <0.00%> (ø)
pkg/indexerlookup/candidatefinder.go 0.00% <0.00%> (ø)
pkg/internal/libp2p.go 0.00% <0.00%> (ø)
...kg/internal/testutil/collectingeventlsubscriber.go 80.00% <ø> (ø)
pkg/internal/testutil/mockcandidatefinder.go 23.80% <ø> (ø)
pkg/internal/testutil/mockclient.go 50.69% <ø> (ø)
pkg/retriever/graphsyncretriever.go 90.52% <ø> (ø)
pkg/retriever/bitswaphelpers/countinglinksystem.go 64.00% <64.00%> (ø)
... and 16 more

g.seq++
p, err := p2ptestutil.RandTestBogusIdentity()
if err != nil {
panic("FIXME") // TODO change signature
Copy link
Member

Choose a reason for hiding this comment

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

what's going on here, are we tied to a particular API for this struct or is this just an annoying TODO to get to later because it's too cumbersome for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

copied from go-bitswap # oy

Comment on lines -42 to +51
httpServer, err := httpserver.NewHttpServer(cctx.Context, address, port)

// create a lassie instance
lassie, err := lassie.NewLassie(cctx.Context, lassie.WithProviderTimeout(20*time.Second))
if err != nil {
return err
}

httpServer, err := httpserver.NewHttpServer(cctx.Context, lassie, address, port)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +57 to +62
if cfg.Host == nil {
var err error
cfg.Host, err = internal.InitHost(ctx, multiaddr.StringCast("/ip4/0.0.0.0/tcp/6746"))
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@hannahhoward hannahhoward merged commit d470217 into main Feb 16, 2023
@rvagg rvagg deleted the feat/bitswap branch February 20, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants