Yet another Coin Control Release

cozz commented at 8:25 pm on March 4, 2013: contributor

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

gmaxwell commented at 8:37 pm on March 4, 2013: contributor

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 commented at 9:47 pm on March 4, 2013: contributor

update: removed “back to input” @gmaxwell I am actually already computing the fees while selecting inputs assuming 2 outputs

cozz commented at 10:08 pm on March 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.

eldentyrell commented at 11:18 pm on March 4, 2013: none

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.

BitcoinPullTester commented at 0:55 am on March 5, 2013: none

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

gavinandresen commented at 1:13 am on March 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.

rebroad commented at 1:55 am on March 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.

eldentyrell commented at 1:57 am on March 5, 2013: none

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 commented at 1:57 am on March 5, 2013: contributor

@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 commented at 2:05 am on March 5, 2013: none

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 commented at 2:21 am on March 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:)

luke-jr commented at 2:41 am on March 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

eldentyrell commented at 3:29 am on March 5, 2013: none

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

gmaxwell commented at 3:35 am on March 5, 2013: contributor

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 commented at 3:40 am on March 5, 2013: contributor

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 commented at 3:51 am on March 5, 2013: contributor

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 commented at 3:52 am on March 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).

gmaxwell commented at 4:00 am on March 5, 2013: contributor

@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 commented at 4:05 am on March 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.

gmaxwell commented at 4:09 am on March 5, 2013: contributor

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 commented at 4:16 am on March 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.

gmaxwell commented at 4:22 am on March 5, 2013: contributor

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 commented at 8:23 pm on March 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?

rebroad commented at 6:53 am on March 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?

cozz commented at 11:47 am on March 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.

tobypinder commented at 10:37 pm on March 14, 2013: none

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 commented at 3:14 pm on March 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!)

Nothing4You commented at 6:42 pm on March 16, 2013: contributor

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

BitcoinPullTester commented at 3:39 am on March 17, 2013: none

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

cozz commented at 3:44 am on March 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.

BitcoinPullTester commented at 4:25 am on March 17, 2013: none

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

pera commented at 9:07 am on March 17, 2013: none

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

cozz commented at 10:10 am on March 17, 2013: contributor

@rebroad I have published binarys also: https://bitcointalk.org/index.php?topic=144331

BitcoinPullTester commented at 1:14 pm on March 18, 2013: none

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

cozz commented at 1:30 pm on March 18, 2013: contributor

UPDATE: rebase

laanwj commented at 1:41 pm on March 18, 2013: member

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 commented at 9:52 pm on March 19, 2013: contributor

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

