bitcoin-core / gui-qml

Bitcoin GUI (experimental QML-based fork)
MIT License
106 stars 40 forks source link

qml: Add PeerDetails page #387

Closed johnny9 closed 4 months ago

johnny9 commented 5 months ago

This commit introduces a new page, PeerDetails, that shows additional information on a Peer after it is selected from the list of Peers. After clicking on the peer, the new page gets pushed onto the settings stack. Two conditions will cause the view to pop, the user clicking the Back at the top left or the peer disconnecting.

For reviewers, I would very much like comments on the conversion of the Stats struct to the QML compatible object that is used.

This commit does not include the disconnect/ban buttons, the top right buttons to cycle through the peers, or the icons that are adjacent to the values.

Link to github actions build artifacts.

[Build Artifacts]()

johnny9 commented 5 months ago

From 845a878 to a660b9c

johnny9 commented 5 months ago

Update from a660b9c to 8c2a918

johnny9 commented 5 months ago

Update from ef6e826 to b5e2de78b5c19f43100a0f49949a3eac4b96ca8a

MarnixCroes commented 5 months ago

b5e2de78b5c19f43100a0f49949a3eac4b96ca8a weird jumping when clicking on peer, at Direction, User agent_, Type, Ip and Network cannot repro at ID

jumping.webm

johnny9 commented 5 months ago

b5e2de7 weird jumping when clicking on peer, at Direction, User agent_, Type, Ip and Network cannot repro at ID jumping.webm

This appears to be due to using the wrong index for the original model. I have updated the branch with a working solution

johnny9 commented 5 months ago

Update from b5e2de7 to b7161c5

GBKS commented 5 months ago

Great work. I tested it out on Android. Looks great overall, just see some inconsistencies to the design in terms of vertical spacing and missing formatting.

For text that does not fit in one line, I think it should just go multi-line. If you shorten it, you then need a way to expand it (which could be a simple tap).

In the image below, the Android screenshot is on the right, and the Figma mock-ups are on the left.

image

A few questions:

MarnixCroes commented 5 months ago

@GBKS

What does a "Synced headers" value of "-1" mean? That the process is not initialized?

yes, it means that that peer didn't announce that info yet

What are permissions? What does "N/A" mean?

any special permissions that have been granted to that peer. N/A means Not Applicable / Not Assessed

What does "Mapped AS" mean?

Mapped Autonomous System, which can be used for diversifying peer selection

For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

As peers are connecting and disconnecting all the time this is quite hard: Do you measure it for all currently connected peers, since the node is up, since first launch... etc. and these values aren't that meaningful: a higher amount sent/received doesn't necessarily mean that the node relies more on this peer than another peer with a lower amount.

So, I get what you mean, but this shouldn't be added imo

johnny9 commented 5 months ago

Great work. I tested it out on Android. Looks great overall, just see some inconsistencies to the design in terms of vertical spacing and missing formatting.

Ill adjust that spacing. Looks like mine is too tight.

For text that does not fit in one line, I think it should just go multi-line. If you shorten it, you then need a way to expand it (which could be a simple tap).

I'll use word wrap on the next update

A few questions:

* What does a "Synced headers" value of "-1" mean? That the process is not initialized

It means that it hasn't synced any headers with this peer yet. -1 is the default vault. I think this should just probably just be a minus as well until this data updates.

* What are permissions? What does "N/A" mean?

These are all of the permission strings "bloomfilter", "noban", "forcerelay" "relay" "mempool" "download" "addr". N/A just means that none are set. These permissions are assigned by using the "-whitelist" configuration option. Here are the description of these flags. "bloomfilter (allow requesting BIP37 filtered blocks and transactions)", "noban (do not ban for misbehavior; implies download)", "forcerelay (relay transactions that are already in the mempool; implies relay)", "relay (relay even in -blocksonly mode, and unlimited transaction announcements)", "mempool (allow requesting BIP35 mempool contents)", "download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)", "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"

* What does "Mapped AS" mean?

Mapped AS means Mapped Autonomous System which is a configuration option that lets you map peers to AS numbers. AS numbers are assigned by top level internet registries to organizations with a bunch of network addresses. You specify a mapping file to bitcoin core with a configuration option.

* For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

Yeah I think there is a lot of value in that.

johnny9 commented 5 months ago

