refactor: Clarify and disable unused ArgsManager flags #22766

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/argscripts changing 9 files +85 −76
  1. ryanofsky commented at 8:20 pm on August 21, 2021: member

    This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.

    Currently, ALLOW_{INT|BOOL|STRING} flags don’t do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).

    An additional complication is that while these flags don’t do any real settings validation, they do affect whether setting negation syntax is allowed.

    Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.

    None of the changes affect behavior in any way.

  2. DrahtBot added the label Consensus on Aug 21, 2021
  3. DrahtBot added the label GUI on Aug 21, 2021
  4. DrahtBot added the label Mining on Aug 21, 2021
  5. DrahtBot added the label P2P on Aug 21, 2021
  6. DrahtBot added the label Refactoring on Aug 21, 2021
  7. DrahtBot added the label RPC/REST/ZMQ on Aug 21, 2021
  8. DrahtBot added the label Utils/log/libs on Aug 21, 2021
  9. DrahtBot added the label UTXO Db and Indexes on Aug 21, 2021
  10. DrahtBot added the label Validation on Aug 21, 2021
  11. DrahtBot added the label Wallet on Aug 21, 2021
  12. DrahtBot commented at 9:51 pm on August 21, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22834 (net: respect -onlynet= when making outbound connections by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  13. MarcoFalke removed the label GUI on Aug 22, 2021
  14. MarcoFalke removed the label Wallet on Aug 22, 2021
  15. MarcoFalke removed the label UTXO Db and Indexes on Aug 22, 2021
  16. MarcoFalke removed the label RPC/REST/ZMQ on Aug 22, 2021
  17. MarcoFalke removed the label P2P on Aug 22, 2021
  18. MarcoFalke removed the label Mining on Aug 22, 2021
  19. MarcoFalke removed the label Validation on Aug 22, 2021
  20. MarcoFalke removed the label Consensus on Aug 22, 2021
  21. in src/util/system.h:336 in 75e3ddb981 outdated 
    332@@ -327,7 +333,7 @@ class ArgsManager
333      * @param nDefault (e.g. 1)
334      * @return command-line argument (0 if invalid number) or default value
335      */
336-    int64_t GetArg(const std::string& strArg, int64_t nDefault) const;
337+    int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const;
    kiminuo commented at 8:46 pm on August 23, 2021:
    I wonder whether maybe the name should be GetInt64Arg. The name is not as nice as you have but it’s easy to think that the return type is simply int. Again, I’m not sure whether it is an issue. So this is rather a question for my information.
    ryanofsky commented at 3:35 pm on August 24, 2021:

    re: #22766 (review)

    I wonder whether maybe the name should be GetInt64Arg. The name is not as nice as you have but it’s easy to think that the return type is simply int. Again, I’m not sure whether it is an issue. So this is rather a question for my information.

    Sure. I wouldn’t mind that name, though I do prefer current name because it’s emphasizing what’s important about the function and how it’s intended to be used.

    The way I would like range checking to work in the future would be to rely more on C++ types and std::numeric_limits. The idea is arguments would be registered using explicit C++ types:

    0const Setting<int> SETTING_myint("-myint", "description");
1const Setting<std::string> SETTING_mystring("-mystring", "description");
2const Setting<std::vector<std::string>> SETTING_mylist("-mylist", "description");
3const Setting<std::optional<std::uint16_t>> SETTING_myopt("-myopt", "description");
4const Setting<SettingsValue> SETTING_mylegacy("-mylegacy", "description");
5
6void RegisterArgs(ArgsManager& args)
7{
8    args.Register({SETTING_myint, SETTING_mystring, SETTING_mylist, SETTING_myopt, SETTING_mylegacy});
9}

    and then they could be retrieved in a type safe way:

    0args.Get(SETTING_myint);    // returns int
1args.Get(SETTING_mystring); // returns std::string
2args.Get(SETTING_mylist);   // returns std::vector<std::string>
3args.Get(SETTING_myopt);    // returns std::optional<uint16_t>
4args.GetArg/GetArgs/GetIntArg/GetBoolArg(SETTING_mylegacy); // returns requested type

    To get to this point, this PR cleans up existing misused flags and misnamed functions. PR #16545 adds type validation and runtime semantics without changing the ArgsManager API, and a followup PR can improve the API and update call sites without changing the semantics. (There is a direct correspondence between the ALLOW_ flags from #16545 and the useful C++ settings types bool/int/std::string/std::optional/std::variant/std::vector)

    ajtowns commented at 9:56 pm on September 2, 2021:

    I think I finally figured out how you can go a little bit further than the above, ending up with something like:

     0struct NetSettings
 1{
 2    int64_t blockreconstructionextratxn;
 3    int64_t maxorphantx;
 4    bool capturemessages;
 5
 6    template<typename C, typename... Args>
 7    static inline void F(Args&... args) {
 8        return C::Do(args...,
 9            C::Defn( &NetSettings::blockreconstructionextratxn, "-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS, DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN ),
10            C::Defn( &NetSettings::maxorphantx, "-maxorphantx=<n>", strprintf("Keep at most <n> unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS, DEFAULT_MAX_ORPHAN_TRANSACTIONS ),
11            C::Defn( &NetSettings::capturemessages, "-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST, false )
12        );
13    }
14};
15
16void RegisterNetProcessingArgs(ArgsManager& args)
17{
18    SettingsRegister<NetSettings>::Register(args);
19}
20
21NetSettings GetNetSettings(const ArgsManager& args)
22{
23    return SettingsRegister<NetSettings>::Get(args);
24}
25
26class PeerManagerImpl
27{
28private:
29    const NetSettings m_settings;
30    PeerManagerImpl(..., const ArgsManager& args) : m_settings{GetNetSettings(args)), ... { ... }
31    ...
32};

    The idea being that this way:

    • it can infer the argument type directly from the type of the struct member so that you can’t accidentally specify different types between args.AddArg<int> and Get<bool>
    • that the settings are const at runtime so can be accessed without any additional locks
    • you only have to access the ArgsManager (and do string parsing) at setup time
    • you don’t have to make up lots of new names for everything or add too much boilerplate

    Branch built on top of this PR that has the above working at https://github.com/ajtowns/bitcoin/tree/202109-settings_struct

    ryanofsky commented at 0:27 am on September 15, 2021:

    re: #22766 (review)

    I think I finally figured out how you can go a little bit further than the above, ending up with something like:

    Filed #22978 to track this, since it’s not really about this PR

  22. DrahtBot added the label Needs rebase on Aug 24, 2021
  23. ryanofsky force-pushed on Aug 24, 2021
  24. ryanofsky commented at 10:19 am on August 24, 2021: member
    Rebased 75e3ddb981f856612e1ef3ac765e2903d32781f4 -> daf070bdee9a1b4eaf3a4976537111f257bc1be6 (pr/argscripts.1 -> pr/argscripts.2, compare) due to conflict with #22220
  25. in src/net_processing.cpp:1310 in daf070bdee outdated 
    1299@@ -1300,7 +1300,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c
1300 
1301 void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
1302 {
1303-    size_t max_extra_txn = gArgs.GetArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN);
1304+    size_t max_extra_txn = gArgs.GetIntArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN);
    kiminuo commented at 10:25 am on August 24, 2021:
    I can see that once the casting is used (here) and here it is not. Maybe it should be consistent.
    ryanofsky commented at 3:35 pm on August 24, 2021:

    re: #22766 (review)

    I can see that once the casting is used (here) and here it is not. Maybe it should be consistent.

    This scripted diff is just trying to rename the function, not change the way the function is called, but this could be reasonable to do separately.

  26. in src/bitcoin-cli.cpp:836 in daf070bdee outdated 
    806@@ -807,7 +807,7 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str
807     UniValue response(UniValue::VOBJ);
808     // Execute and handle connection failures with -rpcwait.
809     const bool fWait = gArgs.GetBoolArg("-rpcwait", false);
810-    const int timeout = gArgs.GetArg("-rpcwaittimeout", DEFAULT_WAIT_CLIENT_TIMEOUT);
811+    const int timeout = gArgs.GetIntArg("-rpcwaittimeout", DEFAULT_WAIT_CLIENT_TIMEOUT);
    kiminuo commented at 10:26 am on August 24, 2021:
    Should there be a cast here as int64_t is converted to int? Mainly to allow the reader to know that the types are different.
    ryanofsky commented at 3:35 pm on August 24, 2021:

    re: #22766 (review)

    Should there be a cast here as int64_t is converted to int? Mainly to allow the reader to know that the types are different.

    This scripted diff is just trying to rename the function, not change the way the function is called, but this could be reasonable to do separately.

  27. in src/util/system.cpp:217 in daf070bdee outdated 
    222+ * Interpret settings value and apply ALLOW checking flags.
223  *
224- * TODO: Add more meaningful error checks here in the future
225- * See "here's how the flags are meant to behave" in
226- * https://github.com/bitcoin/bitcoin/pull/16097#issuecomment-514627823
227+ * Return an error string and nullopt if an invalid value was provided that
    kiminuo commented at 10:40 am on August 24, 2021:
    Should Doxygen @return be used here?
    ryanofsky commented at 3:34 pm on August 24, 2021:

    re: #22766 (review)

    Should Doxygen @return be used here?

    Thanks! Updated

    kiminuo commented at 7:51 am on August 25, 2021:
    It looks great.
  28. in src/util/system.h:166 in daf070bdee outdated 
    166-        // Boolean options can accept negation syntax -noOPTION or -noOPTION=1
167-        ALLOW_BOOL = 0x01,
168-        ALLOW_INT = 0x02,
169-        ALLOW_STRING = 0x04,
170-        ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
171+        ALLOW_ANY = 0x01,    //!< disable validation
    kiminuo commented at 10:46 am on August 24, 2021:
    I wonder whether enum class Flags : uint32_t may be used here as Jon did in https://github.com/bitcoin/bitcoin/pull/21506/ (guideline). Would it make sense to you in this or a follow-up PR?
    ryanofsky commented at 3:34 pm on August 24, 2021:

    re: #22766 (review)

    I wonder whether enum class Flags : uint32_t may be used here as Jon did in #21506 (guideline). Would it make sense to you in this or a follow-up PR?

    This could be fine, but I’m not sure what the specific advantage of doing it would be in this case (not sure what ways it may be safer). I do think it’s basically tangential in any case, and could be better addressed in a standalone PR.

    kiminuo commented at 7:50 am on August 25, 2021:
    I think the rationale in the guideline makes sense and that’s why I suggested it. There was no specific advatange I had in mind. Anyway, it may (not) be a good follow up PR.
    ajtowns commented at 6:16 am on September 1, 2021:
    See NetPermissionsFlags in net_permissions.h for comparison; it gives you lots of namespacing (so you’d be writing ArgsManager::Flags::ALLOW_BOOL everywhere) and means you can’t just treat them as ints (so bitwise or’ing things together means you have to declare your own operator)
    jonatack commented at 6:43 am on September 1, 2021:

    Yes, two sides of a coin, I suppose.

    Namespacing can allow shortening the enumerator names, since they no longer need to be unique in global namespace (and can therefore no longer need to be shouty “there can only be one”), e.g. ArgsManager::ALLOW_STRING could be ArgsManager::Flags::string. Whether that naming is better may be personal taste, but getting them out of global space seems good.

    Using an enum class permits defining allowed operations: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-oper and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-class (“minimize surprises: traditional enums convert to int too readily”). Changing NetPermissionFlags to an enum class found a bug related to this reason.

    ryanofsky commented at 0:26 am on September 15, 2021:

    re: #22766 (review)

    I wonder whether enum class Flags : uint32_t may be used here as Jon did in #21506 (guideline). Would it make sense to you in this or a follow-up PR?

    Filed #22977 to track this, since it’s not really about this PR

  29. DrahtBot removed the label Needs rebase on Aug 24, 2021
  30. ryanofsky force-pushed on Aug 24, 2021
  31. ryanofsky commented at 7:58 pm on August 24, 2021: member

    Thanks for the review!

    Updated daf070bdee9a1b4eaf3a4976537111f257bc1be6 -> 7ca417e3223de2d8bf81de24e5605c8e87fcb7fe (pr/argscripts.2 -> pr/argscripts.3, compare) updating doxygen comments as suggested

  32. amitiuttarwar commented at 7:23 pm on August 25, 2021: contributor

    concept ACK, init can definitely use some cleaning up, thanks for working on this.

    Do I understand the idea & context correctly? - enforcing type validation is the end goal & this is an intermediary step to have the code reflect the level of validation that’s actually currently occurring (aka, not much)?

  33. kiminuo commented at 7:25 pm on August 25, 2021: contributor
    Concept ACK
  34. ryanofsky commented at 11:49 pm on August 25, 2021: member

    Do I understand the idea & context correctly? - enforcing type validation is the end goal & this is an intermediary step to have the code reflect the level of validation that’s actually currently occurring (aka, not much)?

    Yep, that’s all correct but just to be clear this PR has two goals:

    1. Get rid of misleading ArgsManager flags
    2. Simplify future PRs implementing more validation by cleaning up existing validation

    The PR should make sense by itself just for the first goal even if you don’t care about the second goal.

    Maybe another helpful note about the PR is that only the first commit adds any new code. The other two commits are scripted diff cleanups that don’t add any code.

  35. DrahtBot added the label Needs rebase on Aug 26, 2021
  36. ryanofsky force-pushed on Aug 26, 2021
  37. ryanofsky commented at 10:34 am on August 26, 2021: member
    Rebased 7ca417e3223de2d8bf81de24e5605c8e87fcb7fe -> 31a8356714821f818ef476a9581e7830dd5ed940 (pr/argscripts.3 -> pr/argscripts.4, compare) due to conflict with #22183
  38. DrahtBot removed the label Needs rebase on Aug 26, 2021
  39. in src/util/system.cpp:196 in 3ffbb3b828 outdated 
    198+ * @param[in,out]  key      input/output key argument with "section." and "no"
199+ *                          prefixes stripped
200  *
201- * If there was a double negative, it removes "no" from the key, and
202- * returns true.
203+ * @return false if key was negated, true otherwise
    ajtowns commented at 5:20 am on September 1, 2021:
    I found this a little confusing (when reimplementing it, I had it swapped around and wrote bool negated = IntepretKey() without the !). Maybe would be clearer to return struct { std::string section; bool negated; } and write auto keyinfo = InterpretKey(key);instead?
    ryanofsky commented at 0:26 am on September 15, 2021:

    re: #22766 (review)

    I found this a little confusing (when reimplementing it, I had it swapped around and wrote bool negated = IntepretKey() without the !). Maybe would be clearer to return struct { std::string section; bool negated; } and write auto keyinfo = InterpretKey(key);instead?

    Good idea, implemented this.

  40. ajtowns commented at 6:19 am on September 1, 2021: member

    ACK 31a8356714821f818ef476a9581e7830dd5ed940

    I found the first commit a bit hard to follow, so tried redoing your patch to understand it better. Ended up reordering it a bit, which I think turned out to make more sense (in particular it avoids adding code to set DISALLOW_NEGATION in one commit just to remove it in the next one):

    • add DISALLOW_NEGATION to enum and check it in CheckValid
    • switch ALLOW_{INT,BOOL,STRING} to ANY or ANY|DISALLOW_NEGATION
    • split InterpretKey out of InterpretOption
    • switch InterpretOption into separate invocations of InterpretKey/InterpretValue
    • merge CheckValid into InterpretValue, adding flags,error params and returning an optional
    • rename to GetIntArgs

    Branch is at https://github.com/ajtowns/bitcoin/commits/202109-disallownegation though it’s not cleaned up at all. Ends up in the same state as this branch, so might be useful for other reviewers if nothing else.

    (EDIT: bumped commit id)

  41. MarcoFalke commented at 7:19 am on September 1, 2021: member
    Maybe the last commit can be split up, because it is the most uncontroversial one but causes the most (silent) conflicts?
  42. hebasto commented at 6:10 am on September 3, 2021: member

    Concept ACK.

    Wondering what are benefits of the DISALLOW_NEGATION flag in comparison to the ALLOW_NEGATED one (not an issue at all, just trying to understand motivation)?

  43. in src/util/system.h:173 in 3ffbb3b828 outdated 
    173+        ALLOW_INT = 0x04,    //!< unimplemented, draft implementation in #16545
174+        ALLOW_STRING = 0x08, //!< unimplemented, draft implementation in #16545
175+        ALLOW_LIST = 0x10,   //!< unimplemented, draft implementation in #16545
176+        DISALLOW_NEGATION = 0x20, //! disallow -nofoo syntax
177+
178         DEBUG_ONLY = 0x100,
    hebasto commented at 6:44 am on September 3, 2021:
    1. s/ALLOW_ANY/DISABLE_VALIDATION/, and drop the //!< disable validation comment?
    2. as ALLOW_ANY is not a combination of other flags now, would it simplify the code if ALLOW_ANY = 0? (going to check this suggestion later)

    nits:

    1. ~could we switch values to “shifted ones” (1 << N) for better readability?~ nm, out of scope
    2. ~assuming that another type could be added in the future, e.g., ALLOW_PAIR, maybe DISALLOW_NEGATION = 0x80?~ nm, is not worth
    3. clang-format complains about not justified comments
    ryanofsky commented at 0:26 am on September 15, 2021:

    re: #22766 (review)

    1. s/`ALLOW_ANY`/`DISABLE_VALIDATION`/, and drop the `//!< disable validation` comment?

    This would make the PR bigger and conflict with more PRs. Also, it seems like bikeshedding, and like it might relationships between flags less clear. But please do post a separate RFC issue or a separate PR if the name of this flag should be changed or improved. This PR is just trying to add a comment on this line, not change the meaning or semantics of the flag.

    2. as `ALLOW_ANY` is not a combination of other flags now, would it simplify the code if `ALLOW_ANY = 0`? (going to check this suggestion later)

    This would not work with #16545, but in any case seems like it would be bikeshedding to discuss it here before validation is implemented and this matters. Would encourage moving discussion about this to relevant PRs implementing validation.

    3. `clang-format` complains about not justified comments

    Thanks, should be resolved now

  44. in src/util/system.cpp:234 in 31a8356714 outdated 
    243-        error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);
244+static std::optional<util::SettingsValue> InterpretValue(const std::string& key,
245+    const std::string& value,
246+    bool negated,
247+    unsigned int flags,
248+    std::string& error)
    hebasto commented at 7:00 am on September 3, 2021:

    nit: to make clang-format happy:

    0static std::optional<util::SettingsValue> InterpretValue(
1    const std::string& key,
2    const std::string& value,
3    bool negated,
4    unsigned int flags,
5    std::string& error)
    ryanofsky commented at 0:26 am on September 15, 2021:

    re: #22766 (review)

    nit: to make clang-format happy:

    I ran clang-format and it changed it a different way, but rest assured some clang somewhere is happy!

  45. hebasto commented at 8:06 am on September 3, 2021: member

    Approach ACK 31a8356714821f818ef476a9581e7830dd5ed940.

    I really like @ajtowns’s suggestions:

  46. DrahtBot added the label Needs rebase on Sep 6, 2021
  47. ryanofsky force-pushed on Sep 15, 2021
  48. ryanofsky commented at 1:53 am on September 15, 2021: member

    Updated 31a8356714821f818ef476a9581e7830dd5ed940 -> 0768cdc3e77f9670f0115cdf197a282c355bc287 (pr/argscripts.4 -> pr/argscripts.5, compare) making suggested changes. Biggest change is dropping the last commit.

    re: #22766 (comment)

    Maybe the last commit can be split up, because it is the most uncontroversial one but causes the most (silent) conflicts?

    Thanks, dropped last commit from this PR and posted as #22976.

    re: #22766#pullrequestreview-743341262

    Branch is at ajtowns/bitcoin@202109-disallownegation (commits) though it’s not cleaned up at all. Ends up in the same state as this branch, so might be useful for other reviewers if nothing else.

    Thanks! I couldn’t use the branch exactly because scripted-diff b82541a5f48ffb47d8b1bdc161c14956b0435502 was not valid, but I did split up the first commit basically using your approach and tagged you as co-author.

    re: #22766 (comment)

    Wondering what are benefits of the DISALLOW_NEGATION flag in comparison to the ALLOW_NEGATED one (not an issue at all, just trying to understand motivation)?

    There are two types of options:

    • Typed options (int, string, bool, list options with implementation under discussion in #16545)
    • Untyped options (ALLOW_ANY options)

    For untyped options, I think DISALLOW_NEGATION is better than ALLOW_NEGATED because negation has always been allowed for these options. For the 200 or so existing untyped options that allow negation, this PR is leaving them untouched, not changing their behavior or the way they are declared. There are 5 untyped options that don’t allow negation. This PR is changing the declaration of these 5 options to be less misleading and actually match their implemented behavior.

    For typed options, I think DISALLOW_NEGATION should rarely or never be used. Almost all existing options accept negation. Negation obviously makes sense for bool options. Existing usage shows it also makes sense for most int and string options. For some int and string options, it doesn’t make sense, but in these cases generic negation handling and the generic negation message “Negating of -xxx is meaningless and therefore forbidden” is worse than just writing validation logic that has to be written anyway and giving specific errors like “Setting -xxx value false is out of range, the acceptable range is YYY-ZZZ.” “Setting -xxx value false is not recognized, acceptable values are A, B, and C.”

    If I’m wrong about this and the DISALLOW_NEGATION flag ever starts providing more useful messages, or starts being used many more places, it can be enabled by default in the future. But don’t think these things will happen and I think even if they did happen later it would be premature to disallow negation by default and update 200 existing option declarations right now.

    re: #22766#pullrequestreview-745835425

    I really like @ajtowns’s suggestions:

    Thanks, implemented both.

  49. DrahtBot removed the label Needs rebase on Sep 15, 2021
  50. MarcoFalke referenced this in commit 825f4a64e6 on Sep 27, 2021
  51. ryanofsky force-pushed on Sep 27, 2021
  52. ryanofsky commented at 1:39 pm on September 27, 2021: member
    Rebased 0768cdc3e77f9670f0115cdf197a282c355bc287 -> 5c04ca81fd2c44fed6487123500b49434280d514 (pr/argscripts.5 -> pr/argscripts.6, compare) due to silent conflict with #23025 in scripted diff commit
  53. laanwj added this to the "Blockers" column in a project
  54. in src/util/system.cpp:881 in 4be2e10cc2 outdated 
    882-                return false;
883-            }
884-            m_settings.ro_config[section][key].push_back(value);
885+            std::optional<util::SettingsValue> value =
886+                InterpretValue(key, option.second, keyinfo.negated, *flags, error);
887+            if (!value) return false;
    promag commented at 11:33 am on October 16, 2021:

    4be2e10cc250350ad6c81bb75589793dfa27990d

    nit, rename value to parsed, here change to !parsed.has_value(), and below parsed.value().

    ryanofsky commented at 3:11 pm on October 16, 2021:

    re: #22766 (review)

    nit, rename value to parsed, here change to !parsed.has_value(), and below parsed.value().

    Why these requests? Usage of has_value seems less idiomatic and the name parsed seems less descriptive. It’s always helpful when you make suggestions to say what thinking is behind them so different preferences can be reconciled.

    promag commented at 9:43 am on October 17, 2021:

    Just a nit, feel free to ignore. Personally prefer has_value(), especially if above was auto value = InterpretValue(...).

    Edit:has_value() is used in some places. Maybe there should be some guidelines in developer notes or something?

  55. in src/util/system.cpp:195 in 4be2e10cc2 outdated 
    199- * returns true.
200- *
201- * If there was no "no", it returns the string value untouched.
202- *
203- * Where an option was negated can be later checked using the
204+ * @note Where an option was negated can be later checked using the
    promag commented at 11:39 am on October 16, 2021:

    4be2e10cc250350ad6c81bb75589793dfa27990d

    Could keep comment that says key is changed.

    ryanofsky commented at 3:08 pm on October 16, 2021:

    re: #22766 (review)

    Could keep comment that says key is changed.

    Let me know if there’s a comment that’s being removed that you want to keep. Implementation is changed again now, but the new comment said it strips prefixes, so I’m not sure what old comment you wanted to keep.

    kiminuo commented at 3:35 pm on October 16, 2021:

    I agree.

    I would say that a comment like @param[in|out] key ... (not sure how to write it properly) would be great here.

    ryanofsky commented at 4:02 pm on October 16, 2021:

    re: #22766 (review)

    I would say that a comment like @param[in|out] key ... (not sure how to write it properly) would be great here.

    Should be resolved since it’s input only now.

    promag commented at 4:52 pm on October 16, 2021:
    Ya original comment is not relevant now.
  56. in src/util/system.cpp:197 in 4be2e10cc2 outdated 
    207  * that debug log output is not sent to any file at all).
208  */
209-
210-static util::SettingsValue InterpretOption(std::string& section, std::string& key, const std::string& value)
211+struct KeyInfo { std::string section; bool negated{false}; };
212+KeyInfo InterpretKey(std::string& key)
    promag commented at 11:40 am on October 16, 2021:

    4be2e10cc250350ad6c81bb75589793dfa27990d

    Instead of changing the argument, could add key member to KeyInfo and make argument const.

    ryanofsky commented at 3:09 pm on October 16, 2021:

    re: #22766 (review)

    Instead of changing the argument, could add key member to KeyInfo and make argument const.

    Done. This does work nicely with InterpretValue as you suggested.

  57. in src/util/system.cpp:225 in 4be2e10cc2 outdated 
    251  */
252-static bool CheckValid(const std::string& key, const util::SettingsValue& val, unsigned int flags, std::string& error)
253-{
254-    if (val.isBool() && !(flags & ArgsManager::ALLOW_BOOL)) {
255-        error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);
256+static std::optional<util::SettingsValue> InterpretValue(const std::string& key, const std::string& value,
    promag commented at 11:42 am on October 16, 2021:

    4be2e10cc250350ad6c81bb75589793dfa27990d

    nit, if you pick the suggestion to add key to KeyInfo then this could be InterpretValue(const KeyInfo& key_info, ...).

    ryanofsky commented at 3:10 pm on October 16, 2021:

    re: #22766 (review)

    nit, if you pick the suggestion to add key to KeyInfo then this could be InterpretValue(const KeyInfo& key_info, ...).

    Done

  58. promag commented at 11:47 am on October 16, 2021: member
    Concept ACK, code review on 1st commit. I need to understand the big picture from #16545 in order to review the remaining changes here.
  59. ryanofsky commented at 2:37 pm on October 16, 2021: member

    Thanks for review! Just wanted to quickly say you do not need to care anything about #16545 for this PR to make sense. This PR is disabling flags ALLOW_INT/STRING/BOOL which are broken, by commenting them out. #16545 is reenabling these flags by uncommenting them again and implementing them.

    Even if #16545 is rejected, these flags are misleading now and should be disabled. The only effect these flags have now is to disallow negation, so this PR is replacing them with a clearer DISALLOW_NEGATION flag. I expect this flag to be rarely used in the future, and probably to be removed later. Defining it here is just useful to make current code clear and avoid changing behavior in any way with this PR.

    (EDIT: The reason I think DISALLOW_NEGATION will be removed in the future is described #22766#pullrequestreview-754561115. But basically I think the error message DISALLOW_NEGATION shows is vague and a little confusing, and in general you will produce better error messages by checking for allowed values than checking for disallowed values.)

  60. in src/util/system.cpp:196 in 4be2e10cc2 outdated 
    206  * options that are not normally boolean (e.g. using -nodebuglogfile to request
207  * that debug log output is not sent to any file at all).
208  */
209-
210-static util::SettingsValue InterpretOption(std::string& section, std::string& key, const std::string& value)
211+struct KeyInfo { std::string section; bool negated{false}; };
    kiminuo commented at 3:13 pm on October 16, 2021:
    nit: As there are many “keys” in Bitcoin codebase and the word meaning is heavily overloaded, maybe it would make sense to have SettingKeyInfo / SettingKey / ArgKeyInfo or something.
    ryanofsky commented at 4:14 pm on October 16, 2021:

    re: #22766 (review)

    nit: As there are many “keys” in Bitcoin codebase and the word meaning is heavily overloaded, maybe it would make sense to have SettingKeyInfo / SettingKey / ArgKeyInfo or something.

    I’m happy to use a different name, but this was the name suggested by another reviewer so I kept it. If reviewers want to decide on something different, I’m happy with any of these, since this is just a private struct local to system.cpp

  61. in src/util/system.h:167 in 5c04ca81fd outdated 
    163@@ -164,10 +164,10 @@ class ArgsManager
164      */
165     enum Flags : uint32_t {
166         ALLOW_ANY = 0x01,         //!< disable validation
167-        ALLOW_BOOL = 0x02,        //!< unimplemented, draft implementation in #16545
168-        ALLOW_INT = 0x04,         //!< unimplemented, draft implementation in #16545
169-        ALLOW_STRING = 0x08,      //!< unimplemented, draft implementation in #16545
170-        ALLOW_LIST = 0x10,        //!< unimplemented, draft implementation in #16545
171+        // ALLOW_BOOL = 0x02,     //!< unimplemented, draft implementation in #16545
    promag commented at 3:15 pm on October 16, 2021:

    5c04ca81fd2c44fed6487123500b49434280d514

    For the record, I don’t mind these commented out instead of removed.

    ryanofsky commented at 4:42 pm on October 16, 2021:

    re: #22766 (review)

    For the record, I don’t mind these commented out instead of removed.

    The reason these are commented out instead of removed is that flag name ALLOW_ANY doesn’t make sense without seeing other ALLOW_ flags. I could remove or rename ALLOW_ANY, but I don’t want to, because it would blow up the size of this PR and make it conflict with everything, and because as long as some validation is added later, the name ALLOW_ANY should be fine later.

  62. in src/util/system.cpp:210 in 4be2e10cc2 outdated 
    218-        section = key.substr(0, option_index);
219+        result.section = key.substr(0, option_index);
220         key.erase(0, option_index + 1);
221     }
222     if (key.substr(0, 2) == "no") {
223         key.erase(0, 2);
    kiminuo commented at 3:21 pm on October 16, 2021:
    not for this PR but: key.compare(..) seems more efficient than key.substr(..) here. In C++20, there will be starts_with function.
    ryanofsky commented at 4:02 pm on October 16, 2021:

    re: #22766 (review)

    not for this PR but: key.compare(..) seems more efficient than key.substr(..) here. In C++20, there will be starts_with function.

    Agree it would be a good change in a different PR

  63. promag commented at 3:23 pm on October 16, 2021: member

    Code review ACK 5c04ca81fd2c44fed6487123500b49434280d514.

    Thanks for the clarification. Indeed, the existing flags are misleading when in practice they are used to infer if negation is accepted. I got distracted by ALLOW_LIST, just realized it’s only added in #16545.

  64. in src/util/system.h:170 in 6e613a0a30 outdated 
    170-        ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
171+        ALLOW_ANY = 0x01,         //!< disable validation
172+        ALLOW_BOOL = 0x02,        //!< unimplemented, draft implementation in #16545
173+        ALLOW_INT = 0x04,         //!< unimplemented, draft implementation in #16545
174+        ALLOW_STRING = 0x08,      //!< unimplemented, draft implementation in #16545
175+        ALLOW_LIST = 0x10,        //!< unimplemented, draft implementation in #16545
    kiminuo commented at 3:40 pm on October 16, 2021:

    6e613a0a3058e65237907bcf7915864dcef8cc99: Not sure if this commit should add the flag as it does not implement it anyway.

    Also at this point, it’s not entirely clear if ALLOW_LIST allows only [type, type, type,...] lists or even [type1, type2, type3, ...] lists.

    ryanofsky commented at 4:23 pm on October 16, 2021:

    re: #22766 (review)

    6e613a0: Not sure if this commit should add the flag as it does not implement it anyway.

    Right, no point describing a future unimplemented flag.

    Also at this point, it’s not entirely clear if ALLOW_LIST allows only [type, type, type,...] lists or even [type1, type2, type3, ...] lists.

    It’s orthogonal. If you combined ALLOW_LIST with ALLOW_BOOL/INT/STRING it would allow lists of bools, ints, or strings, if you combined ALLOW_LIST with ALLOW_ANY it would allow lists of any type (and values would be parsed as strings).

    EDIT: It might be clearer to change ALLOW_LIST to ALLOW_MULTIPLE since the main effect of the flag is to let settings be specified multiple times in the config file or command line and use all the values instead of just a single value. But it doesn’t really matter for now, since this is commented out and intent is just to show intended scope of ALLOW flags and reserve an enum value.

  65. in src/util/system.cpp:231 in 6e613a0a30 outdated 
    226@@ -227,7 +227,7 @@ static std::optional<util::SettingsValue> InterpretValue(const std::string& key,
227 {
228     // Return negated settings as false values.
229     if (negated) {
230-        if (!(flags & ArgsManager::ALLOW_BOOL)) {
231+        if (flags & ArgsManager::DISALLOW_NEGATION) {
232             error = strprintf("Negating of -%s is meaningless and therefore forbidden", key);
    kiminuo commented at 3:42 pm on October 16, 2021:

    6e613a0a3058e65237907bcf7915864dcef8cc99

    nit: It seems to me that this message may simply say that negation is disallowed/forbidden as I’m not sure that it’s guarranteed that negation is “meaningless”.

    ryanofsky commented at 4:20 pm on October 16, 2021:

    re: #22766 (review)

    nit: It seems to me that this message may simply say that negation is disallowed/forbidden as I’m not sure that it’s guarranteed that negation is “meaningless”.

    I agree this error message is not clear, and this is only kept to keep current behavior.

    In the future, I think it would be good to remove the disallow negation flag entirely and use allowed value checks instead of disallowed values checks as described #22766 (review).

  66. in src/util/system.h:173 in 6e613a0a30 outdated 
    173+        ALLOW_INT = 0x04,         //!< unimplemented, draft implementation in #16545
174+        ALLOW_STRING = 0x08,      //!< unimplemented, draft implementation in #16545
175+        ALLOW_LIST = 0x10,        //!< unimplemented, draft implementation in #16545
176+        DISALLOW_NEGATION = 0x20, //!< disallow -nofoo syntax
177+
178         DEBUG_ONLY = 0x100,
    kiminuo commented at 3:46 pm on October 16, 2021:
    6e613a0a3058e65237907bcf7915864dcef8cc99: Would it be possible to add a comment for DEBUG_ONLY too? Or is it out of scope here?
    ryanofsky commented at 4:06 pm on October 16, 2021:

    re: #22766 (review)

    6e613a0: Would it be possible to add a comment for DEBUG_ONLY too? Or is it out of scope here?

    I’d have to research what the comment should say and I’d rather see it done separately. This is a flag, but not a flag related to the flags in this PR.

  67. kiminuo commented at 3:48 pm on October 16, 2021: contributor
    Concept ACK
  68. ryanofsky force-pushed on Oct 16, 2021
  69. ryanofsky commented at 3:59 pm on October 16, 2021: member

    Thanks for review! Updated just making suggested changes.

    Updated 5c04ca81fd2c44fed6487123500b49434280d514 -> b8fd5d54fafe94cb40a43e3e1366a6855a512e4e (pr/argscripts.6 -> pr/argscripts.7, compare) with suggested changes

  70. ryanofsky commented at 4:10 pm on October 16, 2021: member

    re: #22766 (review)

    nit: As there are many “keys” in Bitcoin codebase and the word meaning is heavily overloaded, maybe it would make sense to have SettingKeyInfo / SettingKey / ArgKeyInfo or something.

    EDIT: moved reply to #22766 (review)

  71. promag commented at 9:04 am on October 22, 2021: member
    Code review ACK b8fd5d54fafe94cb40a43e3e1366a6855a512e4e.
  72. DrahtBot added the label Needs rebase on Oct 22, 2021
  73. refactor: Split InterpretOption into Interpret{Key,Value} functions 
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    26a50ab322
  74. refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage 
    Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation,
so current uses of these flags are misleading and will also break
backwards compatibility whenever these flags are implemented in a future
PR (draft PR is #16545).

An additional complication is that while these flags don't do any real
settings validation, they do affect whether setting negation syntax is
allowed.

Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are
implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is
done in two commits, with this commit adding the DISALLOW_NEGATION flag,
and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
    b8c069b7a9
  75. scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags 
    This commit does not change behavior in any way. See previous commit for
complete rationale, but these flags are being disabled because they
aren't implemented and will otherwise break backwards compatibility when
they are implemented.

-BEGIN VERIFY SCRIPT-
sed -i 's:\(ALLOW_.*\)   \(//!< unimplemented\):// \1\2:' src/util/system.h
sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp
git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)'  | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g'
git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g'
-END VERIFY SCRIPT-
    c5d7e34bd9
  76. ryanofsky force-pushed on Oct 25, 2021
  77. ryanofsky commented at 3:48 pm on October 25, 2021: member
    Rebased b8fd5d54fafe94cb40a43e3e1366a6855a512e4e -> c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab (pr/argscripts.7 -> pr/argscripts.8, compare) due to conflict with #23002
  78. DrahtBot removed the label Needs rebase on Oct 25, 2021
  79. ajtowns commented at 9:55 am on October 26, 2021: member
    utACK c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab
  80. ryanofsky commented at 5:08 pm on October 29, 2021: member
    @promag do you think you could reack? Only change since your previous review was rebasing to avoid a minor wallet-tool conflict, #22766 (comment)
  81. promag commented at 6:33 pm on October 29, 2021: member
    Code review ACK c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab, which as the new argument -legacy.
  82. fanquake merged this on Nov 1, 2021
  83. fanquake closed this on Nov 1, 2021
  84. fanquake removed this from the "Blockers" column in a project
  85. sidhujag referenced this in commit 958b1cad74 on Nov 1, 2021
  86. DrahtBot locked this on Nov 2, 2022


ryanofsky DrahtBot kiminuo ajtowns jonatack amitiuttarwar MarcoFalke hebasto promag

Labels
Refactoring Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me