Yet another Coin Control Release #2343

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 4, 2013

Member

I'm generally in favor of this sort of thing, in that it helps foster understanding of the system (e.g. understanding that it spends coins and not accounts) and avoids people getting pushed to webpages to get similar functionality.

I'm not keen on the "back to input" change option, it's not obvious to some people why they shouldn't reuse addresses and it shouldn't be something the system encourages, it's also not obvious what it means when there is more than one input selected.

It would be nice if fees were computed as soon as inputs are selected as it would make it easier to construct changeless transactions. One way to do that may be to just assume that there is always a change output while computing a conservative size.

Member

gmaxwell commented Mar 4, 2013

I'm generally in favor of this sort of thing, in that it helps foster understanding of the system (e.g. understanding that it spends coins and not accounts) and avoids people getting pushed to webpages to get similar functionality.

I'm not keen on the "back to input" change option, it's not obvious to some people why they shouldn't reuse addresses and it shouldn't be something the system encourages, it's also not obvious what it means when there is more than one input selected.

It would be nice if fees were computed as soon as inputs are selected as it would make it easier to construct changeless transactions. One way to do that may be to just assume that there is always a change output while computing a conservative size.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 4, 2013

Contributor

update: removed "back to input"

@gmaxwell I am actually already computing the fees while selecting inputs assuming 2 outputs

Contributor

cozz commented Mar 4, 2013

update: removed "back to input"

@gmaxwell I am actually already computing the fees while selecting inputs assuming 2 outputs

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 4, 2013

Contributor

sorry guys, the build issues are because I never tested this with Qt 4.7, I will commit an update in the next days.

Contributor

cozz commented Mar 4, 2013

sorry guys, the build issues are because I never tested this with Qt 4.7, I will commit an update in the next days.

@eldentyrell

This comment has been minimized.

Show comment Hide comment
@eldentyrell

eldentyrell Mar 4, 2013

Please merge this. For many users, having this feature in the client (and not some external utility) is a higher priority than anything else that has happened since the 0.6 release. Consider how that affects their incentive to upgrade.

Please merge this. For many users, having this feature in the client (and not some external utility) is a higher priority than anything else that has happened since the 0.6 release. Consider how that affects their incentive to upgrade.

@gmaxwell gmaxwell referenced this pull request Mar 4, 2013

Closed

Coin Control #1359

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Mar 5, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e90eb8f68d245364022c840ceee496a836b0cf5b for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e90eb8f68d245364022c840ceee496a836b0cf5b for binaries and test log.

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Mar 5, 2013

Contributor

How was this or should this be tested? Can you write up (or, better, get somebody to write up) a test plan? See https://github.com/bitcoin/QA/blob/master/TestPlanCreation.md for how.

This is definitely a large enough feature with enough possibility of disastrous consequences if there is a bug (people sending coins to /dev/null) to spend some good solid time testing carefully.

Contributor

gavinandresen commented Mar 5, 2013

How was this or should this be tested? Can you write up (or, better, get somebody to write up) a test plan? See https://github.com/bitcoin/QA/blob/master/TestPlanCreation.md for how.

This is definitely a large enough feature with enough possibility of disastrous consequences if there is a bug (people sending coins to /dev/null) to spend some good solid time testing carefully.

@rebroad

This comment has been minimized.

Show comment Hide comment
@rebroad

rebroad Mar 5, 2013

Contributor

I concur with @eldentyrell - if this isn't merged then a fork will occur, which may potentially be more popular than this one - some diversity of this client is probably a good thing though to maintain what bitcoin is meant to be about - decentralised.

Contributor

rebroad commented Mar 5, 2013

I concur with @eldentyrell - if this isn't merged then a fork will occur, which may potentially be more popular than this one - some diversity of this client is probably a good thing though to maintain what bitcoin is meant to be about - decentralised.

@eldentyrell

This comment has been minimized.

Show comment Hide comment
@eldentyrell

eldentyrell Mar 5, 2013

Actually I'm not worried about a fork so much as people simply using increasingly-ancient versions of the client. Right now there are a bunch of people who will simply keep using coderrr's 0.6-based binaries forever if they aren't offered something strictly more useful.

Actually I'm not worried about a fork so much as people simply using increasingly-ancient versions of the client. Right now there are a bunch of people who will simply keep using coderrr's 0.6-based binaries forever if they aren't offered something strictly more useful.

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

@rebroad I'm wondering why you think it's appropriate to use threatening language over code that doesn't currently compile? Good luck with that fork.

Member

gmaxwell commented Mar 5, 2013

@rebroad I'm wondering why you think it's appropriate to use threatening language over code that doesn't currently compile? Good luck with that fork.

@eldentyrell

This comment has been minimized.

Show comment Hide comment
@eldentyrell

eldentyrell Mar 5, 2013

This is definitely a large enough feature with enough possibility of disastrous consequences if there is a bug (people sending coins to /dev/null) to spend some good solid time testing carefully.

Keep in mind that the people who use this feature will choose an older guaranteed-to-be-more-buggy-since-other-bugs-have-been-fixed-since-then client if the latest bitcoin.org client doesn't offer this. Testing is great, but merging this produces a net decrease in user bug exposure even if it isn't perfect.

