mega update by Stebalien · Pull Request #4610 · ipfs/kubo

Stebalien · 2018-01-25T20:29:24Z

supersedes Update ipns validator #4597 but does not entirely replace it (i.e., not fixed).
supersedes Eliminate GetOfflineLinkService method from LinkService interface #4152
punts [RFC] Add an offline hint to contexts #4009

Features:

fixes build for iOS Building for iOS with gomobile fails with: undefined: syscall.O_DSYNC #4524 (@Stebalien, @magik6k)
new DAG interface from ipfs/go-ipld-format (@Stebalien).
more reliable secio handshake (@Stebalien).
(much) faster base58 (@Stebalien).
faster sha256 on some platforms (@Stebalien, @lgierth).
faster blake2b on all platforms (@Stebalien, @lgierth).
(massively) reduce memory usage of peerstore (@Stebalien, @whyrusleeping, @Arceliar).
Dial error on windows 7 #4512: fix dialing without reuseport/on Windows (@Stebalien).
reliably detect DHT capable peers (@Stebalien).
~~correctly validate DHT records (@dirkmc).~~ Use new DHT record validators.
allocate less in the dht (@Stebalien).
(much) faster metrics (@Stebalien).
accurate metrics, fixes Incorrect Bandwidth #4476 (@Stebalien).
fix dropped pubsub messages (@vyzo, @richardschneider).
fix some potential (rare) connection leaks on dial/accept (@FiloSottile).
reduce excessive memory usage when mass dialing (@whyrusleeping).
updated badgerdb (@magik6k)

If I'm missing something here, please comment.

To review this PR, please do so commit by commit for your own sanity.

Questions:

Are we happy with the fix for [RFC] Add an offline hint to contexts #4009 (offline dag)? Currently, I just take a blockservice instead and construct an offline dagservice on the fly (which is what GetOfflineLinkService did anyways).

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

dirkmc · 2018-01-25T20:44:20Z

namesys/publisher.go

+	if err != nil {
+		return ErrInvalidAuthor
+	}
+	if string(pid) != string(r.Author) {


We can remove the string casts here now right

Our code now better validates peer IDs. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Also: * Update the blockstore/blockservice methods to match. * Construct a new temporary offline dag instead of having a GetOfflineLinkService method. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2018-01-26T05:25:47Z

test/sharness/t0183-namesys-pubsub.sh

-    ipfsi 2 name resolve /ipns/$PEERID_0
+    ipfsi 1 name resolve /ipns/$PEERID_0;
+    ipfsi 2 name resolve /ipns/$PEERID_0;
+    true


@vyzo does this change make sense?

hrm, shouldn't we be testing if they succeeded?
that's why i had the &&.

the assumption is that name resolve returns 0 if it succeeds.

Yeah but shouldn't it not succeed? Nothing has been published yet, as far as I know.

yeah, good point! these are executed for their side-effect of subscribing to the topic.

So yes, change makes sense.

Stebalien

Self reviewed because I kind of doubt anyone will have time for a thorough review...

Stebalien · 2018-01-26T10:27:26Z

core/commands/dht.go

 		ctx := notif.RegisterForQueryEvents(req.Context(), events)

-		k := string(b58.Decode(req.Arguments()[0]))
+		id, err := peer.IDB58Decode(req.Arguments()[0])


This broke a test or two but it's strictly more correct.

sounds like faulty tests!

Yeah. I just wonder if anyone was relying on this in practice. One could theoretically use this to find the peers closest to some base58 encoded thing that's not a valid peer ID.

should probably make sure that this isnt how we do random-walk bootstrapping

oh wait, this is in a command. Nvm

Stebalien · 2018-01-26T10:31:13Z

unixfs/mod/dagmodifier.go

 				return nil, false, err
 			}

-			offset += bs


@whyrusleeping go vet complained. I believe this is the correct fix here but I just wanted to check. Not really related to this PR, just a drive-by fix.

Stebalien · 2018-01-26T10:32:30Z

