Use __func__ for log messages #15981

Remove literal function names and print them using func. There are a few updated log messages in this commit that have been moved to a new function but still refer to their previous function, printing the function name with func will resolve this in future. Using func also removes logging code from grep results when searching on function name.

A previous PR to change a single mislabelled log entry was added here.

#15979

This PR includes the change from the previous one and adds every additional entry found by myself and practicalswift.

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15976 (refactor: move methods under CChainState (pt. 1) by jamesob)
• #15921 (Tidy up ValidationState interface by jnewbery)
• #15824 (docs: Improve netbase comments by dongcarl)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK for places where this doesn't change the log message.

There are a few updated log messages in this commit that have been moved to a new function but still refer to their previous function

There's an argument for not changing these, since any automated log parsing tools may be matching on the old log strings.

In some cases it's logged class::func, does it make sense to normalize - either always have the class or just the function?

I personally prefer class::func where applicable, I'll update this PR. Normalising all logging like this is a much bigger task and will conflict with a lot of other PRs, I'll stick to the ones in this PR for now.

Travis hit the time limit, seems less prone to this in the morning UK, I'll force push again tomorrow morning if it's not been manually run and succeeded by then.

jnewbery: that same argument pretty much argues against doing this, since all these messages will change in refactors. I'm not sure how we ended up with so many function names inside log entries, it isn't generally a good idea: we shouldn't have our errors change due to refactoring. Not just log parsing tools, but also users googling messages.

that same argument pretty much argues against doing this, since all these messages will change in refactors.

Right, so what do you suggest? Removing the function names from logs entirely?

Most of the logs are error conditions and there are two parts of the log message with different purpose:

• The function name/class name to tell the developer where it happened
• The verbose text to explain what happened

I think search engines today can deal with a single word that is off. And including more information for developers can't hurt, as long as it is correct. Not sure how many automated log parsers parse for the error strings including the location...

I think we should feel free to change logs, it isn't like an API, is it?

Changing of the function name will reliably break log parsing. Rather than including potentially changing function names the messages should be descriptive enough that there isn't any trouble finding them (considering how often functions move from file to file it isn't like a search is avoidable!). Then both needs are satisfied.

Macros like this are primarily useful for automatically generated error messages, e.g. asserts and the like.

Couldn't the logs use something like __FILE__ and __LINE__. In combination with the commit from the debug log, this should be sufficient to find the exact location without searching.

@gmaxwell For new code, with new messages, I'm very much in favor of using descriptive searchable and not-code-organization-dependent message texts. But what do you suggest we do with the code that is already there right now? Messages with function names in them have been in the codebase since before the project was on Github. We can replace them, and break the searchability property once, or we can hardcode them and remain searchable but confusing...

@sipa I could go either way-- but using func is not either of those ways. I would personally prefer that we replace them with strings that don't need to be changed unless the underlying behaviour changes. I would rather that they be potentially misleading then generated by func. No one working on the code itself is going to be confused too long by a log entry with an incorrect function name in it: the first thing you do in the code when dealing with an error is find the line that generated it, and if it's not there then its somewhere else.

As an aside, file line macros turn a much wider class of changes into binary changes, already when reviewing stuff for being cosmetic only I change macros, having them peppered all over the codebase would make that much harder. Also, it will further cause random failures in log file comparisons: joe user or operations guy doesn't realize that the number in the message is unstable and will differ from version to version.

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

I think this is way too controversial for a simple logging change, so I'm going to close this, sorry.

I'm not sure how we ended up with so many function names inside log entries, it isn't generally a good idea: we shouldn't have our errors change due to refactoring

I tend to agree with this. It'd make much more sense to prefix messages with a broad category (net, rpc, validation, ...) than the function name which leaks a lot of implementation details for no good reason. This could even be done automatically by the logging API.