Rhm... i think this PR does not change anything.

Your PR would allow the program flow to enter the else if (strCommand == "getaddr") even if if (!pfrom->fInbound),... but it would return true anyways on line 4743(https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4743). Or do i miss something?

Yes, it will exit before displaying the "unknown command" message now.
On Jun 26, 2015 11:13 PM, "Jonas Schnelli" [email protected] wrote:

Rhm... i think this PR does not change anything.

Your PR would allow the program flow to enter the else if (strCommand ==
"getaddr") even if if (!pfrom->fInbound),... but it would return true
anyways on line 4743(
https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4743). Or do
i miss something?

—
Reply to this email directly or view it on GitHub
#6344 (comment).

Ah. Right. Wasn't aware of the else on L4736.

utACK.

If we are changing this code, please also log this exceptional event.

concept ACK

Meh.

a) the message will only appear when debug=net is set
b) at least with the old code it shows that something weird is happening, which is the point of verbose net logging (even though the message could be better) - after this patch it's ignored without any logging

Don't care.

I don't think this is enough of an improvement to merge, so going to close this.

@laanwj I'd consider this a bugfix rather than an improvement, since "getaddr" clearly is NOT an unknown command.

@rebroad The previous comments say that you are removing a line (interesting to at least some users) from the debug.log. Why not keep logging this with a proper message (as @jgarzik suggests)?