Nightly protocol features: compile time feature vs. runtime feature

[bowenwang1996]

In near/NEPs#126, we discussed the use of rust compile time features to gate protocol changes that we would like to take more time to test and stabilize. Since then, we have moved to implementing all new protocol features behind rust features and only compile them when we enable nightly_protocol_features. After several months of working with this model, we have collected some feedback from developers who actively work on nearcore. While such paradigm serves its purpose of isolating new protocol features and allowing more flexibility in terms of releases, there are also some complaints about it:

• Threading compile-time features across the codebase makes code harder to read. Since protocol changes sometimes can be quite invasive, they often lead to the new protocol feature being added to multiple crates and in some cases, the public API of a crate is different depends on the feature and this has caused some confusions and make the code generally harder to understand.
• It is harder to develop with compile-time features. Even though we have developed macros like checked_feature to reduce the amount of boilerplate code, it is still not very ergonomic to use. In addition, it does not always work well with IDEs.
• Testing becomes harder. On a couple of occasions I have to spend more than 30 minutes to figure out why some test does not run or does not behave as expected when I want to test the new feature. It is often caused by some incorrect or missing feature dependency between crates. This can be difficult and annoying to debug.

As @matklad mentioned in his comment, Rust compiler itself uses mostly runtime features instead of compile-time features and in the same comment, @matklad proposes to change to feature gating at runtime to reduce the complexity of the code and make it easier to implement new features. I would like to revive this discussion again in this thread. Please feel free to share your thoughts here!

[joshuajbouw]

I believe an issue with runtime features would be when we add public fields to structs we do still introduce a backward incompatible change that can't -exactly- be fixed by that. I would be interested in a solution for that. Working on the EVM protocol features now, we have ran into that issue already. However generally, I do like that approach as proposed by @matklad.

[frol]

I want to second @joshuajbouw. With compile-time features, we can merge PRs into master and iterate there while not breaking the stable code and avoiding merge conflicts. nearcore is a network, not a standalone isolated tool like Rust compiler. It seems that runtime features will require quite some effort to land (increasing the number of merge conflicts one need to resolve while it is in development)

[frol]

Threading compile-time features across the codebase makes code harder to read.

It seems to be orthogonal to runtime vs compile-time. I would argue that runtime features will require even more threading.

Even though we have developed macros like checked_feature to reduce the amount of boilerplate code, it is still not very ergonomic to use. In addition, it does not always work well with IDEs.

We will still need helpers to check if a given feature is enabled, so the ergonomics and IDE support is something that we may want to address separately.

Testing becomes harder.

I want us to keep in mind that "harder" is relative, and we seem to compare to the state where we did not have the features in the first place. Indeed, it has fewer knobs and whistles when there are no features in play, but I think it is going to be the same complexity either we have compile-time or runtime features. Though, Rust saves us with its compilation errors when we use compile-time features, so 1:0 to compile-time features in my opinion.

[bowenwang1996]

Though, Rust saves us with its compilation errors when we use compile-time features, so 1:0 to compile-time features in my opinion.

I don't think this is what is at play here. With compile-time features it is somewhat difficult to directly see whether a feature is enabled or not. I sometimes fail to understand very quickly why some tests don't run or expose unexpected behavior and I feel like if we have some global Feature that is constructed once (similar to what Alex proposed), it may be easier to understand what happened.

[joshuajbouw]

Yeah a sort of hybrid approach makes perfect sense. Then we can still feature gate new public fields that are not backwards compatible and have a Feature global struct which can give more information, 100% for that.

[matklad]

It just occurred to me that there's a third choice besides "use run-time detection" and "use cargo-features". We can be using non-features cfg flags. Ie, something like #[cfg(protocol_feature = "foo")]. Then, we can use something to set such cfg flags globally.

The problem here is that I don't know of a nice way to set such flags from CLI. Some options are:

• require to explicitly set env variable: RUSTFLAGS="--cfg=protocol_feature=foo,protocol_feature=bar" cargo build
• put this into an alternative .cargo/config to use when building nightlies
• use RUSTC_WRAPPER to wrap rustc entirely with a custom logic to inject the flags.

Don't see an option which would be clearly better than today's situation though.

[bowenwang1996]

The problem here is that I don't know of a nice way to set such flags from CLI. Some options are:

Also this puts more burden on validators and I imagine that it will create a lot of confusion and potentially mistakes, which could lead to disastrous consequences, especially after slashing is enabled.

[matklad]

I am not sure this particular concern is valid: the way I imagine, you can only get stable features by default, and you'd have to go out of your way to enable nightly features. It becomes harder than just passing --features nightly_protocol_features.

[bowenwang1996]

Ah I see. I think I misunderstood your point