consensus: Improve CScriptNum error reporting #16881

1. 
   fanquake commented at 8:08 am on September 16, 2019: member

   This is a rebased, split up version of #15074. I’m going to leave this open for a week to see if it attracts any Concept ACKs, otherwise I’ll close it.

   Changes from the original PR are removing an errant newline in this catch block, and splitting the assert opcodes are in a valid range change from the error reporting changes.

   These changes had been looked at by meshcolider and ajtowns, so have re-notified them.

2. consensus: Improve CScriptNum error reporting

   Instead of reporting "unknown error", more specific messages are returned for CScriptNum related errors.
   
   Original PR: #15074
   
   Co-Authored-By: Johnson Lau <jl2012@xbt.hk>

   87a8cbf5dc
3. consensus: assert that opcodes are in a valid range

   Replace throw with assert in case opcode is negative or bigger than 0xff, which should never happen.
   
   Original PR: 15074
   
   Co-Authored-By: Johnson Lau <jl2012@xbt.hk>

   7260d6b5ec
4. fanquake added the label Consensus on Sep 16, 2019
5. fanquake added the label Needs Conceptual Review on Sep 16, 2019
6. fanquake requested review from ajtowns on Sep 16, 2019
7. fanquake requested review from meshcollider on Sep 16, 2019
8. fanquake added this to the "Chasing Concept ACK" column in a project

   ---

9. 
   DrahtBot commented at 11:11 am on September 16, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

10. in src/script/script.h:222 in 7260d6b5ec

    218@@ -224,7 +219,7 @@ class CScriptNum
    219                         const size_t nMaxNumSize = nDefaultMaxNumSize)
    220     {
    221         if (vch.size() > nMaxNumSize) {
    222-            throw scriptnum_error("script number overflow");
    223+            throw SCRIPT_ERR_NUMOVERFLOW;
    

    ---

    ---

    

    laanwj commented at 12:30 pm on September 18, 2019:

    Throwing a bare enum (or anything not deriving from std::exception) is a bit strange in C++. I think if you want to add an error code it’d be better to add an error code field to the scriptnum_error class and its constructor, and keep throwing that.
11. fanquake closed this on Sep 23, 2019

    ---

12. fanquake deleted the branch on Sep 23, 2019
13. fanquake removed this from the "Chasing Concept ACK" column in a project

    ---

14. DrahtBot locked this on Dec 16, 2021

Contributors
fanquake DrahtBot laanwj

Review Requested
ajtowns meshcollider

Labels
Consensus Needs Conceptual Review