coin control / less change / refactor coin selection #1017

1. 
   dooglus commented at 5:35 AM on March 31, 2012: contributor

   I've updated coderrr's coincontrol patch #415 so it merges cleanly against v0.6.0, and also updated the coin selection refactor and change reduction changes and unit tests from patch #898.

   They touch overlapping code, so I've merged them both here.

   https://github.com/dooglus/bitcoin/tree/coincontrol-v0.6.0 has just the coincontrol patch, without the other coin selection changes.

   I've improved the coincontrol patch, as follows:

   • it's much faster than coderrr's patch - what used to take over a minute now takes a fraction of a second
   • it's reformatted to use 4-space indents like the other bitcoin code
   • instead of showing the balance twice for each address, one with 0.0005 taken off, it now shows the total balance of each group
   • the groups are sorted so the most valuable group is shown first
   • the 'clear all' button on the 'send coins' tab clears the 'send from' input too
2. 
   laanwj commented at 7:04 AM on April 2, 2012: member

   whoa, github makes this confusing list of commits. It may be better to submit this against coderrs coin control branch?

3. 
   dooglus commented at 7:07 AM on April 2, 2012: contributor

   Two problems with that:

   • coderrr's branch isn't rebased against 0.6.0
   • coderr's changes use 2 spaces per tab, so pretty much every line needs changing to fit bitcoin

   I can make a single large commit if that helps.

4. 
   laanwj commented at 7:22 AM on April 2, 2012: member

   That's a good point. My idea was that it is best to keep coin control development coordinated. Let's wait for @coderrr.

5. 
   ghost commented at 7:25 AM on April 2, 2012: none

   just let me know what you guys want me to do

6. 
   dooglus commented at 7:27 AM on April 2, 2012: contributor

   I guess take a look at my pull request and let us know what you think. I changed a bunch of stuff, but for the better I think. The only thing I removed was the "balance plus tx fee" column, and replaced it with "balance of group". The "plus tx fee" isn't particularly useful because the fee changes depending on transaction size and input age.

7. 
   ghost commented at 7:29 AM on April 2, 2012: none

   Yea i've been watching what you were doing and it all seems fine, I didn't take a close look at how you optimized the algorithm but I'm assuming its fine. Do you just need my ok or do you want me to squash your changes into the commit on my pull request or what?

8. 
   dooglus commented at 7:32 AM on April 2, 2012: contributor

   It really doesn't matter to me. @laanwj ?

9. 
   dooglus commented at 10:17 AM on April 9, 2012: contributor

   The coincontrol patch #415 was accessing walletModel outside the if(walletModel){...} block, but only recently started crashing on shutdown, possibly because bitcoin-qt now seems to wait for everything to shut down before closing the GUI? 9a525b2 above fixes the crash.

10. 
    luke-jr commented at 1:47 PM on April 10, 2012: member

    Needs rebasing. Sipa broke everything using locks. >_<

11. 
    dooglus commented at 6:08 PM on April 10, 2012: contributor

    Thanks for the heads up. I rebased it.

12. Refactor coin selection to aid unit testing.

    Add unit test for coin selection.
    Fix coin selection to only include change when it's necessary.
    Add 'coin control' tab to allow selection of coins to use when sending and display linked addresses.

    bd89534957
13. 
    dooglus commented at 11:54 PM on April 12, 2012: contributor

    I rebased it again, since it stopped merging cleanly again.

    I'm a little unclear on what needs doing to get this merged. Is there something in particular that's holding it up?

14. 
    sipa commented at 11:59 PM on April 12, 2012: member

    I think it will be merged for 0.7.0, together with a few other extra features. We're currently doing merges for 0.6.1, which will be a mostly bugfix & cleanups release.

15. 
    dooglus commented at 12:00 AM on April 13, 2012: contributor

    I see. Thanks.

    I'll keep rebasing it then.

16. 
    luke-jr commented at 1:41 AM on April 13, 2012: member

    I do think the blank rows should be replaced with some kind of proper divider before merging, if possible.

17. 
    jgarzik commented at 4:41 PM on April 15, 2012: contributor

    No objections or NAK, but I really think that coin control is wasted code in general.

    1. Average users will never use it. The total userbase we will be able to count on our fingers :)
    2. Cleanups are nice, but coin control is really dead weight. If privacy is the goal, it is safer, easier, and less error-prone to simply use multiple wallets.
18. 
    dooglus commented at 4:47 PM on April 15, 2012: contributor

    I don't care so much about privacy. I use it to (1) see how much is at each address and (2) chose which address(es) to send from. For new users I think it helps them see what's going on behind the scenes, how new change addresses are created for each spend, etc. Maybe they don't care, but I would have appreciated this feature when I was first using the client.

    I don't see how it's easier to use multiple wallets. Doesn't that involve maintaining multiple copies of the blockchain or running with -rescan whenever I switch wallets? And running multiple instances of the client or having to keep shutting down and restarting the client with different datadirs? Using coincontrol seems a lot easier to me.

19. 
    sipa commented at 5:25 PM on April 15, 2012: member

    The current client presents the user with a ledger view to his money. Where this money is stored, and how the wallet transactions are mapped to bitcoin transactions is entirely abstracted away. This makes starting to use it easier, but it's very confusing to people who want to learn what's going on beyond the scenes.

    I think the solution is both supporting multiple wallets in the client (and have actual "open wallet" -> choose file dialogs), as breaking the mapping abstraction for users who want it (this patch). It may not be used by everyone, but for those to whom it is important that they understand it, it's essential.

20. 
    laanwj commented at 5:27 PM on April 15, 2012: member

    I like the "lets them see what happens behind the scenes" idea. It gives advanced users and developers some more control over what happens. Also the feature appears to be pretty popular with the bitcointalk.org crowd.

    What I don't like about the implementation is that it makes CWallet stateful (setSendFromAddressRestriction etc). Would it be possible to pass the address restriction to sendcoins functions?

    Multiple wallet support would also be nice, maybe we could add that as roadmap item for 0.8.

21. 
    jgarzik commented at 6:21 PM on April 15, 2012: contributor

    We already have some multiple wallet support in the base code, thanks to sipa I think

22. 
    sipa commented at 6:25 PM on April 15, 2012: member

    Yes, CWallet was written with that in mind. The problem is the bdb database environment. If you want to make wallets really portable files, which can be opened and closed independently, you need at least a separate environment per wallet, and - my preference - no bdb at all.

    I've been working on an append-only wallet file system, which seems quite viable.

23. 
    luke-jr commented at 2:23 AM on April 18, 2012: member

    Non-trivial rebasing needed.

24. 
    sipa commented at 7:17 PM on June 12, 2012: member

    Is this superceded now?

25. 
    luke-jr commented at 8:07 PM on June 12, 2012: member

    Yes, split into #1416 and #1359

26. 
    Diapolo commented at 8:39 PM on June 12, 2012: none

    Guess if dooglus doesn't work on this anymore we should close this then.

27. dooglus closed this on Jun 12, 2012

    ---

28. suprnurd referenced this in commit 8b13d45b5c on Dec 5, 2017
29. sanch0panza referenced this in commit dee4bfbd70 on Apr 11, 2018
30. lateminer referenced this in commit 8b6819a9d8 on Oct 30, 2019
31. DrahtBot locked this on Sep 8, 2021

Milestone
0.7.0