Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: rename blocklist to banlist to avoid confusion #9591

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xFFFC0000
Copy link
Collaborator

We use the word block in many different contexts. To avoid confusion, I propose to use the word ban anywhere we are banning someone.

Important note : This PR does not have any semantical code change and should not include any semantical code change.

@@ -1560,7 +1560,8 @@ namespace cryptonote

for (auto & entry : white_list)
{
if (!req.include_blocked && m_p2p.is_host_blocked(entry.adr, NULL))
if ((!req.include_blocked || !req.include_banned)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here and below differ (pretty sure this one is wrong, and you want !(included_banned || include_blocked); it might help to just bundle the logic for include_banned into a local constant, given that it's multiple-sourced.

Copy link
Collaborator Author

@0xFFFC0000 0xFFFC0000 Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. The logic is wrong. Actually, I mistyped it. I had ((!req.include_blocked && !req.include_banned) which is same as what you are suggesting. But I mis-typed it or pushed the wrong implementation.

As you can see the line 1577 is correct. I totally forget to fix this line!

Anyway, thanks a lot for catching this. Will push a newer one with corrected code.

@0xFFFC0000 0xFFFC0000 force-pushed the dev/0xfffc/cleanup-blocklist-to-banlist branch from 1bcbbd4 to b5cd7f8 Compare November 24, 2024 17:20
@iamamyth
Copy link

I agree with the premise that a new term helps here. My only concern would be the RPC change: Ideally, the name of the field changes everywhere. Practically, there's a gigantic queue of PRs waiting months to merge, which begs the question: How long would it take for the old field to actually disappear? Until it does, you get additional maintenance burden and potential for bugs (as seen in the PR draft). If it takes, say, five years to effect the actual removal (that's my estimate), I'm not sure the payoff justifies the cost.

@0xFFFC0000
Copy link
Collaborator Author

I agree with the premise that a new term helps here. My only concern would be the RPC change: Ideally, the name of the field changes everywhere. Practically, there's a gigantic queue of PRs waiting months to merge, which begs the question: How long would it take for the old field to actually disappear? Until it does, you get additional maintenance burden and potential for bugs (as seen in the PR draft). If it takes, say, five years to effect the actual removal (that's my estimate), I'm not sure the payoff justifies the cost.

Keep in mind for backward compatibility, we don't change the field name.

@iamamyth
Copy link

Keep in mind for backward compatibility, we don't change the field name.

I assumed the plan was to keep the field name, but deprecate, and eventually, remove it (on a likely rather long timeline). If there's no eventual removal planned, it really doesn't make sense to have two names for it.

@0xFFFC0000
Copy link
Collaborator Author

Keep in mind for backward compatibility, we don't change the field name.

I assumed the plan was to keep the field name, but deprecate, and eventually, remove it (on a likely rather long timeline). If there's no eventual removal planned, it really doesn't make sense to have two names for it.

I see your logic.

I think it is reasonable concern. Let's decide how we should move forward. I was under impression to keep both variables, since they are only 3 lines of code (at most). But it is a valid concern. Once we achieved a consensus, I will fix that part. (That is like 3 lines of code).

Please feel free to raise any issue you see. Thanks for your constructive feedback so far.

@nahuhh
Copy link

nahuhh commented Nov 25, 2024

Keep in mind for backward compatibility, we don't change the field name.

I assumed the plan was to keep the field name, but deprecate, and eventually, remove it (on a likely rather long timeline). If there's no eventual removal planned, it really doesn't make sense to have two names for it.

The only change necessary would be to follow the contribution guide, and to label the help entry for --enable-dns-blocklist as "legacy" or "deprecated", and to schedule a pr for its removal in its entirety.

the cobtribution guide says that you cannot/should not remove stuff without first deprecating, but does not have a guideline for timeframe.

we have other aliases already that arent deprecated, such as --restore-from-seed being an alias for --restore-deterministic-wallet

@iamamyth
Copy link

@nahuhh My concern was purely about the RPC, as it would have the longest removal timeframe (command-line switches are local to the machine, so much easier to synchronize a change). I have no objection to the command-line parameter change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants