Less messy debug #4839

pull rebroad wants to merge 1 commits into bitcoin:master from rebroad:LessMessyDebug changing 1 files +2 −2

rebroad commented at 3:03 AM on September 4, 2014: contributor

Show peer causing the bad messages.
rebroad force-pushed on Sep 4, 2014
rebroad force-pushed on Sep 4, 2014
sipa commented at 9:11 AM on September 4, 2014: member

Untested ACK. I always found those newlines annoying in the debug output...
jgarzik commented at 12:24 PM on September 4, 2014: contributor

ut ACK
in src/main.cpp:None in 2488148aab outdated
```
4120 | @@ -4121,10 +4121,13 @@ bool ProcessMessages(CNode* pfrom)
4121 |  
4122 |          // get next message
4123 |          CNetMessage& msg = *it;
4124 | +        CMessageHeader& hdr = msg.hdr;
4125 | +        unsigned int nMessageSize = hdr.nMessageSize;
4126 | +        string strCommand = hdr.GetCommand();
```
jgarzik commented at 12:32 PM on September 4, 2014:

NAK. You cannot put this code here. It must come after the msg.complete() check.

laanwj commented at 12:34 PM on September 4, 2014:

Good catch.

rebroad commented at 3:24 PM on September 4, 2014:

It's never not worked. The msg doesn't need to be complete for GetCommand() to work, but it's possible it needs to have received >n bytes (any idea what n should be?).

sipa commented at 1:11 AM on September 5, 2014:

The header is only deserialized into msg.hdr once it's been received entirely, so before that point, msg.hdr will always be empty.

rebroad commented at 2:10 AM on September 5, 2014:

@sipa, All I know is that it works, and has been working since at least 4th July (probably since March if I recall correctly). If what you're saying is true, wouldn't that suggest the code as it is wouldn't work?

sipa commented at 9:00 PM on September 5, 2014:

It will return an empty string for command in case an incomplete message is in the buffer. That probably happens rarely.

rebroad commented at 1:58 AM on September 6, 2014:

@sipa, ah, cool. Yes, it's never happened so far in the time I've been testing this. So it sounds like this is safe to pull then, as the strCommand variable is only used for debug logging, so it's displayed if it has it, and most future code won't be interested in using it until the complete message has arrived anyway.

laanwj commented at 3:18 AM on September 6, 2014:

@rebroad I've said this before: don't make logging-related changes change around other things. If there is any risk that this will break something, I'd rather not have it at all than have prettier logging.

This shouldn't be controversial in any way. Let's keep just the changes to the two LogPrintf lines below and I'll merge this.
rebroad force-pushed on Sep 6, 2014
rebroad commented at 7:15 AM on September 6, 2014: contributor

@laanwj first commit removed.
Cleanup messy error messages 346193bd93
rebroad force-pushed on Sep 6, 2014
rebroad commented at 7:22 AM on September 6, 2014: contributor

(in case you'd not noticed, the msg.hdr variable I've added is safe, since msg.hdr must already exist, since it forms part of the if statement preceeding the line I changed), plus @sipa has also checked that the GetCommand() command works and returns a string whatever amount of msg has been received.
BitcoinPullTester commented at 7:53 AM on September 6, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4839_346193bd934bf198dbfd606d8c52c60bc7967858/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
laanwj commented at 8:25 AM on September 6, 2014: member

ACK
laanwj merged this on Sep 6, 2014
laanwj closed this on Sep 6, 2014
laanwj referenced this in commit 6eb427ed6e on Sep 6, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4841 PROCESSMESSAGE: INVALID MESSAGESTART