Skip to content
Snippets Groups Projects

PoW: Consolidate feature flags into a single flag.

Merged wesleyac requested to merge wesleyac/arti:hs-pow-feature-flag-consolidation into main
2 unresolved threads

This replaces the hs-pow, hs-pow-full, hs-pow-v1, pow-v1, and pow-full features with a single hs-pow-full feature.

It's possible that in the future we will want to split different schemes into different features, but we can do that when it comes up.

For now, having this as a single flag makes it clearer what's going on, since the previous thing was not actually expressive enough to capture some things we care about (like "at least one pow scheme is enabled" that works in a future-compatible way).

This change is not semver breaking since it's a experimental feature.

Related: #1751

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
17 17 ope = ["cipher", "zeroize"]
18 18
19 19 # Onion service proof of work schemes
20 pow-v1 = ["arrayvec", "blake2", "equix", "__is_experimental"]
21 pow-full = ["pow-v1", "__is_experimental"]
20 hs-pow-full = ["arrayvec", "blake2", "equix", "__is_experimental"]
  • Nit: I would omit the hs- prefix from the feature name in the tor-hs* crates.

    (but if you want the feature to have the same name in all crates for simplicity/consistency, I won't object)

  • Author Maintainer

    Yeah, I wanted the hs- on everything for consistency sake, I think it's nice to not have to check which context you're in to know which flag to use.

  • Please register or sign in to reply
  • gabi-250
    gabi-250 @gabi-250 started a thread on commit 7846ed24
  • 68 68 onion-service-client = ["arti-client/onion-service-client"]
    69 69 onion-service-service = ["arti-client/onion-service-service", "tor-hsrproxy", "tor-hsservice"]
    70 70 vanguards = ["arti-client/vanguards"]
    71 hs-pow = ["arti-client/hs-pow", "__is_experimental"]
    71 hs-pow-full = ["arti-client/hs-pow-full", "__is_experimental"]
    • Nit: to me, -full implies the existence of a "non-full" version of the feature (but I'm guessing it's called hs-pow-full for future-proofing reasons). Alternatively, why not call the feature hs-pow-v1? (since I think we currently only support the v1 scheme?)

    • Author Maintainer

      Yeah, this is for future-proofing reasons, although I would be perfectly happy with just hs-pow as well. The problem with hs-pow-v1 is that we can't properly express things like "any pow scheme is enabled" in a future-compatible way.

      I think it would also work to have:

      • hs-pow-full which enables hs-pow-v1 (and hs-pow-v2 or whatever in the future)
      • hs-pow-v1 which enables hs-pow-full

      Which I would be fine with as well, but I'm sort of inclined to just do the simple thing first.

    • Ack, sounds good to me! In that case, let's keep hs-pow-full.

    • Please register or sign in to reply
  • Looks good, but I think I'm missing some context on this MR. I think this change is a follow-up from !2609 (merged) requested by @Diziet (who perhaps might want to steal review here?).

  • gabi-250 approved this merge request

    approved this merge request

  • merged

  • gabi-250 mentioned in commit 42ea5222

    mentioned in commit 42ea5222

  • Please register or sign in to reply
    Loading