Move config module out from tendermint into its own crate

[soareschen]

Part of informalsystems/hermes#1158.

With #980 there are still a few remaining use of std in the tendermint crate, which is mostly consist of filesystem and network operations to retrieve the config files. By moving tendermint::config, tendermint::net and their submodules to a separate crate, we are one step closer to no_std support for the tendermint crate.

[thanethomson]

I totally agree on the tendermint::config module, but wouldn't it make more sense to keep tendermint::net where it is and feature-guard any functionality that needs std? (right now that's just the URL parsing and the std::fmt::Display trait impl, right?)

[soareschen]

I totally agree on the tendermint::config module, but wouldn't it make more sense to keep tendermint::net where it is and feature-guard any functionality that needs std? (right now that's just the URL parsing and the std::fmt::Display trait impl, right?)

From my search I see that the only place where tendermint::net is used is with tendermint::config, so I think it makes sense to move them together. Currently the only std dependency in tendermint::net is std::path::PathBuf for the Unix socket path in the Address struct. An alternative is that we set the field type to String.

Nevertheless, I think it would be better to keep the tendermint crate as a pure implementation, and not have it mixed with std/OS-specific features like networking. I also feel that we should be more refrain from using cfg feature guards, as that has make it very difficult to maintain and test the crate with all possible feature flags combination.

The Display trait is still available as core::fmt::Display.

[seanchen1991]

It seems like net::Address is also used in tendermint::node:

tendermint-rs/tendermint/src/node/info.rs

Line 85 in 4a6c450

pub rpc_address: net::Address,

That being said, I would tend to agree with favoring migration over adding feature guards.

[soareschen]

It seems like net::Address is also used in tendermint::node

Hmm in that case perhaps using alloc::string::String in place std::path::PathBuf might be the simplest workaround.

[soareschen]

One thing leads to another: I just discovered that tendermint::net is also using the url crate, which does not work in no_std. I have filed a separate issue for this: #985.

[seanchen1991]

Both config and net import crate::{error::Error, node};, which produces a circular dependency when they include tendermint as a dependency and vice versa. How should this be dealt with?

[soareschen]

Both config and net import crate::{error::Error, node};, which produces a circular dependency when they include tendermint as a dependency and vice versa. How should this be dealt with?

tendermint::config can have its own error type. Not sure about node, we might have to find some way to deal with it together with net. We should make it such that the new config crate depends on the main crate, but not the other way round.

[thanethomson]

What's the major difference between isolating these modules into separate crates vs just feature-guarding them?

The latter seems like less work to me 🙂

[tony-iqlusion]

@thanethomson using features just requires more rigorous testing of various feature combinations in CI

At the very least the crate should be build with each feature in isolation to ensure that works, as well as testing various feature combinations that are known to interact with each other. The latter is the hardest part, as there can be a sort of explosion of combination of features to test.

That said, I'm a fan of fine-grained features, and don't think it's an insurmountable problem. I'd suggest getting a basic setup, and then adding combinations of features to test to CI as a sort of regression-ish test whenever you find an incompatible combination.

Isolating code in crates is a bit easier in that regard, as it's self-contained by definition.

[tony-iqlusion]

I suppose I should also note that TMKMS is a user of tendermint::net::Address, although our usages are trivial and easily moved directly to Url.

[soareschen]

What's the major difference between isolating these modules into separate crates vs just feature-guarding them?

Feature flags are like weaker version of macros, templates, metaprograms or similar program generation methods that generates different version of the code depending on input boolean values. You can think of it as a metaprogram with a signature like:

fn generate_program(feature_a: bool, feature_b: bool, ...) -> Code

The problem is when you have multiple feature flags in a crate, there can be exponential different versions of the code that can be generated. For example with just 4 feature flags, there are technically 2^4 = 16 different versions of your program that needs to be tested. If you combine this with the feature flags of the dependencies, there are just unmanageable number of combinations of programs that need to be tested, even just for passing the type-checking phase. Furthermore, the design of feature flags makes it not possible to "instantiate" multiple versions of the program to co-exist at the same time for testing or different purposes.

The latter seems like less work to me

I feel that feature flags only bring convenience in the short term, but incurs a high cost in the long term for the additional effort required for maintaining the code. A key challenge I have been facing with making changes to tendermint-rs is with managing the feature flags that it currently already have. I would make some changes that seem to pass the compiler, and it is only when submitted to CI then only realize that there are code path that are not covered because the features are disabled by default locally. The unnecessary complexity force me to be dependent on the CI and much longer compilation feedback loop just to find out whether my changes compiles in all possible situation or not.

I feel that a better way of managing compile-time dependencies is by making use of the type system, in particular traits, to help ensure that a program can work generically over all possible compile-time input. An example you can refer to is the design I have for the ErrorTracer trait in flex-error. Rather than having feature flags in flex-error to behave differently, flex-error has generic functions that work for all types that implements ErrorTracer. It is only at the top level that I define DefaultTracer as a type that is expected to implement ErrorTracer, which can either be eyre::Report, anyhow::Error, or StringTracer. With the design, I know that if I ever want to add a new error tracer type in the future, the type only needs to implement ErrorTracer, rather than having to search through the entire code base and modify the individual functions that behave differently depending on the feature flags.

[thanethomson]

I feel that a better way of managing compile-time dependencies is by making use of the type system

How would we apply this to the case of tendermint::net, which depends on url for parsing net::Addresses, but net::Address is used by some of the other domain types?

My sense is that part of the equation in determining whether or not we can even break a module out into a separate crate is the dependency graph. So a module can be broken out if and only if nothing else in the existing crate depends on it. This means that breaking tendermint::config out into its own crate seems okay, but not tendermint::net.

[soareschen]

How would we apply this to the case of tendermint::net, which depends on url for parsing net::Addresses, but net::Address is used by some of the other domain types?

I have taken a look at the use of net::Address, and it does not seem like the actual details are used anywhere. In #986 I moved out tendermint::net to tendermint_config::net, and changed the field in tendermint::node::OtherInfo::rpc_address to just String. If anyone needs to do the actual URL parsing, they can do it at the call site instead.

The main difference I see of using String instead of Address for rpc_address is the way it is deserialized. With the Deserialize implementation provided by Address, an invalid URL would result in an error. However I also do not see how this can be solved using feature flag. In the absense of the url crate, should the deserialization just succeed? That wouldn't be good since the crate would effectively be behaving differently with no invariant in place. Otherwise if it is possible to implement the parsing without using url, then it is better off to just not url in both cases in the first place.

Note that the other alternative is to just wait for servo/rust-url#717 to get merged and released.