# Proposal This section contains what I propose to do and for which I'm asking for team consensus. See also `ProposingBigChanges.md`. This ticket is intended to be guided by, and answer, the questions posed there, but it is not structured as a series of responses to that checklist. ## Proposal - changes to tor-netdoc In tor-netdoc: 1. Unconditionally enable code behind, and abolish, the following cargo features: 1. `encode` - derive-based document encoder (not the same as "builder") (already stable) 2. Parts of `parse2` - derive-based parser and impls on document types (currently experimental) 3. `plain-consensus` `routerdesc` - document types (already stable) 4. `hsdesc-inner-docs` - visibility of pieces of HS descriptors (currently experimental) 2. Coalesce the following cargo feature into new (still experimental) `incomplete` feature: 1. `ns-vote` - votes, whose representation is currently incomplete and broken 2. Parts of `parse2` - incomplete document types, and incomplete impls 3. Retain the following cargo features: 1. `hs-pow-full` - document fields that bring in the pow support crates 2. `retain-unknown` - support for retaining not-understood elements in netstatuses 3. `experimental-api` - some accessors/mutators on `UnvalidatedConsensus` 4. `hs-dir` - nascent types from 2022 for hs-dir support, will probably be reworked We will retain the abolished stable feature names in `Cargo.toml` for a while for compatibility with downstreams. The abolished experimental features will be simply deleted. Additionally, for development convenience reasons these changes will have to be phased in; see below. ## Proposal - future process for moving things out of `incomplete` From now on in tor-netdoc we will ungate `incomplete`-gated items and impls as part of normal code review. The MR author and reviewer will be expected to check: * That the things being declared complete are covered by appropriate tests. * That there are no outstanding TODOs. * For new code, that it is correct, secure, and conforms to the spec; or that it has TODO describing its deficiencies. * When removing a TODO, that the problem named in the TODO has been fixed. I think this is our normal approach to code development and review. (CI will fail if non-`incomplete` things use, in the Rust source code, `incomplete` ones.) # Rationale ## Rationale - "unused code" `CargoFeatures.md` mentions the following possible reason for geature gating: > - The feature involves a substantial amount of code, > and is something that many users won't want. The biggest use case here is clients, which need different code to servers, and which want to be able to operate in more resource-constrained environments. But almost the only benefit in such cases is reduced compile times. We believe that unused code will be removed (or almost entirely removed) by the toolchain. Conversely, the fine-grained feature control of individual types causes a great deal of development friction. Every item (even many `use`s) must be accurately decorated. Our current tests do not test every feature combination; indeed, there would be about 2^13 combinations! So right now there are many feature sets for which tor-netdoc does not build. This reasoning applies both to impls (eg, impls of the encoding traits, which are not generally needed by clients, and the `parse2` traits which the clients do not (yet) use) and to whole types (eg, votes, router descriptors). The exception is where the code being configured out includes unused-by-client *fields* in data structures that *are* used by clients. But we have very few of those. An alternative to unconditional compilation, would be to privilege the client use case, and make a single feature that enables all not-client-only types and facilities. Or, we could separate out some document types into their own crates. I propose that we do these things, in a directed way, if we discover we need it. ## Rationale - visibility, `hsdesc-inner-docs` The `hsdesc-inner-docs` feature exists only to gate exposure of some struct fields and types, for API stability reasons. Some months ago we exposed all netdoc fields as `pub`. This has resulted in more updates to `semver.md` but no actual breakage as far as I'm aware. My view is that it doesn't make much sense to use feature-gating for API stability reasons in a 0.x crate. ## Rationale - enabling `parse2` This feature covers, roughly, three kinds of thing: 1. Traits for parsing, and an associated derive-based parser. These are well-established now and can parse and encode several real document types. Much of the `parse2` derive is shared with `encode`. There are round trip tests for each derive macro feature. It is not impossible that the API will need to change, but I think the generated parsing and encoding code is ready for production, assuming the document types are properly annotated and have suitable tests. I am proposing to enable the traits and the derive macro. 2. impls of the traits on helper types, and, sometimes, bespoke helper types. Many of these are trivial, and/or are tested as part of existing tested documents. Some of the impls are prospective, and sometimes untested, in the sense that nothing that has landed actually uses the type, or the parts that have landed are themselves incomplete and lack tests. I am proposing that we enable those parts which are finished and tested. 3. impls of the traits on documents (typically, derived) There are some complete impls, for example `AuthCert`. But many impls are are not complete because we're still working on them. In some cases they are actually broken (eg, some fields in "vote" structs may not have been updated for differences between votes and consensuses). I am proposing that we enable those parts which are finished and tested. Parts that we don't enable now will remain gated by `experimental`, via `incomplete` rather than `parse2`. ## Rationale - document types These are feature-gated just because tor-netdoc contains code to support many document types that are not needed in every context - especially, manipulations not needed by clients. See "unused code". ## Rationale -- retained features 1. `hs-pow-full` changes the types of fields in documents used also by clients, and pulls in whole crates as dependencies pow. 2. `retain-unknown` changes the type of `RelayFlags` to include the string values of unknown flags. `RelayFlags` appears in every `RouterStatus`. For clients that is is undesirable bloat. 3. `experimental-api` is a couple methods that may not be used, so the feature may not be useful, but the feature's impact on our development is minimal so I propose not to tidy this up now. That avoids having to make any decision about the future of those methods. 4. Likewise `hs-dir` covers a small amount of code of whose usefulness I am unsure. But it is easier to keep it than make a decision. # Implementation details - compatibility, and in-flight work This section is a detailed plan of how this can be done without causing undue disruption to ongoing work in tor-netdoc. Unless you're @cve you don't need to read this part. We have a lot of in-flight work (outstanding branches) which modify code which is decorated with these `cfg`s. 1. Phase 1 - framework Make all the document type features imply each other. Don't change their public docs. Introduce the `incomplete` feature. Deprecate `parse2` in favour of it. Make `parse2` and `incomplete` all imply each other. Make all the document type features, and `incomplete`, imply `encode`. 2. Phase 2 - change in-source-code uses Every `parse2` cfg gate should be either removed, or replaced with an `incomplete` gate. Every `ns-vote` feature should be replaced with `incomplete`. Every document type, `encode`, or `hsdesc-inner-docs` feature gate should be simply removed. We can start to do this to all in-flight work, as and when convenient. 3. Phase 3 - abolish the individual incomplete features; deprecate the old stable features. Remove the `parse2`, `ns-vote` and `hs-dir` features from `Cargo.toml`. Now any code which was in flight which uses that gate, will fail to compile, if we try to merge it later. It will have to be updated before merge. In particular this forces us to make an explicit decision about the status of anything that was previously gated `parse2`. Make the document types and accessors features, and `encode`, empty, and document them as deprecated. 4. Phase 4 - remove the now-obsolete stable features from `Cargo.toml` Remove all the now-unconditionally-stable features, and also some other obsolete features that are hanging around, eg `dangerous-expose-struct-fields`.