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

New libp2p versions #42

Merged
merged 24 commits into from
Aug 11, 2023
Merged

New libp2p versions #42

merged 24 commits into from
Aug 11, 2023

Conversation

iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented Jun 7, 2023

  • updated libp2p libraries to the following versions & golang v1.20.3 compiler accommodations:
github.com/libp2p/go-libp2p v0.28.1
github.com/libp2p/go-libp2p-kad-dht v0.23.0
github.com/libp2p/go-libp2p-kbucket v0.6.3
github.com/libp2p/go-libp2p-pubsub v0.9.3
github.com/multiformats/go-multiaddr v0.9.0

@sstanculeanu sstanculeanu self-requested a review June 8, 2023 07:01
assert.Equal(t, "/ip4/127.0.0.1/tcp/100", addresses[0])
assert.Nil(t, err)
})
t.Run("without port reuse, should work", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Run("without port reuse, should work", func(t *testing.T) {
t.Run("with port reuse, should work", func(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

require.Nil(t, err)
addressToConnect = getAddressMatching(seeder.Addresses(), "/quic-v1/", "")
err = webTransportPeer.ConnectToPeer(addressToConnect)
fmt.Println(webTransportPeer.ID().Pretty())
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed or debug purpose only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initial debug. Removed


mutMessages.Unlock()

defer func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove defer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, removed

miiu96
miiu96 previously approved these changes Jun 8, 2023
}
}

quickAddress := configs.QUICAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
quickAddress := configs.QUICAddress
quicAddress := configs.QUICAddress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

sstanculeanu
sstanculeanu previously approved these changes Jun 9, 2023
miiu96
miiu96 previously approved these changes Jun 9, 2023
@iulianpascalau iulianpascalau dismissed stale reviews from miiu96 and sstanculeanu via c8d7438 June 14, 2023 12:44

quicPeerArgs := createBaseArgs()
quicPeerArgs.P2pConfig.Node.Transports = config.TransportConfig{
QUICAddress: "/ip4/127.0.0.1/udp/%d/quic",
Copy link

Choose a reason for hiding this comment

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

👋 Hi, I discovered this PR from this tweet
It is awesome to see QUIC and WebTransport being added!

A drive-by suggestion I have is to use quic-v1 for QUIC multiaddresses. /quic/ is the draft-29 version of the address and go-libp2p is planning on removing support for it in the next release libp2p/go-libp2p#2369
So it would make sense for any new QUIC deployment to use quic-v1 right away

Also, you are already using quic-v1 for your WebTransport addresses; using it in both places would enable inferring WebTransport listen addrs from observation (via Identify) of quic-v1 addrs libp2p/go-libp2p#2251

Copy link
Contributor Author

@iulianpascalau iulianpascalau Jun 30, 2023

Choose a reason for hiding this comment

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

Thanks for the heads-up!
Will change the mutiaddress to use quic-v1 instead of plain quic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done & pushed

@iulianpascalau iulianpascalau marked this pull request as ready for review June 30, 2023 12:08
- removed unnecessary replace in go.mod
miiu96
miiu96 previously approved these changes Jun 30, 2023
@@ -102,6 +103,9 @@ func (poc *peersOnChannel) refreshPeersOnAllKnownTopics(ctx context.Context) {
case <-time.After(poc.refreshInterval):
}

//TODO remove this
Copy link
Contributor

Choose a reason for hiding this comment

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

is the TODO meant to be done in a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rechecking all PR

- removed TODOs
dragos-rebegea
dragos-rebegea previously approved these changes Jun 30, 2023
miiu96
miiu96 previously approved these changes Jul 4, 2023
@@ -11,10 +11,10 @@ jobs:
name: Unit tests without race detector
runs-on: ubuntu-latest
steps:
- name: Set up Go 1.17.6
- name: Set up Go 1.20.3
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe update to 1.20.5, latest stable.
Also all workflows should be updated.

@@ -37,7 +41,7 @@ import (

const (
// TestListenAddrWithIp4AndTcp defines the local host listening ip v.4 address and TCP used in testing
TestListenAddrWithIp4AndTcp = "/ip4/127.0.0.1/tcp/"
TestListenAddrWithIp4AndTcp = "/ip4/127.0.0.1/tcp/%d"
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be moved to a test file?

@iulianpascalau iulianpascalau dismissed stale reviews from miiu96 and dragos-rebegea via 38ba79c July 4, 2023 12:17
@iulianpascalau iulianpascalau marked this pull request as draft July 7, 2023 07:07
@iulianpascalau iulianpascalau marked this pull request as ready for review August 11, 2023 09:36
@iulianpascalau iulianpascalau merged commit 2dcca30 into master Aug 11, 2023
@iulianpascalau iulianpascalau deleted the try-new-libs branch August 11, 2023 09:36
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.

7 participants