Alternatively: since this functionality is hidden from the user by default, put a "WARNING: alpha quality" next to the checkbox they need to check in order to enable it.

This is definitely a large enough feature with enough possibility of disastrous consequences if there is a bug (people sending coins to /dev/null) to spend some good solid time testing carefully.

Keep in mind that the people who use this feature will choose an older guaranteed-to-be-more-buggy-since-other-bugs-have-been-fixed-since-then client if the latest bitcoin.org client doesn't offer this. Testing is great, but merging this produces a net decrease in user bug exposure even if it isn't perfect.

Alternatively: since this functionality is hidden from the user by default, put a "WARNING: alpha quality" next to the checkbox they need to check in order to enable it.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 5, 2013

Contributor

If someone is eligible and wants to write such a test plan, please contact me.

I can help with some coins for the work, just in case they end up in /dev/null while testing:)

Contributor

cozz commented Mar 5, 2013

If someone is eligible and wants to write such a test plan, please contact me.

I can help with some coins for the work, just in case they end up in /dev/null while testing:)

@luke-jr

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Mar 5, 2013

Member

@eldentyrell They could always merge coincontrol with the relevant stable branch (not to dissuade merging this..)

@gmaxwell I didn't interpret @rebroad as threatening at all O.o

Member

luke-jr commented Mar 5, 2013

@eldentyrell They could always merge coincontrol with the relevant stable branch (not to dissuade merging this..)

@gmaxwell I didn't interpret @rebroad as threatening at all O.o

@eldentyrell

This comment has been minimized.

Show comment Hide comment
@eldentyrell

eldentyrell Mar 5, 2013

@luke-jr, I'm talking about non-{template-heavy-C++}-programmers (the majority of bitcoin users).

@luke-jr, I'm talking about non-{template-heavy-C++}-programmers (the majority of bitcoin users).

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

CNF shouldn't be a check, it should be a count. (if you want to make it change color when IsConfirmed() thats fine with me too). This is because it influences priority, which perhaps should be shown, as well as security— confirmation isn't a binary state and the coincontrol interface should be upfront with that.

Member

gmaxwell commented Mar 5, 2013

CNF shouldn't be a check, it should be a count. (if you want to make it change color when IsConfirmed() thats fine with me too). This is because it influences priority, which perhaps should be shown, as well as security— confirmation isn't a binary state and the coincontrol interface should be upfront with that.

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

If you've selected a bunch of coins in a bunch of different groups it's not easy to unselect them. There should probably be some discoverable way to select all / unselect all.

Member

gmaxwell commented Mar 5, 2013

If you've selected a bunch of coins in a bunch of different groups it's not easy to unselect them. There should probably be some discoverable way to select all / unselect all.

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

Shows transactions which are currently in listlockunspent. These should be greyed out. (perhaps with a padlock on them, bonus if the gui gets the ability to lock and unlock them)

Member

gmaxwell commented Mar 5, 2013

Shows transactions which are currently in listlockunspent. These should be greyed out. (perhaps with a padlock on them, bonus if the gui gets the ability to lock and unlock them)

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 5, 2013

Contributor

@gmaxwell confirmations are shown with tooltip. priority does not matter in my version of coin control.

ALL SELECTED INPUTS ARE GOING INTO THE TRANSACTION FOR SURE.
If you select 100BTC but only send 1 satoshi, still all 100BTC will be inputs, rest will send back as change.

Also there is already select all / unselect all, you have to click on "*" (left table header).

Contributor

cozz commented Mar 5, 2013

@gmaxwell confirmations are shown with tooltip. priority does not matter in my version of coin control.

ALL SELECTED INPUTS ARE GOING INTO THE TRANSACTION FOR SURE.
If you select 100BTC but only send 1 satoshi, still all 100BTC will be inputs, rest will send back as change.

Also there is already select all / unselect all, you have to click on "*" (left table header).

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

@cozz Uh. Priority has nothing to do with which selected inputs go into the transaction. It's one of the factors that determines if the transaction can be free or not, and influences how fast it can be mined. I am totally confused as to what you're thinking there.

A tooltip is effectively hidden, especially one that takes a half second hover on a tiny icon. Why not make it the number?

The asterisk fails on discoverability: I couldn't find it and I looked (tried left and right clicking in several places but not your π symbol * symbol. :P If I can't figure it out, knowing that it should have something there— might want to consider another approach.

Member

gmaxwell commented Mar 5, 2013

@cozz Uh. Priority has nothing to do with which selected inputs go into the transaction. It's one of the factors that determines if the transaction can be free or not, and influences how fast it can be mined. I am totally confused as to what you're thinking there.

A tooltip is effectively hidden, especially one that takes a half second hover on a tiny icon. Why not make it the number?

The asterisk fails on discoverability: I couldn't find it and I looked (tried left and right clicking in several places but not your π symbol * symbol. :P If I can't figure it out, knowing that it should have something there— might want to consider another approach.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 5, 2013

Contributor

actually I am using the listlockunspent methods to realize coin control, so combining the GUI and cli methods is not possible with this. The GUI would delete your locks, if set by cli. if you guys think this is no good, I need to change this.

Contributor

cozz commented Mar 5, 2013

