Add some additional logging to give extra network insight. #3296

pull mikehearn wants to merge 2 commits into bitcoin:master from mikehearn:more_logging changing 4 files +26 −11

mikehearn commented at 1:40 PM on November 21, 2013: contributor

This shows subvers and IPs in the log lines that track mempool / connect activity. It makes it easier to see strange nodes.

in src/main.cpp:None in 006b893cf3 outdated

3424 | @@ -3428,6 +3425,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
3425 |              vWorkQueue.push_back(inv.hash);
3426 |              vEraseQueue.push_back(inv.hash);
3427 |  
3428 | +
3429 | +            LogPrint("mempool", "AcceptToMemoryPool: %s %s : accepted %s (poolsz %"PRIszu")\n",
3430 | +                pfrom->addr.ToString().c_str(), pfrom->strSubVer.c_str(),

Diapolo commented at 4:29 PM on November 21, 2013:

I love your indentation conformance ^^.

gavinandresen commented at 11:31 PM on November 21, 2013: contributor

ACK
sipa commented at 12:23 PM on November 22, 2013: member

ACK.

I think it may make sense to generalize the logging code to add peer information when available in a consistent way, but let's do that another time.
gmaxwell commented at 9:28 AM on November 26, 2013: contributor

pfrom->strSubVer is read in from the network without any sanitation, this change appears that it would allow log entry emulation. It should be changed to prevent that (I think I'd suggested urlcoding the strings elsewhere where we were going to log untrusted strings from network).

Ack the mempool peer logging, since its off by default in any case.
Add some additional logging to give extra network insight. ba6a4ea344
Store and use a sanitized subVer a946aa8d3e
mikehearn commented at 12:38 PM on November 26, 2013: contributor

@gmaxwell Good catch. It was being put into RPC responses too, though I guess JSON-SPIRIT should escape things appropriately. I introduced a cleanSubVer field that's been run through SanitizeString and changed logging and RPC responses to use that one.
BitcoinPullTester commented at 12:57 PM on November 26, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a946aa8d3ec7009ac670eeb65a525efe5eeb6e84 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.
wtogami commented at 9:17 AM on November 27, 2013: contributor

ACK
laanwj referenced this in commit e1169d7eea on Nov 27, 2013
laanwj merged this on Nov 27, 2013
laanwj closed this on Nov 27, 2013
laanwj commented at 9:54 AM on November 27, 2013: member

Also merged into 0.8.6 branch
rebroad commented at 6:24 AM on February 25, 2014: contributor

Huh? I thought my pull request #2126 was denied because I logged IP addresses by default, so how come this pull got accepted?!
DrahtBot locked this on Sep 8, 2021

Contributors