blockservice/blockservice.go

 	if err := s.exchange.HasBlock(o); err != nil {
-		return nil, errors.New("blockservice is closed")
+		// TODO(stebalien): really an error?
+		return errors.New("blockservice is closed")


@whyrusleeping Should we not just log and continue here?

unclear. For now lets leave it as is.

Fair. Reported at #4623 (and the comment has been updated to point to the issue).

Stebalien · 2018-01-26T10:33:22Z

blockservice/blockservice.go

 		if err := s.exchange.HasBlock(o); err != nil {
-			return nil, fmt.Errorf("blockservice is closed (%s)", err)
+			// TODO(stebalien): Should this really *return*?
+			return fmt.Errorf("blockservice is closed (%s)", err)


Stebalien · 2018-01-26T10:34:57Z

core/commands/pin.go

+
+	bs := n.Blocks.Blockstore()
+	DAG := dag.NewDAGService(bserv.New(bs, offline.Exchange(bs)))
+	getLinks := dag.GetLinksWithDAG(DAG)


This is the new way to do offline stuff. We could also use the context trick if we get tired of this dance.

this is 3 lines instead of 1; ergonomically it's not an improvement...

Discussion: #4009

So, first, I made implementing LinkService optional. If your NodeGetter (the get side of the DAGService) doesn't implement the LinkService optimizations, it doesn't have to implement the interface.

Next, I tried adding a OfflineNodeGetter method to the NodeGetter interface. However, that really tricky and messy:

We realized that we'd probably want Offline* methods on every related service (which would kind of suck).

Things like bitswap sessions (which I wanted to turn into valid NodeGetters would also have to implement this method). I simply couldn't find a clean way to do this.

So, in #4009, I suggested adding an "offline" flag to the context. I still think that's the best way to go because I keep running into similar cases where I want to try to do something without touching the network but @kevina reasonably objected that it was a bit magical and easy to screw up.

Hence this solution.

I could add a helper function but, really, I kind of want this to be ugly. I'm not happy with this solution at all and don't want this pattern repeated all over the codebase. For one, it makes the LinkService optimization totally useless because we don't reuse the DAG (we construct a new one with the blockstore when we do the GC).

fair enough.

Stebalien · 2018-01-26T10:36:56Z

core/corerepo/gc.go

 	}

-	return gc.GC(ctx, n.Blockstore, n.DAG, n.Pinning, roots)
+	return gc.GC(ctx, n.Blockstore, n.Pinning, roots)


Now constructs a dag from the blockstore.

Stebalien · 2018-01-26T10:38:39Z

importer/helpers/helpers.go

+		Offset:   offset,
+		FullPath: fullPath,
+		Stat:     stat,
+	}


go vet yelled at me 😨.

Stebalien · 2018-01-26T10:39:08Z

merkledag/batch.go

-var ParallelBatchCommits = runtime.NumCPU() * 2
-
-// Batch is a buffer for batching adds to a dag.
-type Batch struct {


Now in go-ipld-format.

vyzo · 2018-01-26T12:15:18Z

blockservice/blockservice.go

 }

