This needs RPC / CLI support.

Otherwise it's it's a nifty feature.

+1 for CLI support. Otherwise a must-have feature for me.

CLI support done!

bitcoin listaddressgroupings
bitcoin sendtoaddress <bitcoinaddress>[:<sendfromaddress1>[,<sendfromaddress2>[,...]]] <amount> [comment] [comment-to]

+1 I think this is a great change.

+1

The pull has become unmergeable (without conflicts), and will be closed in 15 days from this message if action is not taken.

To prevent closure, kindly rebase the pull to merge cleanly with the current codebase. If a time extension is needed, please respond to this comment or contact [email protected].

fixed

I'll ACK immediately if this is rebased with QT, and holds up to unit test / code read-through (sorry).

finally got around to repatching this onto bitcoin-qt... probably still needs some code cleanup... will look into tests next

+1 Please mainline this killer feature!

+1 advanced features, geek only. This patch with priv key import feature could make a good team :)

I say it'll be good.

+1

+1 key feature in managing anonymity.

+1 I'm looking forward to this and being able to sweep keys

coderr, can you write up a test plan or recruit somebody to write a test plan? What are the corner cases that might break? Does this change the way fees work at all-- if I try to send 0.5 BTC from an address that has exactly 0.5 BTC but it's in a brand-new transaction (and so trigger the pay-a-fee code), what happens?

I'll let laanjw approve/disapprove the GUI changes.

@gavinandresen
Is this the kind of test plan you were looking for? https://gist.github.com/9ec080b2bab70372b60e

What the patch does is basically only allow SelectCoinsMinConf() to "see" the addresses you have selected to send from. So in the case you mention it will inform the user the transaction has failed. Same thing for any case where the total amount required is more than the sum of the addresses you have selected.

+1

I'll test this soon, only just heard this was rebased to Qt.

This is not the place to be +1'ing, if you want to show public support for a feature, please make a forum thread where everyone can +1. If you have some useful comment on the bug, ie "Ive compiled this/been running this and spent more than 10 minutes testing it and it behaves as expected", say it, otherwise keep it to yourself, thanks.

any place is a good place Matt, please stop preaching ppl, this feature had a forgotten thread on the forum and didn't got too much support until ppl started supporting it on git. Try to look at the good side of things ;)

No, not at all. Have you seen the forums? They are so full of crazy people and stupid comments that you can't get any work done. If its such a good feature, post your +1s on the form thread and it will be alive again. Getting people to post +1s on this thread is a waste of time and takes away from the conversation that could be happening about the actual code in question.

Anyway, I was gonna try to test this, but when starting with a .bitcoin that just contains blk* files, all I get is
EXCEPTION: 11DbException
Db::exists: DB_PAGE_NOTFOUND: Requested page not found
Ignore this, my blockchain was corrupted.

In general comments: I really dont think this belongs in mainline. Its a cool feature for those who wish to make their bitcoin usage anonymous, but by itself it does not even get close to providing anonymity. It also pushes the wrong message about bitcoin: it was designed to be, and is anonymous; and that it is great for use in illegal activity. That message should be highly avoided as it is highly detrimental to the adoption of bitcoin. This as a separate branch is cool, but it should not be in the mainline IMO. Additionally, does this work as expected if you are using a OP_EVAL address or a compressed pubkey address?

On the other hand, it is clearly a very popular and much-requested feature.

And please don't promote the stigma that people want anonymity only because they want to do illegal things. It's clear that in today's world there can be many reasons you'd want to be anonymous, for example because of oppressive governments (China, Middle East, etc).

Edit: But I agree it might be too complicated for the standard UI. I've played with the idea of an "advanced mode" before. Maybe this feature is an candidate for that.

No, I know that there are many legitimate uses for anonymity that arent illegal, but that is a stigma that already exists when people view bitcoin, and pushing its anonymous properties (when its actually harder than most people realize to be anonymous with bitcoin) just furthers that stigma.

I'm not saying we have to advertise "anonymity" explicitly any more than we do now. It will remain an advanced feature, manually setting inputs/outputs is not for the faint-hearted.

However, it is a valid use-case. And if people want to do advanced things with addresses and address linkages, and someone is contributing code for it, why not incorporate it?

Also look at this thread. The guy first submitted this pull request, people were positive about it, then it lingered, he ported it Qt even. And now suddenly we should reject it in the mainline, not because of code quality concerns but because we're afraid the project gets a bad name?

IMO this is wrong. Either a feature needs to be rejected immediately or it should be put on the roadmap to be merged for some version.

For me, it's not about the anonymity, but control and transparency. As it is (prior to this pull) we can only see one address from every transaction. One needs to use blockexplorer.com to have any clue what is actually going on. Further more (prior to this pull), one needs to have multiple wallets to have any separation of 'accounts' which is more error prone and confusing. I think hiding advanced features in tabs is a nice compromise between ease and control.

Ok, so I'll be cleaning up the patch's code and removing the word anonymity from it completely. I'll use Gavin's suggested "coin control" wording unless someone else comes up with a better alternative.

hey, thanks for finding all these issues, I'll fix the optionmodel and blank sendfrom bugs. The change issue is a little more complicated.

