Wallet encapsulation: TODOs, and make SetAddress and GetAccountAddress into methods of CWallet #2075
Conversation
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/56e396ce0d7b9299a03f0132c49150335e503ee2 for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/35cda0806a0d0fa780c7435a450ad50da6b5bc9b for binaries and test log. |
|
Thanks for the review, Diapolo. Latest commit closes all of that except the pass-by-reference question you posed. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b62f6de0a2402032be7de3de99713346fae7acc4 for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f342af9bfcfb50a7951fd1882410ebe791deccba for binaries and test log. |
CWallet::GetAccountAddresses()
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1668147ff31bfb36c680e4d4a8aff066fc288b6d for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a16766870ea277f613ec1a3e6e3e7c65c44eee59 for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/801be244dd41790b522fe464f32dea48241fe549 for binaries and test log. |
|
Thanks. I'm going to leave some comments over there. |
|
Looks good in general. Instead of comments in the code about what is supposed to be done, I'd rather see a commit that actually does it. On the other hand, this is probably easier to explain what you're about to do, and get comments without actually coding it. One nit: I don't like the wallet code depending on base58 encoding stuff (it's inevitable in some places because of backward compatibility with the wallet format on disk, but still try to avoid it)... so can you return a CTxDestination instead of a CBitcoinAddress from CWallet methods? |
|
Hi Pieter, thanks for the feedback. Tagging @CodeShark here too. I laid off on turning comments into code simply for want of more feedback. Not too much sense in plowing ahead with a raft of changes which will get rejected for reasons I hadn't anticipated. I think your nit is a serious issue, actually. Just as the json stuff should be kept out of the wallet representation as much as possible, so the base58 stuff as well. |
| @@ -513,6 +398,7 @@ Value getbalance(const Array& params, bool fHelp) | |||
| nMinDepth = params[1].get_int(); | |||
|
|
|||
| if (params[0].get_str() == "*") { | |||
| // TODO: Why is there a "different way" if both "should always return the same number"? | |||
There was a problem hiding this comment.
That's not really a TODO, is it? The reason there are two ways is explained below. One calculates the credits and debits to/from an account (or all accounts), the other just counts the unspent outputs left.
There was a problem hiding this comment.
No, not a TODO, more of a note-to-myself: "figure this out, fool!" :)
I'll snip it out of my next commit. Working on CTxDestination instead of CBitcoinAddress now.
|
Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/c7b1086d9f79722a7f4749cd0b924dff708b3c3a for test log. This pull does not merge cleanly onto current master |
Use shifts instead of going through the MPI representation of BIGNUMs. Be careful to keep the meaning of 0x00800000 as sign bit.
With a change of libs, and specifying NATIVE_WINDOWS as TARGET_OS it should compile libleveldb.a and libmemenv.a just fine, it did for me and Diapolo when testing.
feature in clang. These macros should primarily be used to document which locks protect a given piece of data. Secondary it can be used to document the set of held and excluded locks when entering a function.
- use it for displaying URI parsing warnings - use it for displaying error and information in backup wallet function (the information display is new and the error was a warning before) - cleanup BitcoinGUI::incomingTransaction() -- use message() + the information icon from message -- comment out an unused parameter in the function definition and declaration -- move all pre-checks at the beginning of the function
- remove unused parameter from ThreadSafeAskFee(), which also results in the removal of an orphan translation-string
With MinGW we use .a not .lib
- even if we are allowed to fail pre-allocating, it's better to check for sufficient space before calling AllocateFileRange() and if we are out of disk space return with error() - the above change allows us to remove the CheckDiskSpace() check in CBlock::AcceptBlock()
This problem is like earth (mostly harmless). After/during a -reindex, it means the statistics about the last block file reported in debug.log are always of blk00000.dat instead of the last file. Apart from that, it means a few more database entries need to be read when finding a file to append to the first time.
When the coin database is out of date with the block database, the best block in it is automatically switched to. This reconnection process can take time, so allow it to be interrupted. This also stops block connection as soon as shutdown is requested, leading to a faster shutdown.
- to be able to see threadsafety.h in the Qt Creator IDE the file needs to be added to the HEADERS section
- fix some indentation issues
Break one long comment down into 3 lines so it's readable.
|
something tells me i've failed to do my first rebasing properly. Anyone got an education link that applies to this project's workflow? |
|
To which branch did you intend to rebase? To rebase to current master I normaly do this: |
|
Closing this down in favor of #2130 |
|
You know you can push to a branch that is already pull-requested, to have the pull request updated? |
|
mumble I guess. I'm not over the learning curve in using git yet, by any means. In this case I wanted to rebase cleanly onto bitcoin/bitcoin, screwed it up here, so decided to try fresh on the new branch walletencap3. |
Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability
@sipa: Looking more sensible now without the JSON dependencies.