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

ckeshava · 2022-07-15T21:34:34Z

If merged, this code will:

Implement the simplified condition evaluation
removes the complex bitwise and(&) operator
Implement the second proposed solution in Nik Bougalis's comment - Software does not distinguish between different Conditions (Version: 1.5) #3417 (comment)
I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.

ckeshava · 2022-07-15T21:44:57Z

@nbougalis I have a question about the choice of integers inside the Condition enum. Does it necessarily have to be powers of 2? With respect to the operations in this file, it suffices to choose different integer values right?

I understand that the choice of even/odd values is important because we do bit-wise and operator.

greg7mdp · 2022-07-15T22:17:52Z

They have to be powers of two if you want to be able to set and test the bits individually.

For example if you define:

enum { A=1, B=2, C=3};

and you do:

uint32_t x = C;

the value of x will be 0x3 (the two lower bits set), so you would have both (x & A) != 0 and (x & B) != 0

if you use powers of two the bits can be set and tested individually. I used to use enums like:

enum {
   A = 1 << 0,
   B = 1 << 1,
   C = 1 << 2,
   ...
   };

ckeshava · 2022-07-15T23:18:47Z

They have to be powers of two if you want to be able to set and test the bits individually.

For example if you define:
enum { A=1, B=2, C=3};
and you do:
uint32_t x = C;
the value of x will be 0x3 (the two lower bits set), so you would have both (x & A) != 0 and (x & B) != 0

if you use powers of two the bits can be set and tested individually. I used to use enums like:
enum {
   A = 1 << 0,
   B = 1 << 1,
   C = 1 << 2,
   ...
   };

okay got it, thanks for the explanation! I would individually check all the conditions and that is quite verbose, this is concise 👍

greg7mdp · 2022-07-16T01:04:39Z

You can use a sequence 1,2,3,4,5,... if all the conditions are exclusive from one another. But if they are flags that can be set independently, then it has to be separate bits to be workable.

ckeshava · 2022-07-19T21:27:02Z

You can use a sequence 1,2,3,4,5,... if all the conditions are exclusive from one another. But if they are flags that can be set independently, then it has to be separate bits to be workable.

Okay, got it 👍

HowardHinnant · 2023-03-16T21:53:23Z

src/ripple/rpc/impl/Handler.h

-    NEEDS_CURRENT_LEDGER = 2 + NEEDS_NETWORK_CONNECTION,
-    NEEDS_CLOSED_LEDGER = 4 + NEEDS_NETWORK_CONNECTION,
+    NEEDS_CURRENT_LEDGER = 2 * NEEDS_NETWORK_CONNECTION,
+    NEEDS_CLOSED_LEDGER = 4 * NEEDS_NETWORK_CONNECTION,


Consider this purely stylistic change:

enum Condition { NO_CONDITION = 0, NEEDS_NETWORK_CONNECTION = 1, NEEDS_CURRENT_LEDGER = 2, NEEDS_CLOSED_LEDGER = 4, };

Doesn't make much difference for small enums, but I like:

enum Condition { NO_CONDITION = 0, NEEDS_NETWORK_CONNECTION = 1<<0, NEEDS_CURRENT_LEDGER = 1<<1 NEEDS_CLOSED_LEDGER = 1<<2, };

intelliot · 2023-04-24T18:49:36Z

@ckeshava would you like to pick this up?

ckeshava · 2023-04-25T23:02:02Z

Hello,
I have completed the code changes. The unit tests are passing locally, I'm not sure why the CI tests are failing on the ubuntu machine.

greg7mdp · 2023-04-25T23:06:24Z

src/ripple/rpc/impl/Handler.h

@@ -39,8 +39,8 @@ namespace RPC {
 enum Condition {
    NO_CONDITION = 0,
    NEEDS_NETWORK_CONNECTION = 1,
-    NEEDS_CURRENT_LEDGER = 2 + NEEDS_NETWORK_CONNECTION,
-    NEEDS_CLOSED_LEDGER = 4 + NEEDS_NETWORK_CONNECTION,
+    NEEDS_CURRENT_LEDGER = 1 << 1,


Is NEEDS_NETWORK_CONNECTION not required anymore?

greg7mdp · 2023-04-25T23:08:02Z

src/ripple/rpc/impl/Handler.h

@@ -94,20 +94,18 @@ conditionMet(Condition condition_required, T& context)
    }

    if (context.app.getOPs().isAmendmentBlocked() &&
-        (condition_required & NEEDS_CURRENT_LEDGER ||
-         condition_required & NEEDS_CLOSED_LEDGER))
+        (condition_required != NO_CONDITION))


This is not the same test as before. Is the change on purpose?

yes, I agree that the conditions have changed. I made this change approximately one year ago, so I am spacing out on the details. Let me think/check and get back to you

Based on the old values, I think the conditions are equivalent. Between NEEDS_CURRENT_LEDGER and NEEDS_CLOSED_LEDGER, having any of the three low bits set in condition_required would result in the expression evaluating to true, so that covers all of the != NO_CONDITION conditions. So functionally, they are equivalent.

Now whether that was the intent is a different question.

It turns out there are only two operations which have NEEDS_NETWORK_CONNECTION: ledger_cleaner and tx. I think it's fine to disallow both of them if the server is amendment blocked.

ximinez

I think this looks good, and doesn't appear to actually change any existing behavior.

ximinez · 2023-06-09T21:12:34Z

src/ripple/rpc/impl/Handler.h