-func (s *blockService) AddBlocks(bs []blocks.Block) ([]*cid.Cid, error) {
+func (s *blockService) AddBlocks(bs []blocks.Block) error {


are we losing any functionality by dropping the cids from the return type?

Not really. We can get the CIDs from the blocks so there's no need to return them here. It just complicates the blockservice (and dagservice which is why I removed this).

Although based on the current implementation we are not losing any functionally here. If AddBlocks is ever enhanced to just add some of the blocks (and not return early on error) it will make it difficult to determine which blocks where added.

I wouldn't call that an enhancement

Fair enough. But it is still possible for something to go wrong while adding the blocks, such as running out of space. However, since the underlying BlockStore does not indicate which blocks where added there is no way to get this information. I will make a separate issue for this so we can figure out a proper solution.

vyzo · 2018-01-26T12:16:15Z

core/commands/dht.go

 		ctx := notif.RegisterForQueryEvents(req.Context(), events)

-		k := string(b58.Decode(req.Arguments()[0]))
+		id, err := peer.IDB58Decode(req.Arguments()[0])


sounds like faulty tests!

vyzo · 2018-01-26T12:30:29Z

core/commands/pin.go

+
+	bs := n.Blocks.Blockstore()
+	DAG := dag.NewDAGService(bserv.New(bs, offline.Exchange(bs)))
+	getLinks := dag.GetLinksWithDAG(DAG)


this is 3 lines instead of 1; ergonomically it's not an improvement...

vyzo · 2018-01-26T12:36:55Z

namesys/publisher.go

 		return ErrInvalidAuthor
 	}
-	if string(pid) != string(r.Author) {
+	if pid != r.Author {


clown shoes!

whyrusleeping · 2018-01-27T20:55:50Z

@Stebalien sharness on OSX is failing.

Stebalien · 2018-01-29T05:22:19Z

(well, once someone reviews it)

magik6k

Github doesn't want to load diff past importer/helpers/dagbuilder.go (which is about half of the changes), I'll try to review the rest somehow.

magik6k · 2018-01-29T12:36:56Z

core/coreapi/key_test.go


-	if keys[0].Path().String() != "/ipns/Qmfoo" {
+	if keys[0].Path().String() != "/ipns/"+testPeerID {
 		t.Errorf("expected the key to have path '/ipns/Qmfoo', got '%s'", keys[0].Path().String())


nitpick: this should be

t.Errorf("expected the key to have path '/ipns/%s', got '%s'", testPeerID, keys[0].Path().String())

magik6k · 2018-01-29T13:13:20Z

core/core.go

+	metrics "gx/ipfs/Qmb1QrSXKwGFWgiGEcyac4s5wakJG4yPvCPk49xZHxr5ux/go-libp2p-metrics"
+	cid "gx/ipfs/QmcZfnkapfECQGcLZaf9B79NRg7cRa9EnZh4LSbkCzwNvY/go-cid"
+	dht "gx/ipfs/Qmdm5sm2xHCXNaWdxpjhFeStvSNMRhKQkqpBX7aDcqXtfT/go-libp2p-kad-dht"
+	node "gx/ipfs/Qme5bWv7wtjUNGsK2BNGVUFPKiuxWrsqrtvYwCLRw8YFES/go-ipld-format"


With more things in go-ipld-format we may want to change the naming from node to something like ipld - imo node is a bit confusing for some of the things that are now there.

If we decide to, I'm fine with thin being in another PR, this one is big enough.

Meh. Might as well do this now. Anyways, go makes this pretty easy :).

magik6k · 2018-01-29T14:24:44Z

Looked at the rest of the changes locally, other than the two comments LGTM

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

fixes #4524 License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2018-01-29T21:11:07Z

Please ignore codeclimate. It loves to punish those who refactor for the sins of others.

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2018-01-30T00:10:36Z

CI failure: #3970.

magik6k

LGTM - Used this for a while, didn't notice anything strange.

whyrusleeping

Looked through it locally, nothing was immediately objectionable. But it was a pretty massive diff.

kevina · 2018-01-30T23:22:27Z

pin/gc/gc.go


-	ls = ls.GetOfflineLinkService()
+	bsrv := bserv.New(bs, offline.Exchange(bs))
+	ds := dag.NewDAGService(bsrv)


This was the way we originally did things back before 721df36. The reason I put it in was not for catching but for the filestore. With the current filestore it is not technically needed, but with a richer more functional filestore it might be again. So that the record is straight I will add a comment outside of the review a little later so its not so easily lost.

I'm really starting to think we should just be using a context hint. Anyways, all this will need to be reworked when GC changed to operate over a DAG instead of a blockservice.

kevina · 2018-01-31T05:29:48Z

This was merged before I had a chance to really review it, sorry I was not more on the ball here but I am not happy with the fix for #4009, I'll add my comment to #4009 where it will have a better chance of being seen.

Stebalien · 2018-01-31T06:30:26Z

That wasn't a fix, it was a workable hack to allow us to move forward and agree on a fix (that's why I called it out and asked if it was ok for now).

kevina · 2018-01-31T06:53:31Z

@Stebalien and sorry again I was not on the ball here. That change did not imminently register until you closed my p.r. It would of been very helpful if that change was in a separate p.r.

Stebalien added 2 commits January 24, 2018 15:58

gx: mass update

fe8846f

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

switch to fast base58 library

d82b527

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

ghost assigned Stebalien Jan 25, 2018

ghost added the status/in-progress In progress label Jan 25, 2018

dirkmc reviewed Jan 25, 2018

View reviewed changes

Stebalien force-pushed the gx/update-1m7019 branch from 8e934df to 26a8652 Compare January 25, 2018 21:25

fix tests that use invalid peer IDs

8899e98

Our code now better validates peer IDs. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the gx/update-1m7019 branch from 26a8652 to 3edd726 Compare January 25, 2018 22:34

dirkmc and others added 8 commits January 25, 2018 15:13

Update ipns validator

281d5ea

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

Remove unneccesary split in IpnsValidator

b90d7bd

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

go fmt

f6d507b

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

Fix ipns validator key parsing

fdb0046

License: MIT Signed-off-by: Dirk McCormick <dirkmdev@gmail.com>

gx update badger ds (fix internal deps issue)

49569a6

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

dagmodifier: remove useless offset update

730754d

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

merkledag: switch to new dag interface

d154b4a

Also: * Update the blockstore/blockservice methods to match. * Construct a new temporary offline dag instead of having a GetOfflineLinkService method. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

namesys: remove unecessary peerID cast

0d12a97

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the gx/update-1m7019 branch 2 times, most recently from 67fc3da to e638bc6 Compare January 25, 2018 23:18

make code-climate happier

699b2c4

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the gx/update-1m7019 branch from e638bc6 to 699b2c4 Compare January 25, 2018 23:43

cmds: can't call SetError after SetOutput

1159abd

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien commented Jan 26, 2018

View reviewed changes

Stebalien mentioned this pull request Jan 26, 2018

Update ipns validator #4597

Closed

Stebalien force-pushed the gx/update-1m7019 branch from d503d66 to 2c39138 Compare January 26, 2018 08:51

Stebalien requested review from vyzo and whyrusleeping January 26, 2018 10:26

Stebalien commented Jan 26, 2018

View reviewed changes

vyzo reviewed Jan 26, 2018

View reviewed changes

Stebalien removed the status/in-progress In progress label Jan 29, 2018

magik6k mentioned this pull request Jan 29, 2018

Building for iOS with gomobile fails with: undefined: syscall.O_DSYNC #4524

Closed

magik6k reviewed Jan 29, 2018

View reviewed changes

Stebalien added status/in-progress In progress and removed RFM labels Jan 29, 2018

Stebalien added 2 commits January 29, 2018 11:32

fix error in test case

f1aba97

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

rename import of go-ipld-format from node/format to ipld

f9d935b

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien mentioned this pull request Jan 29, 2018

panic: sync: inconsistent mutex state goroutine 215 (OSX) #4621

Closed

update go-ds-badger

87c6914

fixes #4524 License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

update comment to note that we're punting on this

2379aa0

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien added RFM and removed status/in-progress In progress labels Jan 29, 2018

Stebalien mentioned this pull request Jan 30, 2018

Getting failure: dial attempt failed: context deadline exceeded on macos libp2p/go-libp2p-circuit#27

Closed

magik6k approved these changes Jan 30, 2018

View reviewed changes

whyrusleeping approved these changes Jan 30, 2018

View reviewed changes

whyrusleeping merged commit b386088 into master Jan 30, 2018

whyrusleeping deleted the gx/update-1m7019 branch January 30, 2018 19:12

Stebalien mentioned this pull request Jan 30, 2018

Eliminate GetOfflineLinkService method from LinkService interface #4152

Closed

kevina reviewed Jan 30, 2018

View reviewed changes

kevina mentioned this pull request Jan 31, 2018

[RFC] Add an offline hint to contexts #4009

Closed

Uh oh!

Comments

Conversation

Stebalien commented Jan 25, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whyrusleeping commented Jan 27, 2018

Uh oh!

Stebalien commented Jan 29, 2018

Uh oh!

magik6k left a comment

Stebalien commented Jan 25, 2018 •

edited

Loading