Update from b7161c5 to d737ea3

MarnixCroes commented 5 months ago

running d737ea3406c3e8e99a5a93f31dc03725a06a0823, I now had twice that the peer details page closed itself randomly and returned to the peers page. peer didn't disconnect, not sure what is causing it.

Changed IP to Address

forgot to add the change?

Allowed Text to WordWrap

this looks good 👍

GBKS commented 5 months ago

Thanks for the explainers and tweaking the layout.

These permissions are assigned by using the "-whitelist" configuration option

Does that mean permissions are application-wide and the same for all peers? If so, maybe they can be shown elsewhere...?

I think this should just probably just be a minus as well until this data updates.

Are we using "N/A" and "-" interchangeably? Looks like we need states for

Does that seem about right? What's the likelihood of getting "not available"? Is that even possible?

johnny9 commented 5 months ago

running d737ea3, I now had twice that the peer details page closed itself randomly and returned to the peers page. peer didn't disconnect, not sure what is causing it.

Yeah im worried about this. I will dig into that and make sure my signals are being triggered correctly

GBKS commented 5 months ago

I now had twice that the peer details page closed itself randomly and returned to the peers page

I also had this happen.

yashrajd commented 5 months ago

Good stuff guys.

In line with external feedback we received in past Bitcoin Core calls, a little tooltip and explainers for these items would be extremely useful.

Even the tip that @johnny9 provided about how peer permissions can be changed would be useful (on this page or somewhere else)

@GBKS

What does a "Synced headers" value of "-1" mean? That the process is not initialized?

yes, it means that that peer didn't announce that info yet

What are permissions? What does "N/A" mean?

any special permissions that have been granted to that peer. N/A means Not Applicable / Not Assessed

What does "Mapped AS" mean?

Mapped Autonomous System, which can be used for diversifying peer selection

For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

As peers are connecting and disconnecting all the time this is quite hard: Do you measure it for all currently connected peers, since the node is up, since first launch... etc. and these values aren't that meaningful: a higher amount sent/received doesn't necessarily mean that the node relies more on this peer than another peer with a lower amount.

So, I get what you mean, but this shouldn't be added imo

GBKS commented 5 months ago

For this PR generally, I'm happy to ACK it whenever the functionality is solid and we can tweak visualization details and tips as we live with it during practical usage.

johnny9 commented 5 months ago

Transport version and Connection Time with the peer should be added imo

I think we need to re-sync with main before we add transport type but I can add Connection Time to the Network traffic section

johnny9 commented 5 months ago

Update from b4f8fc1 to c8344d8

MarnixCroes commented 4 months ago

Transport version and Connection Time with the peer should be added imo

and the Network (ipv4, onion etc), sorry forgot about that one

johnny9 commented 4 months ago

Update from c8344d8 to eb5030f

Screenshot from 2024-02-14 00-22-35

johnny9 commented 4 months ago

eb5030f the new spacing can cause some overlap

image

Thank you, updated with the height attached to the lineCount of the value

component PeerKeyValueRow: Row {
        width: parent.width
        property string key: ""
        property string value: ""
        height: 21 * valueText.lineCount
        spacing: 10
        CoreText {
            color: Theme.color.neutral9;
            text: key;
            width: 125;
            horizontalAlignment: Qt.AlignLeft;
        }
        CoreText {
            id: valueText
            color: Theme.color.neutral9;
            elide: Text.ElideRight;
            wrapMode: Text.WordWrap;
            text: value
            width: parent.width - 125;
            horizontalAlignment: Qt.AlignLeft;
        }
    }
yashrajd commented 4 months ago

Just ran the latest artefact, and it looks pretty good. Didn't see any spacing issues. ACK.

GBKS commented 4 months ago

Thank you, updated with the height attached to the lineCount of the value

Do I see it correctly that the line height is hard-coded? Users can typically adjust text size via accessibility settings, which means the line height will also differ. Is there a way to automatically calculate this value?

In Figma and CSS, I usually define the line height as a percentage (see how that is defined in CSS here). Is that possible? Might be easier to work with.

jarolrod commented 4 months ago

https://github.com/bitcoin-core/gui-qml/pull/388 is proposed as an alternative which handles some of the outstanding issues, implements more functionality

johnny9 commented 4 months ago

Closing this as it is superseded with #388