actually I am using the listlockunspent methods to realize coin control, so combining the GUI and cli methods is not possible with this. The GUI would delete your locks, if set by cli. if you guys think this is no good, I need to change this.

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

The lockunspent code is trivial, so if we want to go that route we could basically duplicate it for this. I'll have to contemplate and look through your code. Still just doing really basic tests right now.

Member

gmaxwell commented Mar 5, 2013

The lockunspent code is trivial, so if we want to go that route we could basically duplicate it for this. I'll have to contemplate and look through your code. Still just doing really basic tests right now.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 5, 2013

Contributor

@gmaxwell ah Im sorry with the priority thing, now I know what you mean, youre right, showing this in an extra column could be useful.

Contributor

cozz commented Mar 5, 2013

@gmaxwell ah Im sorry with the priority thing, now I know what you mean, youre right, showing this in an extra column could be useful.

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Mar 5, 2013

Member

Yea, fee behavior is misleading, it fails to override it.

Reproduction:
Select 50 BTC input. Shows fee: 0. Send 49.99999999 BTC. Produces a transaction that pays 1e-8 in fees.

or,
Select 50 BTC input. Shows fee: 0. Send 1e-8 BTC. Prompts to charge for a 0.0005 fee. abort.

or,
Select 50 BTC input. Shows fee: 0. Send 49.99999999 and second output of 1e-8. Insufficient funds.

These are all because the coin control doesn't override the normal fee logic, which is good and fine, overriding it would result in stuck coins. I was using tiny outputs to trigger fees but the same should probably crop up for low priority. But the fact the it claims to display the fees but doesn't really is super confusing.

I might suggest that the fee display actually be brought onto the main send coins display in the panel with the send button. When coins are selected it should show the fee, size, and priority (turning red for low priority, making the fee in the case explicable), and change amounts for the selected inputs and specified outputs.

Member

gmaxwell commented Mar 5, 2013

Yea, fee behavior is misleading, it fails to override it.

Reproduction:
Select 50 BTC input. Shows fee: 0. Send 49.99999999 BTC. Produces a transaction that pays 1e-8 in fees.

or,
Select 50 BTC input. Shows fee: 0. Send 1e-8 BTC. Prompts to charge for a 0.0005 fee. abort.

or,
Select 50 BTC input. Shows fee: 0. Send 49.99999999 and second output of 1e-8. Insufficient funds.

These are all because the coin control doesn't override the normal fee logic, which is good and fine, overriding it would result in stuck coins. I was using tiny outputs to trigger fees but the same should probably crop up for low priority. But the fact the it claims to display the fees but doesn't really is super confusing.

I might suggest that the fee display actually be brought onto the main send coins display in the panel with the send button. When coins are selected it should show the fee, size, and priority (turning red for low priority, making the fee in the case explicable), and change amounts for the selected inputs and specified outputs.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 5, 2013

Contributor

screenshot8

just want to say, that Im working on this, update in 2 or 3 days. gmaxwell made some good points.
Added screenshot, what do you think?

Contributor

cozz commented Mar 5, 2013

screenshot8

just want to say, that Im working on this, update in 2 or 3 days. gmaxwell made some good points.
Added screenshot, what do you think?

@rebroad

This comment has been minimized.

Show comment Hide comment
@rebroad

rebroad Mar 6, 2013

Contributor

Looking good @cozz - I notice @jgarzik commented on the bitcointalk thread about RPC functionality - but I wasn't quite sure of the relevance - is he implying some duplication of effort as a result of this pull?

Contributor

rebroad commented Mar 6, 2013

Looking good @cozz - I notice @jgarzik commented on the bitcointalk thread about RPC functionality - but I wasn't quite sure of the relevance - is he implying some duplication of effort as a result of this pull?

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 6, 2013

Contributor

@rebroad - @jgarzik does not want to patch Bitcoin-Qt but instead provide this functionality through another external app. You could realize this through the lockunspent methods.
(except the extremely important feature that all selected should go into the transaction for sure, you have to patch bitcoin source for this, otherwise selected only maybe included in the transaction)
But I am not interested in this, I will maintain this patch anyway, because I am doing it for myself. If you guys then include this upstream or not is up to you.

Contributor

cozz commented Mar 6, 2013

@rebroad - @jgarzik does not want to patch Bitcoin-Qt but instead provide this functionality through another external app. You could realize this through the lockunspent methods.
(except the extremely important feature that all selected should go into the transaction for sure, you have to patch bitcoin source for this, otherwise selected only maybe included in the transaction)
But I am not interested in this, I will maintain this patch anyway, because I am doing it for myself. If you guys then include this upstream or not is up to you.

@tobypinder

This comment has been minimized.

Show comment Hide comment
@tobypinder

tobypinder Mar 14, 2013

Just want to chime in that this functionality is very cool and also very powerful. I hope this can get sufficiently tested and included (even some reduced subset if necessary).

Just want to chime in that this functionality is very cool and also very powerful. I hope this can get sufficiently tested and included (even some reduced subset if necessary).

@rebroad

This comment has been minimized.

Show comment Hide comment
@rebroad

rebroad Mar 15, 2013

Contributor

@cozz I'd like to install this Perhaps you could include instructions for the novice github users for downloading it..? (including me!)

Contributor

rebroad commented Mar 15, 2013

