Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coin control features: send from address(es), view address linkages, view all addresses #415

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@ghost
Copy link

ghost commented Jul 14, 2011

This patch allows you to:

  • see all addresses, including change
  • see which addresses are linked together (does recursive expansion of address linkages)
  • select which address(es) to send from, rather than letting the client to chose for you

Full details and video here: http://coderrr.wordpress.com/2011/06/30/patching-the-bitcoin-client-to-make-it-more-anonymous/

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 18, 2011

This needs RPC / CLI support.

Otherwise it's it's a nifty feature.

@Benares

This comment has been minimized.

Copy link

Benares commented Aug 3, 2011

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

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Aug 10, 2011

CLI support done!

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

@zellfaze

This comment has been minimized.

Copy link

zellfaze commented Aug 16, 2011

+1 I think this is a great change.

@ripper234

This comment has been minimized.

Copy link

ripper234 commented Sep 2, 2011

+1

@alexwaters

This comment has been minimized.

Copy link
Contributor

alexwaters commented Sep 30, 2011

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.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Oct 9, 2011

fixed

@alexwaters

This comment has been minimized.

Copy link
Contributor

alexwaters commented Oct 20, 2011

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

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 7, 2011

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

@alexgenaud

This comment has been minimized.

Copy link

alexgenaud commented Dec 16, 2011

+1 Please mainline this killer feature!

@paraipan

This comment has been minimized.

Copy link
Contributor

paraipan commented Dec 16, 2011

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

@finway-china

This comment has been minimized.

Copy link

finway-china commented Dec 16, 2011

I say it'll be good.

@K1773R

This comment has been minimized.

Copy link

K1773R commented Dec 17, 2011

+1

@kwukduck

This comment has been minimized.

Copy link

kwukduck commented Dec 18, 2011

+1 key feature in managing anonymity.

@WyseNynja

This comment has been minimized.

Copy link

WyseNynja commented Dec 18, 2011

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

@gavinandresen

This comment has been minimized.

Copy link
Contributor

gavinandresen commented Dec 18, 2011

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.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Dec 19, 2011

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

@simonk83

This comment has been minimized.

Copy link

simonk83 commented Dec 19, 2011

+1

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 20, 2011

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Dec 20, 2011

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.

@paraipan

This comment has been minimized.

Copy link
Contributor

paraipan commented Dec 20, 2011

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Dec 20, 2011

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.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Dec 20, 2011

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.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Dec 20, 2011

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?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 20, 2011

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.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Dec 20, 2011

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 20, 2011

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.

@gavinandresen

View changes

src/wallet.cpp Outdated
@@ -9,6 +9,7 @@

using namespace std;

extern std::string sendFromAddressRestriction;

This comment has been minimized.

@gavinandresen

gavinandresen Dec 20, 2011

Contributor

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 argument to SelectCoins ?

OR, probably better, make it a restriction that can be placed on the wallet (thanks to laanjw for suggesting in IRC)

This comment has been minimized.

@ghost

ghost Dec 20, 2011

Author

@gavinandresen

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.

This comment has been minimized.

@gavinandresen

gavinandresen Dec 21, 2011

Contributor

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.

This comment has been minimized.

@laanwj

laanwj Dec 21, 2011

Member

@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

@alexgenaud

This comment has been minimized.

Copy link

alexgenaud commented Dec 21, 2011

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.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Dec 21, 2011

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.

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 22, 2012

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.

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 23, 2012

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

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 23, 2012

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.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 23, 2012

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.

coderrr
Coin Control features
    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 together
@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 23, 2012

got rid of the ExpandGroupings return too, thanks!

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 24, 2012

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.

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 24, 2012

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?

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

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

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 25, 2012

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

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

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

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

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 :/

@tril0byte

This comment has been minimized.

Copy link

tril0byte commented Mar 25, 2012

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

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

Wrap setPlaceholderText uses in #if QT_VERSION >= 0x040700

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 25, 2012

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.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

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

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 25, 2012

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.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

That license will be a problem to merging.

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 25, 2012

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?

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 25, 2012

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.

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 25, 2012

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().

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 25, 2012

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.

@dooglus

This comment has been minimized.

Copy link
Contributor

dooglus commented Mar 26, 2012

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

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 8, 2012

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

@tril0byte

This comment has been minimized.

Copy link

tril0byte commented Apr 8, 2012

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

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Apr 10, 2012

moved to #1017

@ghost ghost closed this Apr 10, 2012

@Giszmo Giszmo referenced this pull request Oct 24, 2016

Open

Add coin control #306

deadalnix pushed a commit to deadalnix/bitcoin that referenced this pull request Apr 5, 2017

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.