Page MenuHomePhabricator

Make the number service of service flag bits to display a constant
ClosedPublic

Authored by Fabien on Jan 30 2019, 15:17.

Details

Summary

This is a follow-up of D2442. It makes the number of bits to consider
for display in the "peer details" view a constant.

Test Plan
./src/qt/bitcoin-qt -prune=550 &
mkdir /tmp/datadir
./src/qt/bitcoin-qt -datadir=/tmp/datadir -connect=127.0.0.1

In the second instance:
Go to "Help=>Debug window"
Select the tab "Peers"
Click the 127.0.0.1 peer in the upper table
Check in the details that the LIMITED flag is displayed

Diff Detail

Repository
rABC Bitcoin ABC
Branch
constant_service_flag_depth
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4912
Build 7887: Bitcoin ABC Buildbot (legacy)
Build 7886: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Jan 30 2019, 17:35
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/qt/guiutil.cpp
899 ↗(On Diff #7030)

Rather than having an arbitrary constant, we can make it dependent on the flag that we want to detect up to. Something like:

// check is largest flag to detect up to
uint64_t check = NODE_NETWORK_LIMITED;
while (check != 0) {
    if (mask & check) {
        ...
    }
    check = check >> 1;
}
This revision now requires changes to proceed.Jan 30 2019, 17:35

Update using the largest flag position as the boundary instead of a constant

src/qt/guiutil.cpp
891 ↗(On Diff #7049)

Note: I reverted the loop direction from @jasonbcox proposal, in order to keep the same display ordering

src/qt/guiutil.cpp
892 ↗(On Diff #7049)

Add a constant in the enum that indicate the end of the main services range.

src/qt/guiutil.cpp
892 ↗(On Diff #7049)

While not

for (uint64_t check = 1; check <= NODE_LAST_SERVICE_BIT; check <<= 1;)
deadalnix requested changes to this revision.Jan 31 2019, 22:15
This revision now requires changes to proceed.Jan 31 2019, 22:15
deadalnix requested changes to this revision.Feb 1 2019, 17:07
deadalnix added inline comments.
src/qt/guiutil.cpp
92 ↗(On Diff #7087)

No, put it in the enum with a value that define the end of the range that you want to be used for flag (I assume we don't want to display the experimental ones. It's not like this is performance critical and we want to shave off all we can from that loop.

This revision now requires changes to proceed.Feb 1 2019, 17:07

Move the boundary into the service bits enum

deadalnix requested changes to this revision.Feb 1 2019, 23:37
deadalnix added inline comments.
src/protocol.h
313 ↗(On Diff #7104)

No, put the actual value of the last possible flag.

This revision now requires changes to proceed.Feb 1 2019, 23:37

Set the constant as the last non experimental service bit.
No longer filter more than avoiding experimental bits.

deadalnix added inline comments.
src/qt/guiutil.cpp
891

Really, if the maximum value is 1 << 23, I'm not sure we need a uint64_t

jasonbcox requested changes to this revision.Feb 8 2019, 20:13
jasonbcox added inline comments.
src/protocol.h
313

If you want to keep the same behavior, this should be (1 << 11), otherwise there will be a list of UNKNOWN for flags 11 to 23. If you think there is value in keeping this flag as 23, then please re-verify that this renders ok in the RPC console in bitcoin-qt.

This revision now requires changes to proceed.Feb 8 2019, 20:13
Fabien requested review of this revision.Feb 8 2019, 22:57
Fabien added inline comments.
src/protocol.h
313

I checked in BU and XT to see if they defined service flags above 1 << 10, and found none.
Even if this is not a strong guarantee, there should not be a lot of bits to display above the previous limit.
That said, the rendering adapts itself to the width needed for display. The maximum I got with a BU peer is:
NETWORK & BLOOM & XTHIN & CASH & UNKNOWN[64].

src/protocol.h
313

It is unknown only if it is set. And if it is set, why tell the user about it ?

313

Plus, Why the fuck would protocol.h care about the qt interface ?

jasonbcox added inline comments.
src/protocol.h
313

Fair.

This revision is now accepted and ready to land.Feb 8 2019, 23:46
This revision was automatically updated to reflect the committed changes.