@cozz I'd like to install this Perhaps you could include instructions for the novice github users for downloading it..? (including me!)

@Nothing4You

This comment has been minimized.

Show comment Hide comment
@Nothing4You

Nothing4You Mar 16, 2013

Contributor

Could you rebase this so it can be tested with current master?

Contributor

Nothing4You commented Mar 16, 2013

Could you rebase this so it can be tested with current master?

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Mar 17, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b0dc2ad87f2b816dfd2cee1ff5c94c1024c56e8e for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b0dc2ad87f2b816dfd2cee1ff5c94c1024c56e8e for binaries and test log.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 17, 2013

Contributor

UPDATE:

screen_shot5
screen_shot6

code-cleanup and rebase

  • address-label is now shown, if you enter a change address; also warning if change address is not in wallet or invalid address

  • you can now lock/unlock per right-click in the GUI (same as the new cli method "lockunspent")

  • (un)select all is now a button to be easier recognizable

  • confirmations is now a number, no more icons

  • the calculation labels are now on top and also shown in send coins dialog (see screenshots)

  • "priority" added (very low - very high)

  • fee is now calculated for real (before it did not take min-fee into account)
    In order to be able to send a free transaction, you need to follow the rules:

    • transaction size must be < 10000 bytes
    • priority must be at least "medium"
    • any recipient must receive at least 0.01BTC
    • change must be either zero or at least 0.01BTC

    If you violate one rule you will see a min-fee and also the labels turn red:
    Bytes.Priority,Low Output,Change. Depending which rule you violated.
    Those 4 labels also have tool tips explaining this.
    Also remember that violating one of the first 2 rules means 0.0005 PER kilobyte min-fee,
    while violating one of the last 2 means 0.0005 min-fee only.

Contributor

cozz commented Mar 17, 2013

UPDATE:

screen_shot5
screen_shot6

code-cleanup and rebase

  • address-label is now shown, if you enter a change address; also warning if change address is not in wallet or invalid address

  • you can now lock/unlock per right-click in the GUI (same as the new cli method "lockunspent")

  • (un)select all is now a button to be easier recognizable

  • confirmations is now a number, no more icons

  • the calculation labels are now on top and also shown in send coins dialog (see screenshots)

  • "priority" added (very low - very high)

  • fee is now calculated for real (before it did not take min-fee into account)
    In order to be able to send a free transaction, you need to follow the rules:

    • transaction size must be < 10000 bytes
    • priority must be at least "medium"
    • any recipient must receive at least 0.01BTC
    • change must be either zero or at least 0.01BTC

    If you violate one rule you will see a min-fee and also the labels turn red:
    Bytes.Priority,Low Output,Change. Depending which rule you violated.
    Those 4 labels also have tool tips explaining this.
    Also remember that violating one of the first 2 rules means 0.0005 PER kilobyte min-fee,
    while violating one of the last 2 means 0.0005 min-fee only.

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Mar 17, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bc86b7377a4469d97d98765e6c5e8f9d9a07b3a3 for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bc86b7377a4469d97d98765e6c5e8f9d9a07b3a3 for binaries and test log.

@pera

This comment has been minimized.

Show comment Hide comment
@pera

pera Mar 17, 2013

Sorry for interrupting this pull request but imo those are almost essential features that should be included in the official client, please merge :>

pera commented Mar 17, 2013

Sorry for interrupting this pull request but imo those are almost essential features that should be included in the official client, please merge :>

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 17, 2013

Contributor
Contributor

cozz commented Mar 17, 2013

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Mar 18, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/28403fb61e6bec7c7a207f01c23935fa3b6efaa9 for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/28403fb61e6bec7c7a207f01c23935fa3b6efaa9 for binaries and test log.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Mar 18, 2013

Contributor

UPDATE: rebase

Contributor

cozz commented Mar 18, 2013

UPDATE: rebase

@laanwj

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Mar 18, 2013

Owner

I'm not opposed to merging this (for 0.9). Code changes look sane in
general, and as it is disabled by default casual users won't be
overwhelmed. More advanced users get some control over change, priority,
fee etc which is good.
Of course it all needs to be well tested, and conflicts with other coming
features such as multi-wallet support should be checked.

Owner

laanwj commented Mar 18, 2013

I'm not opposed to merging this (for 0.9). Code changes look sane in
general, and as it is disabled by default casual users won't be
overwhelmed. More advanced users get some control over change, priority,
fee etc which is good.
Of course it all needs to be well tested, and conflicts with other coming
features such as multi-wallet support should be checked.

@Nothing4You

This comment has been minimized.

Show comment Hide comment
@Nothing4You

Nothing4You Mar 19, 2013

Contributor