@@ -94,20 +94,18 @@ conditionMet(Condition condition_required, T& context)
    }

    if (context.app.getOPs().isAmendmentBlocked() &&
-        (condition_required & NEEDS_CURRENT_LEDGER ||
-         condition_required & NEEDS_CLOSED_LEDGER))
+        (condition_required != NO_CONDITION))


Based on the old values, I think the conditions are equivalent. Between NEEDS_CURRENT_LEDGER and NEEDS_CLOSED_LEDGER, having any of the three low bits set in condition_required would result in the expression evaluating to true, so that covers all of the != NO_CONDITION conditions. So functionally, they are equivalent.

Now whether that was the intent is a different question.

It turns out there are only two operations which have NEEDS_NETWORK_CONNECTION: ledger_cleaner and tx. I think it's fine to disallow both of them if the server is amendment blocked.

intelliot · 2023-06-09T23:02:12Z

(Holding this for 1.12 unless someone requests otherwise)

intelliot · 2023-06-26T19:29:30Z

This has CI failures which need to be resolved (or explained). @ckeshava or @thejohnfreeman - could you have a look?

thejohnfreeman · 2023-06-26T19:59:24Z

Please merge develop.

- Use powers of two to clearly indicate the bitmask - Replace bitmask with explicit if-conditions to better indicate predicates

ckeshava · 2023-06-26T21:23:15Z

OK, I've rebased to the develop branch, thanks for the tip!

intelliot · 2023-06-28T00:46:41Z

@HowardHinnant because there have been changes pushed after your earlier approval, could you do a quick re-review?

This reverts commit 1c2ae10.

- Use powers of two to clearly indicate the bitmask - Replace bitmask with explicit if-conditions to better indicate predicates Change enum values to be powers of two (fix XRPLF#3417) XRPLF#4239 Implement the simplified condition evaluation removes the complex bitwise and(&) operator Implement the second proposed solution in Nik Bougalis's comment - Software does not distinguish between different Conditions (Version: 1.5) XRPLF#3417 (comment) I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.

intelliot changed the title ~~Implement changes for Issue 3417~~ Change enum values to be powers of two (fix #3417) Mar 14, 2023

intelliot requested review from ximinez and HowardHinnant March 14, 2023 01:05

intelliot added the Tech Debt Non-urgent improvements label Mar 14, 2023

HowardHinnant approved these changes Mar 16, 2023

View reviewed changes

intelliot assigned ckeshava Mar 17, 2023

greg7mdp reviewed Apr 25, 2023

View reviewed changes

ximinez approved these changes Jun 9, 2023

View reviewed changes

intelliot added this to the 1.12 milestone Jun 9, 2023

fix: Update Handler::Condition enum values XRPLF#3417

248e2f0

- Use powers of two to clearly indicate the bitmask - Replace bitmask with explicit if-conditions to better indicate predicates

ckeshava force-pushed the issue3417 branch from 3274b2b to 248e2f0 Compare June 26, 2023 21:22

intelliot requested review from thejohnfreeman and HowardHinnant June 28, 2023 00:46

HowardHinnant approved these changes Jun 29, 2023

View reviewed changes

Merge branch 'develop' into issue3417

a0ffa0a

intelliot merged commit 1c2ae10 into XRPLF:develop Jun 29, 2023

intelliot added a commit that referenced this pull request Jun 29, 2023

Revert "fix: Update Handler::Condition enum values #3417 (#4239)"

de85454

This reverts commit 1c2ae10.

intelliot added a commit that referenced this pull request Jun 30, 2023

Revert "fix: Update Handler::Condition enum values #3417 (#4239)"

c2250d3

This reverts commit 1c2ae10.

intelliot mentioned this pull request Jun 30, 2023

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

Closed

ximinez mentioned this pull request Jul 5, 2023

Some RPC operations can't be executed when they should #4601

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ckeshava commented Jul 15, 2022

ckeshava commented Jul 15, 2022

greg7mdp commented Jul 15, 2022

ckeshava commented Jul 15, 2022

greg7mdp commented Jul 16, 2022

ckeshava commented Jul 19, 2022

HowardHinnant Mar 16, 2023

greg7mdp Mar 16, 2023

intelliot commented Apr 24, 2023

ckeshava commented Apr 25, 2023

greg7mdp Apr 25, 2023

greg7mdp Apr 25, 2023

ckeshava Apr 25, 2023

ximinez Jun 9, 2023

ximinez left a comment

ximinez Jun 9, 2023

intelliot commented Jun 9, 2023

intelliot commented Jun 26, 2023

thejohnfreeman commented Jun 26, 2023

ckeshava commented Jun 26, 2023

intelliot commented Jun 28, 2023

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

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

Conversation

ckeshava commented Jul 15, 2022

ckeshava commented Jul 15, 2022

greg7mdp commented Jul 15, 2022

ckeshava commented Jul 15, 2022

greg7mdp commented Jul 16, 2022

ckeshava commented Jul 19, 2022

HowardHinnant Mar 16, 2023

Choose a reason for hiding this comment

greg7mdp Mar 16, 2023

Choose a reason for hiding this comment

intelliot commented Apr 24, 2023

ckeshava commented Apr 25, 2023

greg7mdp Apr 25, 2023

Choose a reason for hiding this comment

greg7mdp Apr 25, 2023

Choose a reason for hiding this comment

ckeshava Apr 25, 2023

Choose a reason for hiding this comment

ximinez Jun 9, 2023

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

ximinez Jun 9, 2023

Choose a reason for hiding this comment

intelliot commented Jun 9, 2023

intelliot commented Jun 26, 2023

thejohnfreeman commented Jun 26, 2023

ckeshava commented Jun 26, 2023

intelliot commented Jun 28, 2023