These fields were deprecated in #19469, #19770, #19725. @jonatack & @luke-jr, if you get the chance, would appreciate your review :)

Updated to include PR # in release notes

I believe we can get rid of the m_legacyWhitelisted field on CNode, but am still trying to build confidence.

There is a code comment: //If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility). In this PR I'm removing the RPC endpoint, but I'm unable to find anywhere in QT that reveals this field. I'm unfamiliar with QT, maybe it was already removed, or maybe I'm missing it.

@NicolasDorier - since you introduced this, wondering if you have any thoughts?

update: realized QT usage was removed in 784ef8b in bitcoin-core/gui#34. I have since updated this branch to remove m_legacyWhitelisted

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Net: Move ping data to net_processing #20721 (Net: Move ping data to net_processing by jnewbery)
• Add I2P support using I2P SAM #20685 (Add I2P support using I2P SAM by vasild)
• rpc: deprecate addresses and reqSigs from rpc outputs #20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
• net: Add blockfilters white{bind,list} permission flag #18972 (net: Add blockfilters white{bind,list} permission flag by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

I realized this removes the last use of m_legacyWhitelisted, so I deleted the field.

Nice cleanup. Concept ACK

rebased

utACK 454a408

Looks like no one tested this before merging. Fixed in #20791 but as a post mortem should have been tested prior to merging.

Looks like no one tested this before merging. Fixed in #20791 but as a post mortem should have been tested prior to merging.

I am not sure what you mean. clang doesn't emit this warning, as explained in #20791 (comment). Merging any pull will be impossible if maintainers are required to build with all supported compiler versions on all supported platforms.

Regardless, this has long been pointed out during review: #20755 (review) with the comment: "You can remove a no-longer-needed local variable if you touch this again."

Moreover, -Wunused (and other warnings) are known to be broken for more than 13 years (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89180) in gcc. Rushing to "fix" (silence) them with patches that will break code (#20797 (comment)) is not the way to go. Personally, I prefer correct code over code that is incorrect, but doesn't emit any warnings.

This could have been easily caught with a simple manual grep right? For something basic like this someone should do that grep before it is merged? Or am I missing something?

The above is interesting context though, thanks.

Rushing to "fix" (silence) them with patches that will break code (#20797 (comment)) is not the way to go. Personally, I prefer correct code over code that is incorrect, but doesn't emit any warnings.

Diligence and vigilance does not mean I am suggesting we rush to break the code to silence the compiler. I'm not interested in blame games, only making bitcoin more robust.

This could have been easily caught with a simple manual grep right?

Yes, and I assume that either grep or --function-context was used to find it in #20755 (review).

@jonatack Sorry, didn't want to blame you. It was the most recent example that came to mind. There are many more examples, including many by me. I should have used one of myself.

Thanks. I think given enough time it happens to everyone anyway. Teamwork makes the dream work.

Looks like no one tested this before merging. Fixed in #20791 but as a post mortem should have been tested prior to merging.

This is an entirely unhelpful comment. The 'problem' here is that a variable is now unused and certain compilers warn about it. Unused variables are exactly that - unused - and so have no impact on the logic of the program. In fact, compilers will optimize them out, so I expect the produced object is exactly the same before and after #20791.

This was spotted in review, found to not be an issue, and would have been fixed if the branch was retouched. Instead it was fixed in a follow-up PR. There's absolutely no problem here.

I assume that either grep or --function-context was used to find it in #20755 (review).

Just careful review. If a change means that a variable is no longer used in one place, it's good practice to see where else that variable is used.

It seems sloppy to me. Either remove it in the PR or merge it with an instruction that someone should open a new PR to remove it. I don't think it is great hygiene to just leave it hanging in a past comment (but maybe that is just me)

Don't want to belabor the point though (especially when being told it is "entirely unhelpful")