Use -Wswitch for TxoutType where possible #20211

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.

test: Add missing script_standard_Solver_success cases fa59e0b5bd

Use -Wswitch for TxoutType where possible fa650ca7f1

MarcoFalke added the label Refactoring on Oct 21, 2020

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])

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

Concept ACK

hebasto commented at 3:14 PM on November 12, 2020: member

Concept ACK

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 ;)

hebasto approved

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.

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.

mjdietzx approved

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?

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.

mjdietzx commented at 6:18 PM on November 23, 2020: contributor

Well once we have (would require #20286):

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

Could we do something like:

TxoutType type;
CTxDestination dest;
ExtractDestination(m_script, type, dest);
switch (type) {
case TxoutType::PUBKEYHASH:
case TxoutType::SCRIPTHASH: return OutputType::LEGACY;
case TxoutType::WITNESS_V0_SCRIPTHASH:
case TxoutType::WITNESS_V0_KEYHASH:
case TxoutType::WITNESS_V1_TAPROOT:
case TxoutType::WITNESS_UNKNOWN: return OutputType::BECH32;
case TxoutType::PUBKEY:
case TxoutType::MULTISIG:
case TxoutType::NONSTANDARD:
case TxoutType::NULL_DATA: return nullopt; // no default case, so the compiler can warn about missing cases
}
assert(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?

MarcoFalke commented at 6:22 PM on November 23, 2020: member

would require #20286

Let's discuss this in the other pull then

practicalswift commented at 8:55 AM on February 11, 2021: contributor

cr ACK fa650ca7f19307a9237e64ac311488c8947fc12a: patch looks correct and assert(false); is better than UB :)

MarcoFalke merged this on Feb 11, 2021

MarcoFalke closed this on Feb 11, 2021

MarcoFalke deleted the branch on Feb 11, 2021

sidhujag referenced this in commit a9a55253de on Feb 11, 2021

DrahtBot locked this on Aug 18, 2022