search

monero-project / monero

Monero: the secure, private, untraceable cryptocurrency
https://getmonero.org
Other
9.03k stars 3.12k forks source link

src: rename blocklist to banlist to avoid confusion #9591

Open 0xFFFC0000 opened 2 days ago

0xFFFC0000 commented 2 days ago

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.

iamamyth commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 12 hours ago

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 commented 12 hours ago

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 commented 10 hours ago

@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.