Use -Wswitch for TxoutType where possible #20211

1. 
   MarcoFalke commented at 11:57 am on October 21, 2020: member

   This removes unused default: cases for all switch statements on TxoutType and adds the cases (MULTISIG, NULL_DATA, NONSTANDARD) to ExtractDestination for clarity.

   Also, the compiler is now able to use -Wswitch.

2. test: Add missing script_standard_Solver_success cases fa59e0b5bd
3. Use -Wswitch for TxoutType where possible fa650ca7f1
4. MarcoFalke added the label Refactoring on Oct 21, 2020
5. 
   practicalswift commented at 12:48 pm on October 21, 2020: contributor

   Concept ACK: -Wswitch is good, and termination via assert(false); is better than UB :)

   From the standard: “Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function.” (C++11, [stmt.return])

6. 
   jonatack commented at 6:49 pm on October 21, 2020: member

   Concept ACK
7. 
   hebasto commented at 3:14 pm on November 12, 2020: member

   Concept ACK
8. in src/wallet/rpcdump.cpp:939 in fa650ca7f1

   933@@ -934,9 +934,9 @@ static std::string RecurseImportData(const CScript& script, ImportData& import_d
   934     case TxoutType::NONSTANDARD:
   935     case TxoutType::WITNESS_UNKNOWN:
   936     case TxoutType::WITNESS_V1_TAPROOT:
   937-    default:
   938         return "unrecognized script";
   939-    }
   940+    } // no default case, so the compiler can warn about missing cases
   941+    CHECK_NONFATAL(false);
   

   ---

   ---

   

   hebasto commented at 3:24 pm on November 12, 2020:

   I understand that RPC code shouldn’t terminate the server regardless of user’s input. But TxoutType is an enum class, and its set of values does not depend on user’s input. Therefore, I think the usual assert(false); should be used here.

   ---

   

   MarcoFalke commented at 3:51 pm on November 12, 2020:

   What is the difference in practise? This is dead code either way, so it doesn’t matter what it does as long as it ensures that the function doesn’t return normally when the dead code is executed (for whatever reason).

   ---

   

   hebasto commented at 3:56 pm on November 12, 2020:

   The only difference is a uniform approach through the repo.

   ---

   

   MarcoFalke commented at 4:21 pm on November 12, 2020:

   The rpc code has a linter to check for a uniform CHECK_NONFATAL in all rpc code ;)
9. hebasto approved
10. 
    hebasto commented at 3:56 pm on November 12, 2020: member

    ACK fa650ca7f19307a9237e64ac311488c8947fc12a, I have reviewed the code and it looks OK, I agree it can be merged.
11. 
    DrahtBot commented at 5:36 am on November 21, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

12. mjdietzx approved
13. 
    mjdietzx commented at 4:42 pm on November 23, 2020: contributor

    Ack https://github.com/bitcoin/bitcoin/pull/20211/commits/fa650ca7f19307a9237e64ac311488c8947fc12a

    We do switch (dest.which()) { in a few places as well. This seems pretty similar, and I’m wondering if this should be refactored as well? (Probably in a different PR though?)

    In #20286 I added TxoutType& typeRet to ExtractDestination here, and I was wondering if this would allow us to make these switch (dest.which()) { statements more readable by doing it on TxoutType as done in this PR. Thoughts, and once #20286 is merged, is this something that should be done?

14. 
    MarcoFalke commented at 4:48 pm on November 23, 2020: member

    We do switch (dest.which()) { in a few places as well. This seems pretty similar, and I’m wondering if this should be refactored as well? (Probably in a different PR though?)

    What do you suggest? which decays the enum class into an integer. The compiler can’t do -Wswitch static analysis on those switch statements.

15. 
    mjdietzx commented at 6:18 pm on November 23, 2020: contributor

    Well once we have (would require #20286):

    0bool ExtractDestination(const CScript& scriptPubKey, TxoutType& typeRet, CTxDestination& addressRet);
    

    Could we do something like:

     0TxoutType type;
     1CTxDestination dest;
     2ExtractDestination(m_script, type, dest);
     3switch (type) {
     4case TxoutType::PUBKEYHASH:
     5case TxoutType::SCRIPTHASH: return OutputType::LEGACY;
     6case TxoutType::WITNESS_V0_SCRIPTHASH:
     7case TxoutType::WITNESS_V0_KEYHASH:
     8case TxoutType::WITNESS_V1_TAPROOT:
     9case TxoutType::WITNESS_UNKNOWN: return OutputType::BECH32;
    10case TxoutType::PUBKEY:
    11case TxoutType::MULTISIG:
    12case TxoutType::NONSTANDARD:
    13case TxoutType::NULL_DATA: return nullopt; // no default case, so the compiler can warn about missing cases
    14}
    15assert(false);
    

    Note: I messed around with this, and probably made some mistakes here, but didn’t see any unit tests or functional tests fail. So maybe some test coverage is needed here as well?

16. 
    MarcoFalke commented at 6:22 pm on November 23, 2020: member

    would require #20286

    Let’s discuss this in the other pull then

17. 
    practicalswift commented at 8:55 am on February 11, 2021: contributor

    cr ACK fa650ca7f19307a9237e64ac311488c8947fc12a: patch looks correct and assert(false); is better than UB :)
18. MarcoFalke merged this on Feb 11, 2021
19. MarcoFalke closed this on Feb 11, 2021

    ---

20. MarcoFalke deleted the branch on Feb 11, 2021
21. sidhujag referenced this in commit a9a55253de on Feb 11, 2021
22. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke practicalswift jonatack hebasto DrahtBot mjdietzx

Labels
Refactoring