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
  1. 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().

  2. maaku commented at 2:28 pm on January 4, 2016: contributor
    Why get rid of FormatMoney? That seems like a regression of sorts.
  3. 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.
  4. 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.
  5. 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).

  6. jtimon force-pushed on Jan 4, 2016
  7. jtimon commented at 5:31 pm on January 4, 2016: contributor
    Squashed fixes.
  8. jtimon renamed this:
    Consensus: Remove calls to error(), FormatStateMessage() and FormatMoney()
    Consensus: Remove calls to error() and FormatStateMessage()
    on Jan 4, 2016
  9. jtimon force-pushed on Jan 4, 2016
  10. laanwj added the label Refactoring on Jan 7, 2016
  11. jtimon force-pushed on Jan 13, 2016
  12. jtimon commented at 8:05 pm on January 13, 2016: contributor
    Rebased (1)
  13. in src/main.cpp: 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.
  14. dcousens commented at 3:15 pm on January 28, 2016: contributor
    concept ACK, utACK a22b7a4
  15. in src/main.cpp: 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…

  16. in src/main.cpp: 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.
  17. jtimon force-pushed on Jan 29, 2016
  18. jtimon force-pushed on Jan 29, 2016
  19. jtimon commented at 4:00 pm on January 29, 2016: contributor
    Updated, hopefully solving @dcousens and @laanwj ’s nits.
  20. Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main 93fc58c742
  21. in src/main.cpp: 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…
  22. jtimon force-pushed on Jan 29, 2016
  23. 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
  24. laanwj commented at 10:18 am on January 30, 2016: member
    Tested ACK 93fc58c
  25. laanwj merged this on Feb 1, 2016
  26. laanwj closed this on Feb 1, 2016
  27. laanwj referenced this in commit 31ec14b74b on Feb 1, 2016
  28. laanwj referenced this in commit 44757729a4 on Feb 24, 2016
  29. laanwj referenced this in commit 8fc81e0983 on Feb 24, 2016
  30. makevoid referenced this in commit 596a2f5ad3 on Jun 13, 2016
  31. codablock referenced this in commit 6b513cd444 on Sep 16, 2017
  32. codablock referenced this in commit 2daf44c124 on Sep 19, 2017
  33. codablock referenced this in commit 27ef426bd3 on Dec 9, 2017
  34. codablock referenced this in commit acf952f7ac on Dec 9, 2017
  35. codablock referenced this in commit 6789bf6e13 on Dec 11, 2017
  36. vecopay referenced this in commit 84a5777682 on Dec 16, 2018
  37. codablock referenced this in commit 6aed24bf6e on Jan 4, 2020
  38. codablock referenced this in commit 9fde756dbc on Jan 5, 2020
  39. codablock referenced this in commit 652ffdba6a on Jan 7, 2020
  40. codablock referenced this in commit c037c238a8 on Jan 8, 2020
  41. UdjinM6 referenced this in commit 91b4a38398 on Jan 11, 2020
  42. furszy referenced this in commit 8d7e7808d0 on Jun 20, 2020
  43. ckti referenced this in commit 3170bf03b0 on Mar 28, 2021
  44. gades referenced this in commit 211cda13b3 on Jun 30, 2021
  45. MarcoFalke locked this on Sep 8, 2021


jtimon maaku dcousens laanwj

Labels
Refactoring

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-19 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me