Coin Control

[luke-jr]

Superceding #1017 since @dooglus isn't rebasing it anymore. Also sanitized JSON-RPC changes and did some other cleanup. (#1416 has the coin selection refactoring)

[sipa]

bah!

[sipa]

A bit more elaborate: why is the coin selection a property of CWallet being set and unset? It should just be passed to the coin selection functions.

[gmaxwell]

I've started testing this some— I think that after some more review (and perhaps some minor cleanups) we should pull this. I've long supported having it— there are some use-cases where it's handy and it fosters a deeper understanding of the Bitcoin system

At the same time I'm concerned that it's quite rough from an interface perspective (the multiselect on another tab driving a text field with undisclosed syntax), and from a core feature set perspective (doesn't appear to be a way to do an inverted select "except theses")… but continuing to forestall pulling this is not going to encourage further development and testing.

So I think after this gets through a bit more review we should pull it and then make it known that if we don't see testing and improvement on it before 0.7.0's release that we'll disable it.

[sipa]

I certainly agree we need to have this merged. I think the functionality it adds is limited (it feels like micro-management where more high-level constructions should be used instead), but its educational value is very high. It helps breaking the "bitcoin-transactions-as-a-ledger" abstraction (without txins/txouts/change/keys/blocks) for those who want to learn the inner workings (and at least for us, we all had to learn the inner workings before we trusted the system, no?)

That said - and I should probably have noticed this earlier - I really don't like the coinselection being a CWallet property.

[Diapolo]

Is this UI-wise harmonized with other used dialogs (e.g. add as many settings as possible into the XML-file and remove code)?

[gmaxwell]

From reports in #bitcoin, it sounds like you just get a Transaction Failed when fees are required when the inputs selected don't have enough coin to pay the fee. (e.g. no fee request dialog or anything). This is a little confusing and should be fixed eventually.

[luke-jr]

Rebased on top of #1416 to split Coin Control from selection refactoring.

[dooglus]

Thanks Luke. Sorry I wasn't able to do that for you.

[ghost]

I was having trouble using this fork with the bitcoinrpc interface. Problem was, the sendFromAddressRestriction variable in cwallet was never getting set, so I would specify a list of "from" addresses and the system would ignore them, processing my transaction as if I hadn't specified them.

I eventually traced the problem to this line:

CScopedSendFromAddressRestrictionstd::set<std::string >(*pwalletMain, fromAddresses);

The CScopedSendFromAddressRestriction class is constructed without a name, and so is immediately destroyed, and the class's destructor clears sendFromAddressRestriction. I changed the line to this, and now it works for me:

CScopedSendFromAddressRestrictionstd::set<std::string > myAddressRestriction(*pwalletMain, fromAddresses);

[luke-jr]

Good catch. Fixed.

[tril0byte]

Luke, this branch doesn't build on debian squeeze (libqt4-dev 4:4.6.3-4+squeeze1),

In file included from src/qt/sendcoinsdialog.cpp:2:
build/ui_sendcoinsdialog.h: In member function ‘void Ui_SendCoinsDialog::retranslateUi(QDialog_)’:
build/ui_sendcoinsdialog.h:167: error: ‘class QLineEdit’ has no member named ‘setPlaceholderText’
make: *_* [build/sendcoinsdialog.o] Error 1

This is the same problem I reported on coderr's branch, which was fixed on dooglus's (#1017) and seems to have reverted.
#415 (comment)

The goal is to get build/ui_sendcoinsdialog not to have this line (as dooglus's doesn't) and I don't know how:

•    sendFrom->setPlaceholderText(QApplication::translate("SendCoinsDialog", "Restrict client to only send from these Bitcoin addresses", 0, QApplication::UnicodeUTF8));
  

[luke-jr]

@tril0byte That bug is in git master for now, not related to this at all.

[Diapolo]

This should indeed be moved to SendCoinsDialog::SendCoinsDialog() via as it's considered standard to currently not use the placeholderText property in ui-files.

#if (QT_VERSION >= 0x040700)
    /* Do not move this to the XML file, Qt before 4.7 will choke on it */
    ui->sendFrom->setPlaceholderText(tr("Restrict the client to only send from these Bitcoin addresses."));
#endif

[luke-jr]

@Diapolo Please post on the pullrequest (you're posting on an old commit). I don't foresee this getting merged any time soon, unless someone steps up to clean it up - including your changes. If you'd prefer, I can merge a patch you make into my branch.

[Diapolo]

@luke-jr It seems I took a route to a dead-end ... dunno how that happened, had no beer yet ^^. If you are willing to teach me the Git magic I need to do to be able to create a patch, you're welcome :). Do I need to fork your repo and create a pull or is there another way?

[luke-jr]

@Diapolo

git remote add luke-jr git://gitorious.org/~Luke-Jr/bitcoin/luke-jr-bitcoin.git
git fetch luke-jr
git checkout luke-jr/coincontrol -b coincontrol
# make your changes
git commit -a
git push # however you normally do it!

Then just point me at your branch...

[Diapolo]

luke-jr: I'm going to start here, when some of my last GUI commits get merged or final, okay?

[Diapolo]

Thanks for the git commands, I now have that branch and can start working with :).

[Diapolo]

Can this really safely be removed? Is this change directly related to coincontrol?

[Diapolo]

@luke-jr https://github.com/Diapolo/bitcoin/tree/coincontrol

[luke-jr]

Rebased with @Diapolo 's changes.

[dooglus]

Is src/qt/forms/coincontrolpage.ui meant to be in this commit?

qmake complains:
WARNING: Failure to find: src/qt/forms/coincontrolpage.ui

and make errors:
make: *** No rule to make target src/qt/forms/coincontrolpage.ui', needed bybuild/ui_coincontrolpage.h'. Stop.

[luke-jr]

Fixed

[Diapolo]

@luke-jr You merged the whole patch ;)?

[luke-jr]

Yes, but it doesn't work :(

[Diapolo]

What does not work? You did try it before merging, no?

@dooglus: Yes I added that .ui file. Anything wrong with it?

[dooglus]

This is what I see:

Program received signal SIGSEGV, Segmentation fault.
0x081d34f4 in OptionsModel::getDisplayUnit() ()
(gdb) where
#0  0x081d34f4 in OptionsModel::getDisplayUnit() ()
#1  0x0809b429 in CoinControlPage::UpdateTable() ()
#2  0x08087113 in BitcoinGUI::gotoCoinControlPage() ()
#3  0x08260af8 in BitcoinGUI::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) ()

[Diapolo]

Is OptionsModel::getDisplayUnit() the function which starts with that seg-fault?
Did you ever set the display unit via optionsdialog before?

What happens if you comment out in coincontrolpage.cpp:

    if(model)
        unit = model->getDisplayUnit();

[dooglus]

The .ui file didn't find its way into Luke's branch at first, but it's there now.

The display unit is set to BTC. Changing it causes the amounts shown in other tabs to change as expected.

Commenting out those 2 lines fixes the problem, and then everything seems to work.

[Diapolo]

@dooglus I'm glad this hot-fixes the segfault ... will have to check this in depth later. Strange thing is that does not happen for me on Windows. Thanks for testing my patch. I'm willing to bring this forward now GUI wise and perhaps we can fix the "bad" internals somehow to get this merged in the near future?

[Diapolo]

How can I update my local repo with the now current version?

git fetch luke-jr
git rebase luke-jr/coincontrol

Seems to generate to much merge-conflicts, so I believe there is sth. wrong.

[luke-jr]

This will discard all local changes and commits in your current branch and set its HEAD to latest coincontrol:

git reset --hard luke-jr/coincontrol

[Diapolo]

@luke-jr But this should be safe, as you merged my patch and did only some small further changes, right? When I have a new commit, how shall we proceed further?

[luke-jr]

I did not merge your changes unrelated to coin control. To see the difference between your current HEAD and my coincontrol, do:

git diff luke-jr/coincontrol

[Diapolo]

@laanwj That post was purely ironic, I want all of them to be 0 to stay consistent, nothing more :).

