Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function #18165

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:svcflags2str changing 4 files +36 −31
  1. luke-jr commented at 1:54 AM on February 17, 2020: member

    Side effect: this results in the RPC showing unknown service bits as "UNKNOWN[n]" like the GUI.

    Note that there is no common mask-to-vector<string> function because both GUI and RPC would need to iterate through it to convert to their desired target formats.

  2. DrahtBot added the label GUI on Feb 17, 2020
  3. DrahtBot added the label P2P on Feb 17, 2020
  4. DrahtBot added the label RPC/REST/ZMQ on Feb 17, 2020
  5. hebasto commented at 1:41 PM on February 17, 2020: member

    Concept ACK

  6. Sjors commented at 10:32 AM on February 19, 2020: member

    Concept ACK

  7. Bugfix: GUI: Use unsigned long long type to avoid implicit conversion of MSB check cea91a1e40
  8. Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function
    Side effect: this results in the RPC showing unknown service bits as "UNKNOWN[n]" like the GUI.

Note that there is no common mask-to-vector<string> function because both GUI and RPC would need to iterate through it to convert to their desired target formats.
    c31bc5bcfd
  9. luke-jr force-pushed on Feb 21, 2020
  10. in src/protocol.cpp:203 in c31bc5bcfd 
     198 | @@ -199,3 +199,27 @@ const std::vector<std::string> &getAllNetMessageTypes()
 199 |  {
 200 |      return allNetMessageTypesVec;
 201 |  }
 202 | +
 203 | +std::string serviceFlagToStr(const uint64_t mask, const int bit)
    laanwj commented at 5:51 PM on February 26, 2020:

    Seems it would be enough to only pass in bit, or what am I missing?

    luke-jr commented at 8:17 PM on February 26, 2020:

    This avoid recalculating mask

    laanwj commented at 9:02 AM on April 30, 2020:

    That's one bit shift 1ull << bit;. Given how rarely this function is called that seems like an over-optimization.

  11. jonasschnelli commented at 8:44 AM on February 27, 2020: contributor

    utACK cea91a1e40e12029140ebfba969ce3ef2965029c c31bc5bcfddf440e9a1713f7ba2ca2bf9cfa8e2e

  12. DrahtBot commented at 9:34 PM on March 1, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19070 (p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS by jnewbery)
    • #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
    • #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)

    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.

  13. DrahtBot cross-referenced this on Mar 2, 2020 from issue Serve BIP 157 compact filters by jimpo
  14. DrahtBot cross-referenced this on Mar 2, 2020 from issue Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli
  15. DrahtBot cross-referenced this on May 5, 2020 from issue [WIP] Serve BIP 157 compact filters by jnewbery
  16. DrahtBot cross-referenced this on May 20, 2020 from issue Implement ADDRv2 support (part of BIP155) by vasild
  17. in src/qt/guiutil.cpp:748 in c31bc5bcfd 
     763 | -        uint64_t check = 1LL << i;
 764 | +        uint64_t check = 1ull << i;
 765 |          if (mask & check)
 766 |          {
 767 | -            strList.append(serviceFlagToStr(check, i));
 768 | +            strList.append(QString::fromStdString(serviceFlagToStr(mask, i)));
    vasild commented at 6:33 PM on May 22, 2020:

    Here we should pass check instead of mask? I did not test it but I think with the current patch formatServicesStr(NODE_NETWORK | NODE_WITNESS) will return a string like UNKNOWN[9] & UNKNOWN[9].

    The arguments to serviceFlagToStr() are redundant (one can be derived from the other easily) which I think is confusing and could lead to slippages. I agree with @laanwj that just one of them should be passed.

  18. vasild commented at 6:34 PM on May 22, 2020: contributor

    Approach ACK

  19. fanquake cross-referenced this on May 27, 2020 from issue doc: Separate repository for the gui by MarcoFalke
  20. DrahtBot cross-referenced this on May 27, 2020 from issue p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS by jnewbery
  21. jonasschnelli merged this on May 29, 2020
  22. jonasschnelli closed this on May 29, 2020
  23. vasild commented at 9:47 AM on May 29, 2020: contributor

    #18165 (review)

    Now that this PR got merged I tested whether my concern above was legit. It was:

    instead of

    NETWORK & WITNESS

    now I see

    UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]
  24. fanquake commented at 9:55 AM on May 29, 2020: member

    @vasild Thanks for following up and testing, did you want to open a PR to fix this?

  25. jonasschnelli commented at 11:41 AM on May 29, 2020: contributor

    @vasild. Oh. I missed that. Would be great if you can fix it via a new PR.

  26. vasild commented at 11:49 AM on May 29, 2020: contributor

    I am on it, hold on!

  27. vasild referenced this in commit 0b6f4a0729 on May 29, 2020
  28. vasild cross-referenced this on May 29, 2020 from issue util: simplify the interface of serviceFlagToStr() by vasild
  29. vasild commented at 1:58 PM on May 29, 2020: contributor

    Here is a fix: #19106

    Cheerz!

  30. vasild referenced this in commit fbacad1880 on May 29, 2020
  31. jonasschnelli referenced this in commit 8ad5f1c376 on May 29, 2020
  32. Stackout referenced this in commit dca2c9b3fd on May 30, 2020
  33. sidhujag referenced this in commit d05c602105 on May 31, 2020
  34. sidhujag referenced this in commit 5434622aca on May 31, 2020
  35. janus referenced this in commit d7dd95d724 on Nov 15, 2020
  36. jasonbcox referenced this in commit 81a1f3236e on Nov 25, 2020
  37. jasonbcox referenced this in commit 3114174d88 on Nov 25, 2020
  38. kwvg referenced this in commit fa83c26349 on Aug 22, 2021
  39. kwvg referenced this in commit 76c857dd6b on Aug 22, 2021
  40. kwvg referenced this in commit 79f560cf18 on Sep 16, 2021
  41. kwvg referenced this in commit 84c25db459 on Sep 18, 2021
  42. UdjinM6 referenced this in commit 4ffd42de63 on Sep 21, 2021
  43. PastaPastaPasta referenced this in commit c5f3b478bf on Sep 21, 2021
  44. thelazier referenced this in commit c1ccaeb9e0 on Sep 25, 2021
  45. kwvg referenced this in commit 3161118ad0 on Oct 12, 2021
  46. kwvg referenced this in commit 83c7694c59 on Oct 12, 2021
  47. gades referenced this in commit d9c8a29bef on May 30, 2022
  48. bitcoin locked this on Aug 16, 2022
Contributors
luke-jrhebastoSjorslaanwjjonasschnelliDrahtBotvasildfanquake
Labels
GUIRPC/REST/ZMQP2P
Linked (view graph)
#19071 doc: Separate repository for the gui#19106 util: simplify the interface of serviceFlagToStr()
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: 2026-05-19 06:54 UTC