feat: feature gates #5287

brianp · 2023-04-04T13:28:29Z

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

None

CjS77

Looks good. Some minor readability and maintenance suggestions

applications/tari_app_utilities/src/network_check.rs

CjS77 · 2023-04-04T13:53:31Z

common/tari_features/src/lib.rs

+
+pub use feature::Feature;
+pub use status::Status;
+


Some docs here on when / how / why to maintain this list would be highly useful.

SWvheerden

This looks good
The only thing I would add is a default network, so if the user does not select a network, it will default to mainnet, or perhaps development as building from dev that's the most likely? But this is open to discussion

common/tari_features/Cargo.toml

applications/tari_app_utilities/Cargo.toml

applications/tari_app_utilities/src/network_check.rs

common/tari_features/src/lib.rs

SWvheerden

We need to update the readme with all this as well.

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>

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>

SWvheerden · 2023-04-12T04:15:10Z

Ack

## [0.50.0-pre.0](v0.49.0-pre.6...v0.50.0-pre.0) (2023-04-12) ### ⚠ BREAKING CHANGES * add paging to utxo stream request (5302) ### Features * add extended mask recovery ([5301](#5301)) ([23d882e](23d882e)) * add network name to data path and --network flag to the miners ([5291](#5291)) ([1f04beb](1f04beb)) * add other code template types ([5242](#5242)) ([93e5e85](93e5e85)) * add paging to utxo stream request ([5302](#5302)) ([3540309](3540309)) * add wallet daemon config ([5311](#5311)) ([30419cf](30419cf)) * define different network defaults for bins ([5307](#5307)) ([2f5d498](2f5d498)) * feature gates ([5287](#5287)) ([72c19dc](72c19dc)) * fix rpc transaction conversion ([5304](#5304)) ([344040a](344040a))

brianp force-pushed the feature-tari-gating branch from 564914a to 0b843e7 Compare April 4, 2023 13:32

brianp requested a review from SWvheerden April 4, 2023 13:33

CjS77 reviewed Apr 4, 2023

View reviewed changes

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Apr 4, 2023

CjS77 added the P-merge Process - Queued for merging label Apr 4, 2023

SWvheerden reviewed Apr 5, 2023

View reviewed changes

brianp and others added 6 commits April 11, 2023 15:08

Add check to ensure the network matched the binary

3b9b63f

Update applications/tari_app_utilities/src/network_check.rs

4042b16

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>

Change crate version

34426a3

Remove commented example in favour of not yet existing docs

9ffed0e

Remember to update the version number

926706c

brianp force-pushed the feature-tari-gating branch from a3a238f to 926706c Compare April 11, 2023 13:22

SWvheerden approved these changes Apr 11, 2023

View reviewed changes

ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Apr 11, 2023

Merge branch 'development' into feature-tari-gating

6f914a1

SWvheerden merged commit 72c19dc into tari-project:development Apr 12, 2023
9 checks passed

brianp deleted the feature-tari-gating branch April 12, 2023 05:16

brianp mentioned this pull request Apr 12, 2023

Feature gating #5135

Closed

SWvheerden mentioned this pull request Apr 12, 2023

chore(version): v0.50.0-pre.0 #5314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: feature gates #5287

feat: feature gates #5287

brianp commented Apr 4, 2023

CjS77 left a comment •

edited

CjS77 Apr 4, 2023

SWvheerden left a comment •

edited

SWvheerden left a comment

SWvheerden commented Apr 12, 2023

feat: feature gates #5287

feat: feature gates #5287

Conversation

brianp commented Apr 4, 2023

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

CjS77 left a comment • edited

Choose a reason for hiding this comment

CjS77 Apr 4, 2023

Choose a reason for hiding this comment

SWvheerden left a comment • edited

Choose a reason for hiding this comment

SWvheerden left a comment

Choose a reason for hiding this comment

SWvheerden commented Apr 12, 2023

CjS77 left a comment •

edited

SWvheerden left a comment •

edited