more information for invalid txs (but not peer) #1338

pull rebroad wants to merge 1 commits into bitcoin:master from rebroad:ShowMoreTxInfoWhenErrors changing 1 files +97 −49

rebroad commented at 8:31 PM on May 17, 2012: contributor

This is a subset of pull #1311, but just the part that adds more info on txs which have issues with them.

For really naughty txs, that DoS(100) it also reveals the peer that sent it, otherwise peer information is not revealed. (although there will be a pull request coming shortly providing a command line flag should peer information for all txs be desired).

Also, free transactions are identifies in the output. Not particularly useful, but these may become a rare thing in the not too distant future.. maybe, maybe not.

Apologies for the txnode variable. The only other way was to change all the functions to allow it to be passed. I think this is the neatest and least intrusive way, and can't see any harm in it, but please correct me if I'm wrong here.
jgarzik commented at 10:59 PM on May 17, 2012: contributor
1. the txnode bit is NAK'd. not thread safe or really 100% accurate, and the value of the information is too low compared to adding that variable
2. the top, oh, 75% of this patch seems ACK-worthy, though it is changing High Value Code for a bunch of debug output, so the justification is not very large.
rebroad commented at 8:30 AM on May 18, 2012: contributor

@gmaxwell how would you code it so that it reports the peer in question for a DoS(100) event? I've been testing this for over a month and not noticed any problems. What sort of symptoms from being thread-unsafe would you expect to see?

Regarding changing of HVC for additional information, does this mean that we're stuck with not being able to log this information as we don't want to touch HVC? It's only the printf() and errors() affected, none of the actual logic.
in src/main.cpp:None in 967ee985df outdated
```
 416 | -    if (vin.empty())
 417 | -        return DoS(10, error("CTransaction::CheckTransaction() : vin empty"));
 418 | -    if (vout.empty())
 419 | -        return DoS(10, error("CTransaction::CheckTransaction() : vout empty"));
 420 | +    if (vin.empty()) {
 421 | +        return DoS(10, error("checktx: %s vin empty", hash.ToString().substr(0,10).c_str()));
```
luke-jr commented at 7:42 PM on May 18, 2012:

Do we really want to rename functions in log messages alone?

gavinandresen commented at 1:21 PM on May 19, 2012:

NACK: don't abbrv th mthd nms

rebroad commented at 10:26 AM on May 21, 2012:

I would argue that given it's a log message, as long as the message can be correlated to the code, then that's all that matters. I don't see a strong argument for filling debug.log with more bytes than needed to communicate what is happening. In fact, there's an argument that it doesn't need to be human readable at all, but then that would require additional tools to translate it into something human readable. This patch was an attempt at a convenient in-between between excess data, but keeping it human readable/understandable, and including information that's useful (e.g. the hash, rather than superfluous static bytes).
show more tx info, and which node from for Dos(100) events. f05bacdf0f
jgarzik commented at 3:55 PM on August 1, 2012: contributor

This does seem interesting, but I think we will have the TX hash logged elsewhere, if an error occurs.

Just not a big driving need for this change, and it's not collecting ACKs, so closing.

PS. You added a ton of redundant braces { }

This is too big. If you can (a) remove the redundant braces you added all over the place and (b) submitted this in smaller pieces
jgarzik closed this on Aug 1, 2012
lateminer referenced this in commit ce1d6178df on May 6, 2020
lateminer referenced this in commit d00ac8c3d2 on May 6, 2020
lateminer referenced this in commit 0c8950a8ec on May 6, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1311 Improve debug.log output.#1375 Show peer that sent transaction when it causes a DoS(100).