[Diapolo]

@luke-jr That git diff luke-jr/coincontrol doesn't help, as it's too much difference ... I regularly rebase patches to current master, but coincontrol seems to be not up to date, which makes comparing the branches very hard. My joy to push here fades away, when I don't know what you left out of my patch or changed for yourself. Things you consider unrelated (even if that may be true feature-wise) but I would like to see in that pull (or I would do when opening that pull) make things not better in the end. Any suggestion for this issue?

[luke-jr]

Don't put unrelated things in pulls.

[mb300sd]

I think this should be an object rather than an array, having trouble deserializing an array of different types. And this makes it match the output of every other command better.

        Object addressInfo;
        addressInfo.push_back(Pair("address",       address));
        addressInfo.push_back(Pair("balance",       ValueFromAmount(balances[address])));
        {
            LOCK(pwalletMain->cs_wallet);
            if (pwalletMain->mapAddressBook.find(CBitcoinAddress(address).Get()) != pwalletMain->mapAddressBook.end())
                addressInfo.push_back(Pair("account",       pwalletMain->mapAddressBook.find(CBitcoinAddress(address).Get())->second));
        }
        jsonGrouping.push_back(addressInfo);

[mb300sd]

I also added an option to hide the groups/addresses that have 0 balance. Might be useful for someone else as well.

