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

Downgrade duplicate nonce logs to debug #4933

Merged
merged 5 commits into from
Nov 23, 2020

Conversation

zgfzgf
Copy link
Contributor

@zgfzgf zgfzgf commented Nov 20, 2020

fix #4932

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Please don't change the grouping of imports.

Also, what exactly does this fix?

@zgfzgf
Copy link
Contributor Author

zgfzgf commented Nov 20, 2020

Should have been printed, not printed
and Printing too much useless information

@zgfzgf zgfzgf requested a review from vyzo November 20, 2020 07:56
chain/messagepool/messagepool.go Show resolved Hide resolved
Comment on lines 246 to 249
log.Infof("add with duplicate nonce. message from %s with nonce %d already in mpool,"+
" increase GasPremium to %s from %s to trigger replace by fee: %w",
m.Message.From, m.Message.Nonce, minPrice, m.Message.GasPremium,
ErrRBFTooLowPremium)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no reason to construct a wrapped error for this log, just log what you need; Also note you are repeating the information from the error, which will get logged together with the previous log message.

So I don't see this as adding anything.

@vyzo
Copy link
Contributor

vyzo commented Nov 20, 2020

I don't think this pr adds anything!

"newpremium", m.Message.GasPremium, "addr", m.Message.From, "nonce", m.Message.Nonce)
} else {
log.Info("add with duplicate nonce")
log.Debugf("add with duplicate nonce. message from %s with nonce %d already in mpool,"+
" increase GasPremium to %s from %s to trigger replace by fee: %w",
Copy link
Contributor

Choose a reason for hiding this comment

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

this %w formatting directive is invalid in the log, it's only valid in the error wrapping function; it should be %s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @vyzo thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI to all, the best format to use for errors is %v, or if you want detailed info %+v.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not necessary here - it's not a structured error.

@vyzo vyzo changed the title Fix messagepool print Downgrade duplicate nonce logs to debug Nov 21, 2020
@vyzo
Copy link
Contributor

vyzo commented Nov 21, 2020

Can we squash this down to one commit? It's really messy for something that changes two incosequential lines of code.

@magik6k magik6k merged commit c13ade3 into filecoin-project:master Nov 23, 2020
@zgfzgf zgfzgf deleted the fix-messagepool-print branch December 4, 2020 03:27
bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Jan 8, 2021
* add messagepool duplicate nonce print info

* update log.Info to log.debug and go fmt

* recover the grouping of imports

* update print from info to debug

* print format %w to %s
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.

messagepool print bug
4 participants