Also, this need a rebase (again :( )

rebroad commented at 1:02 pm on March 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…

laanwj commented at 2:53 pm on April 1, 2013: member

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

cozz commented at 10:50 am on April 2, 2013: contributor

@Nothing4You You you dont need to select you can direct right click and copy to clipboard @rebroad if you cant reproduce the error I cant help you here.

From my standpoint its not possible to create a corrupt wallet with the coin control patch. I guess the changes are less critical than some people think. The only critical code is in wallet.cpp thats 2 if-statements and some get-set methods. It is only tampering with inputs and outputs of transactions thats it, its not touching the wallet directly.

Anyway I hope I can kick myself in the ass and create a testing plan this week, I will send this gavin for review and then publish if he says testing plan is ok. Then I will test the new multi-wallet commit with this and then publish testing plan and rebase this, to get this finally merged. Until then theres no sense rebasing this all the time.

luke-jr commented at 4:01 am on April 3, 2013: member

Does this correctly calculate fee/kB in the case of unconfirmed (change) inputs? More specifically, does it include the size of unconfirmed “parent” transactions in the calculation? If not, should it?

in src/wallet.h: in 28403fb61e outdated

119@@ -120,6 +120,8 @@ class CWallet : public CCryptoKeyStore
120     CPubKey vchDefaultKey;
121 
122     std::set<COutPoint> setLockedCoins;
123+    std::set<COutPoint> setCoinControl;
124+    std::string sCoinControlChange;

sipa commented at 11:19 pm on April 5, 2013:

I’d like to keep the string representation of addresses out of CWallet as much as possible. Can you turn this into a CTxDestination?

sipa commented at 11:27 pm on April 5, 2013: member

The changes to core look very sane (I haven’t looked at the Qt code), but one thing I’m unconfortable with: coin control settings effectively become per-wallet state. That means that when using coin control from within Qt, it will also affect transactions created via RPC (which doesn’t have the same feature at all). Ideally, I’d say the coin control capabilities are abstracted into a separate class, an instance of which can be passed to the sendtransaction & co methods.

laanwj commented at 6:03 pm on April 6, 2013: member

Agreed @sipa, it should not be state but an extra input to the sendtransactions

luke-jr commented at 7:15 pm on April 16, 2013: member

Bug: Clicking on any part of the list other than the checkbox column (eg, clicking on an item’s Confirmation count) sets the focus to that column, and the space-bar cannot be used to check/uncheck the checkbox.

luke-jr commented at 10:08 pm on April 19, 2013: member

Why is change grouped in with other addresses? Seems to me as change is only every used as an input once, it should never be grouped…

eldentyrell commented at 7:00 pm on April 20, 2013: none

Seems to me as change is only every used as an input once, it should never be grouped…

I think the grouping is by output, no? A change address appears grouped with the wallet address which was used as an input to the transaction which has the change address as an output.

Of course, this isn’t the only way in which a change-address and non-change-address might be “related” in the blockchain, and it’s possible for two non-change-addresses to be “related” this way. The addresses won’t appear “related” in the tree view in these situations. But both of these situations arise as the result of some deliberate action on the user’s part (or some other user’s part), so I think the tree view is just showing you address relationships that arise as a result of the client’s automatic change-management decisions rather than the user’s deliberate transaction requests.

luke-jr commented at 8:25 pm on April 20, 2013: member

I don’t see how a relationship is inferred to an output (that it is change is not represented in the transaction itself).

leijurv commented at 10:11 pm on April 20, 2013: contributor

Just chiming in, this all sounds like a great idea.

sipa commented at 10:14 pm on April 20, 2013: member

@luke-jr Without special measures to try to make the change look like a regular output (try to mimic precision/patterns in amount values, for example), I think it’s safer to assume that change can be inferred.

eldentyrell commented at 4:16 am on April 21, 2013: none

that it is change is not represented in the transaction itself @luke-jr, do you mean:

In the blockchain? Of course not.
In a client’s local transaction records? The client can make note of the purpose for which a private key was created (to receive change or not) at the time of its creation.
In the bitcoin-qt client implementation in particular? See wallet.cpp, bool CWallet::IsChange() – it’s essentially doing (2) using the “address book” part of the wallet to determine the creation purpose. If the user created the address for non-change purposes it gets put in the address book. If it’s not in there, the cilent assumes it was created in order to receive change.

luke-jr commented at 8:22 pm on May 6, 2013: member

Thought: it would be nice if the coin control had a way to run the coin auto-selector, and display the choices in the coin control screen for any changes/review.

ahdinosaur commented at 2:58 am on May 27, 2013: none

can this please be implemented in RPC too? <3

sipa commented at 3:31 am on May 30, 2013: member

@ahdinosaur You can do coin control using the raw transaction api in RPC.

laanwj commented at 6:44 am on May 30, 2013: member

We’d like to merge this ASAP, but it really needs @sipa’s comment fixed (no per-wallet state, use CTxDestination) before it can be merged.

sipa commented at 2:46 am on May 31, 2013: member

I have rebased this branch here: https://github.com/sipa/bitcoin/commits/coincontrol

It misses:

Updates to the new dust logic
The coincontrol settings class I suggested instead of wallet state
CTxDestination instead of CBitcoinAddress

cozz commented at 10:36 am on May 31, 2013: contributor

I only did not submit any more updates, because I thought you guys require a testing plan for merging. Anyway I will look at this and push either an update or the current state after the weekend.

cozz commented at 6:57 pm on June 3, 2013: contributor

update:

new class CCoinControl in wallet.h class has
- CTxDestination for change
- Coin Control Methods
- Lock Unspent Methods I have merged lockunspent - methods into this class, because otherwise we would have again same state for gui and rpc for lockunsppent. There are 2 instances of this class created, 1 in walletmodel for gui and 1 in wallet for rpc. I have not implemented rpc, but would be trivial now. CCoinControl class is passed as parameter into createTransaction() also in AvailableCoins(). In AvailableCoins() this is where the actual coin control is done, its the same line of code where you asked if isLockedUnspent before
dust logic
- at least in my tests it turned out that dust is actual 5460 satoshis, not 5430
- if you enter dust as recipient Amount the label Low Ouput shows “DUST”
- fee is calculated correctly according to dust, this is when change would be dust, its added to the fee
fixed a (for me) very annoying compiler warning in a separate commit
some minor improvements, not really worth mentioning, like improving start-up time of the popup if you had a lot of outputs selected, change address warnings are now red etc. @luke-jr: sry, your spacebar-bug would require to create a separate treewidget class and overwrite keypressevent, so I leave this unfixed for now

sipa commented at 7:30 pm on June 3, 2013: member

Maybe my comment wasn’t clear, but this isn’t what I meant at all. You’ve now moved the coin control logic to a separate class, but it’s still kept as state in CWallet (and in addition, it holds a backreference to the wallet it is part of…).

The idea was to have something like CCoinControl, but just use it as a optional argument passed to CreateTransactions & co, not to store it in the wallet itself. RPC’s that call createtransaction wouldn’t pass it, normal GUI transaction wouldn’t pass it, but creating a transaction using coin control would construct such an object from what is set in the GUI, and pass it along. If you’re storing it in the wallet itself, it may interfere with other calls to CreateTransaction that don’t want coin control. Also, I don’t think the CoinControl object needs methods that interfact with the wallet; it should just group some settings.

In short: don’t let coin control be part of the wallet state, just make it an optional argument.

Apart from that, thanks for rebasing and updating. I’d really like to see this get merged.

cozz commented at 8:01 pm on June 3, 2013: contributor

thx for quick response.

ok, I kinda see. I will give this another shot. Are you fine with merging the lockunspent into the class? Should I create a new file src/coincontrol.h for the class?

sipa commented at 8:03 pm on June 3, 2013: member

Yes, I think the lockunspent stuff much more belongs in CCoinControl than in CWallet - I disliked it being there in the first place. But then that means that the RPC module gets its own CCoinControl instance, to maintain the lockunspent data.

jgarzik commented at 1:29 pm on June 4, 2013: contributor

Although I do agree w/ @sipa RE lockunspent data, it was an open question at the time whether it should be stored in the wallet or not. Some people requested a second iteration of lockunspent store the lock list in stable storage, thereby solving an issue with the current implementation: one must reload the lock list the first time the wallet is used, post-bitcoind-restart. That is a window for errors, that storing the locks in stable storage would solve.

cozz commented at 2:44 pm on June 5, 2013: contributor

update:

I have not merged lockunspent into coincontrol now, to not complicate things
class CCoinControl is now in src/coincontrol.h and independent from wallet
- the class is optional parameter to createTransaction,AvailableCoins,SelectCoins,sendCoins
- class is only passed in GUI coincontrol case

wtogami commented at 8:28 pm on June 6, 2013: contributor

Yes, jgarzik agreed, put these into other pull requests unless they are directly related to Coin Control.

BTW, it seems that some incarnation of #2651 will likely happen. Your patches have incompatibilities. Would you consider rebasing on current master and checking if things remain compatible with #2651?

wtogami commented at 10:20 pm on June 6, 2013: contributor

I rebased your Coin Control on top of master and #2651 “display txfee in first sendCoinsDialog message box”. When used in combination, “display txfee in first sendCoinsDialog message box” selects from available coins which do not include the inputs chosen by the user using Coin Control. Work would be necessary to allow the two patches to work together.

jonasschnelli commented at 7:32 am on June 7, 2013: contributor

@wtogami is right. If the pull get’s ACKed and merged, pull #2651 needs overhaul and some merge effort.

wtogami commented at 10:41 am on June 7, 2013: contributor

@jonasschnelli It seems #2651 is closer to acceptance than #2343 Coin Control, so Coin Control should rebase on master with #2651 as a dependency.

in src/wallet.cpp: in 1df65e9dff outdated

1243                     // Fill a vout to ourself
1244                     // TODO: pass in scriptChange instead of reservekey so
1245                     // change transaction isn't always pay-to-bitcoin-address
1246                     CScript scriptChange;
1247-                    scriptChange.SetDestination(vchPubKey.GetID());
1248+                    CBitcoinAddress addressChange;

sipa commented at 7:02 am on June 9, 2013:

Converting to a CBitcoinAddress shouldn’t be necessary.

in src/coincontrol.h: in 1df65e9dff outdated

44+        setSelected.clear();
45+    }
46+
47+    void ListSelected(std::vector<COutPoint>& vOutpoints)
48+    {
49+        for (std::set<COutPoint>::iterator it = setSelected.begin(); it != setSelected.end(); it++)

sipa commented at 7:04 am on June 9, 2013:

You can just use vOutpoints.assign(setSelected.begin(), setSelected.end());, I think.

sipa commented at 7:08 am on June 9, 2013: member

The changes look good overall to me (though I haven’t checked the GUI code or whether the creation logic matches the current filtering policies), apart from two nits I left inline. I think it’s likely #2154 gets merged before this, so you may need some rebasing, but I don’t think it’ll be particularly hard (#2154 is just code movement and dependency changes). No need to rebase right now, though.

in src/json/json_spirit_writer_template.h: in 1df65e9dff outdated

27@@ -28,7 +28,7 @@
28     template< class String_type >
29     String_type non_printable_to_string( unsigned int c )
30     {
31-        typedef typename String_type::value_type Char_type;
32+        //typedef typename String_type::value_type Char_type;

laanwj commented at 7:18 am on June 9, 2013:

What is this?

sipa commented at 7:19 am on June 9, 2013:

Assuming it’s a “we don’t use this”, it has no place in this PR.

wtogami commented at 7:20 am on June 9, 2013:

Yes, I previously asked him to remove the two unrelated commits from this PR.

in src/main.cpp: in 1df65e9dff outdated

367@@ -368,10 +368,10 @@ bool CTxOut::IsDust() const
368     // which has units satoshis-per-kilobyte.
369     // If you'd pay more than 1/3 in fees
370     // to spend something, then we consider it dust.
371-    // A typical txout is 33 bytes big, and will
372+    // A typical txout is 34 bytes big, and will
373     // need a CTxIn of at least 148 bytes to spend,
374     // so dust is a txout less than 54 uBTC
375-    // (5430 satoshis) with default nMinRelayTxFee
376+    // (5460 satoshis) with default nMinRelayTxFee

wtogami commented at 7:21 am on June 9, 2013:

This is the other commit that doesn’t belong in this PR.

sipa commented at 7:23 am on June 9, 2013:

It is correct, however… but yes, we can fix that separately.

in src/qt/coincontroldialog.cpp: in 1df65e9dff outdated

464+        CTxDestination address;
465+        if(ExtractDestination(out.tx->vout[out.i].scriptPubKey, address))
466+        {
467+            CPubKey pubkey;
468+            CKeyID keyid;
469+            CBitcoinAddress(address).GetKeyID(keyid);

sipa commented at 7:42 am on June 9, 2013:

No need to convert to a CBitcoinAddress. Use CKeyID *pkeyid = boost::get< CKeyID >(&address), which results in NULL or a pointer to a valid CKeyID.

cozz commented at 9:39 pm on June 10, 2013: contributor

update: sipa changes

wtogami commented at 3:19 pm on June 11, 2013: contributor

Is anyone else seeing this layout problem? I’ve seen it with at least the past two versions of Coin Control, including the current master plus the latest commit. Fedora native and gitian linux builds are equally affected. cc-layout-problem

cozz commented at 5:27 pm on June 11, 2013: contributor

update: solve layout issue. I had used fixed heights at some point, which was no good.

wtogami commented at 7:19 am on June 12, 2013: contributor

Hi cozz, did you see my email? My team would like to make a donation to you for this work.

in src/qt/sendcoinsdialog.cpp: in 4d6b1afb0b outdated

522+    {
523+        // actual coin control calculation
524+        CoinControlDialog::updateLabels(model, this);
525+        
526+        // show coin control stats
527+        ui->labelCoinControlAutomaticallySelected   ->hide();

Diapolo commented at 12:01 pm on June 12, 2013:

I don’t really like such in-code-formatting, consider this a nit, as long as @laanwj has no problem with it :).

in src/qt/coincontroldialog.cpp: in 4d6b1afb0b outdated

180+    if(item)
181+    {
182+        contextMenuItem = item;
183+        
184+        // disable some items (like Copy Transaction ID, lock, unlock) for tree roots in context menu
185+        if (item->text(7).length() == 64)

Diapolo commented at 12:03 pm on June 12, 2013:

Is this a good way to do what you want? My quick looking at this doesn’t reveal, what you are doing with this clause ^^. Perhaps we can use some symbolic-name instead of an index number?

in src/qt/coincontroldialog.cpp: in 4d6b1afb0b outdated

336+        if (sortColumn == logicalIndex)
337+            sortOrder = ((sortOrder == Qt::AscendingOrder) ? Qt::DescendingOrder : Qt::AscendingOrder);
338+        else
339+        {
340+            sortColumn = logicalIndex;
341+            sortOrder = ((sortColumn == 9 || sortColumn == 4 || sortColumn == 10 || sortColumn == 5) ? Qt::DescendingOrder : Qt::AscendingOrder); // if amount,date,conf,priority then default => desc, else default => asc

Diapolo commented at 12:07 pm on June 12, 2013:

After some months or when other people want to contribute here it’s not easy to see what colums are meant by 9 or 4 etc…

in src/qt/coincontroldialog.cpp: in 4d6b1afb0b outdated

382+// helper function, return human readable label for priority number
383+QString CoinControlDialog::getPriorityLabel(double dPriority)
384+{
385+    if (dPriority > 57600000ULL) // at least medium, this number is from AllowFree(), the other thresholds are kinda random
386+    {
387+        if      (dPriority > 576000000000ULL)   return tr("very high");

Diapolo commented at 12:08 pm on June 12, 2013:

highest?

in src/qt/coincontroldialog.cpp: in 4d6b1afb0b outdated

391+    }
392+    else
393+    {
394+        if      (dPriority > 576000ULL) return tr("low-medium");
395+        else if (dPriority > 5760ULL)   return tr("low");
396+        else                            return tr("very low");

Diapolo commented at 12:08 pm on June 12, 2013:

lowest?

cozz commented at 3:31 pm on June 12, 2013: contributor

update: Diapolo changes

I am using labels now, instead of index numbers.

wtogami commented at 2:09 am on June 13, 2013: contributor

Space Bar Bug It seems spacebar often fails to check/uncheck. Not sure what causes the instances where it does not work. Clicking and moving around a bit sometimes restores the spacebar ability.

Isolated Cause Clicking on the column titles like Priority to sort causes the spacebar to fail to check/uncheck.

wtogami commented at 2:31 am on June 13, 2013: contributor

Move OK button to bottom right Could you also please move the OK button on the input selection dialog to the bottom right? That seems to be where other dialogs have their button when the user wants to “continue”.

wtogami commented at 2:32 am on June 13, 2013: contributor

s/Minus Fee/After Fee/ Would you consider renaming the “Minus Fee” label? It is currently a bit confusing. Perhaps “After Fee” would be more understandable?

wtogami commented at 3:49 am on June 13, 2013: contributor

Possible bug with priority calculation. I have sent a few Coin Control transactions to combine tiny p2pool dust. On the 5th attempt, the input selection dialog has dust listed as “low-medium”. I sort by Priority and manually select the 67 lowest of the low-medium inputs, keeping the tx size below 10KB. Priority: at the top says “medium” and Fee: is zero. This should not be possible right? I then closed and re-opened the client, selected the same 67 low-medium inputs, and now the aggregate Priority: is low-medium with a fee.

Update I am not sure what’s going on here. With 45 low-medium inputs the aggregate priority is low-medium. When I add one more low-medium input, the aggregate priority becomes medium and the fee drops to zero. Is this expected behavior?

wtogami commented at 3:51 am on June 13, 2013: contributor

Possible bug with byte size calculation The “Bytes:” of a 67 input transaction in Coin Control is 9994, but after confirmation, ABE block explorer shows those tx’s with byte sizes like 9955 or 9962.

Update I might have figured out the cause. The byte calculation is assuming non-compressed key when in fact it is compressed? Not sure if there’s an easy way to fix your calculator to be more accurate.

wtogami commented at 11:49 am on June 13, 2013: contributor

Escape button bug This may be related to the spacebar bug above. Often the Escape button fails to dismiss the CC input selection dialog.

wtogami commented at 9:07 am on July 1, 2013: contributor

More regarding byte size calculation I now realized, is the discrepency really about leaving room for a possible change output? What happens if there is a change output and the target address is uncompressed? Will the actual tx size be in excess of the estimate in that case?

cozz commented at 3:15 pm on July 3, 2013: contributor

update:

labels are now selectable, because people often ask “how to copy to clipboard?”
fix spacebar/esc bug, I had to subclass the treewidget for this
s/Minus Fee/After Fee/
s/nBaseFee/nFee/
remove (nAmount - nPayFee) parenthesis
added a “~” before bytes and say “Can vary +/- 1 Byte per input” in tooltip, this is because ecdsa signature for a uncompressed public key is: with a 25% chance 179 bytes, with a 50% chance 180 bytes, with a 25% chance 181 bytes. I simply assume 180 bytes for all. Same for compressed, but with 32 bytes less. The only way to give 100% accurate bytes would be to actually create and sign the transaction for every click in coin control, which would be overkill in my opinion. So bytes is just an extreme good guess. @wtogami
I put the OK button on the left on purpose, because checkboxes are on the left, otherwise my mouse ways were to long, I had to move mouse from left to right all the time, I hated that
I have tested some compressed/uncompressed inputs, I can not say more than “It works for me”. In your case it really looks like there has been a false detection of compressed/uncompressed. But I dont know what else to test.
your phenomenom with the 45 low-medium switching to medium is correct behaviour. This is because if you only select 1 input, I have to add 78 bytes for the output here. This is 2 * 34 + 10. 2 outputs and 10 bytes transaction overhead. Now if you select 45 inputs, also I only add 78 bytes, because selecting more inputs does not mean you have more outputs, you still have 2 outputs. So the more inputs you select, the less “meaningful” are 78 bytes for the math-calculation. This means you can actually “cheat” your priority up, by selecting more inputs. But thats not my fault, thats how bitcoin priority calculation works. I mean with the power of coin control you can always cheat up your priority, simply by selecting a huge input, although it is not needed and then send this coins back to yourself as change.
Your last question above does not really make sense. An output is always 34 bytes, its a ripe160 hash, there is no compressed/uncompressed. Only the input is either 180 or 148 bytes, depending on compressed/uncompressed.

wtogami commented at 4:20 pm on July 3, 2013: contributor

@cozz Sounds great! Could you please provide this new version rebased to 0.8.2 as well?

cozz commented at 5:27 pm on July 3, 2013: contributor

@wtogami http://sourceforge.net/projects/bitcoincoincont/files/bitcoin-0.8.3-coincontrol.tar.gz/download

wtogami commented at 0:46 am on July 4, 2013: contributor

https://github.com/litecoin-project/litecoin-0.8/commits/awesomecoin-cc8 We are testing it in this side-branch. I’m liking the improvements over the previous version! https://github.com/litecoin-project/litecoin-0.8/commit/874db0996bc34aacf9d848b83d9cdbfb148c433d Not Bitcoin’s problem, but I am not sure if I got the 4x faster blocks priority threshold correct here.

I am looking forward to this and #2651 to be merged.

wtogami commented at 0:52 am on July 4, 2013: contributor

@cozz a tiny request. Could you please push each new revision of Coin Control to a new branch on your github? It is difficult to compare the changes to previous versions when the earlier versions are gone.

luke-jr commented at 1:16 am on July 4, 2013: member

@wtogami It’s not difficult at all. Pushing to the same branch is necessary to update the pull request.

wtogami commented at 3:32 am on July 4, 2013: contributor

@luke-jr I may be mistaken, but isn’t what he is doing push –force, which wipes access to the previous revisions?

luke-jr commented at 3:34 am on July 4, 2013: member

@wtogami Presumably. Don’t you have a local copy?

luke-jr commented at 2:53 am on August 11, 2013: member

This has been stable for a while now, and I’m not aware of any unaddressed concerns. Time to merge?

gmaxwell commented at 3:18 am on August 11, 2013: contributor

ACK behavior, just tried it out some, and I’m very happy with how it behaves now.

in src/coincontrol.h: in 8a099a904e outdated

21+    bool HasSelected() const
22+    {
23+        return (setSelected.size() > 0);
24+    }
25+    
26+    bool IsSelected(uint256 hash, unsigned int n) const

sipa commented at 8:15 am on August 11, 2013:

Nit: pass hash by reference.

in src/wallet.cpp: in 8a099a904e outdated

1275                     // change transaction isn't always pay-to-bitcoin-address
1276                     CScript scriptChange;
1277-                    scriptChange.SetDestination(vchPubKey.GetID());
1278+                    
1279+                    // coin control: send change to custom address
1280+                    if (coinControl && !boost::get< CNoDestination >(&coinControl->destChange))

sipa commented at 8:16 am on August 11, 2013:

Coding style nit: no spaces around CNoDestination.

sipa commented at 8:20 am on August 11, 2013: member

ACK on the code changes to core. I didn’t check the GUI code in detail, but it seems there’s a lot of duplication of fee logic there.

cozz commented at 9:56 pm on August 11, 2013: contributor

Yes, there is some duplication of fee logic. The stuff hardcoded in CreateTransaction(..) in wallet.cpp is not so easy to get rid of. But we could change the GetMinFee(..) method in main.cpp, so that it takes a parameter nBytes, and then do not serialize the CTransaction in this method, but use this parameter nBytes as size. When we call GetMinFee(..), we serialize the transaction anyway, and have nBytes available, so actually we serialize the transaction again all the time in GetMinFee(..) which is not really necessary.

If you want I can change the GetMinFee(..) in main.cpp, this would then remove like 8 lines of fee logic in coin control. Maybe in a separate pull?

luke-jr commented at 10:19 pm on August 11, 2013: member

That sounds like a good idea. But I would to it in a separate commit, not pull. So it would be part of this pull request, but also clearly distinguishable from the coin control functionality in git.

cozz commented at 4:46 pm on August 12, 2013: contributor

update:

added second commit “pass nBytes as parameter to GetMinFee(..)”
remove some duplicated fee logic and use GetMinFee(..) instead
sipa nits
change context menu order “Copy address,…” to match order from the Transactions tab

gavinandresen commented at 11:44 pm on August 12, 2013: contributor

I’d still be much more comfortable ACK’ing this if there was a written-up QA test plan. I see a lot of ad-hoc testing (e.g. @wtogami testing priority calculations sweeping up dust) which is fine, but the purpose of thorough testing is to find edge cases that aren’t tested because “it works for what the developer uses it for.”

wtogami commented at 8:46 pm on September 11, 2013: contributor

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

wtogami commented at 8:48 pm on September 11, 2013: contributor

@cozz Needs rebase.

gavinandresen commented at 10:20 pm on September 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

cozz commented at 6:40 pm on September 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?

gavinandresen commented at 7:22 am on September 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?

cozz commented at 5:39 pm on September 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?

petertodd commented at 7:15 pm on September 25, 2013: contributor

@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 commented at 11:33 pm on September 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

mumblerit commented at 9:23 pm on October 1, 2013: none

@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 commented at 8:55 pm on October 5, 2013: contributor

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

wtogami commented at 7:57 am on October 14, 2013: contributor

Any remaining concerns blocking this merge?

HostFat commented at 8:17 pm on October 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 …

gavinandresen commented at 1:29 am on October 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).

cozz commented at 1:39 pm on October 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

mumblerit commented at 4:55 pm on October 23, 2013: none

Can we have this backported to bitcoin-0.8.5 please?

in src/qt/coincontroldialog.cpp: in 6f614d4562 outdated

282+}
283+
284+// copy label "After fee" to clipboard
285+void CoinControlDialog::clipboardAfterFee()
286+{
287+    QApplication::clipboard()->setText(ui->labelCoinControlAfterFee->text().left(ui->labelCoinControlAfterFee->text().indexOf(" ")));

laanwj commented at 6:40 am on October 24, 2013:

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

0QApplication::clipboard()->setText(..., QClipboard::Clipboard)
1QApplication::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.

in src/qt/walletmodel.cpp: in 6f614d4562 outdated

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

laanwj commented at 6:48 am on October 24, 2013:

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

laanwj commented at 6:49 am on October 24, 2013: member

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

Diapolo commented at 7:32 am on October 24, 2013: none

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

in src/qt/sendcoinsdialog.cpp: in 6f614d4562 outdated

540+        if (text.isEmpty())
541+            ui->labelCoinControlChangeLabel->setText("");
542+        else if (!CBitcoinAddress(text.toStdString()).IsValid())
543+        {
544+            ui->labelCoinControlChangeLabel->setStyleSheet("QLabel{color:red;}");
545+            ui->labelCoinControlChangeLabel->setText(tr("WARNING: Invalid Bitcoin address"));

Diapolo commented at 7:33 am on October 24, 2013:

Nit: Can you use just Warning?

in src/qt/sendcoinsdialog.cpp: in 6f614d4562 outdated

557+                if (model->getPubKey(keyid, pubkey))
558+                    ui->labelCoinControlChangeLabel->setText(tr("(no label)"));
559+                else
560+                {
561+                    ui->labelCoinControlChangeLabel->setStyleSheet("QLabel{color:red;}");
562+                    ui->labelCoinControlChangeLabel->setText(tr("WARNING: unknown change address"));

Diapolo commented at 7:34 am on October 24, 2013:

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

in src/qt/forms/coincontroldialog.ui: in 6f614d4562 outdated

139+         </property>
140+         <property name="contextMenuPolicy">
141+          <enum>Qt::ActionsContextMenu</enum>
142+         </property>
143+         <property name="text">
144+          <string>0.00 BTC</string>

Diapolo commented at 7:37 am on October 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 commented at 7:38 am on October 24, 2013: none

Do all labels in your GUI honor changed display units?

cozz commented at 1:36 pm on October 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

pass nBytes as parameter to GetMinFee(..) e74f3ccec5

Coin Control Features 51a2d9cfab

BitcoinPullTester commented at 2:46 pm on October 25, 2013: none

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 commented at 2:04 am on October 29, 2013: member

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

wtogami commented at 7:56 pm on October 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.

https://github.com/litecoin-project/litecoin/pull/77#issuecomment-27046107 Litecoin’s Coin Control had this simple debug patch to help verify the priority threshold where the fee is allowed to be zero.

gmaxwell commented at 8:03 pm on October 29, 2013: contributor

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

luke-jr commented at 10:11 am on November 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

cozz commented at 11:20 pm on November 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?

laanwj commented at 11:28 am on November 14, 2013: member

Rebasing this now…

See #3253

cozz commented at 5:49 pm on November 14, 2013: contributor

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

cozz closed this on Nov 14, 2013

tmagik commented at 6:59 pm on January 19, 2014: none

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

cozz commented at 4:29 am on January 25, 2014: contributor

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

DrahtBot locked this on Sep 8, 2021

Yet another Coin Control Release #2343