Revert "Change enum values to be powers of two (fix #3417)"

[intelliot]

Consider the function conditionMet, specifically lines 121 and 122 and 146-147 of Handler.h:

1c2ae10#diff-41d8d23b15cb894b075204dcd88d7bf9ca223fa77f43e28bb22cf38b805cabfcL121-L122

1c2ae10#diff-41d8d23b15cb894b075204dcd88d7bf9ca223fa77f43e28bb22cf38b805cabfcL146-L147

The change is broken: where it previously, it checked for a specific condition (e.g. NEEDS_CLOSED_LEDGER) and only entered the if block if it was met, it now executes that code if any condition is set.

A prime example of a RPC request that fails is the tx command, which has NEEDS_NETWORK_CONNECTION and will now not work unless the server has both a current and a closed ledger.

The problem isn't the powers of two change. It's how the checks where restructured.

Originally posted by @nbougalis in #4601 (comment)

Reverts #4239

[ckeshava]

Yes, reverting the commit will fix the problem. But I feel we also need to add additional unit tests, so that such blindspots are covered for the future.

[intelliot]

This is ready to merge because it just reverts PR #4239, and the original author of that PR has approved.

[HowardHinnant]

This revertible code isn't readable because it checks like a bit-mask even though it is not a bit-mask.

If the power-of-2 fix doesn't have the right behavior, it should be fixed with code that is more readable than this revert results in. E.g. change the checks to use == instead of &.

That being said, @ximinez has an analysis that says the behavior between these two versions is the same, in which case I strongly prefer the power-of-2 version.

[ximinez]

Veto. Copying my comment from #4601 (comment):

A prime example of a RPC request that fails is the tx command, which has NEEDS_NETWORK_CONNECTION and will now not work unless the server has both a current and a closed ledger.

I don't think there's a behavior change at all. I commented on the original PR to that effect: #4239 (comment)

Here's more detail.

In the old code,

NO_CONDITION == 0
NEEDS_NETWORK_CONNECTION == 1 == 0b0001
NEEDS_CURRENT_LEDGER     == 3 == 0b0011
NEEDS_CLOSED_LEDGER      == 5 == 0b0101

Consider the condition on line 110:

condition_required & NEEDS_NETWORK_CONNECTION

which is equivalent to

condition_required & 0b0001

condition_required can be any of those 4 values.

condition_required	condition_required & NEEDS_NETWORK_CONNECTION	boolean result
NO_CONDITION == 0	0	false
NEEDS_NETWORK_CONNECTION == 0b0001	0b0001	true
NEEDS_CURRENT_LEDGER == 0b0011	0b0001	true
NEEDS_CLOSED_LEDGER == 0b0101	0b0001	true

Basically, for any value except NO_CONDITION (0), the expression evaluates to true.

The new values are 0, 1, 2, and 4, respectively, but the result is the same. condition_required != NO_CONDITION in the new code is functionally equivalent to condition_required & NEEDS_NETWORK_CONNECTION in the old code.

It should be clear that the results are the same for the other values.

[intelliot]

Closing because @ckeshava and @ximinez agree that this should not be reverted; see #4601 (comment)