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

Coin Control Sorting #427

Closed
proletesseract opened this issue Feb 25, 2019 · 16 comments

Comments

Projects
None yet
4 participants
@proletesseract
Copy link
Member

commented Feb 25, 2019

When i open coin control and i have it in tree mode, i try to sort the coins by confirmations but it doesn't sort correctly. It sorts in descending order by the left most digit instead of right.

eg. the confirmations show:

8766
8755
867
8657

The tx with confirmation 867 should not appear here.

The other digit groupings are the same e.g.

7547
7541
7536
75
7487

The tx with confirmation 75 should not appear here.

Expected behaviour is that it should correctly sort the transactions by confirmation in true numerical descending / ascending order.

NavCoin Core v4.5.2
Linux x86_64

@aguycalled

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

not seeing it

image

@proletesseract

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

I'm not sure if it makes a difference but i was in tree mode and also looking at a wallet with cold staking spending keys in it on linux. I'll poke around a bit more and see if i can replicate or refute it on other OS or other transaction types. What OS are you testing on? OSX?

@proletesseract

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

in addition to this ticket, the reason why i was in coin control is that i was trying to spend coin's from a specific address. It took quite a long time to use coin control to select the individual inputs i wanted to use from the address i was trying to move coins out of. I'm not sure if it's worth the effort, but i was wondering if an extension of the coin control feature could be to add the capability to choose a specific address to spend from instead of needing to choose specific outputs to use. so if i had multiple addresses in my wallet that act kind of like savings / checking accounts, i could use it to say "spend 5000 NAV from my savings account please" and it would do that and just figure out the best inputs to use that match the chosen address.

Know what i mean? it's probably quite specific to how im using the wallet, but i thought it could be a good feature after my experience. i could split this out into its own ticket if you think it has merit?

@roast-nav

This comment has been minimized.

Copy link

commented Mar 15, 2019

The sort order appears to be alphabetic rather than numeric. In excel, this happens when cells are being treated as text rather than numbers.

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

not seeing it

image

I think it might not show on your part, since your inputs are so close together.

I think @roast-nav is correct, it seems to sort by alpha instead of numeric on my wallet as well.

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Seems I can't replicate this issue on master branch as of now.

Screenshot from 2019-05-01 17-22-58

Was this perhaps fixed already in the 4.6.0 RC changes?

@aguycalled

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Might be system-dependent?

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Might be system-dependent?

Could be, will have to do more testing.

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I did some additional testing.

I found the problem, this bug is caused by the QT version that we use in depends.

I tested with a newer version 5.9.x and the sorting was working as expected, when tested with the version in /depends DIR, the sorting is wrong.

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

This issue is also present on windows builds that are using qt 5.7.1

So it's confirmed a qt 'bug'

Capture

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@aguycalled I think there are 2 ways we can solve this, either we patch qt 5.7.1 (Add a new patch file in depends) or we upgrade to qt 5.9

I can confirm that the bug in 5.9 is squashed.

@aguycalled

This comment has been minimized.

Copy link
Member

commented May 1, 2019

i just checked bitcoin core is using qt 5.9.7, so it might be worth a try

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

i just checked bitcoin core is using qt 5.9.7, so it might be worth a try

Ok, i'll get a branch cooking and upgrade to 5.9.7? or should we use latest? (5.9.8)

@aguycalled

This comment has been minimized.

Copy link
Member

commented May 1, 2019

5.9.8 includes mostly only security fixes so i'd say 5.9.8
https://blog.qt.io/blog/2019/04/18/qt-5-9-8-released/

@mxaddict

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Roger that.

@aguycalled

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Paid 191c03a461c0950fb477d4c2584783071e5ad097c01821790a0099eb96678d0d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.