Make logging for validation optional #6519

pull laanwj wants to merge 6 commits into bitcoin:master from laanwj:2015_08_validation_error_spam changing 4 files +80 −77
  1. laanwj commented at 1:36 pm on August 5, 2015: member

    Avoid ERROR messages like ERROR: AcceptToMemoryPool : non-final. Logging of incoming transaction validation failures can be enabled optionally with-debug=mempoolrej.

    Also introduces a consistent format for transaction validation failures that always includes the txid.

    02015-08-05 13:25:26 txfail 4e5014c4084aab04aac280e4dc6e26ffbc76ea0feb817b98a1a67d8b18d4e80b: AcceptToMemoryPool: inputs already spent
    

    I have opted for this approach instead of the one outlined in #5794 to remove all messages, because they provide extra troubleshooting information.

    Fixes #5794.

  2. laanwj added the label Validation on Aug 5, 2015
  3. sipa commented at 1:52 pm on August 5, 2015: member

    Hmm, have you considered to not print the error immediately, but only report through ValidationState, and then have ProcessMessage choose (optionally) to report the error string it, if any?

    That’s also more compatible with future modularization of the consensus code, where you want to do the actual logging externally.

  4. laanwj commented at 2:59 pm on August 5, 2015: member
    Yes let’s do that…
  5. Add debug message to CValidationState for optional extra information
    Add a field `strDebugMessage` which can be passed to DoS or Invalid,
    and queried using GetDebugMessage() to add extra troubleshooting
    information to the validation state.
    fbf44e6f3e
  6. laanwj force-pushed on Aug 6, 2015
  7. laanwj commented at 8:19 am on August 6, 2015: member

    Done. CValidationState is augmented with optional debug information, which is logged upstream where possible.

    This makes the change larger than originally intended, but the end result is better. See commit messages/details for more information.

    N.B.: I’ve also removed the logging in CScriptCheck::operator()(). This is consistent but I’m uncertain about this, because this means that the script error is no longer logged in case of verification through CCheckQueue (so in the case of blocks). However it is logged when called directly from CheckInputs (https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1394).

  8. in src/main.cpp: in 0cbeb26fa6 outdated
    768@@ -778,6 +769,14 @@ CAmount GetMinRelayFee(const CTransaction& tx, unsigned int nBytes, bool fAllowF
    769     return nMinFee;
    770 }
    771 
    772+/** Convert CValidationState to a human-readable message for logging */
    773+static std::string FormatStateMessage(const CValidationState &state)
    774+{
    775+    return strprintf("%s%s (code %i)",
    776+        state.GetRejectReason(),
    777+        state.GetDebugMessage().empty() ? "" : (", "+state.GetDebugMessage()),
    


    sipa commented at 9:32 am on August 6, 2015:
    No closing bracket?

    laanwj commented at 9:49 am on August 6, 2015:
    The bracket is not in the string.

    laanwj commented at 10:14 am on August 6, 2015:
    Removed the parenthesis around “, “+… - they made the code harder to read instead of easier.
  9. sipa commented at 9:46 am on August 6, 2015: member
    @laanwj That’s not very consistent, as I think that means it will be printed with -par=1 and not with other values. I don’t know of a better solution, though.
  10. laanwj commented at 9:49 am on August 6, 2015: member
    @sipa So we just call par=1 mode ‘safe mode with debugging’. Voila.
  11. sipa commented at 10:01 am on August 6, 2015: member
    Untested ACK (but see nit about closing bracket).
  12. laanwj force-pushed on Aug 6, 2015
  13. dexX7 commented at 8:50 pm on August 7, 2015: contributor

    Also introduces a consistent format for transaction validation failures that always includes the txid.

    Great PR, I’m running it for some hours, and this is really useful!

  14. in src/main.h: in 72f786422f outdated
    454@@ -455,7 +455,14 @@ extern CBlockTreeDB *pblocktree;
    455  */
    456 int GetSpendHeight(const CCoinsViewCache& inputs);
    457 
    458+/** Reject codes greater or equal to this can be returned by AcceptToMemPool
    459+ * for transactions, to signal internal conditions. They cannot and should not
    460+ * be sent over the P2P network.
    461+ */
    462+static const unsigned int REJECT_INTERNAL = 0x100;
    463 /** local "reject" message codes for RPC which can not be triggered by p2p trasactions */
    


    morcos commented at 6:34 pm on August 10, 2015:
    This comment isn’t accurate is it?

    laanwj commented at 3:25 pm on August 11, 2015:
    Correct. This comment only holds for HIGHFEE.
  15. morcos commented at 6:37 pm on August 10, 2015: member
    I really like this idea, there are going to be a whole slew of new possible reject reasons with 6470 and with limited mempool chains. It will be nice to be able to output them in debugging situations.
  16. dcousens commented at 4:21 am on August 11, 2015: contributor
    concept ACK
  17. skarra commented at 10:46 am on August 11, 2015: none
    :+1:
  18. Introduce REJECT_INTERNAL codes for local AcceptToMempool errors
    Add status codes specific to AcceptToMempool procession of transactions.
    These can never happen due to block validation, and must never be sent
    over the P2P network. Add assertions where appropriate.
    dc58258adf
  19. Add function to convert CValidationState to a human-readable message
    It is necessary to be able to concisely log a validation state.
    Convert CValidationState to a human-readable message for logging.
    9003c7cdd8
  20. Remove most logging from transaction validation
    Remove unnecessary direct logging in CheckTransaction,
    AcceptToMemoryPool, CheckTxInputs, CScriptCheck::operator()
    
    All status information should be returned in the CValidationState.
    Relevant debug information is also added to the CValidationState using
    the recently introduced debug message.
    
    Do keep the "BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags"
    error as it is meant to appear as bug in the log.
    6cab808272
  21. Add information to errors in ConnectBlock, CheckBlock
    Add detailed state information to the errors, as it is no longer being
    logged downstream.
    
    Also add the state information to mempool rejection debug message in
    ProcessMessages.
    66daed5e1b
  22. Move mempool rejections to new debug category
    Move mempool rejections to debug category `mempoolrej`, to make it possible
    to show them without enabling the entire category `mempool` which is
    high volume.
    7f1f8f5edf
  23. laanwj force-pushed on Aug 11, 2015
  24. laanwj merged this on Aug 11, 2015
  25. laanwj closed this on Aug 11, 2015

  26. laanwj referenced this in commit 87f37e259d on Aug 11, 2015
  27. in src/main.h: in 7f1f8f5edf
    454@@ -455,7 +455,16 @@ extern CBlockTreeDB *pblocktree;
    455  */
    456 int GetSpendHeight(const CCoinsViewCache& inputs);
    457 
    458-/** local "reject" message codes for RPC which can not be triggered by p2p trasactions */
    459+/** Reject codes greater or equal to this can be returned by AcceptToMemPool
    460+ * for transactions, to signal internal conditions. They cannot and should not
    461+ * be sent over the P2P network.
    462+ */
    463+static const unsigned int REJECT_INTERNAL = 0x100;
    


    droark commented at 4:23 am on August 12, 2015:
    Just curious, was the duplication of the REJECT_HIGHFEE value intentional?
  28. jtimon commented at 8:27 pm on September 3, 2015: contributor
    I’ve been trying to “move error messages up” for consensus and policy functions in different ways and with multiple attempts since at least January 15th 2015 (the first version of #5669 which also moved CheckTransaction to consensus/consensus did that). I would have appreciated to be asked for review on this one. Now it’s too late to complain about its flaws but I still will do it.
  29. luke-jr referenced this in commit 1cbe3b123b on Nov 30, 2015
  30. luke-jr referenced this in commit efe69c81ed on Dec 8, 2015
  31. laanwj referenced this in commit 44757729a4 on Feb 24, 2016
  32. laanwj referenced this in commit 8fc81e0983 on Feb 24, 2016
  33. makevoid referenced this in commit 596a2f5ad3 on Jun 13, 2016
  34. furszy referenced this in commit 8d7e7808d0 on Jun 20, 2020
  35. zkbot referenced this in commit 19a8c45f42 on Aug 13, 2021
  36. zkbot referenced this in commit 8e9f44cbe2 on Aug 13, 2021
  37. zkbot referenced this in commit cf9a0799b4 on Aug 17, 2021
  38. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me