[WIP] Extract provider logic from BitSwap by magik6k · Pull Request #4333 · ipfs/kubo

magik6k · 2017-10-21T18:47:12Z

<WIP>
Fixes #4311

~~This is not even remotely close to done~~ Getting there, PR mainly for comments / tests / progress tracking

whyrusleeping · 2017-10-24T15:01:16Z

blockservice/blockservice.go


 // NewBlockService creates a BlockService with given datastore instance.
-func New(bs blockstore.Blockstore, rem exchange.Interface) BlockService {
+func New(bs blockstore.Blockstore, rem exchange.Interface, p providers.Interface) BlockService {


The blockservice should not have to know anything about the providers system.

whyrusleeping · 2017-10-24T15:02:32Z

core/commands/bitswap.go

 			buf := new(bytes.Buffer)
 			fmt.Fprintln(buf, "bitswap status")
-			fmt.Fprintf(buf, "\tprovides buffer: %d / %d\n", out.ProvideBufLen, bitswap.HasBlockBufferSize)
+			fmt.Fprintf(buf, "\tprovides buffer: %d / %d\n", out.ProvideBufLen, provider.HasBlockBufferSize)


future note: we might want to have a separate command for the providers subsystem, and remove this information from here (since its no longer relevant to bitswap itself).

whyrusleeping · 2017-10-24T15:03:05Z

exchange/bitswap/bitswap.go

 	// kMaxPriority is the max priority as defined by the bitswap protocol
 	kMaxPriority = math.MaxInt32
+
+	self = peer.ID("")


whyrusleeping · 2017-10-24T15:04:20Z

exchange/offline/offline.go

 	return false
 }
+
+type offlineProviders struct{}


I think we can get away with not even having this at all

whyrusleeping · 2017-10-24T15:05:30Z

exchange/providers/providers.go

@@ -0,0 +1,87 @@
+package providers


I think this should probably not live in the exchange subdirectory, maybe make a new top level directory (ideally, we can move it out of the go-ipfs repo, but that can wait)

magik6k · 2017-11-22T18:57:54Z

This PR get's quite noisy with the providers interface propagation, I'm going to defer the strategies to the next PR while this will focus on extracting provider logic from bitswap.

magik6k · 2017-11-22T19:04:40Z

path/resolver.go

+	prov providers.Interface

-	ResolveOnce func(ctx context.Context, ds dag.DAGService, nd node.Node, names []string) (*node.Link, []string, error)
+	ResolveOnce func(ctx context.Context, ds dag.DAGService, prov providers.Interface, nd node.Node, names []string) (*node.Link, []string, error)


https://github.com/ipfs/go-ipfs/pull/4333/files#diff-cb03f3339782273ac5936e321903f907R16 needs this, though it's probably safe to not pass the interface here and use offline.Providers there.

whyrusleeping · 2017-11-23T00:24:12Z

exchange/bitswap/network/ipfs_impl.go


 // Provide provides the key to the network
 func (bsnet *impl) Provide(ctx context.Context, k *cid.Cid) error {
-	return bsnet.routing.Provide(ctx, k, true)


Does this call here actually send out a message?

It probably does, I'll try to write a test for that

We probably want to move away from that now. The equivalent of setting the bsnet.routing.Provide final parameter to 'false'

Agreed, though it may be worth making this a configurable thing.

It would be best to have the strategies apply here, though it would require a lot of refactoring to make it fit into the code base nicely.

whyrusleeping · 2017-11-23T00:24:35Z

exchange/bitswap/stat.go

 func (bs *Bitswap) Stat() (*Stat, error) {
 	st := new(Stat)
-	st.ProvideBufLen = len(bs.newBlocks)
+	st.ProvideBufLen = -1


its probably fine to remove this entirely

frrist

Other than my 2 nits - LGTM

frrist · 2017-11-30T19:41:32Z

core/commands/object/object.go

 		}
+
+		err = n.Providers.Provide(k)
+		if err != nil {


As you have done in other files, these error checks should be condensed to a single line. This will keep err in the scope of the if statement.

frrist · 2017-11-30T20:00:02Z

importer/helpers/helpers.go


-	_, err = db.batch.Add(childnode)
+	_, err = db.batch.Add(childnode) //TODO: do we need to provide here?
+	err = db.prov.Provide(childnode.Cid())


Minor - We should be consistent with how we pass the values returned from Add to Provide. Here we are ignoring the returned cid since it is available on the childnode. Elsewhere we use a separate variable to store the cid when calling Provide. I am not sure which way is more appropriate, what do you think?

We should use node.Cid(), Add won't be returning cid after refactor in ipfs/go-ipld-format#8 happens, good catch

magik6k · 2018-03-26T09:43:30Z

Squashed most of the commits to make it less painful to resolve conflicts.

I think it's ready for a review. 2 things to note:

The point of this PR is to extract providing logic from bitswap, it shouldn't change what gets provided
ipfs add will do providing after adding stuff, this can take a while and could use a separate progress bar, but it's quite likely to be removed when provider strategies get implemented. I think that this ux can be somewhat improved by looking if we run add on a daemon and do the providing background.

Stebalien · 2018-06-20T03:54:43Z

providers/providers.go

+	Provide(k *cid.Cid) error
+	ProvideRecursive(ctx context.Context, n ipld.Node, serv ipld.NodeGetter) error
+
+	FindProviders(ctx context.Context, k *cid.Cid) error


I assume this method exists so we can connect to providers and then automatically send them our wantlists? Really, we want to stop doing that. We'd rather send our wantlist to a select set of peers.

If we do keep this, let's call it "ConnectToProviders"

I'd rather not keep it.

It also does dht querying. I guess what you mean that instead of this method bitswap should use FindProvidersAsync and only send wantlists to those peers?

For now, I'd rename the method, and remove it as a part of other PR

Stebalien · 2018-06-20T03:54:54Z

providers/providers.go

+}
+
+// Interface defines providers interface to libp2p routing system
+type Interface interface {


Interface?

its a pretty common go thing. ends up being used as providers.Interface. see: https://golang.org/pkg/sort/#Interface

Stebalien · 2018-06-20T03:55:26Z

providers/providers.go

+// Interface defines providers interface to libp2p routing system
+type Interface interface {
+	Provide(k *cid.Cid) error
+	ProvideRecursive(ctx context.Context, n ipld.Node, serv ipld.NodeGetter) error


Should document if these are async or synchronous.

Stebalien · 2018-06-20T03:55:44Z

providers/providers.go

+	Ctx context.Context
+}
+
+// Interface defines providers interface to libp2p routing system


Could you expand on this?

Stebalien · 2018-06-20T05:51:07Z

providers/providers.go

+	return nil
+}
+
+func (p *providers) provideRecursive(ctx context.Context, n ipld.Node, serv ipld.NodeGetter, done *cid.Set) error {


We'll have to be careful not to pass an online dag service in here.

Stebalien · 2018-06-20T05:57:14Z

What is the end goal here? Is the end goal to have the provider service remember which blocks we should be providing? That is, are we planning on supporting options like ipfs add --provide=[root|files|...]?

As implemented, this patch forces the programmer to remember to call the provider system every time they add a block they want to provide to the network. Unless we're going to allow the programmer to make provider decisions at that point, this feels like a step backward. If this is the case, I'd rather add some form of EventedBlockstore interface and have a provider service subscribe to events from this service.

On the other hand, providing a way for users to specify how/when they want to provide blocks is useful and could further be used for access control.

whyrusleeping · 2018-06-21T15:57:46Z

@Stebalien

That is, are we planning on supporting options like ipfs add --provide=[root|files|...]?

Yes, we're planning on adding that.

Here's an excerpt from another doc i'm writing:

Content Routing, (most importantly the providing side of things) should not be inside bitswap, and bitswap should not ever block on some external service (like the DHT) being slow. This is currently a problem as for every block bitswap receives, it pushes that block to the provider service. The provider service is slow, and therefore must either buffer infinitely, or apply backpressure. That backpressure is significantly slowing down bitswap at the moment. There is already a PR open by @magik6k that separates these pieces out, but does not change the behavior. The next step after that PR is merged is to change the way we reason about providing. Every block added through the blockservice should not cause a separate providing event. Higher layers should inform the provider subsystem that they wish to provide a given graph, in a certain way. Then it is up to the provider subsystem to traverse the graph as needed and send out the appropriate provider messages. This way, the provider subsystem does not get one independent call for every node in the graph (requiring O(n) memory to buffer and track), it gets a single call for the root of the graph. This will help us avoid needing to apply backpressure on bitswap to avoid using too much memory there.

whyrusleeping · 2018-06-21T15:59:04Z

In general, having 'write data to the local store' also mean 'broadcast the fact that we have that piece of data' is pretty bad, both in terms of privacy and performance. 'forcing' the programmer to remember to call Provide is the right thing to do IMO.

Stebalien · 2018-06-21T17:10:54Z

In general, having 'write data to the local store' also mean 'broadcast the fact that we have that piece of data' is pretty bad, both in terms of privacy and performance. 'forcing' the programmer to remember to call Provide is the right thing to do IMO.

As long as we bring the reprovider in line with this. I just don't want to end up in an inconsistent situation where users don't explicitly provide but then we end up providing in the background anyways.

whyrusleeping · 2018-06-21T17:12:21Z

Yeah, the reprovider gets folded into the 'provider' as part of this

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

michaelavila · 2018-11-20T22:33:37Z

core/corehttp/gateway_handler.go

 			return
 		}

+		if err := i.node.Providers.Provide(c); err != nil {


@magik6k I have a question: why c here? It seems like this will try to provide the same Cid for each iteration of the loop. Is that intentional? If so, I'm curious why. I'm using this PR to inform a similar change I'm making and am caught up on this detail. Thanks.

Yeah, this is wrong. This should be either newnode.Cid() or more likely not at all here (Provide below should handle this, not 100% sure)

Stebalien · 2018-12-18T03:49:33Z

@magik6k given the work in #5774, should we close this?

magik6k · 2018-12-18T14:45:46Z

Yep, don't delete the branch yet though

ghost assigned magik6k Oct 21, 2017

ghost added the status/in-progress In progress label Oct 21, 2017

magik6k force-pushed the feat/provider-strategies branch 6 times, most recently from 561d784 to d571f6b Compare October 24, 2017 14:41

whyrusleeping reviewed Oct 24, 2017

View reviewed changes

magik6k force-pushed the feat/provider-strategies branch 2 times, most recently from f57e691 to 9f3ff87 Compare October 25, 2017 18:05

magik6k force-pushed the feat/provider-strategies branch 5 times, most recently from 63e7a42 to 52925ad Compare November 22, 2017 00:37

magik6k changed the title ~~[WIP] Provider strategies when adding blocks~~ [WIP] Extract provider logic from BitSwap Nov 22, 2017

magik6k commented Nov 22, 2017

View reviewed changes

whyrusleeping requested review from Kubuxu and frrist November 22, 2017 23:35

whyrusleeping reviewed Nov 23, 2017

View reviewed changes

magik6k force-pushed the feat/provider-strategies branch 2 times, most recently from 630ceb2 to dc9ed4d Compare November 30, 2017 12:59

frrist approved these changes Nov 30, 2017

View reviewed changes

magik6k force-pushed the feat/provider-strategies branch 2 times, most recently from 57a0c81 to 6fded34 Compare March 26, 2018 07:57

magik6k added the need/review Needs a review label Mar 26, 2018

magik6k force-pushed the feat/provider-strategies branch from 6fded34 to 0015e1a Compare March 26, 2018 09:49

whyrusleeping mentioned this pull request Jun 5, 2018

Slow transfer over LAN #5037

Closed

whyrusleeping requested a review from Stebalien June 5, 2018 05:43

magik6k force-pushed the feat/provider-strategies branch from 0015e1a to 30831b3 Compare June 5, 2018 21:05

Stebalien reviewed Jun 20, 2018

View reviewed changes

magik6k added 4 commits June 25, 2018 20:23

bitswap: extract provider workers

25ceb80

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

providers: ProvideRecursive

61d2e94

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

providers: Cleanup after rebase

d59cbe7

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

providers: add some docs

981daa7

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/provider-strategies branch from 30831b3 to 981daa7 Compare June 25, 2018 19:02

Stebalien mentioned this pull request Jul 19, 2018

Slow requests ipfs/go-ipfs-api#104

Closed

magik6k mentioned this pull request Jul 19, 2018

coreapi: DHT API #4804

Merged

3 tasks

Stebalien mentioned this pull request Aug 21, 2018

Massive DHT traffic on adding big datasets #2828

Closed

magik6k mentioned this pull request Oct 7, 2018

Add options to select provider preferences #3813

Closed

michaelavila mentioned this pull request Nov 13, 2018

Provider Subsystem + Strategies #5774

Closed

44 tasks

michaelavila reviewed Nov 20, 2018

View reviewed changes

magik6k closed this Dec 18, 2018

ghost removed the status/in-progress In progress label Dec 18, 2018

hacdias deleted the feat/provider-strategies branch May 9, 2023 10:55

Uh oh!

Conversation

magik6k commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frrist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien commented Jun 20, 2018

Uh oh!

whyrusleeping commented Jun 21, 2018

Uh oh!

whyrusleeping commented Jun 21, 2018

Uh oh!

Stebalien commented Jun 21, 2018

Uh oh!

whyrusleeping commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien commented Dec 18, 2018

Uh oh!

magik6k commented Dec 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

magik6k commented Oct 21, 2017 •

edited

Loading