This patch allows you to:
Full details and video here: http://coderrr.wordpress.com/2011/06/30/patching-the-bitcoin-client-to-make-it-more-anonymous/
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 QA@BitcoinTesting.org.
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.
8 | @@ -9,6 +9,7 @@ 9 | 10 | using namespace std; 11 | 12 | +extern std::string sendFromAddressRestriction;
I really don't like the loose coupling here using a global variable to influence the behavior of SelectCoins... how hard would it be to pass that down through the call chain as an extra std::vector<CBitcoinAddress> argument to SelectCoins ?
OR, probably better, make it a restriction that can be placed on the wallet (thanks to laanjw for suggesting in IRC)
It was purposely written like this to be as minimally intrusive as possible for two reasons. First, I figured the less code I changed the less argument I'd get from devs about it. Secondly, it'd be easier to continually re-merge as master changed.
If you guys believe this feature should be merged into mainline then I'll gladly rewrite it in a "cleaner" way. But I'd rather not do that until it's been agreed the core devs think this belongs.
See IRC discussion from today here: http://bitcoinstats.com/irc/bitcoin-dev/logs/2011/12/20/5#l2405995
My only concern is over-promising "anonymity", when actually staying anonymous is hard work. But there is definitely a desire for this feature, so if it is cleaned up a little I think it should be pulled.
@coderr It is also a bit my fault, I've never really explained the design of Bitcoin-Qt. My idea is that the UI bits (dialogs, widgets, screens) are independent of a specific bitcoin implementation. The UI bits communicate with the models through method calls and Qt signals. The models communicate with the core in whatever way they need to; that includes changing globals if you really need to, though I'd personally prefer it as parameter to minimize the factor of surprise.
Thanks
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.
I think it's a very good idea to rename it. Gives us the functionality and prevents the 'bad' publicity that 'anonymity' gives according to some people :)
ok guys, no more anonymity, just coin control
510 | + "sendtoaddress <bitcoinaddress>[:<sendfromaddress1>[,<sendfromaddress2>[,...]]] <amount> [comment] [comment-to]\n" 511 | + "<amount> is a real and is rounded to the nearest 0.00000001" + crypt_usage); 512 | + 513 | + string strAddress = params[0].get_str(); 514 | + vector<string> splitAddresses; 515 | + boost::split(splitAddresses, strAddress, boost::is_any_of(":"));
I don't think the JSON-RPC server should be parsing strings like this... remember, this is for automation/services, not just some CLI tool.
The motivation for this was to make the sendtoaddress RPC interface change as unobtrusive as possible. What's your suggested alternative?
Based on the existing JSON-RPC commands, it seems the historical way of doing this is to add a new JSON-RPC method for different parameters. Putting it at the end would be another option, and might be extensible to the other variants as well. Another possibility might be a rule that where there used to just be a string "destination address", there could also be an array [["source", ...], ["destination", ...]] or object {"source": [...], "destination": ["address", ...]} possibly integrating sendmany as well...
Also, shouldn't there be a way to select specific coins, rather than just addresses?
re select specific coins: If you read the original motivation for this patch it was to prevent linkages of addresses/private-keys. Selecting individual coins doesn't matter for that.
re the RPC command arguments: I'm not sold that it's worth doing what you suggested purely because of your aversion to bitcoind parsing strings. The way it's done now is pretty minimally invasive and the other ways sound like a lot more changes. But I'd like to hear other people's opinions on the matter.
I agree with @luke-jr in principle. In JSON-RPC, arrays are a first-class object. Even though this is easy for CLI usage, there is really no need to concatenate things into strings and parse it out later. Using multiple kinds of parsing/concatenation imposes extra work on the caller (and other implementations of the RPC protocol).
IMO the optional "overloading" using an array instead of a string as destination address is the best way, but if you notice it doesn't fit the "sendtoaddress" paradigm at all anymore, make it a new call.
This definitely should have lower visibility (tab-wise) than sendmessage IMO...
sorry, I didn't follow that, sendmessage? tab-wise?
This pull request.
Ah, you must mean against master (so far I've only been keeping it rebased against the most recent release). After gavin or whoever has OKd this to be merged I'll rebase it against master.
Again with the+ 1s, seriously? Anyway, has this been tested to properly mark send to ip txes as linked with their punishment equivalents, also what about op_eval?
It's getting +1s because a thread is requesting them. Regardless of its current (stylistically correct or not) form, please let's not bikeshed this into oblivion. Each of us has a style we prefer. I think everyone is nuts for not using 1TBS, for example, but this is an important feature that allows people greater control (for those who want it.) So.. +1 for potential motility, especially in light of the huge publicity surrounding all the talks at 28c3 (Kaminsky's for e.g.) that whine about Bitcoin strawmen.
@midnightmagic every pull request to bitcoin is asked to not break anything or not interact negatively with existing features. Every other pull request (including ones which many would argue are much more important than this one) is subject to the same requirements. This is a cool feature for people who want to be anonymous or just have more control over their transactions, but if it breaks or acts really weird when it finds an OP_EVAL or Send-to-IP transactions it shouldn't be merged. AFAIK no one has even tested this with either, so it should wait. That isn't bikesheding, thats just wait until it is known to work. Also, the +1s are completely worthless, if this pull request is well-tested and known to work, I would think no one would have objections to it being merged, so +1s arent worth anything.
If it breaks something, fine, or if it requires testing beyond what it's been tested with, fine. I'm writing both to explain why you keep getting +1 and to suggest we ignore stylistic complaints about command form. github should have an upvote function. :)
@midnightmagic if any bitcoin pull request has style choices which the final pull-er doesn't agree with it wont get pulled. This pull is not special, and it should adapt to match the stylistic choices of the pull-ers. If you don't agree with those choices you can argue them, but if the developers dont agree, thats what forks are for. Also, people discussing the stylistic choices is healthy, this pull should wait on more testing imo anyway, so who cares?
What does it matter if the addresses are serially encoded in JSON-like structs or separated by commas..? Bitcoin took a huge beating at 28c3, and this would allow the biggest pseudo-complaint to be addressed. Therefore this patch has become special. Either way, I intended to put down support for this: this is my +1, minus the lack of "why." coderrr your work is VERY much appreciated, even if it doesn't soon make it into core.
If I don't like patch process, fork huh.. hrm.. I guess the same could be said to Charles Hannum. Yea fair enough, point taken, and reciprocated: if you don't like my comments, don't answer them. That's what everyone else does lol.
People, if you want to speed up the process for this patch please try to be constructive. Help coderr out with coding, testing, documenting and so on. Posting +1 (vote for someone else to do the work?) or arguing doesn't help.
I'm currently working on op_eval support
AFAIK OP_EVAL has been withdrawn. https://en.bitcoin.it/wiki/BIP_0012
Please do not spend time for it's support.
Probably whatever the OP_EVAL support was before, applies equally to the newer P2SH BIPs.
I've rebased this to the current master: https://github.com/laanwj/bitcoin/tree/2012_02_coincontrol
Re: BIP16 support -- isn't this a moving target? It should certainly be able to cope with BIP16 transactions without crashing or doing weird stuff, but I'm not sure that it already needs full support yet. The rest of the GUI doesn't, either.
I can't get this to merge cleanly to the current master branch. Does it need rebasing again?
Has for a while. Coderrr said maybe soon.
Rebased against 0.6.0rc4. Seems to work fine with p2sh. Doesn't do anything fancy yet like link the addresses of the keys used to redeem a p2sh tx. Only operates on the p2sh address itself.
Nice.
I think you missed this bit:
src/qt/optionsmodel.cpp:
- boolOptions << "bDisplayAddresses" << "fMinimizeToTray" << "fMinimizeOnClose" << "fUseProxy" << "fUseUPnP";
+ boolOptions << "bDisplayAddresses" << "bCoinControlFeatures" << "fMinimizeToTray" << "fMinimizeOnClose" << "fUseProxy" << "fUseUPnP";
so that in future the value of bCoinControlFeatures will be preserved when upgrading to the next wallet version.
Also yesterday I sent a payment to myself from a single large output to lots of small new previously unused addresses, and sent the change back to the input address. In the coin control tab I'm seeing all the new addresses separated from each other and the change by blank lines, suggesting there's no way of tying the together, but they're all outputs of the same transaction. Is that a bug? I would expect all the new outputs as well as the change to be lumped in with the big group that the input was part of before this transaction.
After further experimentation, it turns out that if I send coins from an address that has existing links in the coincontrol tab and explicitly send the change back to the same address, then the old linkage is lost.
I also found a different bug. If you leave the "Send From" box empty in the "Send coins" tab and try to send coins, CWallet::setSendFromAddressRestriction(string addresses) is called with an empty string, boost:split makes a set of restrictions cointaining one element, the empty string, and then the this->sendFromAddressRestriction.empty() check in CWallet::SelectCoinsMinConf is false, and so no coins are acceptable as input.
Something like this will fix it:
- boost::split(sendFromAddressRestriction, addresses, boost::is_any_of(";,"));
+ if (addresses.empty())
+ this->sendFromAddressRestriction.clear();
+ else
+ boost::split(sendFromAddressRestriction, addresses, boost::is_any_of(";,"));
The fact that such a bug exists suggests that this patch hasn't really been tested enough.
After further experimentation, it turns out that if I send coins from an address that has existing links in the coincontrol tab and explicitly send the change back to the same address, then the old linkage is lost.
This is because explicitly sending coins to an existing change address adds the change address to the address book, causing IsChange() to fail on the original output to the change address, and stopping the "group change with first in addr" code from grouping the change with the inputs.
Replacing IsChange() with IsMine() fixes the problem, and also causes all "payments to self" to link inputs with outputs:
// group change with first in addr, only need to group w first cuz all in addrs already grouped
BOOST_FOREACH(CTxOut txout, pcoin->vout) {
- if (IsChange(txout)) {
+ if (IsMine(txout)) {
CWalletTx tx = mapWallet[pcoin->vin[0].prevout.hash];
string addr = tx.GetAddressOfTxOut(pcoin->vin[0].prevout.n);
When I click the 'coin control' tab, the GUI freezes for over 70 seconds while the groupings are calculated. Most of the time is spent in the recursive ExpandGrouping() function which returns its results both by reference and by value. Changing its type to void speeds things up so it only takes 2.5 seconds instead of 70 seconds.
I'll commit all my changes to my repository tomorrow.
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.
Hidden by default, can be enabled through display options dialog
Restrict sending to only use specific source address(es)
Show which addresses have already been linked togethergot 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:
https://github.com/dooglus/bitcoin/commit/87168d9a8ecd270306f7d1deb7a14f2d773d2fec
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:
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 ;)
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.
yes 1017 (dooglus) coin control window opens quickly and also builds on Debian squeeze.