"I sent a payment to myself from a single large output to lots of small new previously unused addresses"

Someone looking at the chain wouldn't be able to prove that the output addresses are controlled by the same person so it doesn't show them as linked. It only links input addresses (since the same person obviously controls them) as well as the change (which can usually be identified). So in general it tries to link things that someone else would be able to link, not what you would be able to link. If we changed the logic to be what you would be able to link, why not just link your whole wallet right off the bat? That said, the change linkage disappearing after you send to it is a legit problem.

got rid of the ExpandGroupings return too, thanks!

I rewrote the code that makes the unique groupings so it runs a lot faster. Now it takes about half a second on my wallet instead of 7 seconds and makes the code simpler to read too. It's odd - after changing the return type of ExpandGroupings to be void, the time it took dropped from 72 to 2.5 seconds, but then the next day it went back up to 7 seconds. I made a few new big transactions, but I was surprised it added such a delay.

I committed my change here:

dooglus@87168d9

I'm not sure what to do with the IsDisjoint() function - it's only used in wallet.cpp, and isn't really anything to do with wallets, so I made it a file local (static) function, then didn't need to declare it in the header.

https://github.com/dooglus/bitcoin/commits/coincontrol has 2 more commits:

1. remove the blank line from the top of the coin control table
2. order groups in order of total balance, so most valuable groups appear first

I'd also like to reformat the whole patch to match the existing bitcoin code (4 space indents, etc). but that will mess up any merges you have planned. Is now a good time?

This (tested without dooglus's changes) is exceedingly slow! Every time I select the Coin Control tab, I have to wait about 2 full minutes before it loads. :(

Once it does load, the blank rows between groups feels pretty hackish - how hard would it be to get a thick divider of some sort instead?

What is "Balance Minus Tx Fee"?

Finally, I don't see any way to easily get addresses to the "Send From" line (which should probably really be above the destinations, as multiple rows...).

That being said, it does seem to work, and not interfere with normal use if disabled. I'm putting it in today's next-test build ;)

@luke-jr

My changes make a huge difference to the speed. It's almost instantaneous with them.

I removed the top blank line which I thought looked really bad. I don't know anything about Qt, but also don't like the blank lines.

"Balance Minus Tx Fee" is (balance-MIN_TX_FEE), and:
static const int64 MIN_TX_FEE = 50000;
I'd rather not see that column.

To get addresses to the "Send From" line, select addresses (using ctrl-click or shift-click to select multiple addresses) then go to the 'Send Coins' tab. It took me a while to realise that was how to do it too.

Cool, I'll see if I can merge your changes on top of next-test

Dooglus's changes don't build for Win32:

src/wallet.cpp:18: error: using 'typename' outside of template
src/wallet.cpp:19: error: using 'typename' outside of template

No idea why it builds on Linux, that looks totally invalid :/

Trying to build v0.5.3+coderrr on debian squeeze, I get this error. Checking the qlineedit.h indicates coderrr's patch would seem to require qt4.7 and squeeze has only 4.6.4. I don't need this to be fixed, just tell me if I need to upgrade.

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

Wrap setPlaceholderText uses in #if QT_VERSION >= 0x040700

No idea why it builds on Linux, that looks totally invalid :/

That is odd. It builds without warning in what claims to be "gcc version 4.6.1 (Ubuntu/Linaro 4.6.1-9ubuntu3) ". I've removed the two "typename" keywords that snuck in there so it should build for you now too.

Removing the 'typename's seems to have fixed the problem.

I copied that IsDisjoint() function from http://stackoverflow.com/a/1964252/1253362 which is under this license: http://creativecommons.org/licenses/by-sa/3.0/ - it requires attribution, so I added the link to a comment above the function.

That license will be a problem to merging.

Then I don't know how to proceed. I could rewrite the function, but the way it's written seems to be pretty optimal to me, and I'd just rewrite it much like how it's already written. Even if I wrote it slightly differently, wouldn't it be considered a derivative work, and so still subject to the same license?

It could be. I didn't look at what the function actually does or how it works, so I (or anyone else) should be safe to rewrite it. Can you describe in English how the input(s) and output(s) are related?

Alternatively, you might email the author and ask for a MIT license to contribute it to Bitcoin.

I think I can rewrite the higher level function so it never needs to call IsDisjoint, and maybe make it more effecient in the process.

The problem with emailing the author of that answer is that he just improved on a previous author's answer, so I'd have to get agreement from at least 2 of them, maybe more.

The function does this:

Input two references to sets of strings, return a bool. True if the two input sets are disjoint (have no members in common).
Sets are sorted, so you can iterate through them in order rather than having to compare every member of the first with every member of the second.

But first give me a chance to rewrite the GetAddressGroupings function to no longer need IsDisjoint().

https://github.com/dooglus/bitcoin/commits/coincontrol is now safe to merge again, and faster still.

The formatting is still all wrong though, relative to the rest of the Bitcoin source.

https://github.com/dooglus/bitcoin/commits/coincontrol is now reformatted to match the other Bitcoin code - 4 space tabs, etc.

I'd like to close this request in favor of #1017.

yes 1017 (dooglus) coin control window opens quickly and also builds on Debian squeeze.

moved to #1017