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 flag to enable logging on rejected gossip message #11524

Merged
merged 10 commits into from
Oct 5, 2022

Conversation

saolyn
Copy link
Contributor

@saolyn saolyn commented Sep 30, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR adds a new feature flag to enable logging on rejected gossip messages.

Which issues(s) does this PR fix?

Fixes #11431

Other notes for review

If we add this flag to the docs we need to make sure we add a note about the potential risks of running a node with this flag enabled as pointed out on the issue.
Essentially that this can easily become a potential DoS vector.

@saolyn saolyn requested a review from a team as a code owner September 30, 2022 21:08
Comment on lines 770 to 775
func rejectGossipMessage(msg *pubsub.Message) pubsub.ValidationResult {
if features.Get().EnableFullSSZDataLogging {
log.WithField("rawMessage", msg).Debug("Rejected gossip message with data")
}
return pubsub.ValidationReject
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a function level comment?

What is the intended behavior of this function?

@prestonvanloon
Copy link
Member

prestonvanloon commented Oct 1, 2022

If you are trying to log the message as a ssz hex string, I don't think printing the *pubsub.Message would work. Could you add a test to ensure the behavior is as expected?

Edit: Here's an example of something testing a log message:

require.LogsContain(t, hook, "Exiting mode to save hot states in DB")

Also see how require.LogsContain works. It should be able to read content from fields like when you do log.WithField(...), it should be able to assert that content is present.


func rejectGossipMessage(msg *pubsub.Message) pubsub.ValidationResult {
if features.Get().EnableFullSSZDataLogging {
log.WithField("rawMessage", msg).Debug("Rejected gossip message with data")
Copy link
Member

@prestonvanloon prestonvanloon Oct 1, 2022

Choose a reason for hiding this comment

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

If you want to match Teku's log, they also log the reason or error and topic.

https://github.com/ConsenSys/teku/blob/cdf161742a7abb091ab04fb6c23fee622b9d1df7/infrastructure/logging/src/main/java/tech/pegasys/teku/infrastructure/logging/P2PLogger.java#L50

Edit: Added "and topic". Teku logs the message, topic, and rejection reason / error

@potuz
Copy link
Contributor

potuz commented Oct 1, 2022

If you are trying to log the message as a ssz hex string, I don't think printing the *pubsub.Message would work. Could you add a test to ensure the behavior is as expected?

Yeah I think you want to convert this to a hex string that can be directly fed to a ssz marshaller. Also notice that it's mandatory to have the expected object at least, otherwise we won't know what schema to use to unmarshal

@prestonvanloon
Copy link
Member

prestonvanloon commented Oct 1, 2022

If you are trying to log the message as a ssz hex string, I don't think printing the *pubsub.Message would work. Could you add a test to ensure the behavior is as expected?

Yeah I think you want to convert this to a hex string that can be directly fed to a ssz marshaller. Also notice that it's mandatory to have the expected object at least, otherwise we won't know what schema to use to unmarshal

I think msg.Data is the ssz payload? It might be snappy compressed, i don't remember, but it's the raw message payload

https://pkg.go.dev/github.com/libp2p/go-libp2p-pubsub@v0.8.1/pb#Message

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

This isnt the correct place to check for it, it needs to be in our gossip handler wrapper.

@potuz
Copy link
Contributor

potuz commented Oct 1, 2022

I think msg.Data is the ssz payload? It might be snappy compressed, i don't remember, but it's the raw message payload

https://pkg.go.dev/github.com/libp2p/go-libp2p-pubsub@v0.8.1/pb#Message

Yes but if you print a byte slice with the logger it won't print it as a hex string, and also to decode the ssz you need to know what object the message was.

@saolyn saolyn requested a review from nisdas October 4, 2022 16:18
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

LGTM

@rauljordan rauljordan added Ready For Review A pull request ready for code review OK to merge labels Oct 5, 2022
@prylabs-bulldozer prylabs-bulldozer bot merged commit 8049060 into develop Oct 5, 2022
@delete-merged-branch delete-merged-branch bot deleted the log-rejected-gossip-messages branch October 5, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to log full ssz data on rejected gossip messages
6 participants