Could you make the values selectable (so it's possible to copy them)?

Also, this need a rebase (again :( )

Contributor

Nothing4You commented Mar 19, 2013

Could you make the values selectable (so it's possible to copy them)?

Also, this need a rebase (again :( )

@rebroad

This comment has been minimized.

Show comment Hide comment
@rebroad

rebroad Mar 21, 2013

Contributor

I've been testing this, and I seem to be having some crashes with wallet.db being unable to be read. So far it's not happened on the build that didn't have this coincontrol merged. Will explore further...

Contributor

rebroad commented Mar 21, 2013

I've been testing this, and I seem to be having some crashes with wallet.db being unable to be read. So far it's not happened on the build that didn't have this coincontrol merged. Will explore further...

@laanwj

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Apr 1, 2013

Owner

@rebroad: sounds like a very serious problem; did you manage to get more information from the crashes?

Owner

laanwj commented Apr 1, 2013

@rebroad: sounds like a very serious problem; did you manage to get more information from the crashes?

@wtogami

This comment has been minimized.

Show comment Hide comment
@wtogami

wtogami Sep 11, 2013

Contributor

@gavinandresen What sort of test plan? Completely automated is necessary?

Contributor

wtogami commented Sep 11, 2013

@gavinandresen What sort of test plan? Completely automated is necessary?

@wtogami

This comment has been minimized.

Show comment Hide comment
@wtogami

wtogami Sep 11, 2013

Contributor

@cozz Needs rebase.

Contributor

wtogami commented Sep 11, 2013

@cozz Needs rebase.

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Sep 11, 2013

Contributor

@wtogami : no, completely automated is NOT necessary. The kind of test plan described here:
https://github.com/bitcoin/QA

e.g. https://github.com/gavinandresen/QA/blob/master/PaymentRequestTest.md

Contributor

gavinandresen commented Sep 11, 2013

@wtogami : no, completely automated is NOT necessary. The kind of test plan described here:
https://github.com/bitcoin/QA

e.g. https://github.com/gavinandresen/QA/blob/master/PaymentRequestTest.md

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Sep 24, 2013

Contributor

update:

  • rebase/fix merge conflicts
  • replace hardcoded 57600000 with AllowFree(..)
  • minor fee bug: occured for sub-cent change and unusual fee setting, 0 < fee < 10000
  • minor gui: hide change label when checkbox unchecked

Testplan:
https://github.com/cozz/bitcoin/blob/cozz1/cctestplan.md

@gavinandresen What do you think about the testplan?

Contributor

cozz commented Sep 24, 2013

update:

  • rebase/fix merge conflicts
  • replace hardcoded 57600000 with AllowFree(..)
  • minor fee bug: occured for sub-cent change and unusual fee setting, 0 < fee < 10000
  • minor gui: hide change label when checkbox unchecked

Testplan:
https://github.com/cozz/bitcoin/blob/cozz1/cctestplan.md

@gavinandresen What do you think about the testplan?

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Sep 25, 2013

Contributor

Excellent test plan! The only section missing is interaction with JSON-RPC -- e.g. could things break in weird ways if a JSON-RPC send is done while the input selection dialog is up?

Contributor

gavinandresen commented Sep 25, 2013

Excellent test plan! The only section missing is interaction with JSON-RPC -- e.g. could things break in weird ways if a JSON-RPC send is done while the input selection dialog is up?

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Sep 25, 2013

Contributor

Not break, but the rpc scenario to consider is:
Coin control thinks the selected output is spendable, while actually the output is spent.
This can happen if you spent an output with rpc while working around in the sendcoinsdialog.
So you select an output with coin control, then spend it with rpc.
The selected spent output is then stucked in the coin control selection, because it is selected, but does not appear in the popup anymore.
Worst case here is getting a weird "Amount Exceeds Balance" and you dont understand why.
A clear() which would remove the stucked output and solve the problem is triggered by:

  • click unselectAll
  • disable coin control
  • sending a succesful tx in gui. This happens if the balance of the other selected was enough
    to satisfy the transaction. Of course the spent/locked output would not be part of the transaction.
    If stucked is the only one, you would get "Amount Exceeds Balance" all the time.

The scenario is unlikely and uncritical, but maybe I should write a simple check function, which checks if all selected are still unlocked and spendable, and then call this
on popup open and before click "Send". If you click "Send" and there had been a stucked output, I would return "transaction creation failed" and the coin control labels would refresh, so one can see, that someting has changed, like the output has been spent in the meantime.

What do you think?

Contributor

cozz commented Sep 25, 2013

Not break, but the rpc scenario to consider is:
Coin control thinks the selected output is spendable, while actually the output is spent.
This can happen if you spent an output with rpc while working around in the sendcoinsdialog.
So you select an output with coin control, then spend it with rpc.
The selected spent output is then stucked in the coin control selection, because it is selected, but does not appear in the popup anymore.
Worst case here is getting a weird "Amount Exceeds Balance" and you dont understand why.
A clear() which would remove the stucked output and solve the problem is triggered by:

  • click unselectAll
  • disable coin control
  • sending a succesful tx in gui. This happens if the balance of the other selected was enough
    to satisfy the transaction. Of course the spent/locked output would not be part of the transaction.
    If stucked is the only one, you would get "Amount Exceeds Balance" all the time.

The scenario is unlikely and uncritical, but maybe I should write a simple check function, which checks if all selected are still unlocked and spendable, and then call this
on popup open and before click "Send". If you click "Send" and there had been a stucked output, I would return "transaction creation failed" and the coin control labels would refresh, so one can see, that someting has changed, like the output has been spent in the meantime.

What do you think?

@petertodd

This comment has been minimized.

Show comment Hide comment
@petertodd

petertodd Sep 25, 2013

Member

@cozz That kind of thing can probably even happen in the regular wallet with co-current RPC operations, so yeah, if you could think it through carefully and come up with a good solution and user experience that'd be quite valuable. Just make sure the error message you got is reasonable friendly - "TRANSACTION CREATION FAILED!!!!!" isn't, but something along the lines of "Sorry, looks like some coins you wanted to spend were spent elsewhere. Retry?" is good.

FWIW keep in mind what you described can also be thought of as a double-spend, and we don't handle them well in general.

Member

petertodd commented Sep 25, 2013

@cozz That kind of thing can probably even happen in the regular wallet with co-current RPC operations, so yeah, if you could think it through carefully and come up with a good solution and user experience that'd be quite valuable. Just make sure the error message you got is reasonable friendly - "TRANSACTION CREATION FAILED!!!!!" isn't, but something along the lines of "Sorry, looks like some coins you wanted to spend were spent elsewhere. Retry?" is good.

FWIW keep in mind what you described can also be thought of as a double-spend, and we don't handle them well in general.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Sep 30, 2013

Contributor

update:

  • selected but spent outputs are now automatically removed from the coincontrol-selection on label update (which is also triggered just by opening the dialog)

This is now a simple solution to the problem described above, solved with one if-statement.

test-plan update:

  • added 12. Double Spend (the weird "Amount Exceeds Balance" I was talking above is also part of this)
  • added lockunspent interaction with rpc
Contributor

cozz commented Sep 30, 2013

update:

  • selected but spent outputs are now automatically removed from the coincontrol-selection on label update (which is also triggered just by opening the dialog)

This is now a simple solution to the problem described above, solved with one if-statement.

test-plan update:

  • added 12. Double Spend (the weird "Amount Exceeds Balance" I was talking above is also part of this)
  • added lockunspent interaction with rpc
@mumblerit

This comment has been minimized.

Show comment Hide comment
@mumblerit

mumblerit Oct 1, 2013

@cozz Great work! Could you please post a new backport to 0.8.5? You have many testers there that would help to validate the recent changes.

@cozz Great work! Could you please post a new backport to 0.8.5? You have many testers there that would help to validate the recent changes.

@wtogami

This comment has been minimized.

Show comment Hide comment
@wtogami

wtogami Oct 14, 2013

Contributor

Any remaining concerns blocking this merge?

Contributor

wtogami commented Oct 14, 2013

Any remaining concerns blocking this merge?

@HostFat

This comment has been minimized.

Show comment Hide comment
@HostFat

HostFat Oct 20, 2013

Contributor

https://bitcointalk.org/index.php?topic=144331.msg3375525#msg3375525

"Transaction times shown on CC window are not equal to times shown on Transactions tab. They are -2 hours off in my case (GMT +1 zone)."

I'm not sure if it's already fixed, I'm just reporting it ...

Contributor

HostFat commented Oct 20, 2013

https://bitcointalk.org/index.php?topic=144331.msg3375525#msg3375525

"Transaction times shown on CC window are not equal to times shown on Transactions tab. They are -2 hours off in my case (GMT +1 zone)."

I'm not sure if it's already fixed, I'm just reporting it ...

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Oct 21, 2013

Contributor

How is the test-plan testing going, or how did it go? Has anybody run through the entire test plan on both Windows and Linux and yet? (ideally, it would be tested on OSX also).

Contributor

gavinandresen commented Oct 21, 2013

How is the test-plan testing going, or how did it go? Has anybody run through the entire test plan on both Windows and Linux and yet? (ideally, it would be tested on OSX also).

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Oct 23, 2013

Contributor

update:

  • fix transaction times bug reported by @HostFat
  • because of #3008
    • change labels from 10000 to 1000 bytes
      (I did not have to change fee-logic, only tooltip label and label turn red threshold)
    • testplan: change from 10000 to 1000 bytes
  • because of #2945
    • ignore bytes from the inputs for priority calculation
    • adjusted getPriorityLabel(..) thresholds (because priorities are higher now and
      they just add up now when selecting more inputs, this
      resulted in getting "highest" priority label too quickly)

Testplan still the same link: https://github.com/cozz/bitcoin/blob/cozz1/cctestplan.md

Contributor

cozz commented Oct 23, 2013

update:

  • fix transaction times bug reported by @HostFat
  • because of #3008
    • change labels from 10000 to 1000 bytes
      (I did not have to change fee-logic, only tooltip label and label turn red threshold)
    • testplan: change from 10000 to 1000 bytes
  • because of #2945
    • ignore bytes from the inputs for priority calculation
    • adjusted getPriorityLabel(..) thresholds (because priorities are higher now and
      they just add up now when selecting more inputs, this
      resulted in getting "highest" priority label too quickly)

Testplan still the same link: https://github.com/cozz/bitcoin/blob/cozz1/cctestplan.md

@mumblerit

This comment has been minimized.

Show comment Hide comment
@mumblerit

mumblerit Oct 23, 2013

Can we have this backported to bitcoin-0.8.5 please?

Can we have this backported to bitcoin-0.8.5 please?

@laanwj

View changes

src/qt/coincontroldialog.cpp
+// copy label "After fee" to clipboard
+void CoinControlDialog::clipboardAfterFee()
+{
+ QApplication::clipboard()->setText(ui->labelCoinControlAfterFee->text().left(ui->labelCoinControlAfterFee->text().indexOf(" ")));

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Oct 24, 2013

Owner

For compatibility with the middle-click clipboard in Unix/Linux, it's better to do:

QApplication::clipboard()->setText(..., QClipboard::Clipboard)
QApplication::clipboard()->setText(..., QClipboard::Selection)

To prevent blowing up this code it might be better to add a function to GUIUtil to do both in one go.

@laanwj

laanwj Oct 24, 2013

Owner

For compatibility with the middle-click clipboard in Unix/Linux, it's better to do:

QApplication::clipboard()->setText(..., QClipboard::Clipboard)
QApplication::clipboard()->setText(..., QClipboard::Selection)

To prevent blowing up this code it might be better to add a function to GUIUtil to do both in one go.

@laanwj

View changes

src/qt/walletmodel.cpp
@@ -186,12 +186,19 @@ bool WalletModel::validateAddress(const QString &address)
return DuplicateAddress;
}
- if(total > getBalance())
+ // we do not use getBalance() here, because some coins could be locked or coin control could be active

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Oct 24, 2013

Owner

Would be cleaner to add an optional coinControl argument to WalletModel::getBalance(), instead of putting this code here verbatiim.

@laanwj

laanwj Oct 24, 2013

Owner

Would be cleaner to add an optional coinControl argument to WalletModel::getBalance(), instead of putting this code here verbatiim.

@laanwj

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Oct 24, 2013

Owner

Apart from some minor nits the code looks good enough for merging now.
How is the testing coming along?

Owner

laanwj commented Oct 24, 2013

Apart from some minor nits the code looks good enough for merging now.
How is the testing coming along?

@wtogami wtogami referenced this pull request in litecoin-project/litecoin Oct 24, 2013

Merged

Coin Control for Litecoin #77

@Diapolo

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 24, 2013

How does this interact with payment requests, when they show up in sendcoinsdialog?

Diapolo commented Oct 24, 2013

How does this interact with payment requests, when they show up in sendcoinsdialog?

@Diapolo

View changes

src/qt/sendcoinsdialog.cpp
+ else if (!CBitcoinAddress(text.toStdString()).IsValid())
+ {
+ ui->labelCoinControlChangeLabel->setStyleSheet("QLabel{color:red;}");
+ ui->labelCoinControlChangeLabel->setText(tr("WARNING: Invalid Bitcoin address"));

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 24, 2013

Nit: Can you use just Warning?

@Diapolo

Diapolo Oct 24, 2013

Nit: Can you use just Warning?

@Diapolo

View changes

src/qt/sendcoinsdialog.cpp
+ else
+ {
+ ui->labelCoinControlChangeLabel->setStyleSheet("QLabel{color:red;}");
+ ui->labelCoinControlChangeLabel->setText(tr("WARNING: unknown change address"));

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 24, 2013

Same here Warning: Unknown... and start uppercase after Warning:.

@Diapolo

Diapolo Oct 24, 2013

Same here Warning: Unknown... and start uppercase after Warning:.

@Diapolo

View changes

src/qt/forms/coincontroldialog.ui
+ <enum>Qt::ActionsContextMenu</enum>
+ </property>
+ <property name="text">
+ <string>0.00 BTC</string>

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 24, 2013

All these <string>s will show up in translations, can you remove such defaults for the final pre-merge rebase or make them untranslatable.

@Diapolo

Diapolo Oct 24, 2013

All these <string>s will show up in translations, can you remove such defaults for the final pre-merge rebase or make them untranslatable.

@Diapolo

This comment has been minimized.

Show comment Hide comment
@Diapolo

Diapolo Oct 24, 2013

Do all labels in your GUI honor changed display units?

Diapolo commented Oct 24, 2013

Do all labels in your GUI honor changed display units?

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Oct 25, 2013

Contributor

update:

  • code nits
    • introduced GUIUtil::setClipboard
    • calling getBalance(coinControl) now in walletmodel.cpp
    • replaced "WARNING" with "Warning"
    • added notr="true" to ui non translatable strings
  • added "coinControlUpdateLabels();" at the end of SendCoinsDialog::pasteEntry(..).
    This is because payment-request did not trigger label refresh.
  • reverted some changes from the last commit for #2945:
    I forgot uncompressed keys are over the limit. The ignore limit introduced by #2945
    is 151 bytes. An input with uncompressed key is 180 bytes. So I credit 29 bytes per uncompressed input. This is priority calculation only.

@Diapolo yes, I honor display units

Tested payment request. No problems found, besides the "coinControlUpdateLabels();" call mentioned above.

Latest 0.8.5 backport: http://sourceforge.net/projects/bitcoincoincont/files/bitcoin-0.8.5-coincontrol.tar.gz/download

Testplan still the same: https://github.com/cozz/bitcoin/blob/cozz1/cctestplan.md

Contributor

cozz commented Oct 25, 2013

update:

  • code nits
    • introduced GUIUtil::setClipboard
    • calling getBalance(coinControl) now in walletmodel.cpp
    • replaced "WARNING" with "Warning"
    • added notr="true" to ui non translatable strings
  • added "coinControlUpdateLabels();" at the end of SendCoinsDialog::pasteEntry(..).
    This is because payment-request did not trigger label refresh.
  • reverted some changes from the last commit for #2945:
    I forgot uncompressed keys are over the limit. The ignore limit introduced by #2945
    is 151 bytes. An input with uncompressed key is 180 bytes. So I credit 29 bytes per uncompressed input. This is priority calculation only.

@Diapolo yes, I honor display units

Tested payment request. No problems found, besides the "coinControlUpdateLabels();" call mentioned above.

Latest 0.8.5 backport: http://sourceforge.net/projects/bitcoincoincont/files/bitcoin-0.8.5-coincontrol.tar.gz/download

Testplan still the same: https://github.com/cozz/bitcoin/blob/cozz1/cctestplan.md

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Oct 25, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/51a2d9cfabf4264a8833da708d19eac4a1f83d87 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/51a2d9cfabf4264a8833da708d19eac4a1f83d87 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@luke-jr

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Oct 29, 2013

Member

ACK, whether the code is perfect yet or not, it works and isn't hacky or crazy.

Member

luke-jr commented Oct 29, 2013

ACK, whether the code is perfect yet or not, it works and isn't hacky or crazy.

@wtogami

This comment has been minimized.

Show comment Hide comment
@wtogami

wtogami Oct 29, 2013

Contributor

ACK after rebase.

Please just merge. Litcoin has found no show-stopping bugs in the past few months of testing while this patch has continued to receive polish and tiny fixes.

litecoin-project#77 (comment)
Litecoin's Coin Control had this simple debug patch to help verify the priority threshold where the fee is allowed to be zero.

Contributor

wtogami commented Oct 29, 2013

ACK after rebase.

Please just merge. Litcoin has found no show-stopping bugs in the past few months of testing while this patch has continued to receive polish and tiny fixes.

litecoin-project#77 (comment)
Litecoin's Coin Control had this simple debug patch to help verify the priority threshold where the fee is allowed to be zero.

@gmaxwell

This comment has been minimized.

Show comment Hide comment
@gmaxwell

gmaxwell Oct 29, 2013

Member

@wtogami "Please just merge" is not helpful. "Mr. Foo. Went through the test plan and completed all the tests successfully" is.

Member

gmaxwell commented Oct 29, 2013

@wtogami "Please just merge" is not helpful. "Mr. Foo. Went through the test plan and completed all the tests successfully" is.

@luke-jr

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Nov 8, 2013

Member

Coin Control tells me 3 inputs and 1 output uses 618 bytes, but this doesn't seem to be the case?
https://blockchain.info/tx/939a4e1b3264eee96a86e56fc1f07b2d52ae6240896d8b196e6af1cf9967ee06

Member

luke-jr commented Nov 8, 2013

Coin Control tells me 3 inputs and 1 output uses 618 bytes, but this doesn't seem to be the case?
https://blockchain.info/tx/939a4e1b3264eee96a86e56fc1f07b2d52ae6240896d8b196e6af1cf9967ee06

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Nov 8, 2013

Contributor

618 = 3 * 180 + 2 * 34 + 10
This would be 3 inputs from uncompressed keys and 2 outputs.

590 = 3 * 148 + 4 * 34 + 10
This would be 3 inputs from compressed keys and 4 outputs.

Calculation depends on how many inputs/outputs and also if inputs are from compressed or uncompressed ecdsa public key. If it is unknown how many outputs, I assume 2 outputs.

@luke-jr Is this a question in general or did coincontrol actually show you 618 bytes and when you sent the transaction, it only had 590?

Contributor

cozz commented Nov 8, 2013

618 = 3 * 180 + 2 * 34 + 10
This would be 3 inputs from uncompressed keys and 2 outputs.

590 = 3 * 148 + 4 * 34 + 10
This would be 3 inputs from compressed keys and 4 outputs.

Calculation depends on how many inputs/outputs and also if inputs are from compressed or uncompressed ecdsa public key. If it is unknown how many outputs, I assume 2 outputs.

@luke-jr Is this a question in general or did coincontrol actually show you 618 bytes and when you sent the transaction, it only had 590?

@laanwj

This comment has been minimized.

Show comment Hide comment
@laanwj

laanwj Nov 14, 2013

Owner

Rebasing this now...

See #3253

Owner

laanwj commented Nov 14, 2013

Rebasing this now...

See #3253

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Nov 14, 2013

Contributor

Closing this then. We dont need to have two coin control pull reqs open.

Contributor

cozz commented Nov 14, 2013

Closing this then. We dont need to have two coin control pull reqs open.

@cozz cozz closed this Nov 14, 2013

@tmagik

This comment has been minimized.

Show comment Hide comment
@tmagik

tmagik Jan 19, 2014

How do I find the original code that did have a 'Back to inputs' option? It's not obvious where that code went.

tmagik commented Jan 19, 2014

How do I find the original code that did have a 'Back to inputs' option? It's not obvious where that code went.

@cozz

This comment has been minimized.

Show comment Hide comment
@cozz

cozz Jan 25, 2014

Contributor

@tmagik its gone, sorry. checked my hard drive, did not find it.

Contributor

cozz commented Jan 25, 2014

@tmagik its gone, sorry. checked my hard drive, did not find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment