Skip to content
This repository

Wallet encapsulation: TODOs, and make SetAddress and GetAccountAddress into methods of CWallet #2075

Closed
wants to merge 49 commits into from
Mike Gogulski

@Sipa: Looking more sensible now without the JSON dependencies.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/56e396ce0d7b9299a03f0132c49150335e503ee2 for binaries and test log.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/35cda0806a0d0fa780c7435a450ad50da6b5bc9b for binaries and test log.

P. Kaufmann

This should cause a compilation warning, as there is no default return value?

I was wondering about that and tested with gcc before putting in a goofy return; //notreached. GCC doesn't complain.

P. Kaufmann

I'm rather sure address's is invalid spelling, but can't verify that feeling ;).

The actress's agent, the princess's tiara, the witness's statement...

As I said, just a feeling and I'm no native english speaking person :-D.

P. Kaufmann

IMO you should mention changed code parts when moving and not just leave them in as comments.

Fair point, but mention where? Commit note? "Inlined single-use CWalletTx& wtx variable"?

It's hard as there is no real diff, when moving stuff around. So perhaps the commit message is a good place. Orphan code parts as comments are not the way to go.

P. Kaufmann

Some indentation issues here and below (4 spaces).

P. Kaufmann

I'll push a new commit to fix these, and now I've got Eclipse set to use spaces for tabs properly, I hope.

P. Kaufmann

I'm not sure, could strAccount be a reference?

Could be, but is that important here? I see the reference-passing stuff as important in the case of compute-intensive code and huge objects where a copy op is expensive.... critical for a lot of blockchain stuff, not so in the wallet.

I'm a huge fan of micro-optimisations, so I wanted to suggest it. As the core devs ACK / NACK pulls anyway, you are free to leave this as is.

P. Kaufmann

Why introduce a new name, we pass an address, so IsMine() is rather self explanatory IMHO.

Point taken. New commit will use the IsMine name.

Mike Gogulski

Thanks for the review, Diapolo. Latest commit closes all of that except the pass-by-reference question you posed.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b62f6de0a2402032be7de3de99713346fae7acc4 for binaries and test log.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f342af9bfcfb50a7951fd1882410ebe791deccba for binaries and test log.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1668147ff31bfb36c680e4d4a8aff066fc288b6d for binaries and test log.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a16766870ea277f613ec1a3e6e3e7c65c44eee59 for binaries and test log.

P. Kaufmann

Indentation nit ;).

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/801be244dd41790b522fe464f32dea48241fe549 for binaries and test log.

Eric Lombrozo

Nice effort. The Wallet interface could certainly use cleaning up.

I would like to eventually merge this with #2124 and #2121

Mike Gogulski

Thanks. I'm going to leave some comments over there.

Pieter Wuille
Collaborator

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?

Mike Gogulski

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.

src/rpcwallet.cpp
@@ -513,6 +398,7 @@ Value getbalance(const Array& params, bool fHelp)
513 398
         nMinDepth = params[1].get_int();
514 399
 
515 400
     if (params[0].get_str() == "*") {
  401
+    	// TODO: Why is there a "different way" if both "should always return the same number"?
2
Pieter Wuille Collaborator
sipa added a note December 25, 2012

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
BitcoinPullTester

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

and others added some commits September 15, 2012
Christian von Roques tests for SetCompact and GetCompact of CBigNum 895e618
Christian von Roques reimplement CBigNum's compact encoding of difficulty targets
Use shifts instead of going through the MPI representation of BIGNUMs.
Be careful to keep the meaning of 0x00800000 as sign bit.
087d971
Trevor Add NATIVE_WINDOWS
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.
3e12b39
Alexander Kjeldaas o Added threadsafety.h - a set of macros using the -Wthread-safety
  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.
714fc57
Alexander Kjeldaas o Added AnnotatedMixin which adds locking annotations to the mutex
  API, compatible with clang's -Wthread-safety
e622ef1
Alexander Kjeldaas o Annotated lock-like functions in net.h.
o Removed unused function EndMessageAbortIfEmpty
637075a
Add new RPC "lockunspent", to prevent spending of selected outputs
and associated RPC "listlockunspent".

This is a memory-only filter, which is empty when a node restarts.
000f859
P. Kaufmann split of createTrayIconMenu() from createTrayIcon() in BitcoinGUI
- this allows to setup the trayicon before we have and want a trayicon menu
- should be of great use, when we remove that splash screen
- fixes a small bug with the toggleHideAction icon, which is not only used with
  trayicon but also with the Mac dock
1cb1882
Add "checkpoints" option, to permit disabling of checkpoint logic. b13fab5
New 'checkpoints' option should default to true. 6824732
Pieter Wuille Enable script verification for reorganized mempool tx 31f532b
Pieter Wuille Only send reorged txn to mempool after checkpoint 0d4c380
P. Kaufmann FlushBlockFile(): check for valid FILE pointer
- don't call FileCommit() and fclose() if no valid FILE pointer was
  returned by OpenBlockFile()
2334194
Pieter Wuille Make SetBestChain() atomic
In case a reorganisation fails, the internal state could become
inconsistent (memory only). Previously, a cache per block connect
or disconnect action was used, so blocks could not be applied in
a partial way. Extend this to a cache for the entire reorganisation,
making it atomic entirely. This also simplifies the code a bit.
0a94022
Peter Todd Replace text on how to enable IPv6 with disable
IPv6 support is now enabled by default, thus documentation should tell
you how to disable it.

Similarly the build-osx use of the flag can be removed.
eec557b
P. Kaufmann add 2 constructors in CDiskBlockPos to simplify class usage
- add a default-constructor, which simply calls SetNull() and a
  constructor to directly pass nFile and nPos
- change code to use that new constructors
a70ce53
P. Kaufmann Bitcoin-Qt: remove obsolete modal flag from GUI APIs
- as we (can) supply the CClientUIInterface::MODAL flag via the style
  parameter, we don't need a separate bool for checking the modality
f804203
P. Kaufmann use new message() function in BitcoinGUI
- 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
46ec79c
P. Kaufmann remove unneeded flag from MSG_INFORMATION and fix an indentation 6496a83
Simon added build instructions for Ubuntu >= 12.04 7fbec98
P. Kaufmann rework ThreadSafeAskFee() / askFee() functions
- remove unused parameter from ThreadSafeAskFee(), which also results in
  the removal of an orphan translation-string
a761c8c
Trevor Update src/makefile.mingw
With MinGW we use .a not .lib
1ead321
Pieter Wuille Add GetTimeMicros() for ore accurate benchmarking f1e482a
Pieter Wuille Add -benchmark for reporting block processing times d32c91a
P. Kaufmann call CheckDiskSpace() before pre-allocating space
- 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()
39bc56d
Pieter Wuille Reconstruct coins/ from scratch when missing. 5c80b21
Pieter Wuille Update the block file counter in database when using -reindex
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.
982502f
Pieter Wuille Allow lengthy block reconnections to be interrupted
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.
77f7236
Michael Cassano add rescan bool to importprivkey to control whether to do a rescan af…
…ter import
0c556cb
Gavin Andresen Checkpoint at first 25-btc-reward block (210,000) 6a421ca
Gavin Andresen Compile c/objective-c code max compatiblity when RELEASE b3a18c1
Andrey Alekseenko OptionsModel now has MapPortUPnP=false if UPNP is not supported 6b47769
Richard Schwab Change timestamps to use ISO8601 formatting 969a8ff
P. Kaufmann add threadsafety.h to bitcoin-qt.pro
- to be able to see threadsafety.h in the Qt Creator IDE the file needs to
  be added to the HEADERS section
13d773f
P. Kaufmann LevelDB: build_detect_platform fix NATIVE_WINDOWS indentation
- fix some indentation issues
992d365
Michael Ford Fix two typos in main.h
Break one long comment down into 3 lines so it's readable.
013d36f
Pieter Wuille Split off hash.h from util.h c235cc1
Pieter Wuille Convert fRescan argument to importprivkey to bool 1709ddf
Mike Gogulski

something tells me i've failed to do my first rebasing properly. Anyone got an education link that applies to this project's workflow?

P. Kaufmann

To which branch did you intend to rebase?

To rebase to current master I normaly do this:
git fetch upstream -p
git rebase upstream
git push origin %BRANCHNAME% -f

Mike Gogulski mikegogulski closed this December 26, 2012
Mike Gogulski

Closing this down in favor of #2130

Pieter Wuille
Collaborator

You know you can push to a branch that is already pull-requested, to have the pull request updated?

Mike Gogulski

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 49 unique commits by 14 authors.

Dec 04, 2012
Mike Gogulski Just some comments on what I'm aiming to do here 96079cc
Mike Gogulski more TODO comments for things to be moved to rpchelpers.cpp b2551e2
Dec 05, 2012
Mike Gogulski Make GetAccountAddress and SetAccount into methods of CWallet 56e396c
Mike Gogulski move some stuff touched by setaccount into methods of CWallet 35cda08
Dec 06, 2012
Mike Gogulski Some touch-ups per Diapolo's diff comments. b62f6de
Dec 07, 2012
Mike Gogulski make GetAccountBalance a method of CWallet; clean up some spacing f342af9
Dec 08, 2012
Mike Gogulski make GetAccountAddresses into a method of CWallet 18f7e1d
Mike Gogulski snip an unneeded comment; inline a couple of variables in
CWallet::GetAccountAddresses()
1668147
Mike Gogulski use CWallet::GetAccountAddresses() in getaddressesbyaccount() a167668
Mike Gogulski indentation fix 801be24
Dec 25, 2012
Mike Gogulski Remove CBitcoinAddress deps in wallet.cpp; Remove an inappropriate TODO. c7b1086
Dec 26, 2012
Christian von Roques tests for SetCompact and GetCompact of CBigNum 895e618
Christian von Roques reimplement CBigNum's compact encoding of difficulty targets
Use shifts instead of going through the MPI representation of BIGNUMs.
Be careful to keep the meaning of 0x00800000 as sign bit.
087d971
Trevor Add NATIVE_WINDOWS
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.
3e12b39
Alexander Kjeldaas o Added threadsafety.h - a set of macros using the -Wthread-safety
  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.
714fc57
Alexander Kjeldaas o Added AnnotatedMixin which adds locking annotations to the mutex
  API, compatible with clang's -Wthread-safety
e622ef1
Alexander Kjeldaas o Annotated lock-like functions in net.h.
o Removed unused function EndMessageAbortIfEmpty
637075a
Add new RPC "lockunspent", to prevent spending of selected outputs
and associated RPC "listlockunspent".

This is a memory-only filter, which is empty when a node restarts.
000f859
P. Kaufmann split of createTrayIconMenu() from createTrayIcon() in BitcoinGUI
- this allows to setup the trayicon before we have and want a trayicon menu
- should be of great use, when we remove that splash screen
- fixes a small bug with the toggleHideAction icon, which is not only used with
  trayicon but also with the Mac dock
1cb1882
Add "checkpoints" option, to permit disabling of checkpoint logic. b13fab5
New 'checkpoints' option should default to true. 6824732
Pieter Wuille Enable script verification for reorganized mempool tx 31f532b
Pieter Wuille Only send reorged txn to mempool after checkpoint 0d4c380
P. Kaufmann FlushBlockFile(): check for valid FILE pointer
- don't call FileCommit() and fclose() if no valid FILE pointer was
  returned by OpenBlockFile()
2334194
Pieter Wuille Make SetBestChain() atomic
In case a reorganisation fails, the internal state could become
inconsistent (memory only). Previously, a cache per block connect
or disconnect action was used, so blocks could not be applied in
a partial way. Extend this to a cache for the entire reorganisation,
making it atomic entirely. This also simplifies the code a bit.
0a94022
Peter Todd Replace text on how to enable IPv6 with disable
IPv6 support is now enabled by default, thus documentation should tell
you how to disable it.

Similarly the build-osx use of the flag can be removed.
eec557b
P. Kaufmann add 2 constructors in CDiskBlockPos to simplify class usage
- add a default-constructor, which simply calls SetNull() and a
  constructor to directly pass nFile and nPos
- change code to use that new constructors
a70ce53
P. Kaufmann Bitcoin-Qt: remove obsolete modal flag from GUI APIs
- as we (can) supply the CClientUIInterface::MODAL flag via the style
  parameter, we don't need a separate bool for checking the modality
f804203
P. Kaufmann use new message() function in BitcoinGUI
- 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
46ec79c
P. Kaufmann remove unneeded flag from MSG_INFORMATION and fix an indentation 6496a83
Simon added build instructions for Ubuntu >= 12.04 7fbec98
P. Kaufmann rework ThreadSafeAskFee() / askFee() functions
- remove unused parameter from ThreadSafeAskFee(), which also results in
  the removal of an orphan translation-string
a761c8c
Trevor Update src/makefile.mingw
With MinGW we use .a not .lib
1ead321
Pieter Wuille Add GetTimeMicros() for ore accurate benchmarking f1e482a
Pieter Wuille Add -benchmark for reporting block processing times d32c91a
P. Kaufmann call CheckDiskSpace() before pre-allocating space
- 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()
39bc56d
Pieter Wuille Reconstruct coins/ from scratch when missing. 5c80b21
Pieter Wuille Update the block file counter in database when using -reindex
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.
982502f
Pieter Wuille Allow lengthy block reconnections to be interrupted
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.
77f7236
Michael Cassano add rescan bool to importprivkey to control whether to do a rescan af…
…ter import
0c556cb
Gavin Andresen Checkpoint at first 25-btc-reward block (210,000) 6a421ca
Gavin Andresen Compile c/objective-c code max compatiblity when RELEASE b3a18c1
Andrey Alekseenko OptionsModel now has MapPortUPnP=false if UPNP is not supported 6b47769
Richard Schwab Change timestamps to use ISO8601 formatting 969a8ff
P. Kaufmann add threadsafety.h to bitcoin-qt.pro
- to be able to see threadsafety.h in the Qt Creator IDE the file needs to
  be added to the HEADERS section
13d773f
P. Kaufmann LevelDB: build_detect_platform fix NATIVE_WINDOWS indentation
- fix some indentation issues
992d365
Michael Ford Fix two typos in main.h
Break one long comment down into 3 lines so it's readable.
013d36f
Pieter Wuille Split off hash.h from util.h c235cc1
Pieter Wuille Convert fRescan argument to importprivkey to bool 1709ddf
Something went wrong with that request. Please try again.