Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Add canonical log for misbehaving peers #258

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

MarcoPolo
Copy link
Contributor

Add canonical logging for misbehaving peers. See libp2p/go-libp2p#1598 for more details.

Why go-libp2p-core? I wish I could put this in the go-libp2p mono repo, but I don't want to require go-libp2p just so that the resource manager can use this. But I don't have strong opinions on where this lives. It is unfortunate that this causes a lot of extra stuff to be added to go.mod/sum

I do have strong opinions on consolidating this repo into go-libp2p, we should. Managing multiple changes across go-libp2p-core, go-libp2p, and resource manager has been like walking through a tar pit. More discussion here libp2p/go-libp2p#1556.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Should we add a “component” string, so that we know where the error occurred? For example, this would be the application protocol, or TLS or Noise.

// Protocols should use this to identify a misbehaving peer to allow the end
// user to easily identify these nodes across protocols and libp2p.
func LogMisbehavingPeerNetAddr(p peer.ID, peerAddr net.Addr, originalErr error, msg string) {
ipStrandPort := strings.Split(peerAddr.String(), ":")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use manet.FromNetAddr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice! Thanks

@MarcoPolo
Copy link
Contributor Author

Should we add a “component” string, so that we know where the error occurred? For example, this would be the application protocol, or TLS or Noise.

Yeah I think this makes sense, but I want to highlight that this doesn't need to be super specific unless we notice that it's useful to differentiate if the TLS handshake or Noise handshake failed. Otherwise we can start at component=handshake.

// Protocols should use this to identify a misbehaving peer to allow the end
// user to easily identify these nodes across protocols and libp2p.
func LogMisbehavingPeer(p peer.ID, peerAddr multiaddr.Multiaddr, component string, err error, msg string) {
log.Errorf("CANONICAL_MISBEHAVING_PEER: peer=%s addr=%s component=%s err=%v msg=%s", p, peerAddr.String(), component, err, msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion, but should we use Errorfw instead? Might make it easier for tools to parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean Errorw? In my experience parsing key=value pairs are much easier than parsing json. But not a strong opinion here.

@MarcoPolo MarcoPolo merged commit 31262b0 into master Jun 17, 2022
@MarcoPolo MarcoPolo mentioned this pull request Jul 7, 2022
41 tasks
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants