Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature gating #5135

Closed
brianp opened this issue Jan 23, 2023 · 2 comments
Closed

Feature gating #5135

brianp opened this issue Jan 23, 2023 · 2 comments
Assignees
Labels
A-base_node Area - The Tari base node executable and libraries C-enhancement Category - New feature or request

Comments

@brianp
Copy link
Contributor

brianp commented Jan 23, 2023

Problem

Once mainnet is live we want to ensure the safe development and testing of features occur on the appropriate networks, and that we prevent the use of these features on the wrong network.

Solution 1

Define features

In build.rs, we define our features.

Features have a

  • name - will also be the attribute name you use in code
  • description - a short description of the features
  • issue tracking number on github
  • The current status.

Status

  • New: New feature, may not even be working or compiling. Will be present on dibbler testnet
  • Testing: Potentially active feature, but still gated. Potentially present on nextnet,
  • Active: Feature is live and gate has been removed. Will be running on stagenet and mainnet
  • Removed: Feature has been cancelled. Can not be invoked anywhere

In build.rs, we maintain the list of feature flags. For example:

const FEATURE_LIST: [Feature; 4] = [
    Feature::new("add_pair", "Allows you to add two numbers together", Some(123), Status::New),
    Feature::new("about_to_launch", "Live in NextNet. If we stabilise, will go to mainnet", Some(123), Status::Testing),
    Feature::new("live_feature", "This feature has been stabilised and is live!", Some(150), Status::Active),
    Feature::new("old_feature", "We decided not to go with this featuree", Some(145), Status::Removed),
];

Demarcate feature flag code

In your code, you can now use any of the flags as attributes to mark code belonging to a feature.

Example

#[tari_feature(add_pair)]
use add_pair::add_pair;

fn main() {
    println!("Hello world!");
    #[tari_feature(add_pair)]
    println!("40 + 2 = {}", add_pair(40, 2));
    println!("foo={}", foo());
    println!("Bye, world!");
}

#[tari_feature(add_pair)]
fn foo() -> usize {
    1
}

#[tari_feature(!add_pair)]
fn foo() -> usize {
    2
}

Specify the target network when building

This PoC uses the TARI_NETWORK envar to specify the target network, but in principle, we can also read the Cargo.toml
file, or compiler flags.

$ TARI_NETWORK=dibbler cargo run -vv

Filtered output:

Building for Dibbler
These features are ACTIVE on mainnet (no special code handling is done)
live_feature. This feature has been stabilised and is live!. Tracking issue: https://github.com/tari-project/tari/issues/150

These features are DEPRECATED and will never be compiled
old_feature. We decided not to go with this featuree. Tracking issue: https://github.com/tari-project/tari/issues/145

** Activating add_pair. Allows you to add two numbers together. Tracking issue: https://github.com/tari-project/tari/issues/123 **
** Activating about_to_launch. Live in NextNet. If we stabilise, will go to mainnet. Tracking issue: https://github.com/tari-project/tari/issues/123 **

Finished dev [unoptimized + debuginfo] target(s) in 7.44s
     Running `target/debug/feature_gates`
Hello world!
40 + 2 = 42
foo=1
Bye, world!

Or compiling for MainNet:

$ TARI_NETWORK=mainnet cargo run -vv

Filtered output:

Building for MainNet

These features are ACTIVE on mainnet (no special code handling is done)
live_feature. This feature has been stabilised and is live!. Tracking issue: https://github.com/tari-project/tari/issues/150

These features are DEPRECATED and will never be compiled
old_feature. We decided not to go with this featuree. Tracking issue: https://github.com/tari-project/tari/issues/145
    Finished dev [unoptimized + debuginfo] target(s) in 6.15s
     Running `target/debug/feature_gates`
Hello world!
foo=2
Bye, world!

Solution 2

Define features, and demarcate code the same as Solution 1.
Perform feature authorization checks at runtime based on the network in use.

Rabbit Holes

All crates across the repo require the feature gating functionality and access to the current feature list.

No Gos

Gating the configuration settings of other networks. For example, a mainnet binary may still know about the settings for testnets.
...

@brianp brianp self-assigned this Jan 23, 2023
@brianp
Copy link
Contributor Author

brianp commented Jan 23, 2023

I've been working on a PR for solution 1 as originally proposed but the more I look at it the more I dislike it.
It feels like we're creating a lot of extra work for ourselves in compilation management and distribution. Providing different bins for each application based on the network they would run on. Where currently we offer the ability to quickly switch the network based on config, or startup flags. Is the eventual plan to prevent the switching of networks and force the network based on the bin?
I can see this causing issues for us in other apps, like Aurora, and Launchpad. They have the ability to switch networks but their libraries would be compiled to a specific feature set.

