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

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:consensus-decouple-util-0.13.99 changing 1 files +32 −51

jtimon commented at 2:03 PM on January 4, 2016: contributor

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 commented at 2:28 PM on January 4, 2016: contributor

Why get rid of FormatMoney? That seems like a regression of sorts.
jtimon commented at 2:31 PM on January 4, 2016: contributor

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 commented at 2:35 PM on January 4, 2016: contributor

My 'regression' comment was regarding type encapsulation. It should not be assumed that CAmount is a type understood by sprintf.
jtimon commented at 2:44 PM on January 4, 2016: contributor

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 https://github.com/jtimon/bitcoin/commit/85124904479f2c2276c26c825925cdfa5998cab6#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 force-pushed on Jan 4, 2016
jtimon commented at 5:31 PM on January 4, 2016: contributor

Squashed fixes.
jtimon renamed this:
~~Consensus: Remove calls to error(), FormatStateMessage() and FormatMoney()~~
Consensus: Remove calls to error() and FormatStateMessage()
on Jan 4, 2016
jtimon force-pushed on Jan 4, 2016
laanwj added the label Refactoring on Jan 7, 2016
jtimon force-pushed on Jan 13, 2016
jtimon commented at 8:05 PM on January 13, 2016: contributor

Rebased (1)

in src/main.cpp:None in a22b7a4bc1 outdated

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

dcousens commented at 3:14 PM on January 28, 2016:

could be const

jtimon commented at 2:28 PM on January 29, 2016:

will change.

dcousens commented at 3:15 PM on January 28, 2016: contributor

concept ACK, utACK a22b7a4
in src/main.cpp:None in a22b7a4bc1 outdated
```
2908 | @@ -2909,13 +2909,11 @@ bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool f
2909 |  {
2910 |      // Check proof of work matches claimed amount
2911 |      if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus()))
2912 | -        return state.DoS(50, error("CheckBlockHeader(): proof of work failed"),
2913 | -                         REJECT_INVALID, "high-hash");
2914 | +        return state.DoS(50, false, REJECT_INVALID, "high-hash");
```
laanwj commented at 2:05 PM on January 29, 2016:

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.

jtimon commented at 2:17 PM on January 29, 2016:

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.

laanwj commented at 2:36 PM on January 29, 2016:

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

jtimon commented at 2:51 PM on January 29, 2016:

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...
in src/main.cpp:None in a22b7a4bc1 outdated
```
3543 | @@ -3565,7 +3544,8 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview,
3544 |              return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
```
laanwj commented at 2:06 PM on January 29, 2016:

You're replacing the VerifyDB() with func below, but not here

jtimon commented at 2:24 PM on January 29, 2016:

Yes and neither do I in the other 5 occurrences of the string "VerifyDB():" in this function. The goal of this PR is not to fix that. I only do it in the line below because I had to touch that line anyway. But I'd rather not cause more conflicts than necessary. Of course somebody else can cleanup the whole function (or the whole file) in some other PR (preferably after #7310 ).

laanwj commented at 4:15 PM on January 29, 2016:

Sure, just wondered if there was a reason for doing this change only in one place, but that seems a good reason.
jtimon force-pushed on Jan 29, 2016
jtimon force-pushed on Jan 29, 2016
jtimon commented at 4:00 PM on January 29, 2016: contributor

Updated, hopefully solving @dcousens and @laanwj 's nits.
Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main 93fc58c742

in src/main.cpp:None in 8d777437c0 outdated

2970 |          if (!CheckTransaction(tx, state))
2971 | -            return error("CheckBlock(): CheckTransaction of %s failed with %s",
2972 | -                tx.GetHash().ToString(),
2973 | -                FormatStateMessage(state));
2974 | +            return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),
2975 | +                                 strprintf("Transaction check failed (tx hash %s)", tx.GetHash().ToString(), state.GetDebugMessage()));

laanwj commented at 4:50 PM on January 29, 2016:

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

jtimon commented at 5:14 PM on January 29, 2016:

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

jtimon force-pushed on Jan 29, 2016
jtimon commented at 5:40 PM on January 29, 2016: contributor

Updated fixing a mistake introduced with the latest changes, thanks for noticing so quickly @laanwj
laanwj commented at 10:18 AM on January 30, 2016: member

Tested ACK 93fc58c
laanwj merged this on Feb 1, 2016
laanwj closed this on Feb 1, 2016
laanwj referenced this in commit 31ec14b74b on Feb 1, 2016
laanwj referenced this in commit 44757729a4 on Feb 24, 2016
laanwj referenced this in commit 8fc81e0983 on Feb 24, 2016
makevoid referenced this in commit 596a2f5ad3 on Jun 13, 2016
codablock referenced this in commit 6b513cd444 on Sep 16, 2017
codablock referenced this in commit 2daf44c124 on Sep 19, 2017
codablock referenced this in commit 27ef426bd3 on Dec 9, 2017
codablock referenced this in commit acf952f7ac on Dec 9, 2017
codablock referenced this in commit 6789bf6e13 on Dec 11, 2017
vecopay referenced this in commit 84a5777682 on Dec 16, 2018
codablock referenced this in commit 6aed24bf6e on Jan 4, 2020
codablock referenced this in commit 9fde756dbc on Jan 5, 2020
codablock referenced this in commit 652ffdba6a on Jan 7, 2020
codablock referenced this in commit c037c238a8 on Jan 8, 2020
UdjinM6 referenced this in commit 91b4a38398 on Jan 11, 2020
furszy referenced this in commit 8d7e7808d0 on Jun 20, 2020
ckti referenced this in commit 3170bf03b0 on Mar 28, 2021
gades referenced this in commit 211cda13b3 on Jun 30, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7310 MOVEONLY: Move consensus functions out of main #7592 mempool: Re-remove ERROR logging for mempool rejects #8341 Consensus: Remove calls to error() from ContextualCheckBlock