Heres a diff, I haven't figured out how to use git yet. :(
http://pastebin.com/YSRxKBqx

[Diapolo]

I think every progress here is a good thing. Perhaps luke can merge your changes. Have you setup Git on your machine yet?

[mb300sd]

I figured out how to create my own repo, but not much point forking the bitcoin code, the above is about all I have to contribute.

[dooglus]

I would like this extended to work on individual coins, if possible. Currently I can see the balance at each address, but can't tell how many separate transaction outputs each address balance corresponds to, and can't select individual outputs to spend.

I'd also like it if all the selected inputs (whether addresses or individual 'coins') were summed so I know how the value of the selected coins.

[gmaxwell]

@dooglus I was thinking it would be interesting if it supported a hierarchy— accounts/labels / addresses / inputs, and if you could select groups at each level (e.g. select an account to get all its addresses and all their inputs, select an address to get all its inputs)— with each level showing a subtotal, and there being a total of selected. Also a 'select all', which would then let you go exclude things. But... lots of gui work to do all that.

[dooglus]

Accounts are something different I think. There's no direct correspondence between addresses and accounts is there? It's possible to move coins from one account to another without creating a transaction.

But it could work with labels. It's possible to have the same label on multiple addresses.

[sipa]

Receive addresses are associated with an account. Coins that originally arrived via an address that - at the time - was associated with an account, remain associated to that account.

Splitting the available coin list based on account is dangerous I think, as it will re-enforce the notion that coins belong to an account. Splitting per destination address is fine, imho.

[gmaxwell]

@sipa fair enough.

[luke-jr]

Rebased with more of @Diapolo 's GUI improvements

[dooglus]

@luke-jr I just built your rebased version from an hour ago. The coin control tab is now lacking a scrollbar. I can scroll the contents using the right-hand edge of the touchpad (probably that's simulating a mouse's scrollwheel), but the scrollbar itself is missing. It was there the last time I built from your same branch.

I can't see what you've changed because it appears that you've overwritten the previous commit with the new one.

Remember when @Diapolo said:

"can we work with seperate commits until we are final? It's impossible to track changes when it's rebased all the time"

and you replied:

"I suppose separate commits makes sense for this one..."

and I thought that meant we'd be using separate commits for this one?

[Diapolo]

@dooglus As I currently don't work on this (e.g. found it boring to discuss about cosmetic changes that luke refused to accept, no commits, lack of under the hood updates etc.) I can only guess, that perhaps in the UI file scrollbars are disabled?

[dooglus]

@Diapolo It's a shame. I find it to be a useful feature to have, but after rebasing it over and over I too had to give up on it.

[Diapolo]

@dooglus I put great efforts in improving parts of the GUI client over the last weeks, but I think it could need a few more helping hands. You are absolutely right!

[luke-jr]

@dooglus Yes, I see that @Diapolo 's latest changes removed the scrollbar. I'll add it back, I suppose.

As far as just adding commits, I had intended to just pull (fast-forward style) from others. Unfortunately @Diapolo did his own rebasing and merging of his old changes with his new changes, so I had to dig them out and merge them in by hand (so not even cherry-picking would work). And then there's all the completely unrelated changes that @Diapolo insists on keeping in his branch...

@Diapolo I didn't refuse any of your cosmetic changes related to Coin Control at all, until this removal of the scrollbar that I agree with @dooglus makes no sense.

[Diapolo]

@luke-jr I like your feedback most of the time as coding-wise it's valuable ... but most of the time a team-play like here (where you are the master-chief of this patch currently) seems to not work. I rebase all my patches agains the current master in regular intervals, which minimizes merge-conflicts. And coding-style wise we have different points of view.

[jgarzik]

Consensus: we like "listaddressgroupings", and request that that be submitted in a new pull request.

The current UI, as proposed, is superceded by the raw transaction RPC API, which permits full coin control, and is accessible via the GUI RPC console.

Full discussion took place on #bitcoin-dev around ~12:30PM EDT August 1.

[rebroad]

so.. what's happening with this? Is this the best pull to use currently for coin control?

[gmaxwell]

It's closed. There is no current pull. Enough was merged so that you could spend unlinked inputs manually with the raw transaction API— as not a single person was willing to step up to maintain the GUI stuff.

[eldentyrell]

Enough was merged so that you could spend unlinked inputs manually with the raw transaction API

That's nice, but if the raw transaction API were a substitute for the GUI the client wouldn't have a GUI in the first place.

not a single person was willing to step up to maintain the GUI stuff

Not merging this is causing people to refuse to upgrade. Just keep that in mind.

[gmaxwell]

@eldentyrell Perhaps you are interested in Pull #2343 ?