Flag checking at runtime would introduce a minor performance hit but offer far more flexibility for all other applications.

brianp added a commit to brianp/tari that referenced this issue Feb 7, 2023
This PR is for reflective purposes regarding feature gating at compile time. See tari-project#5135 for more info. This PR as it stands is to be used as an example of a network _type_ based feature gate at compile time.
It allows setting an ENVVAR for the network with conditional compilation for features. The feature sets can be imported across any crate in the project easily and then used with the `#[cfg(tari_feature_...)]` attribute macro.

I played with a few styles of gating code. In the case of burning, and validator registration what I found was it became easier to focus on the entry and exit points of the code. These functions don't have much outside effect other than their distinct purpose so preventing someone from calling them is good enough conditional compilation. It's easier than trying to remove every aspect of their existence throughout. This wouldn't always be the case though- with a feature that changes an existing feature the style of code writing has the potential to change heavily with possible duplication of entire enums or structs. It could be useful to have more examples to help guide people with, but also is something we'll probably see patterns for emerge naturally. The nice part is we have the flexibility to support different needs.

There was a desire to use the network based feature gating to prevent a "mainnet" compiled bin from compiling with testnet configuration at all. To remove conensus constants, or even the knowledge of the testnets. This proved more difficult with the current code base and would require a lot of refactoring. I've made another PR with a branch I consider broken demonstrating the minimal amount of changes needed to perform this kind of task and it doesn't seem to bring us a major benefit for the time being.

---------

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
brianp added a commit that referenced this issue Feb 7, 2023
Description
---
This PR is for reflective purposes regarding feature gating at compile time. See #5135 for more info. This PR as it stands is to be used as an example of a network _type_ based feature gate at compile time. 
It allows setting an ENVVAR for the network with conditional compilation for features. The feature sets can be imported across any crate in the project easily and then used with the `#[cfg(tari_feature_...)]` attribute macro. 

I played with a few styles of gating code. In the case of burning, and validator registration what I found was it became easier to focus on the entry and exit points of the code. These functions don't have much outside effect other than their distinct purpose so preventing someone from calling them is good enough conditional compilation. It's easier than trying to remove every aspect of their existence throughout. This wouldn't always be the case though- with a feature that changes an existing feature the style of code writing has the potential to change heavily with possible duplication of entire enums or structs. It could be useful to have more examples to help guide people with, but also is something we'll probably see patterns for emerge naturally. The nice part is we have the flexibility to support different needs.

There was a desire to use the network based feature gating to prevent a "mainnet" compiled bin from compiling with testnet configuration at all. To remove conensus constants, or even the knowledge of the testnets. This proved more difficult with the current code base and would require a lot of refactoring. I've made another PR with a branch I consider broken demonstrating the minimal amount of changes needed to perform this kind of task and it doesn't seem to bring us a major benefit for the time being.

---------


Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
@SWvheerden SWvheerden added C-enhancement Category - New feature or request A-base_node Area - The Tari base node executable and libraries labels Mar 13, 2023
brianp added a commit to brianp/tari that referenced this issue Apr 4, 2023
Description
---
This PR is for reflective purposes regarding feature gating at compile time. See tari-project#5135 for more info. This PR as it stands is to be used as an example of a network _type_ based feature gate at compile time.
It allows setting an ENVVAR for the network with conditional compilation for features. The feature sets can be imported across any crate in the project easily and then used with the `#[cfg(tari_feature_...)]` attribute macro.

I played with a few styles of gating code. In the case of burning, and validator registration what I found was it became easier to focus on the entry and exit points of the code. These functions don't have much outside effect other than their distinct purpose so preventing someone from calling them is good enough conditional compilation. It's easier than trying to remove every aspect of their existence throughout. This wouldn't always be the case though- with a feature that changes an existing feature the style of code writing has the potential to change heavily with possible duplication of entire enums or structs. It could be useful to have more examples to help guide people with, but also is something we'll probably see patterns for emerge naturally. The nice part is we have the flexibility to support different needs.

There was a desire to use the network based feature gating to prevent a "mainnet" compiled bin from compiling with testnet configuration at all. To remove conensus constants, or even the knowledge of the testnets. This proved more difficult with the current code base and would require a lot of refactoring. I've made another PR with a branch I consider broken demonstrating the minimal amount of changes needed to perform this kind of task and it doesn't seem to bring us a major benefit for the time being.

