Coin Control #1359

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)

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.

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.

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

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.

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

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

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);

Good catch. Fixed.

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));

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

@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.

@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?

@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...

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

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

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

Rebased with @Diapolo 's changes.

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.

Fixed

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

Yes, but it doesn't work :(

What does not work? You did try it before merging, no? @dooglus: Yes I added that .ui file. Anything wrong with it?

This is what I see:

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

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:

<pre> if(model) unit = model->getDisplayUnit(); </pre>

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.

@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?

I think the problem is that CoinControlPage::setModel(OptionsModel *model) isn't being called, and so the model pointer is uninitialised. I don't know how you get away with indirecting through it on Windows. Oh, except that it's probably 0 on Windows, so the if(model) check prevents the pointer being used.

What if you switch to mBTC - does the coin control page show mBTC values for you?

This fixes the crash for me without commenting out the 2 lines you suggested, indicating that an uninitialised 'model' member is indeed the cause of the trouble:

diff --git a/src/qt/coincontrolpage.cpp b/src/qt/coincontrolpage.cpp
index 3be6813..c7290b9 100644
--- a/src/qt/coincontrolpage.cpp
+++ b/src/qt/coincontrolpage.cpp
@@ -20,6 +20,7 @@ CoinControlPage::CoinControlPage(QWidget *parent) :
     QDialog(parent),
     ui(new Ui::CoinControlPage)
 {
+   model = 0;
     ui->setupUi(this);

     gui = (BitcoinGUI *)parent;

@dooglus Great, yes this will fix it ... currently model is not initialized to 0. This should be done via:

<pre> CoinControlPage::CoinControlPage(QWidget *parent) : QDialog(parent), ui(new Ui::CoinControlPage), model(0) </pre>

Will create a patch later... btw. I find it hard to follow the changes when they occur as rebase. Shall we just use seperate commits for now and rebase the whole thing in the end?

That's not enough to fix the real problem though. We need to arrange for setModel to be called somehow, or it will always be 0.

It is called in bitcoingui.cpp, see https://github.com/bitcoin/bitcoin/pull/1359/files#L2R393

There are several things todo for the GUI, e.g. update units when you change them in optionsdialog, but that is in the pipe :).

Umm... guys... I fixed this last night :p

While we were discussing this was not available here ... as I wrote above can we work with seperate commits until we are final? It's impossible to track changes when it's rebased all the time.

I would also like to use model(0) as everywhere else in the GUI part of the code, not NULL.

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

NULL is correct here. 0 is an integer, not a pointer.

Can you tell @laanwj he uses a wrong init value all over the GUI.

Btw: http://www2.research.att.com/~bs/bs_faq2.html#null

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.

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

@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?

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

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

@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?

Don't put unrelated things in pulls.

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.

@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.

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.

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.

@sipa fair enough.

Rebased with more of @Diapolo 's GUI improvements

@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?

@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?

@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.

@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!

@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.

@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.

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.

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

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.

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.

@eldentyrell Perhaps you are interested in Pull #2343 ?