Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Honor inbound/outbound arrow prefix when sorting peer address column #400

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Aug 8, 2021

Fixes #397

This re-establishes a functionality that was lost I guess since 0.21.

Peer address column sort with current master:
Peers 27 and 51 are sorted without honoring the connection direction.
address_sort_without_prefix

Peer address column sort with this PR:
Peers 16 and 22 are sorted honoring the connection direction.
address_sort_with_prefix

I would be grateful for implementation improvement suggestions.

@hebasto hebasto added the UX All about "how to get things done" label Aug 8, 2021
@hebasto
Copy link
Member

hebasto commented Aug 8, 2021

This re-establishes a functionality that was lost I guess since 0.21.

This is not true:

case PeerTableModel::Address:
return pLeft->addrName.compare(pRight->addrName) < 0;

The implementation remains unchanged since v0.10.0 when it was introduced in bitcoin/bitcoin#4225.


UPDATE:

@wodry The arrows were added to addresses in your bitcoin/bitcoin#13537 (since v0.17.0).

@ghost
Copy link
Author

ghost commented Aug 8, 2021

OK thanks, then my guess was wrong and i did really not stumble upon this issue by chance. I thought it might have been lost with the new sortProxy.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Ubuntu 20.04

The sorting of addresses in Master is purely lexicographical, irrespective of the direction of peer. This PR adds the functionality of taking the direction of peers as a prior consideration while sorting the addresses. The PR still sorts addresses in lexicographical order for the same direction peers.
I was able to compile and test the PR on Ubuntu 20.04 successfully.

Though I like the approach, since we want #317, it is not worth merging this PR because PR #317 would revert these changes.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that the meaning of arrows are ambiguous, I am personally concept 0 on this in favor of #317. These PR's are essentially competing as #317 is just going to remove the arrows.

@hebasto
Copy link
Member

hebasto commented Aug 11, 2021

Closing, as #317 has been merged.

@hebasto hebasto closed this Aug 11, 2021
@ghost ghost deleted the address_prefix_sort branch August 12, 2021 07:28
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Peer address column sorting does not honor the inbound/outbound arrow prefix
3 participants