I find this far more useful for than the totals. For example, I can easily see which nodes are not really talking with this metric, and not so much with the totals (as is current functionality).
Example:-
rebroad marked this as ready for review
on Apr 29, 2021
hmmm. well, the units indicate that it is a different metric, so I’m not sure if the addition of the word “total” is needed. What do other people think?
@rebroad
Since you’re adding this function (formatBps) you should add the Doxygen comment with it. formatBytes not including a Doxygen when it was introduced is another issue that can be addressed in another PR.
@jarolrod given that formatBps is so similar to formatBytes, I’m averse to doing the Doxygen for formatBytes as part of this pull, and I’m also averse to doing Doxygen for just formatBps without formatBytes in this pull, but I’m fine with doing a later pull request to add the Doxygen for both formatBytes and formatBps. I think the Doxygen therefore belongs in a separate pull request tackling a number of missing functions.
A translator comment is the easiest way to let translators know what “bps” is.
jarolrod
commented at 6:37 am on April 30, 2021:
member
Concept ACK
I agree that this is more useful than showing the totals. One can still see the totals by clicking on the peer.
One issue is that Sent and Received are now used in two different contexts. In the peer table, it’s denoting how many bytes are currently being sent/received. In the sidebar it’s denoting the total sent/received.
My suggestion is to change the Sent/Received text in the sidebar to Total Sent/Total Received.
in
src/qt/peertablemodel.cpp:127
in
6cfd19b682outdated
ISTM this calculation will be useful shortly after the connection time but for long-lived peers would provide little to no real-time feedback. Maybe use the time in seconds since last message sent/received like in -netinfo?
jonatack
commented at 7:51 am on April 30, 2021:
contributor
Worth exploring. The sidebar probably should be updated to display the same data, possibly in addition to the totals.
rebroad renamed this:
Display send/recv in Bps instead of totals in the debug window
WIP - Display send/recv in Bps instead of totals in the debug window
on Apr 30, 2021
hebasto
commented at 10:28 am on April 30, 2021:
member
Changed to WIP as I just noticed the column sorting is still using the old values.
You also could convert it to a draft:
ah, thank you - I was wondering if that was possible… the terminology confuses me though as I’d say it’s ready for review, but not perhaps ready to be merged.
rebroad force-pushed
on Apr 30, 2021
rebroad
commented at 9:42 pm on April 30, 2021:
contributor
Worth exploring. The sidebar probably should be updated to display the same data, possibly in addition to the totals.
I considered doing this, but I think it’s an overall advantage to not have too much data there. It’s more useful to have bandwidth in the columns, and totals in the sidebar, IMHO.
rebroad renamed this:
WIP - Display send/recv in Bps instead of totals in the debug window
Display send/recv in Bps instead of totals in the debug window
on Apr 30, 2021
rebroad marked this as ready for review
on Apr 30, 2021
hebasto removed the label
Design
on May 2, 2021
hebasto added the label
UX
on May 2, 2021
hebasto
commented at 9:44 am on May 2, 2021:
member
What is the base period to calculate data-transfer rate?
@hebasto actually, they are bits, but I ought to change the variable name to reflect that. Will do so, and thanks for the guidance on the commentation.
rebroad
commented at 10:15 am on May 7, 2021:
contributor
What is the base period to calculate data-transfer rate?
In my first pull request it was from connection time to current time. Now it’s from connection time to time of last send/recv.
rebroad force-pushed
on May 7, 2021
rebroad force-pushed
on May 7, 2021
rebroad force-pushed
on May 7, 2021
rebroad force-pushed
on May 7, 2021
rebroad force-pushed
on May 7, 2021
luke-jr
commented at 8:25 pm on June 12, 2021:
member
Maybe have both and let the user decide which to hide/show?
rebroad
commented at 2:32 pm on June 26, 2021:
contributor
Maybe have both and let the user decide which to hide/show?
where would you place the option?
rebroad force-pushed
on Jun 26, 2021
rebroad force-pushed
on Jun 26, 2021
luke-jr
commented at 2:57 pm on June 26, 2021:
member
Just have 4 columns, total in, total out, rate in, rate out.
Other applications let you rightclick the header area to hide/show them, but even if we can’t have that, users can resize them to ~0 width.
rebroad force-pushed
on Jun 26, 2021
rebroad
commented at 3:45 pm on June 26, 2021:
contributor
Just have 4 columns, total in, total out, rate in, rate out.
Other applications let you rightclick the header area to hide/show them, but even if we can’t have that, users can resize them to ~0 width.
It’s a pain having to resize columns, and also they cannot be resized to zero. I’d rather wait until there’s the ability to disable columns before creating a pull to add rate in addition to total bytes. There are already too many columns IMHO.
shaavan
commented at 4:51 pm on July 30, 2021:
contributor
By the logic used by op, the displayed rate is the average rate of data transferred by the peer. But I personally would prefer that if the displayed rate was instantaneous and not average because it might happen that a node is inactive for quite some time, but the data they transferred earlier was so high, that the rate column is showing a significantly large rate of data transfer.
And also:
For values larger than 1000 bps the data should be shown in kbps, but it is not happening as in the highlighted example.
rebroad
commented at 7:04 pm on September 14, 2021:
contributor
For values larger than 1000 bps the data should be shown in kbps, but it is not happening as in the highlighted example.
I agree. I’ll upload an update shortly.
rebroad force-pushed
on Sep 16, 2021
rebroad
commented at 5:17 pm on September 16, 2021:
contributor
By the logic used by op, the displayed rate is the average rate of data transferred by the peer. But I personally would prefer that if the displayed rate was instantaneous and not average because it might happen that a node is inactive for quite some time, but the data they transferred earlier was so high, that the rate column is showing a significantly large rate of data transfer.
Yes, this does happen, for example in the case where there’s IBD (initial block download) which has much higher transfer rates and then skews the numbers once that completes. I have another commit, which deals with this, and allows the calculation to be reset at the end of IBD. Shall I include that with this pull, or perhaps raise another pull in the future to include that?
shaavan
commented at 1:08 pm on September 20, 2021:
contributor
Changes since my last review:
The sent data is displayed in kbps when it is greater than 1000bps.
bits variable has been changed from uint64_t to float.
Modifies Sent and Recieve case in peertablemodel.cpp to avoid division by zero.
Changing bits variable from int to float has to lead to displaying sent and receive value with decimals. This makes the displayed data look a lot better compared to the previous version. Great Work, @rebroad!
I am adding some screenshots to emphasize the difference between the old commit and the latest commit.
Old Commit
Latest Commit
Shall I include that with this pull, or perhaps raise another pull in the future to include that?
IMO, I think it would be better to add this as an additional commit to this PR. This is a significant change. Let’s not wait for the opening of another and then waiting for it to get merged.
Display send/recv in bps instead of totals in the debug windowc1a692b69d
rebroad force-pushed
on Oct 12, 2021
shaavan
commented at 11:44 am on October 14, 2021:
contributor
Changes since my last review:
Correction has been made at line 809: 10'000 -> 100'000
I have another commit, which deals with this, and allows the calculation to be reset at the end of IBD. Shall I include that with this pull, or perhaps raise another pull in the future to include that?
Any updates on the above. Have you planned on adding it as another commit, or have you decided to do so in a future PR?
rebroad
commented at 9:01 pm on October 18, 2021:
contributor
@shaavan I am still testing the functionality for resetting at the end of IBD - have you already created a method for detecting this? I’d be interested in having a look at your code if so.
shaavan
commented at 12:44 pm on October 19, 2021:
contributor
have you already created a method for detecting this?
Unfortunately, not yet. However, I have some ideas on how to do so. Let me try to implement this as soon as possible.
DrahtBot added the label
Needs rebase
on Dec 10, 2021
DrahtBot
commented at 9:47 am on December 10, 2021:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
DrahtBot
commented at 12:11 pm on March 21, 2022:
contributor
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
hebasto
commented at 3:15 pm on April 13, 2022:
member
This is a metadata mirror of the GitHub repository
bitcoin-core/gui.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-03 10:20 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me