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

p2p/discover: add more packet information in logs #26307

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Dec 5, 2022

This adds more fields to discv5 packet logs. These can be useful when
debugging multi-packet interactions.

The FINDNODE message also an additional field, OpID, also for
debugging purposes. This field is not encoded onto the wire. The
OpID can be used to attribute requests to a specific lookup process.

I'm also removing topic system related message types in this change.
These will come back in the future, where support for them will be
guarded by a config flag.

This adds more fields to discv5 packet logs. These can be useful when
debugging multi-packet interactions.

The FINDNODE message also gets an additional field, OpID for debugging
purposes. This field is not encoded onto the wire.

I'm also removing topic system related message types in this change.
These will come back in the future, where support for them will be
guarded by a config flag.
The new name captures the meaning of this field better.
@fjl fjl requested a review from zsfelfoldi as a code owner December 5, 2022 13:44
@fjl fjl changed the title P2p discover logcontext p2p/discover: add more packet information in logs Dec 5, 2022
Copy link

@mohammadfarari1360 mohammadfarari1360 left a comment

Choose a reason for hiding this comment

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

Good

@fjl
Copy link
Contributor Author

fjl commented Jan 3, 2023

Here's what this looks like in logs. The new thing are the fields req (request ID), enrseq, tot, n

TRACE[01-03|12:06:32.580] >> FINDNODE/v5      id=9bc9c52326031ed6 addr=174.56.32.79:12000    req=0xb6d091792c318516
TRACE[01-03|12:06:32.592] >> PING/v5          id=9bd3e274cdb86527 addr=79.143.177.97:30303   req=0xa1d528e27933ae48 enrseq=1,672,743,991,625
TRACE[01-03|12:06:32.610] << NODES/v5         id=9bcfeb4d9e6e700b addr=150.230.200.126:58646 req=0x5384de1e602da905 tot=1 n=0
TRACE[01-03|12:06:32.618] << PONG/v5          id=9bd3e274cdb86527 addr=79.143.177.97:30303   req=0xa1d528e27933ae48 enrseq=1,639,405,062,025

@@ -59,7 +61,7 @@ type (
Nonce Nonce
}

// Whoareyou contains the handshake challenge.
// WHOAREYOU contains the handshake challenge.
Copy link
Contributor

@holiman holiman Jan 3, 2023

Choose a reason for hiding this comment

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

Why change these? They are "correct" as they were. You could change to

// Whoareyou contains the handshake challenge, representing the WHOAREYOU message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made these upper case because packet names are always uppercase (in the spec, and also in logs).

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, aside from what I think is a "wrong" way do write docstrings, but unless the linter complains, who am I to do so...

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

@fjl fjl merged commit a251bca into ethereum:master Jan 3, 2023
@fjl fjl added this to the 1.11.0 milestone Jan 3, 2023
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
* p2p/discover: add more packet information in logs

This adds more fields to discv5 packet logs. These can be useful when
debugging multi-packet interactions.

The FINDNODE message also gets an additional field, OpID for debugging
purposes. This field is not encoded onto the wire.

I'm also removing topic system related message types in this change.
These will come back in the future, where support for them will be
guarded by a config flag.

* p2p/discover/v5wire: rename 'Total' to 'RespCount'

The new name captures the meaning of this field better.
This pull request was closed.
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.

4 participants