init: Disallow negation of blockversion #19827

1. 
   MarcoFalke commented at 9:33 am on August 28, 2020: member

   The block version is numeric. Allowing boolean values for it doesn’t really make sense.
2. init: Disallow negation of blockversion fabd6e7f13
3. MarcoFalke added the label Utils/log/libs on Aug 28, 2020
4. 
   DrahtBot commented at 12:10 pm on August 28, 2020: member

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

   Coverage

   Coverage	Change (pull 19827, e7bf8b78a3fcdfd1ad46d7bb17c8e14c000ced73)	Reference (master, 862fde88be706adb20a211178253636442c3ae00)
   Lines	+0.0139 %	90.9299 %
   Functions	+0.0000 %	86.0199 %
   Branches	+0.0121 %	52.5911 %

   Updated at: 2020-08-28T12:10:57.740259.

5. 
   laanwj commented at 12:19 pm on August 28, 2020: member

   Good catch, code review ACK fabd6e7f130cece3ce3fa2986183ef777a08f58a
6. 
   jonatack commented at 2:06 pm on August 28, 2020: member

   ACK fabd6e7f130ce

   A couple nice-to-haves in the same file if you retouch:

    0--- a/test/functional/feature_config_args.py
    1+++ b/test/functional/feature_config_args.py
    2@@ -16,6 +16,7 @@ class ConfArgsTest(BitcoinTestFramework):
    3 
    4     def test_config_file_parser(self):
    5+        self.log.info('Test config file parser')
    6         # Assume node is stopped
    7 
    8         inc_conf_file_path = os.path.join(self.nodes[0].datadir, 'include.conf')
    9@@ -83,6 +84,7 @@ class ConfArgsTest(BitcoinTestFramework):
   10 
   11     def test_log_buffer(self):
   12+        self.log.info('Test log buffer')
   13         with self.nodes[0].assert_debug_log(expected_msgs=['Warning: parsed potentially confusing double-negative -connect=0\n']):
   14             self.start_node(0, extra_args=['-noconnect=0'])
   

7. 
   hebasto commented at 2:06 pm on August 28, 2020: member

   Concept ACK. But I think this should go after #16545.

   Btw, are any other options that also must be assigned with ALLOW_INT?

8. 
   MarcoFalke commented at 2:11 pm on August 28, 2020: member

   this should go after #16545.

   Why?

9. 
   hebasto commented at 2:17 pm on August 28, 2020: member

   this should go after #16545.

   Why?

   Only #16545 guaranties:

   Trigger startup errors if bitcoin is configured with bad setting values according to flags. Also raise internal errors if settings are registered and retrieved with inconsistent flags.

   Without error checking it could lead to false safety sense.

10. 
    ryanofsky commented at 2:21 pm on August 28, 2020: member

    I’m not sure what’s happening with this PR, but I’d ask this not to be rushed. At least give me a day or two to rebase #16545.

    I think a DISALLOW_NEGATION flag could be a good idea, but I think would be a misuse of the ALLOW_INT flag to affect negation. The ALLOW_INT flag is meant to allow -value=123 settings to be parsed while disallowing settings like -value=string. Just because a setting accepts integer values doesn’t mean the setting should disallow negation. Sometimes in cases like this, you may want to disallow negation. Sometimes in other cases, negation may be ok. Maybe in these cases it is a nice convenience to stick with historical behavior of interpretting -novalue the same as -value=0. Other times maybe it makes sense implement special behavior for -novalue clear a list, or disable a feature, or eliminiate a limit, while value=number sets limit.

11. 
    MarcoFalke commented at 3:21 pm on August 28, 2020: member

    Ok, going to reopen later
12. MarcoFalke closed this on Aug 28, 2020

    ---

13. MarcoFalke deleted the branch on Aug 28, 2020
14. 
    MarcoFalke commented at 7:42 am on August 18, 2021: member

    @ryanofsky At least give me a day or two to rebase #16545.

    It looks like there is outstanding review (https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-579068737), so I am wondering if I should wait another day or two?

15. 
    ryanofsky commented at 6:43 pm on August 19, 2021: member

    so I am wondering if I should wait another day or two?

    So impatient. It only took me 569 days to respond to that depressing review comment. Any case I updated #16545 to keep the “Negating of -%s is meaningless and therefore forbidden” behavior this PR relies on.

    I would still like this PR to be merged after #16545 and to be accompanied by release notes, so stricter -blockversion validation happens once in a single PR instead of incrementally in two PRs, and so the behavior changes are clearly documented.

16. DrahtBot locked this on Aug 19, 2022

Contributors
MarcoFalke DrahtBot laanwj jonatack hebasto ryanofsky

Labels
Utils/log/libs