banman: BanLevel enum #16018

pull dakk wants to merge 1 commits into bitcoin:master from dakk:ban-level-enum changing 3 files +14 −11

dakk commented at 3:38 PM on May 13, 2019: none

This pull request modify the banman IsBannedLevel return value from an int to an enum which describes the BanLevel value itself, without using magic numbers to identify different types of ban.

https://github.com/bitcoin/bitcoin/issues/15929
gmaxwell commented at 4:52 PM on May 13, 2019: contributor

I had expected that this would eventually become a bitmask or an arbitrary disconnection priority.
DrahtBot added the label P2P on May 13, 2019
dakk commented at 1:40 PM on May 14, 2019: none

@gmaxwell I think bitmask in unsuitable for this, since we are comparing two BanLevel in https://github.com/bitcoin/bitcoin/pull/16018/commits/1f3c389b94dc5e872fb96513f56966bcfc2439e9#diff-9a82240fe7dfe86564178691cc57f2f1R940
MarcoFalke commented at 2:21 PM on May 14, 2019: member

Could squash into one commit?
dakk force-pushed on May 14, 2019
dakk commented at 3:06 PM on May 14, 2019: none

@MarcoFalke done
gmaxwell commented at 10:29 PM on May 14, 2019: contributor

@dakk Yes, currently there are only two banning levels (and not-banned, of course)-- but there are clear cases for multiple kinds of banning. The PR that introduced the levels also discussed using it instead a fully ordered banning priority score which, again, would be incompatible with an enum.
jonnygrant commented at 9:35 AM on May 15, 2019: none

I see someone started working on my proposed change I was working on myself. Rather than duplicating effort, worth always checking with the person who reports an issue to be changed before rushing in and wasting someones time. Collaboration is better.

The coding style is odd, with enum in CAPS and again the duplicate comments. This isn't a pre-processor macro! Do I just submit my own PR with the changes I proposed?
banman: migrate banlevel from int to enum 6ad74db2d8

in src/banman.h:40 in 42a0467706 outdated

  39 | +    // 0 - Not banned
  40 | +    NOT_BANNED                  =    0,
  41 | +    // 1 - Automatic misbehavior ban
  42 | +    AUTOMATIC_MISBEHAVIOUR_BAN  =    1,
  43 | +    // 2 - Any other ban
  44 | +    ANY_OTHER_BAN               =    2

Empact commented at 7:12 PM on May 15, 2019:

nit: comments seem redundant; if kept, could use the in-line doxygen format: //!<

dakk commented at 7:14 AM on May 16, 2019:

Removed, as you told they were redundant

dakk force-pushed on May 16, 2019
laanwj commented at 2:43 PM on May 16, 2019: member

Sorry, but I'm not convinced that this is an improvement. I think it's good to document the ban levels but I don't think converting to an enum makes it better.
DrahtBot commented at 10:48 PM on May 16, 2019: member



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



Conflicts

No conflicts as of last run.
sdaftuar commented at 12:44 AM on May 17, 2019: member

Concept NACK, see #15929 (comment)
fanquake commented at 3:05 AM on May 17, 2019: member

Going to close this in light of essentially 3 NACKs. #16038 (an alternative approach) has also been closed, see similar comments there. Any discussion of either PR/approach can continue in #15929 if necessary.
fanquake closed this on May 17, 2019
DrahtBot locked this on Dec 16, 2021