[rpc] Remove deprecated fields from getpeerinfo #20755

amitiuttarwar commented at 6:34 PM on December 23, 2020: contributor

This PR removes support for 3 fields on the getpeerinfo RPC that were deprecated in v0.21- addnode, banscore & whitelisted.

amitiuttarwar commented at 6:35 PM on December 23, 2020: contributor

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

amitiuttarwar force-pushed on Dec 23, 2020

amitiuttarwar commented at 6:41 PM on December 23, 2020: contributor

Updated to include PR # in release notes

DrahtBot added the label Docs on Dec 23, 2020

DrahtBot added the label P2P on Dec 23, 2020

DrahtBot added the label RPC/REST/ZMQ on Dec 23, 2020

amitiuttarwar commented at 9:37 PM on December 23, 2020: contributor

~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 784ef8be41c7e5130a6b063b359031ee1ce75aff in https://github.com/bitcoin-core/gui/pull/34. I have since updated this branch to remove m_legacyWhitelisted

DrahtBot commented at 9:40 PM on December 23, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20721 (Net: Move ping data to net_processing by jnewbery)
#20685 (Add I2P support using I2P SAM by vasild)
#20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
#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.

in doc/release-notes.md:68 in 3653ac587f outdated

  62 | @@ -63,6 +63,11 @@ P2P and network changes
  63 |  
  64 |  Updated RPCs
  65 |  ------------
  66 | +- `getpeerinfo` no longer supports the following fields: `addnode`, `banscore`,
  67 | +  and `whitelisted`, which were previously deprecated in 0.21. Instead of
  68 | +  `addnode`, the `connection_type` field will return manual. Instead of

jonatack commented at 9:46 PM on December 23, 2020:

maybe s/no longer supports/no longer returns/

and s/will return manual/returns "manual"/

amitiuttarwar commented at 6:42 AM on December 24, 2020:

sure, done.

jonatack commented at 9:47 PM on December 23, 2020: member

Approach ACK

amitiuttarwar force-pushed on Dec 24, 2020

amitiuttarwar commented at 6:44 AM on December 24, 2020: contributor

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

MarcoFalke removed the label Docs on Dec 24, 2020

MarcoFalke removed the label P2P on Dec 24, 2020

MarcoFalke commented at 6:49 AM on December 24, 2020: member

Nice cleanup. Concept ACK

DrahtBot added the label Needs rebase on Dec 26, 2020

[rpc] Remove deprecated "addnode" field from getpeerinfo 537053336f

[rpc] Remove deprecated "banscore" field from getpeerinfo 094c3beaa4

[rpc] Remove deprecated "whitelisted" field from getpeerinfo b1a936d4ae

[doc] Add release notes for removed getpeerinfo fields. 454a4088a8

amitiuttarwar force-pushed on Dec 26, 2020

amitiuttarwar commented at 9:33 PM on December 26, 2020: contributor

rebased

DrahtBot removed the label Needs rebase on Dec 26, 2020

sipa commented at 5:26 AM on December 27, 2020: member

utACK 454a4088a87eac5878070b26d13d5574da891877

in src/net.cpp:1127 in 454a4088a8

1120 | @@ -1123,8 +1121,6 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1121 |      CNode* pnode = new CNode(id, nodeServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion);
1122 |      pnode->AddRef();
1123 |      pnode->m_permissionFlags = permissionFlags;
1124 | -    // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility)
1125 | -    pnode->m_legacyWhitelisted = legacyWhitelisted;

jnewbery commented at 9:05 AM on December 27, 2020:

You can get rid of the local variable legacyWhitelisted entirely.

jnewbery commented at 9:05 AM on December 27, 2020: member

ACK 454a4088a87eac5878070b26d13d5574da891877.

You can remove a no-longer-needed local variable if you touch this again.

MarcoFalke merged this on Dec 28, 2020

MarcoFalke closed this on Dec 28, 2020

sidhujag referenced this in commit a12c4eaf6e on Dec 28, 2020

sidhujag referenced this in commit 00d5544a33 on Dec 28, 2020

michaelfolkson commented at 1:39 PM on December 29, 2020: contributor

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

MarcoFalke commented at 2:10 PM on December 29, 2020: member

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#pullrequestreview-558913726 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 (https://github.com/bitcoin/bitcoin/pull/20797#discussion_r549701244) is not the way to go. Personally, I prefer correct code over code that is incorrect, but doesn't emit any warnings.

michaelfolkson commented at 2:22 PM on December 29, 2020: contributor

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.

jonatack commented at 2:35 PM on December 29, 2020: member

Rushing to "fix" (silence) them with patches that will break code (https://github.com/bitcoin/bitcoin/pull/20797#discussion_r549701244) 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.

MarcoFalke commented at 2:44 PM on December 29, 2020: member

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#pullrequestreview-558913726. @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.

jonatack commented at 2:48 PM on December 29, 2020: member

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

jnewbery commented at 3:08 PM on December 29, 2020: member

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.

michaelfolkson commented at 4:31 PM on December 29, 2020: contributor

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")

DrahtBot locked this on Aug 18, 2022