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

Add trace support for IDONTWANT control messages #37

Merged
merged 9 commits into from
Sep 25, 2024

Conversation

cortze
Copy link
Contributor

@cortze cortze commented Sep 24, 2024

Decription

After the recent addition of IDONTWANT messages to the go-libp2p-pubsub repo (GossipSub v1.2), this PR aims to extend the libp2p/gossipsub tracer to track these same messages.

Some extra features:

  • Update the go-libp2p and the go-libp2p-pubsub dependencies.
  • Update the interface to define the data-streamer with a new flag a the app-root level.
  • Extension of the Makefile to build, install, and format the code

@cortze
Copy link
Contributor Author

cortze commented Sep 24, 2024

I've been testing locally the changes (using the logger data-stream), and looks like it's working:

{"Type":"RECV_RPC","PeerID":"16Uiu2HAmGXD7jgbr6JRaAEP3sueEtgnmKt5Kz9CBsDwoaxh9M1iP","Timestamp":"2024-09-24T17:08:14.647175206+02:00","Data":{"PeerID":"16Uiu2HAmNyBDzhiZKGMRDLRbsbxwAyr6xDcBSPJwt4qazXrRYGss","Control":{"Idontwant":[{"MsgIDs":["c94a99f24e6ae9d3fc287592ebc7288ac77fec8e"]}]}}}
{"Type":"RECV_RPC","PeerID":"16Uiu2HAmGXD7jgbr6JRaAEP3sueEtgnmKt5Kz9CBsDwoaxh9M1iP","Timestamp":"2024-09-24T17:08:14.684288724+02:00","Data":{"PeerID":"16Uiu2HAmNyBDzhiZKGMRDLRbsbxwAyr6xDcBSPJwt4qazXrRYGss","Control":{"Idontwant":[{"MsgIDs":["b6a2851eac225ada7c7cc935ef480c57456c017e"]}]}}}
{"Type":"RECV_RPC","PeerID":"16Uiu2HAmGXD7jgbr6JRaAEP3sueEtgnmKt5Kz9CBsDwoaxh9M1iP","Timestamp":"2024-09-24T17:08:14.686044398+02:00","Data":{"PeerID":"16Uiu2HAmNyBDzhiZKGMRDLRbsbxwAyr6xDcBSPJwt4qazXrRYGss","Control":{"Idontwant":[{"MsgIDs":["b6a2851eac225ada7c7cc935ef480c57456c017e"]}]}}}
{"Type":"RECV_RPC","PeerID":"16Uiu2HAmGXD7jgbr6JRaAEP3sueEtgnmKt5Kz9CBsDwoaxh9M1iP","Timestamp":"2024-09-24T17:08:14.693322474+02:00","Data":{"PeerID":"16Uiu2HAmNyBDzhiZKGMRDLRbsbxwAyr6xDcBSPJwt4qazXrRYGss","Control":{"Idontwant":[{"MsgIDs":["e1e130e9d2487bb2d9f97f3497c63f6bd7bb68de"]}]}}}
{"Type":"RECV_RPC","PeerID":"16Uiu2HAmGXD7jgbr6JRaAEP3sueEtgnmKt5Kz9CBsDwoaxh9M1iP","Timestamp":"2024-09-24T17:08:14.696461042+02:00","Data":{"PeerID":"16Uiu2HAmNyBDzhiZKGMRDLRbsbxwAyr6xDcBSPJwt4qazXrRYGss","Control":{"Idontwant":[{"MsgIDs":["3e5f313168ed113b863a9a27b8827c730e4db06b"]}]}}}
{"Type":"RECV_RPC","PeerID":"16Uiu2HAmGXD7jgbr6JRaAEP3sueEtgnmKt5Kz9CBsDwoaxh9M1iP","Timestamp":"2024-09-24T17:08:14.700000251+02:00","Data":{"PeerID":"16Uiu2HAmNyBDzhiZKGMRDLRbsbxwAyr6xDcBSPJwt4qazXrRYGss","Control":{"Idontwant":[{"MsgIDs":["e1e130e9d2487bb2d9f97f3497c63f6bd7bb68de"]}]}}}

The only thing that worries me is that there seems to be a specific Control RPC for each of the IDONTWANT messages (I need to check if this is an intended behaviour)

eth/node_config.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
host/host.go Outdated Show resolved Hide resolved
host/trace_logger.go Show resolved Hide resolved
var _ DataStream = (*TraceLogger)(nil)

func (t *TraceLogger) Start(ctx context.Context) error {
<-ctx.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

blocking on the context isn't necessary IIUC

Suggested change
<-ctx.Done()

Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

LGTM

@cortze cortze merged commit f3c2886 into main Sep 25, 2024
4 checks passed
@cortze cortze deleted the feat/support-idontwants branch September 25, 2024 14:15
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.

2 participants