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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Jan 30 2019, 15:17
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 30 2019, 15:17
Herald added a subscriber: schancel. · View Herald Transcript
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
Fabien updated this revision to Diff 7049.Jan 31 2019, 11:00

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

Fabien added inline comments.Jan 31 2019, 11:02
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

deadalnix added inline comments.Jan 31 2019, 22:10
src/qt/guiutil.cpp
892 ↗(On Diff #7049)

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

deadalnix added inline comments.Jan 31 2019, 22:12
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
Fabien updated this revision to Diff 7087.Feb 1 2019, 09:23

Update as per feedback

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
Fabien updated this revision to Diff 7104.Feb 1 2019, 20:54

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
Fabien updated this revision to Diff 7237.Feb 8 2019, 08:40

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

deadalnix accepted this revision.Feb 8 2019, 13:03
deadalnix added inline comments.
src/qt/guiutil.cpp
891 ↗(On Diff #7237)

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 ↗(On Diff #7237)

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 ↗(On Diff #7237)

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

deadalnix added inline comments.Feb 8 2019, 23:04
src/protocol.h
313 ↗(On Diff #7237)

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

313 ↗(On Diff #7237)

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

jasonbcox accepted this revision.Feb 8 2019, 23:46
jasonbcox added inline comments.
src/protocol.h
313 ↗(On Diff #7237)

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.