---------

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
@brianp brianp mentioned this issue Apr 4, 2023
1 task
brianp added a commit to brianp/tari that referenced this issue Apr 4, 2023
Description
---
This PR is for reflective purposes regarding feature gating at compile time. See tari-project#5135 for more info. This PR as it stands is to be used as an example of a network _type_ based feature gate at compile time.
It allows setting an ENVVAR for the network with conditional compilation for features. The feature sets can be imported across any crate in the project easily and then used with the `#[cfg(tari_feature_...)]` attribute macro.

I played with a few styles of gating code. In the case of burning, and validator registration what I found was it became easier to focus on the entry and exit points of the code. These functions don't have much outside effect other than their distinct purpose so preventing someone from calling them is good enough conditional compilation. It's easier than trying to remove every aspect of their existence throughout. This wouldn't always be the case though- with a feature that changes an existing feature the style of code writing has the potential to change heavily with possible duplication of entire enums or structs. It could be useful to have more examples to help guide people with, but also is something we'll probably see patterns for emerge naturally. The nice part is we have the flexibility to support different needs.

There was a desire to use the network based feature gating to prevent a "mainnet" compiled bin from compiling with testnet configuration at all. To remove conensus constants, or even the knowledge of the testnets. This proved more difficult with the current code base and would require a lot of refactoring. I've made another PR with a branch I consider broken demonstrating the minimal amount of changes needed to perform this kind of task and it doesn't seem to bring us a major benefit for the time being.

---------

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
brianp added a commit to brianp/tari that referenced this issue Apr 11, 2023
Description
---
This PR is for reflective purposes regarding feature gating at compile time. See tari-project#5135 for more info. This PR as it stands is to be used as an example of a network _type_ based feature gate at compile time.
It allows setting an ENVVAR for the network with conditional compilation for features. The feature sets can be imported across any crate in the project easily and then used with the `#[cfg(tari_feature_...)]` attribute macro.

I played with a few styles of gating code. In the case of burning, and validator registration what I found was it became easier to focus on the entry and exit points of the code. These functions don't have much outside effect other than their distinct purpose so preventing someone from calling them is good enough conditional compilation. It's easier than trying to remove every aspect of their existence throughout. This wouldn't always be the case though- with a feature that changes an existing feature the style of code writing has the potential to change heavily with possible duplication of entire enums or structs. It could be useful to have more examples to help guide people with, but also is something we'll probably see patterns for emerge naturally. The nice part is we have the flexibility to support different needs.

There was a desire to use the network based feature gating to prevent a "mainnet" compiled bin from compiling with testnet configuration at all. To remove conensus constants, or even the knowledge of the testnets. This proved more difficult with the current code base and would require a lot of refactoring. I've made another PR with a branch I consider broken demonstrating the minimal amount of changes needed to perform this kind of task and it doesn't seem to bring us a major benefit for the time being.

---------

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
SWvheerden added a commit that referenced this issue Apr 12, 2023
Description
---
This PR is for reflective purposes regarding feature gating at compile
time. See #5135 for more
info. This PR as it stands is to be used as an example of a network
_type_ based feature gate at compile time.
It allows setting an ENVVAR for the network with conditional compilation
for features. The feature sets can be imported across any crate in the
project easily and then used with the `#[cfg(tari_feature_...)]`
attribute macro.

I've also introduced a secondary commit for binaries to perform a quick
network check against itself to ensure the intended network is supported
by the binary. This check is made non-invasive (not conditionally
compiling possible network) as that method of check ends up being much
more tedious.

Motivation and Context
---
We want some features on some networks, but maybe not all.

How Has This Been Tested?
---
Manually

What process can a PR reviewer use to test or verify this change?
---
Compile a binary with a set network
`TARI_NETWORK=nextnet cargo build --release --bin tari_base_node`

then run the binary with a test network as a parameter:

`./tari_base_node --network esme` 

and receive an error:

```
The application exited because of an internal network error. The network esmeralda is invalid for this binary built for NextNet
```

*Please note*

That running the binary will cause the build to default to the test
network _always_. Meaning this will not work:

```
$ TARI_NETWORK=nextnet cargo build --release --bin tari_base_node
$ cargo run --release --bin tari_base_node --network nextnet
```

The second command `cargo run` will re-build the binary, and without the
`TARI_NETWORK` env set the binary will default to a test binary.

In development if you want to call run and connect to a non test network
you need to also pass the TARI_NETWORK envvar instead of the `--network`
flag.
`TARI_NETWORK=nextnet cargo run --bin tari_base_node --release --
--network nextnet`


Breaking Changes
---

- [x] None

---------

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
@brianp
Copy link
Contributor Author

brianp commented Apr 12, 2023

Closed with #5287

@brianp brianp closed this as completed Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-base_node Area - The Tari base node executable and libraries C-enhancement Category - New feature or request
Projects
Status: Done
Development

No branches or pull requests

2 participants