Coin control by cozz (rebased) #3253

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2013_11_cointrol_rebase changing 25 files +2562 −48

laanwj commented at 2:14 pm on November 14, 2013: member

Rebased version of Coin Control #2343 (all changes were trivial; biggest conflicts were uint64->uint64_t and the header refactor)
pass nBytes as parameter to GetMinFee(..) 8dfd8c62dc
Coin Control Features 6a86c24db1
BitcoinPullTester commented at 2:38 pm on November 14, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6a86c24db146d9ca5d1d5c83099d935c3feb63bb for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
cozz commented at 6:12 pm on November 14, 2013: contributor

Regarding watch-only addresses: I would suggest to merge watch-only first, then I could maybe add watch-only-support to coin control. Depending on whether this is an easy change or not, I would like to have watch-only also visible in coin control. Maybe with a checkbox “show watch-only”. I would add my brainwallet outputs as watch-only. It would be nice, if I could see those outputs in the coin control popup. Maybe even enabled, so I can play around with them. CreateTransaction should just fail, I guess, if you try to spend watch-only. Coin control is an expert-only feature anyway. Those outputs could be marked with an “eye-icon” for example. What do you think?
laanwj commented at 7:03 am on November 15, 2013: member

I rebased this to run through the test plan one last time and collect a last round of ACKs then finally get this merged (before a 0.9 feature freeze).

Are you sure you want to postpone this further? Watch-only does not have a consensus to be merged. Though I think the functionality is extremely useful, my main user-facing nit with the current implementation is that it shows one balance that includes both spendable and unspendable coins. This has the potential for confusing users.
luke-jr commented at 7:18 am on November 15, 2013: member

Watch-only, as it currently is implemented, is not fit for merging ever… I agree with @laanwj , let’s get this in now.
cozz commented at 10:08 am on November 15, 2013: contributor

ok then, I just thought watch-only will be merged soon.

I can also submit follow-up coin control pull requests to add more features like “add watch-only support” later.
laanwj commented at 12:00 pm on November 15, 2013: member

Yes, you can always make changes again later after other things have been merged.
kuzetsa commented at 6:37 pm on November 15, 2013: none

@cozz Thanks for all the work on this. I’ve been following this ever since I first heard about it.
in src/coincontrol.h: in 6a86c24db1
0@@ -0,0 +1,59 @@ 1+#ifndef COINCONTROL_H
Diapolo commented at 7:00 pm on November 15, 2013:

Missing license :). Also in other files I guess…
sipa commented at 10:25 pm on November 15, 2013: member

ACK changes to core; haven’t tested or looked at GUI changes.
laanwj commented at 8:54 am on November 16, 2013: member

Thanks @sipa GUI can always be improved (but it works and is pretty nice already), it’s mostly the core changes that need to be fool proof. Going to merge this now, license headers can be added in a seperate pull.
laanwj referenced this in commit 3443adecf1 on Nov 16, 2013
laanwj merged this on Nov 16, 2013
laanwj closed this on Nov 16, 2013
laanwj deleted the branch on Nov 16, 2013
Bushstar referenced this in commit cecbbab3cd on Apr 8, 2020
Bushstar referenced this in commit 2d080d4060 on Apr 8, 2020
DrahtBot locked this on Sep 8, 2021

Contributors
laanwj BitcoinPullTester cozz luke-jr kuzetsa Diapolo sipa