[rpc] Remove deprecated fields from getpeerinfo #20755

pull amitiuttarwar wants to merge 4 commits into bitcoin:master from amitiuttarwar:2020-12-getpeerinfo-deprecate changing 10 files +17 −106
  1. 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.
  2. 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 :)
  3. amitiuttarwar force-pushed on Dec 23, 2020
  4. amitiuttarwar commented at 6:41 pm on December 23, 2020: contributor
    Updated to include PR # in release notes
  5. DrahtBot added the label Docs on Dec 23, 2020
  6. DrahtBot added the label P2P on Dec 23, 2020
  7. DrahtBot added the label RPC/REST/ZMQ on Dec 23, 2020
  8. 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

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

  10. 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.
  11. jonatack commented at 9:47 pm on December 23, 2020: member
    Approach ACK
  12. amitiuttarwar force-pushed on Dec 24, 2020
  13. 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.
  14. MarcoFalke removed the label Docs on Dec 24, 2020
  15. MarcoFalke removed the label P2P on Dec 24, 2020
  16. MarcoFalke commented at 6:49 am on December 24, 2020: member
    Nice cleanup. Concept ACK
  17. DrahtBot added the label Needs rebase on Dec 26, 2020
  18. [rpc] Remove deprecated "addnode" field from getpeerinfo 537053336f
  19. [rpc] Remove deprecated "banscore" field from getpeerinfo 094c3beaa4
  20. [rpc] Remove deprecated "whitelisted" field from getpeerinfo b1a936d4ae
  21. [doc] Add release notes for removed getpeerinfo fields. 454a4088a8
  22. amitiuttarwar force-pushed on Dec 26, 2020
  23. amitiuttarwar commented at 9:33 pm on December 26, 2020: contributor
    rebased
  24. DrahtBot removed the label Needs rebase on Dec 26, 2020
  25. sipa commented at 5:26 am on December 27, 2020: member
    utACK 454a4088a87eac5878070b26d13d5574da891877
  26. 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.
  27. 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.

  28. MarcoFalke merged this on Dec 28, 2020
  29. MarcoFalke closed this on Dec 28, 2020

  30. sidhujag referenced this in commit a12c4eaf6e on Dec 28, 2020
  31. sidhujag referenced this in commit 00d5544a33 on Dec 28, 2020
  32. 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.
  33. 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.

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

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

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

  37. 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.
  38. 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.

  39. 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”)

  40. DrahtBot locked this on Aug 18, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-25 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me