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.

Sign up for GitHub

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

Jump to bottom

feat: use libp2p 0.28.x #217

Merged
merged 8 commits into from
Jun 5, 2020
Merged

feat: use libp2p 0.28.x #217

merged 8 commits into from
Jun 5, 2020

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Apr 21, 2020 •
edited
Loading

Once js-libp2p@0.28 ships, we will have to update its breaking changes in here and js-ipfs.

For what is relevant for this module, we are deprecating peer-info, changing how js-libp2p gathers its listening multiaddr, changing the ConnectionManager, PeerStore and DHT APIs.

Needs:

BREAKING CHANGE: bitswap start and stop return a promise

@vasco-santos vasco-santos force-pushed the chore/use-libp2p-0.28 branch 3 times, most recently from 17e3747 to f83007d Compare April 30, 2020 10:01
@vasco-santos vasco-santos mentioned this pull request May 4, 2020
Merged
3 tasks
@vasco-santos vasco-santos force-pushed the chore/use-libp2p-0.28 branch 5 times, most recently from ae16874 to 494d63e Compare May 25, 2020 11:29
@vasco-santos vasco-santos mentioned this pull request May 26, 2020
Closed
18 tasks
@vasco-santos vasco-santos force-pushed the chore/use-libp2p-0.28 branch 2 times, most recently from 28fe2fd to 4d05404 Compare May 26, 2020 17:47
@vasco-santos vasco-santos mentioned this pull request May 26, 2020
Merged
@vasco-santos vasco-santos force-pushed the chore/use-libp2p-0.28 branch 3 times, most recently from 6abb64d to 7e19612 Compare May 26, 2020 18:55
@vasco-santos
Copy link
Member Author

@dirkmc @achingbrain this is ready for review. Once this and ipfs/js-ipfs#3019 is good to go, we should release the final version of libp2p

@vasco-santos vasco-santos marked this pull request as ready for review May 28, 2020 12:05
achingbrain
achingbrain approved these changes Jun 2, 2020
View reviewed changes
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, just a few small nits & needs libp2p@0.28.x to be released before merging

package.json Outdated
@@ -52,8 +52,8 @@
"iso-random-stream": "^1.1.1",
"it-all": "^1.0.2",
"it-drain": "^1.0.1",
"libp2p": "^0.27.0",
"libp2p-kad-dht": "^0.18.3",
"libp2p": "^0.28.0-rc.0",
Copy link
Member

Choose a reason for hiding this comment

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

Needs to change to final, non-rc version before merging

package.json Outdated Show resolved Hide resolved
test/network/gen-bitswap-network.node.js Outdated Show resolved Hide resolved
dirkmc
dirkmc approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

Sorry Vasco I missed the notifications somehow.
LGTM 👍

@vasco-santos
Copy link
Member Author

FYI I addressed the review and we are now only pending on the final release of js-libp2p. We will do it, once the js-ipfs integration PR is reviewed, so that we are comfortable with the final release.

I will let you know once that happens

@vasco-santos
Copy link
Member Author

@dirkmc @achingbrain

I was experiencing intermittent issues using this in js-ipfs.

Let's consider the following scenario:

  • PeerB wants a block from the network
  • PeerA dials PeerB's multiaddr
    • a connection is established between them
    • PeerA knows one of the PeerB's listen multiaddrs, since it was used for dial
    • PeerB still does not know any PeerA's listen multiaddr, since it was dialled.
  • PeerB is notified by the connectionManager of the new connection by its event
    • PeerB tries to dial PeerA to get the intended block, but it does not have multiaddrs for it in the AddressBook
    • Dial fails and PeerB will not be able to ask PeerA for the block
  • (meanwhile) The identify protocol finishes and PeerB now knows PeerA listen multiaddrs

The issue here was that PeerB got notified of the connection between both peers and it should be notified only when that peer is dialable on bitswap protocol.

I needed to change that flow to use the libp2p/js-libp2p-interfaces//topology. This will trigger the onConnect when the identify protocol ran and the peer knowns that the other peer is running one of the multicodecs of bitswap. By this time, the peer already added the other peer multiaddrs to the AddressBook and this intermittent issue does not happen.

It is important pointing out that this is a breaking change in the bitswap API, since start and stop will now return Promise

achingbrain
achingbrain reviewed Jun 4, 2020
View reviewed changes
src/network.js Outdated
onDisconnect: this._onPeerDisconnect
}
})
this._registrarId = await this.libp2p.registrar.register(topology)
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be async? The Registrar.register method doesn't appear to be an async method: https://github.com/libp2p/js-libp2p/blob/master/src/registrar.js#L64-L78

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, yes my bad! 😓We had it async before, but not anymore. I will change this according

Copy link
Member Author

Choose a reason for hiding this comment

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

just amended the previous commit removing the async part, as well as the breaking change message. Thanks for noticing this

@dirkmc
Copy link
Contributor

dirkmc commented Jun 4, 2020

Thanks for the update vasco, I appreciate that you're doing such thorough testing that you found this 👍

jacobheun
jacobheun approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

1 missing todo, otherwise LGTM

test/network/network.node.js Outdated
@@ -134,7 +136,8 @@ describe('network', () => {

bitswapMockB._receiveError = (err) => deferred.reject(err)

const { stream } = await p2pA.dialProtocol(p2pB.peerInfo, '/ipfs/bitswap/' + version.num)
// TODO: set addr
Copy link
Contributor

Choose a reason for hiding this comment

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

@vasco-santos the address should be added. The previous test is causing this to pass but if that changes this will break

@vasco-santos
Copy link
Member Author

final comments were addressed in the last commit

@achingbrain achingbrain changed the title chore: use libp2p 0.28.x feat: use libp2p 0.28.x Jun 5, 2020
@achingbrain achingbrain merged commit c4ede4d into master Jun 5, 2020
@achingbrain achingbrain deleted the chore/use-libp2p-0.28 branch June 5, 2020 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achingbrain achingbrain achingbrain approved these changes

@jacobheun jacobheun jacobheun approved these changes

@dirkmc dirkmc dirkmc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vasco-santos @dirkmc @jacobheun @achingbrain