Consensus: Remove calls to error() and FormatStateMessage() #7287

jtimon · 2016-01-04T14:03:39Z

Remove calls to error() and FormatStateMessage() ~~and FormatMoney()~~ from some consensus code in main.

This is necessary because libconsensus cannot depend on util.cpp, which exposes globals among other things. Doing this before moving this consensus code out of main allows consensus/consensus.cpp to never depend on util.cpp and it removes the necessity to make FormatStateMessage() non-static to call it, temporarily, from consensus/consensus.cpp.

This also restores some error reporting that seems to have been lost, maybe while moving to use FormatStateMessage().

maaku · 2016-01-04T14:28:01Z

Why get rid of FormatMoney? That seems like a regression of sorts.

jtimon · 2016-01-04T14:31:58Z

There's no reason for utilmoneystr.o to be part of libconsensus.
The only reject reason that was using it is in https://github.com/bitcoin/bitcoin/pull/7287/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR1633 which can just express the amounts in satoshis.

maaku · 2016-01-04T14:35:24Z

My 'regression' comment was regarding type encapsulation. It should not be assumed that CAmount is a type understood by sprintf.

jtimon · 2016-01-04T14:44:35Z

I can restore the call to FormatMoney there and make utilmoneystr.o part of libconsensus only for one that reject reason line if that is preferred.
If you have another option in mind, please, say so.

Also, the python tests are failing because I have improved the information provided by the reject reason in jtimon@8512490#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR2989 At a first glance it doesn't seem trivial to adapt it to the new message, so the simplest option seems to be to just leave the same reject reason that we had (but then some information like the hash of the transaction will be lost in the error messages printed by the callers).

jtimon · 2016-01-04T17:31:07Z

Squashed fixes.

jtimon · 2016-01-13T20:05:49Z

Rebased (1)

dcousens · 2016-01-28T15:14:28Z

src/main.cpp

@@ -814,12 +814,13 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
                              bool* pfMissingInputs, bool fOverrideMempoolLimit, bool fRejectAbsurdFee,
                              std::vector<uint256>& vHashTxnToUncache)
 {
+    uint256 hash = tx.GetHash();


could be const

will change.

dcousens · 2016-01-28T15:15:06Z

concept ACK, utACK a22b7a4

laanwj · 2016-01-29T14:05:10Z

src/main.cpp

@@ -2909,13 +2909,11 @@ bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool f
 {
    // Check proof of work matches claimed amount
    if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus()))
-        return state.DoS(50, error("CheckBlockHeader(): proof of work failed"),
-                         REJECT_INVALID, "high-hash");
+        return state.DoS(50, false, REJECT_INVALID, "high-hash");


Not sure if it makes sense in this case, depends on whether the extra explanation offered is worthwhile, but instead of deleting the error message here you could pass it to DoS as strDebugMessage. That's why that field was introduced.

Sure, I can set that field wherever we think it's valuable. Please, point out the cases where you think it's worth it and I'll happily change it.

I do think the old messages in all these cases are more informative than the two-word codes, which are documented nowhere.

In all cases? I think I disagree, but I don't care enough to discuss it, I'll change all of them to use all the long messages.

The non-documented codes can be grep, but I think at some point we could just use ConsensusError_t that could replace ScriptError_t. Anyway, one step at a time...

jtimon · 2016-01-29T16:00:54Z

Updated, hopefully solving @dcousens and @laanwj 's nits.

laanwj · 2016-01-29T16:50:30Z

src/main.cpp

-                tx.GetHash().ToString(),
-                FormatStateMessage(state));
+            return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),
+                                 strprintf("Transaction check failed (tx hash %s)", tx.GetHash().ToString(), state.GetDebugMessage()));


strprintf: Only one %s, but two arguments (this likely causes the travis issue)

Good catch, I didn't really need to even touch that line to fix your nit...

… consensus code in main

jtimon · 2016-01-29T17:40:57Z

Updated fixing a mistake introduced with the latest changes, thanks for noticing so quickly @laanwj

laanwj · 2016-01-30T10:18:10Z

Tested ACK 93fc58c

93fc58c Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main (Jorge Timón)

Continues "Make logging for validation optional" from bitcoin#6519. The idea there was to remove all ERROR logging of rejected transaction, and move it to one message in the class 'mempoolrej' which logs the state message (and debug info). The superfluous ERRORs in the log "terrify" users, see for example issue bitcoin#5794. Unfortunately a lot of new logging was introduced in bitcoin#6871 (RBF) and bitcoin#7287 (misc refactoring). This pull updates that new code.

…eMessage() 93fc58c Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main (Jorge Timón)

…rmatStateMessage() dashpay/dash@93fc58c Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main (Jorge Timón)

This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py

