Warn on unknown rw_settings #19624

1. 
   MarcoFalke commented at 8:51 am on July 30, 2020: member

   Log a warning to debug log if unknown settings are encountered. This should probably only ever happen when the software is upgraded.

   Something similar is already done for the command line and config file. See:

   • test: Add test for unknown args #16234 (commit fa7dd88b71a1c6641bd450fae29a4a31849b1afd)
2. MarcoFalke added the label Utils/log/libs on Jul 30, 2020
3. MarcoFalke force-pushed on Jul 30, 2020
4. 
   DrahtBot commented at 9:40 am on July 30, 2020: member

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

   Conflicts

   No conflicts as of last run.

5. IhostVlad approved
6. MarcoFalke force-pushed on Jul 30, 2020
7. in src/util/system.cpp:435 in faec629663 outdated

   439+                SaveErrors({val_error}, errors);
   440+                return false;
   441+            }
   442+        } else {
   443+            LogPrintf("Ignoring unknown rw_settings value %s\n", setting.first);
   444+        }
   

   ---

   ---

   

   ryanofsky commented at 9:20 pm on August 12, 2020:

   In commit “Interpret and validate rw_settings” (faec629663a92f28c5328ab3877ff58847bf9e35)

   Would suggest just:

   0if (!flags) {
   1    LogPrintf("Ignoring unknown rw_settings value %s\n", setting.first);
   2}
   

   I don’t think this can return false and prevent loading all settings when one has an unrecognized value, because it could break downgrading if allowed values are expanded, similar to what’s described #15935 (comment). I also tend to think there shouldn’t call a new call to CheckValid while that function is basically broken and its behavior is being worked out in other PRs.

   ---

   

   MarcoFalke commented at 9:44 am on August 28, 2020:

   Ok, removed CheckValid for now
8. in test/functional/feature_settings.py:62 in fa1c4c9c7d outdated

   54@@ -55,6 +55,13 @@ def run_test(self):
   55             fp.write("invalid json")
   56         node.assert_start_raises_init_error(expected_msg='Unable to parse settings file', match=ErrorMatch.PARTIAL_REGEX)
   57 
   58+        # Test invalid type
   59+        with settings.open("w") as fp:
   60+            json.dump({"blockversion": False}, fp)
   61+        node.assert_start_raises_init_error(
   62+            expected_msg='Error: Failed loading settings file:\n- Negating of -blockversion is meaningless and therefore forbidden',
   

   ---

   ---

   

   ryanofsky commented at 9:30 pm on August 12, 2020:

   In commit “Disallow negation of blockversion” (fa1c4c9c7d10c11b0a2324ff0377919fa9fbc2fe)

   I don’t think having this second commit in this PR is a good idea. It’s not directly related to the extra logging that this PR provides (which seems unambiguously good), and it’s not a backwards compatible change, and it diverges from previous and still widespread behavior where -noarg is equivalent to arg=0. Changing command line processing could be good but I think it’d better to do it in a dedicated PR with more consistency and without tying it to another change

   ---

   

   MarcoFalke commented at 9:45 am on August 28, 2020:

   ok, split commit into new pull
9. 
   ryanofsky commented at 9:37 pm on August 12, 2020: member

   Concept ACK. This adds useful logging, but see suggestions below because in some ways I think the PR currently does too much.
10. Warn on unknown rw_settings fa48405ef8
11. MarcoFalke force-pushed on Aug 28, 2020
12. MarcoFalke renamed this:
    Interpret and validate rw_settings
    Warn on unknown rw_settings
    on Aug 28, 2020
13. ryanofsky approved
14. 
    ryanofsky commented at 0:02 am on September 2, 2020: member

    Code review ACK fa48405ef84985e5a9d38ec38e90d16596ea45b5. Looks good and I could see this being helpful for debugging. Thanks for taking suggestions
15. MarcoFalke merged this on Oct 19, 2020
16. MarcoFalke closed this on Oct 19, 2020

    ---

17. sidhujag referenced this in commit 3cd2920fba on Oct 19, 2020
18. Fabcien referenced this in commit 345d8e187c on Nov 25, 2021
19. DrahtBot locked this on Feb 15, 2022
20. MarcoFalke deleted the branch on Apr 28, 2022

Contributors
MarcoFalke DrahtBot IhostVlad ryanofsky

Labels
Utils/log/libs