coin control / less change / refactor coin selection #1017
Conversation
|
whoa, github makes this confusing list of commits. It may be better to submit this against coderrs coin control branch? |
|
Two problems with that:
I can make a single large commit if that helps. |
|
That's a good point. My idea was that it is best to keep coin control development coordinated. Let's wait for @Coderrr. |
|
just let me know what you guys want me to do |
|
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. |
|
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? |
|
It really doesn't matter to me. @laanwj ? |
|
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. |
|
Needs rebasing. Sipa broke everything using locks. >_< |
|
Thanks for the heads up. I rebased it. |
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.
|
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? |
|
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. |
|
I see. Thanks. I'll keep rebasing it then. |
|
I do think the blank rows should be replaced with some kind of proper divider before merging, if possible. |
|
No objections or NAK, but I really think that coin control is wasted code in general.
|
|
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. |
|
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. |
|
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. |
|
We already have some multiple wallet support in the base code, thanks to sipa I think |
|
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. |
|
Non-trivial rebasing needed. |
|
Is this superceded now? |
|
Guess if dooglus doesn't work on this anymore we should close this then. |
…tcoin#1017) * Remove unused CNode::AskFor() This was a leftover from before the time the request manager was built. The only thing that we need to carry over is the check for max requests. * Remove mapAskFor, setAskFor and mapAlreadyAskedFor and rely only on the request manager to make txn requests. * Batch any transation requests * Send right away if we have >=1000 txn request This prevents us from creating a huge request when txn rates are very high or if we received a very large INV message. * Only AddRef() once for each node in maps for Batch requests Each map entry is of type <CNode *, CInv>, so we only have to add and release references once for each node rather than for every time we add and entry to the map. In other words, we only do an AddRef() for the first map entry, and then release that ref later.
5a52e0a [GUI][Trivial] Remove hardcoded address. (Matias Furszyfer) Pull request description: The `addnewaddressdialog.ui` has a hardcoded address used to draft the UI forms, even when it's cleaned in the view object initialization, it's better to remove it. ACKs for top commit: Fuzzbawls: ACK 5a52e0a random-zebra: ACK 5a52e0a Tree-SHA512: 02dd4e991232588181cacec4738b0d4463e1727586e0b28ad53fd7c25d3f019f17bb90f77fd2ecabe44b1f36074ee2e141e5886724ec24f085cc8cbbba4a1984
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: