Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function by luke-jr · Pull Request #18165 · bitcoin/bitcoin

luke-jr · 2020-02-17T01:54:31Z

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.

hebasto · 2020-02-17T13:41:32Z

Concept ACK

Sjors · 2020-02-19T10:32:10Z

Concept ACK

… of MSB check

…agToStr 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.

laanwj · 2020-02-26T17:51:56Z

src/protocol.cpp

    return allNetMessageTypesVec;
 }
+
+std::string serviceFlagToStr(const uint64_t mask, const int bit)


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

This avoid recalculating mask

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

jonasschnelli · 2020-02-27T08:44:18Z

utACK ~~cea91a1~~ c31bc5b

DrahtBot · 2020-03-01T21:34:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS #19070 (p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS by jnewbery)
Implement ADDRv2 support (part of BIP155) #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #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.

vasild

Approach ACK

vasild · 2020-05-22T18:33:19Z

src/qt/guiutil.cpp

-        uint64_t check = 1LL << i;
+        uint64_t check = 1ull << i;
        if (mask & check)
        {
-            strList.append(serviceFlagToStr(check, i));
+            strList.append(QString::fromStdString(serviceFlagToStr(mask, i)));


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.

vasild · 2020-05-29T09:47:24Z

#18165 (comment)

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]

fanquake · 2020-05-29T09:55:58Z

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

jonasschnelli · 2020-05-29T11:41:07Z

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

vasild · 2020-05-29T11:49:14Z

I am on it, hold on!

vasild · 2020-05-29T13:58:36Z

Here is a fix: #19106

Cheerz!

Don't take two redundant arguments in `serviceFlagToStr()`. As a side effect this fixes an issue introduced in bitcoin#18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`.

189ae0c util: dedup code in callers of serviceFlagToStr() (Vasil Dimov) fbacad1 util: simplify the interface of serviceFlagToStr() (Vasil Dimov) Pull request description: Don't take two redundant arguments in `serviceFlagToStr()`. Introduce `serviceFlagsToStr()` which takes a mask (with more than one bit set) and returns a vector of strings. As a side effect this fixes an issue introduced in #18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`. ACKs for top commit: MarcoFalke: ACK 189ae0c jonasschnelli: Tested ACK 189ae0c Tree-SHA512: 000c490f16ebbba04458c62ca4ce743abffd344d375d95f5bbd5008742012032787655db2874b168df0270743266261dccf1693761906567502dcbac902bda50

Don't take two redundant arguments in `serviceFlagToStr()`. As a side effect this fixes an issue introduced in bitcoin/bitcoin#18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`.

…to a shared serviceFlagToStr function c31bc5b Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function (Luke Dashjr) cea91a1 Bugfix: GUI: Use unsigned long long type to avoid implicit conversion of MSB check (Luke Dashjr) Pull request description: 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. ACKs for top commit: jonasschnelli: utACK ~~cea91a1e40e12029140ebfba969ce3ef2965029c~~ c31bc5b Tree-SHA512: 32c7ba8ac7ef2d4087f4f317447ae93a328ec9fb9ad81301df2fbaeeb21a3db7a503187a369552b05a9414251b7cf8e15bcde74c1ea2ef36591ea7ffb6721f60

Don't take two redundant arguments in `serviceFlagToStr()`. As a side effect this fixes an issue introduced in bitcoin#18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`.

Don't take two redundant arguments in `serviceFlagToStr()`. As a side effect this fixes an issue introduced in bitcoin/bitcoin#18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`.

…agToStr function Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#18165 | PR18165]]. Depends on D8516. Test Plan: ninja all check-all ./src/qt/bitcoin-qt -server Check the service bits are displayed correctly in the peers window ./src/bitcoin-cli getnetworkinfo Check the service bits are displayed correctly Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8518

Summary: ``` Don't take two redundant arguments in `serviceFlagToStr()`. As a side effect this fixes an issue introduced in bitcoin/bitcoin#18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`. util: dedup code in callers of serviceFlagToStr() ``` Backport of core [[bitcoin/bitcoin#19106 | PR19106]]. Depends on D8518. Test Plan: ninja all check-all ./src/qt/bitcoin-qt -server Check the service bits display correctly ./src/bitcoin-cli getnetworkinfo Check the service bits display correctly Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8519

…to a shared serviceFlagToStr function

merge bitcoin#16787, bitcoin#17474, bitcoin#17687, bitcoin#18165, bitcoin#18877, bitcoin#18960, bitcoin#19010, bitcoin#19044, bitcoin#19070: block filters

189ae0c util: dedup code in callers of serviceFlagToStr() (Vasil Dimov) fbacad1 util: simplify the interface of serviceFlagToStr() (Vasil Dimov) Pull request description: Don't take two redundant arguments in `serviceFlagToStr()`. Introduce `serviceFlagsToStr()` which takes a mask (with more than one bit set) and returns a vector of strings. As a side effect this fixes an issue introduced in bitcoin#18165 due to which the GUI could print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]` instead of `NETWORK & WITNESS`. ACKs for top commit: MarcoFalke: ACK 189ae0c jonasschnelli: Tested ACK 189ae0c Tree-SHA512: 000c490f16ebbba04458c62ca4ce743abffd344d375d95f5bbd5008742012032787655db2874b168df0270743266261dccf1693761906567502dcbac902bda50

…to a shared serviceFlagToStr function

DrahtBot added GUI P2P RPC/REST/ZMQ labels Feb 17, 2020

luke-jr added 2 commits February 21, 2020 19:10

Bugfix: GUI: Use unsigned long long type to avoid implicit conversion…

cea91a1

… of MSB check

luke-jr force-pushed the svcflags2str branch from 38f8b69 to c31bc5b Compare February 21, 2020 19:10

laanwj reviewed Feb 26, 2020

View reviewed changes

This was referenced Mar 2, 2020

Serve BIP 157 compact filters #16442

Closed

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

DrahtBot mentioned this pull request May 5, 2020

[WIP] Serve BIP 157 compact filters #18876

Closed

DrahtBot mentioned this pull request May 20, 2020

Implement ADDRv2 support (part of BIP155) #19031

Closed

11 tasks

vasild reviewed May 22, 2020

View reviewed changes

fanquake mentioned this pull request May 27, 2020

doc: Separate repository for the gui #19071

Merged

DrahtBot mentioned this pull request May 27, 2020

p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS #19070

Merged

jonasschnelli merged commit de369c7 into bitcoin:master May 29, 2020

vasild mentioned this pull request May 29, 2020

util: simplify the interface of serviceFlagToStr() #19106

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Sep 18, 2021

merge bitcoin#18165: Consolidate service flag bit-to-name conversion …

84c25db

…to a shared serviceFlagToStr function

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 30, 2022

merge bitcoin#18165: Consolidate service flag bit-to-name conversion …

d9c8a29

…to a shared serviceFlagToStr function

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

luke-jr commented Feb 17, 2020

Uh oh!

hebasto commented Feb 17, 2020

Uh oh!

Sjors commented Feb 19, 2020

Uh oh!

laanwj Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild May 22, 2020

Choose a reason for hiding this comment

Uh oh!

vasild commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented May 29, 2020

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

vasild commented May 29, 2020

Uh oh!

vasild commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonasschnelli commented Feb 27, 2020 •

edited

Loading

DrahtBot commented Mar 1, 2020 •

edited

Loading

vasild commented May 29, 2020 •

edited

Loading