Beta-0.9.1 Feature Flag

[jessicadaugherty]

Objective

Allow pocket-core to reproduce the behavior of omitting rewards for a subset of nodes (from this point on "the undesired behavior") when the non-custodial flag was turned on, while also allowing pocket-core to have the behavior we want for "Non-Custodial".

There are several ways to achieve this; letting the original flag preserve its undesired behavior and creating a new flag for the desired behavior is a mechanism that allows preserving the history of the blockchain and allows for quick testing of both the undesired and desired behaviors which would be needed to synch the blockchain.

Goals / Deliverables

• [ ] Allowing the undesired behavior to happen when the existing flag is turned on
• [ ] Allowing the desired behavior to happen when a new flag is turned on
• [ ] Notify @emedrano2112 to re-test

Non-goals

• Disallowing the feature flag that causes the undesired behavior

Testing Methodology

• Confirm that turning on the original flag causes the undesired behavior
• Confirm that turning off the original flag stops the undesired behavior
• Confirm that turning on the new flag causes non-custodial to kick in
• Confirm that turning off the new flag causes non-custodial to stop

Creator: @jessicadaugherty

[iajrz]

Fleshing out the alternatives we have for the historical replay of the NC issue (part of this is documented here, I'm aiming to distill pros and cons here).

The alternatives to compare are these:

• We can do as it's done in #1476 and we've done it before (from now on Classical Style)
• We can create a flag for the desired behavior and allow the undesired behavior to have its own flag (from now on Proposed Style)

Classical Style Pros

• We have done this before, which implies
  • It's tried and true
  • Cases are handled homogeneously

Classical Style Cons

• We increase the amount of blocks we need to process in an isolated cluster in order to test the flag behaviors by 30%
• We're overloading behaviors, which means activating the flag in an isolated cluster has two different behaviors which are height dependent.
• It might require an increasing the load of the codec.TestMode variable to allow unit testing to verify the behavior in isolation

Proposed Style Pros

• We make it possible to test the NC issue behavior and the desired behavior at any height
  • it is self contained; no need to overload the codec.TestMode variable
• We isolate behaviors, which means that activating the flag in an isolated cluster has a specific behavior regardless of height, increasing predictability
• We know how to set it up: we've added flags before

Proposed Style Cons

• It diverges from behavior changes we've had to introduce previously for flags, introducing heterogeneity
• It requires the creation of a new flag

[Olshansk]

@iajrz Thank you for the clear outline.

tl;dr After reading this I left a comment and approved https://github.com/pokt-network/pocket-core/pull/1476#pullrequestreview-1117458974.

My opinion is, given:

1. We want to ship AC ASAP
2. The classic approach is slow/clumsy, but is simple, proven and tested.
3. Because of the urgency of (1) and the benefit of (2), I say we go with this approach.

This way, we won't be under duress and have the time to design a better approach (what you suggested in the proposed style) for future v0 changes, as well as transfer those learnings to v1.

[Olshansk]

@iajrz Also, if we ship this (clumsy and hard-to-test approach) and implement the other one in the future, would it not technically be a consensus-breaking change?

[iajrz]

Both are consensus breaking: both imply introducing a new behavior, the working-as-intended NC.

[Olshansk]

Yea, my question was moreso:

0. Current chain
1. Implement the "classic approach" -> consensus breaking relative to (0)
2. Implement the "new approach" -> is this still consensus-breaking relative to (1)?

[iajrz]

Ah, I understand. Yes, they are mutually incompatible. We can't swap out implementations of the fix.

[iajrz]

We have chosen the classic approach~! This issue can serve as a reference for the future if we ever need to amend the behavior of a network-wide flag in v0 or v1.

Thank you everybody for weighing in!

[Olshansk]

Thank you @iajrz for outlining the approaches & tradeoffs so clearly!