…nTestFramework (#3277) * [tests] Change feature_block.py to use BitcoinTestFramework * [tests] Fix flake8 warnings in feature_block.py * [tests] Tidy up feature_block.py - move all helper methods to the end - remove block, create_tx and create_and_sign_tx shortcuts - remove --runbarelyexpensive option, since it defaults to True and it's unlikely that anyone ever runs the test with this option set to false. * [tests] Add logging to feature_block.py * [tests] Improve assert message when wait_until() fails * Merge bitcoin#13048: [tests] Fix feature_block flakiness c1d7420 [tests] Fix feature_block flakiness (John Newbery) Pull request description: feature_block.py occasionally fails on Travis. I believe this is due to a a race condition when reconnecting to bitcoind after a subtest that expects disconnection. If the test runs ahead and sends the INV for the subsequent test before we've received the initial sync getheaders, then we may end up sending two headers messages - one as a response to the initial sync getheaders and one in response to the INV getheaders. If both of those headers fail validation with a DoS score of 50 or higher, then we'll unexpectedly be disconnected. There is only one validation failure that has a DoS score bewteen 50 and 100, which is high-hash. That's why the test is failing immediately after the "Reject a block with invalid work" subtest. Fix is to wait for the initial getheaders from the peer before we start populating our blockstore. That way we won't have any invalid headers to respond to it with. Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f * Temporarely rename MAX_BLOCK_SIZE -> MAX_BLOCK_BASE_SIZE We'll undo this after the next commit. This avoids merge many conflicts and makes reviewing easier. * Rename MAX_BLOCK_BASE_SIZE back to MAX_BLOCK_SIZE * Use DoS score of 100 for bad-blk-sigops This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py * Use allowOptimisticSend=true when sending reject messages This fixes test failures in p2p-fullblocktest.py which expects reject messages to be sent/received before connections get closed. * Fix p2p-fullblocktest.py - CBlock and friends are still in test_framework.mininode - "-whitelist" causes connections to not be dropped, which in turn causes sync_blocks with reconnect=True to fail - "bad-cb-amount" does not cause a ban in Dash, so reconnect must be False - Dash already bans when a header is received which is a child of an invalid header, causing block requests to never happen * Backport missing changes from bitcoin#13003 bitcoin#13003 was backported out of order which causes missed changes. * Bump p2p-fullblocktest timeouts * Increase RPC timeout in p2p-fullblocktest.py Co-authored-by: John Newbery <jonnynewbs@gmail.com> Co-authored-by: MarcoFalke <falke.marco@gmail.com>

ca489c9 [Tests] Fix expected error messages (random-zebra) 9f84c52 Consensus: Remove calls to error() and FormatStateMessage() (random-zebra) 0e4d964 Move mempool rejections to new debug category (random-zebra) e0db16d Add information to errors in ConnectBlock, CheckBlock (random-zebra) d4ed81c Remove most logging from transaction validation (random-zebra) 94b2577 Add function to convert CValidationState to a human-readable message (random-zebra) 1ddc88f Introduce REJECT_INTERNAL codes for local AcceptToMempool errors (random-zebra) 55e00a2 Add debug message to CValidationState for optional extra information (random-zebra) 05cf74b Add absurdly high fee message to validation state (for RPC propagation) (random-zebra) Pull request description: - change `CValidationState::chRejectCode` to int - add `CValidationState::strDebugMessage` for optional information - add `FormatStateMessage` function to convert CValidationState to a human-readable message - introduce REJECT_INTERNAL rejection codes for ATMP and make their logging optional - remove unnecessary direct logging in `CheckTransaction`, `AcceptToMemoryPool`, `CheckTxInputs`, `CScriptCheck::operator()` - add detailed state information to the errors in `CheckBlock` and `ConnectBlock` Backported from: - bitcoin#5913 - bitcoin#6519 - bitcoin#7287 ACKs for top commit: Fuzzbawls: ACK ca489c9 furszy: re ACK ca489c9 and merging Tree-SHA512: ab972801fa45c2f84abf84790b0f0f22dc5668e170f51785f3cfbf806bda7988f55bbd43c24ae591ffe3bd62190f6cd99a3b640373c431ab92c0ddcabea1c999

…nTestFramework (dashpay#3277) * [tests] Change feature_block.py to use BitcoinTestFramework * [tests] Fix flake8 warnings in feature_block.py * [tests] Tidy up feature_block.py - move all helper methods to the end - remove block, create_tx and create_and_sign_tx shortcuts - remove --runbarelyexpensive option, since it defaults to True and it's unlikely that anyone ever runs the test with this option set to false. * [tests] Add logging to feature_block.py * [tests] Improve assert message when wait_until() fails * Merge bitcoin#13048: [tests] Fix feature_block flakiness c1d7420 [tests] Fix feature_block flakiness (John Newbery) Pull request description: feature_block.py occasionally fails on Travis. I believe this is due to a a race condition when reconnecting to bitcoind after a subtest that expects disconnection. If the test runs ahead and sends the INV for the subsequent test before we've received the initial sync getheaders, then we may end up sending two headers messages - one as a response to the initial sync getheaders and one in response to the INV getheaders. If both of those headers fail validation with a DoS score of 50 or higher, then we'll unexpectedly be disconnected. There is only one validation failure that has a DoS score bewteen 50 and 100, which is high-hash. That's why the test is failing immediately after the "Reject a block with invalid work" subtest. Fix is to wait for the initial getheaders from the peer before we start populating our blockstore. That way we won't have any invalid headers to respond to it with. Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f * Temporarely rename MAX_BLOCK_SIZE -> MAX_BLOCK_BASE_SIZE We'll undo this after the next commit. This avoids merge many conflicts and makes reviewing easier. * Rename MAX_BLOCK_BASE_SIZE back to MAX_BLOCK_SIZE * Use DoS score of 100 for bad-blk-sigops This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py * Use allowOptimisticSend=true when sending reject messages This fixes test failures in p2p-fullblocktest.py which expects reject messages to be sent/received before connections get closed. * Fix p2p-fullblocktest.py - CBlock and friends are still in test_framework.mininode - "-whitelist" causes connections to not be dropped, which in turn causes sync_blocks with reconnect=True to fail - "bad-cb-amount" does not cause a ban in Dash, so reconnect must be False - Dash already bans when a header is received which is a child of an invalid header, causing block requests to never happen * Backport missing changes from bitcoin#13003 bitcoin#13003 was backported out of order which causes missed changes. * Bump p2p-fullblocktest timeouts * Increase RPC timeout in p2p-fullblocktest.py Co-authored-by: John Newbery <jonnynewbs@gmail.com> Co-authored-by: MarcoFalke <falke.marco@gmail.com>

jtimon force-pushed the consensus-decouple-util-0.13.99 branch from b3518dc to 7a7ed08 Compare January 4, 2016 17:26

jtimon changed the title ~~Consensus: Remove calls to error(), FormatStateMessage() and FormatMoney()~~ Consensus: Remove calls to error() and FormatStateMessage() Jan 4, 2016

jtimon force-pushed the consensus-decouple-util-0.13.99 branch from 7a7ed08 to 3fa51ee Compare January 4, 2016 17:39

laanwj added the Refactoring label Jan 7, 2016

jtimon mentioned this pull request Jan 7, 2016

MOVEONLY: Move consensus functions out of main #7310

Closed

jtimon force-pushed the consensus-decouple-util-0.13.99 branch from 3fa51ee to a22b7a4 Compare January 13, 2016 20:05

dcousens reviewed Jan 28, 2016
View reviewed changes

laanwj reviewed Jan 29, 2016
View reviewed changes

jtimon force-pushed the consensus-decouple-util-0.13.99 branch 2 times, most recently from 450c06f to 8d77743 Compare January 29, 2016 15:27

laanwj reviewed Jan 29, 2016
View reviewed changes

Consensus: Remove calls to error() and FormatStateMessage() from some…

93fc58c

… consensus code in main

jtimon force-pushed the consensus-decouple-util-0.13.99 branch from 8d77743 to 93fc58c Compare January 29, 2016 17:39

laanwj merged commit 93fc58c into bitcoin:master Feb 1, 2016

laanwj added a commit that referenced this pull request Feb 1, 2016

Merge #7287: Consensus: Remove calls to error() and FormatStateMessage()

31ec14b

93fc58c Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main (Jorge Timón)

laanwj mentioned this pull request Feb 24, 2016

mempool: Re-remove ERROR logging for mempool rejects #7592

Merged

NicolasDorier mentioned this pull request Jul 15, 2016

Consensus: Remove calls to error() from ContextualCheckBlock #8341

Merged

kyuupichan mentioned this pull request May 31, 2017

[WIP][port] Prepare to move some concensus code out of main.cpp BitcoinUnlimited/BitcoinUnlimited#516

Closed

codablock added a commit to codablock/dash that referenced this pull request Jan 4, 2020

Use DoS score of 100 for bad-blk-sigops

6aed24b

This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py

codablock added a commit to codablock/dash that referenced this pull request Jan 5, 2020

Use DoS score of 100 for bad-blk-sigops

9fde756

This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py

codablock added a commit to codablock/dash that referenced this pull request Jan 7, 2020

Use DoS score of 100 for bad-blk-sigops

652ffdb

This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py

codablock added a commit to codablock/dash that referenced this pull request Jan 8, 2020

Use DoS score of 100 for bad-blk-sigops

c037c23

This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py

random-zebra mentioned this pull request Jun 9, 2020

[Refactoring] CValidationState logging PIVX-Project/PIVX#1670

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Consensus: Remove calls to error() and FormatStateMessage() #7287

Consensus: Remove calls to error() and FormatStateMessage() #7287

Uh oh!

Conversation

jtimon commented Jan 4, 2016

Uh oh!

maaku commented Jan 4, 2016

Uh oh!

jtimon commented Jan 4, 2016

Uh oh!

maaku commented Jan 4, 2016

Uh oh!

jtimon commented Jan 4, 2016

Uh oh!

jtimon commented Jan 4, 2016

Uh oh!

jtimon commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Jan 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 29, 2016

Uh oh!

laanwj commented Jan 30, 2016

Uh oh!

Uh oh!