refactor(p2p)!: Extract TCP transport by melekes · Pull Request #4148 · cometbft/cometbft

melekes · 2024-09-21T15:53:08Z

This PR refactors the peer-to-peer (p2p) module by extracting the TCP transport into its own component, laying groundwork for potential QUIC transport integration. It introduces a breaking change to the Go API, particularly in the handling of peer connections and transport interfaces.

Important: no logic has been changed; just moving stuff and updating interfaces.

Changes

Public API

Updated Transport interface

// Transport emits and connects to Peers. The implementation of Peer is left to
// the transport. Each transport is also responsible to filter establishing
// peers specific to its domain.
type Transport interface {
	// NetAddr returns the network address of the local node.
	NetAddr() na.Addr

	// Accept waits for and returns the next connection to the local node.
	Accept() (net.Conn, *na.Addr, error)

	// Dial dials the given address and returns a connection.
	Dial(addr na.Addr) (net.Conn, error)

	// Cleanup any resources associated with the given connection.
	//
	// Must be run when the peer is dropped for any reason.
	Cleanup(conn net.Conn) error
}

New Go packages (extracted from `p2p`)

netaddress - network address
nodeinfo - node's info
nodekey - ID of the node
transport/tcp - TCP transport

Internalized Go packages

fuzz - fuzzing connection

#go-api-breaking

cason · 2024-09-26T15:20:55Z

A general fast comment here: if we don't rename GetChannels() reactor's method and the type that stores it, we have much less changes and maybe is not (that) API breaking?

github-actions · 2024-10-13T00:18:48Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

faddat · 2024-10-15T09:25:22Z

thanks!

Closes #4301

remove FuzzConn, FuzzConnFromConfig, FuzzConnAfter

melekes · 2024-10-17T13:16:54Z

A general fast comment here: if we don't rename GetChannels() reactor's method and the type that stores it, we have much less changes and maybe is not (that) API breaking?

It will be API-breaking anyway because I had to extract many packages. But if you think the renaming is unjustified, I can revert it to the "channels" concept.

andynog

Overall looks good to me. Made some comments and suggestions. Also ran a few tests. Good job. 👍

internal/blocksync/reactor.go

andynog · 2024-10-23T20:32:54Z

internal/consensus/reactor.go


-// GetChannels implements Reactor.
-func (*Reactor) GetChannels() []*p2p.ChannelDescriptor {
+// StreamDescriptors implements Reactor.


I was just trying to understand the semantics for the naming difference, why the ChannelDescriptor changed to StreamDescriptor. Looking at some related code, eg. in node/node.go CustomReactors seems there's a mixing of these semantics like

for _, chDesc := range reactor.StreamDescriptors() { if !ni.HasChannel(chDesc.StreamID()) { ni.Channels = append(ni.Channels, chDesc.StreamID()) } }

Just want to understand the difference between channels and streams

So, "channel" is what we were using before, but I don't know where this naming concept is coming from. Channels are not mentioned anywhere in TCP or multiplexed TCP.

Where's in QUIC, there's a direct concept of data streams. Hence, I've decided to change the name.

p2p/peer.go

andynog · 2024-10-23T21:29:30Z

p2p/peer.go


-	return cmtconn.NewMConnectionWithConfig(
+	// filter out non-tcpconn.ChannelDescriptor streams
+	chDescs := make([]*tcpconn.ChannelDescriptor, 0, len(streams))


should we pre-allocate capacity here? I'm wondering if most of the streams will be non-tcpconn, then maybe we don't pre-allocate. The GC will eventually kick-in but just a suggestion if it makes sense.

This part will be refactored out later :)

andynog · 2024-10-23T21:36:59Z

p2p/peer.go

 	)
 }
+
+func wrapPeer(c net.Conn, ni ni.NodeInfo, cfg peerConfig, socketAddr *na.NetAddress, mConfig tcpconn.MConnConfig) Peer {


Could you please add some context here ? e.g. comments why wrapPeer? I tried to check with the logic in the switch acceptRoutine and it seems this method changes the logic a little bit, I couldn't find logic related to persistence check in the previous acceptRoutine, having additional comments may provide more context for that. Thanks.

spec/p2p/reactor-api/reactor.qnt

spec/p2p/reactor-api/reactor.md

andynog · 2024-10-23T22:00:54Z

p2p/transport/tcp/tcp.go

+	if err != nil {
+		return nil, err
+	}
+


just noticed that in the previous file, there was a cfg parameter and the outbound was set. I'm assuming this is not needed or was removed on purpose but just wanted to highlight here

cfg.outbound = true

tac0turtle · 2024-10-26T17:21:41Z

nice job!!

melekes self-assigned this Sep 21, 2024

melekes force-pushed the 2128-quic-go branch from 5b3000f to a919d07 Compare September 21, 2024 15:54

melekes added p2p breaking A breaking change labels Sep 22, 2024

github-actions bot added stale For use by stalebot and removed stale For use by stalebot labels Oct 13, 2024

melekes added 2 commits October 17, 2024 16:57

refactor(p2p)!: extract TCP transport

a1b248f

Closes #4301

feat(p2p)!: internalize fuzz (FuzzedConnection)

af5f040

remove FuzzConn, FuzzConnFromConfig, FuzzConnAfter

melekes force-pushed the 2128-quic-go branch from 02df03b to af5f040 Compare October 17, 2024 13:08

melekes added 2 commits October 17, 2024 22:39

fix TestConnectionSpeedForPeerReceivedFromSeed

d7080b9

cleanup conn

244ea02

melekes marked this pull request as ready for review October 17, 2024 18:41

melekes requested review from a team as code owners October 17, 2024 18:41

melekes requested a review from a team October 17, 2024 18:41

melekes mentioned this pull request Oct 17, 2024

p2p: QUIC transport [tracking issue] #4302

Open

14 tasks

melekes added 2 commits October 17, 2024 22:48

add missing outbound values

cc15df6

add changelog

cd2eb2d

melekes added the hygiene Any work relating to code legibility/hygiene to make it easier to read label Oct 18, 2024

andynog added 2 commits October 21, 2024 11:21

Merge branch 'main' into 2128-quic-go

c05d2cd

Merge branch 'main' into 2128-quic-go

9c83cd1

andynog approved these changes Oct 23, 2024

View reviewed changes

rename chDescs to streamDescs

d8e23c1

melekes added this pull request to the merge queue Oct 24, 2024

Merged via the queue into main with commit eef3464 Oct 24, 2024

melekes deleted the 2128-quic-go branch October 24, 2024 05:34

cason mentioned this pull request Nov 26, 2024

p2p: restore the type p2p.ID #4549

Closed

Conversation

melekes commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Public API

New Go packages (extracted from p2p)

Internalized Go packages

Uh oh!

cason commented Sep 26, 2024

Uh oh!

github-actions bot commented Oct 13, 2024

Uh oh!

faddat commented Oct 15, 2024

Uh oh!

melekes commented Oct 17, 2024

Uh oh!

andynog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andynog Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

melekes Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andynog Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

melekes Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

andynog Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andynog Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

tac0turtle commented Oct 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

melekes commented Sep 21, 2024 •

edited

Loading

New Go packages (extracted from `p2p`)