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

mikegogulski commented at 11:05 am on December 5, 2012: none

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

Just some comments on what I'm aiming to do here 96079cce38

more TODO comments for things to be moved to rpchelpers.cpp b2551e24cb

Make GetAccountAddress and SetAccount into methods of CWallet 56e396ce0d

BitcoinPullTester commented at 11:27 am on December 5, 2012: none

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

move some stuff touched by setaccount into methods of CWallet 35cda0806a

BitcoinPullTester commented at 5:39 pm on December 5, 2012: none

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

Some touch-ups per Diapolo's diff comments. b62f6de0a2

mikegogulski commented at 5:18 pm on December 6, 2012: none

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

BitcoinPullTester commented at 2:32 am on December 7, 2012: none

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

make GetAccountBalance a method of CWallet; clean up some spacing f342af9bfc

BitcoinPullTester commented at 11:05 pm on December 7, 2012: none

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

make GetAccountAddresses into a method of CWallet 18f7e1d388

snip an unneeded comment; inline a couple of variables in

CWallet::GetAccountAddresses()

1668147ff3

BitcoinPullTester commented at 0:04 am on December 8, 2012: none

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

use CWallet::GetAccountAddresses() in getaddressesbyaccount() a16766870e

BitcoinPullTester commented at 0:32 am on December 8, 2012: none

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

indentation fix 801be244dd

BitcoinPullTester commented at 9:59 pm on December 8, 2012: none

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

CodeShark commented at 6:55 am on December 24, 2012: contributor

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

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

mikegogulski commented at 11:34 pm on December 24, 2012: none

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

sipa commented at 5:30 pm on December 25, 2012: member

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?

mikegogulski commented at 6:13 pm on December 25, 2012: none

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.

Remove CBitcoinAddress deps in wallet.cpp; Remove an inappropriate TODO. c7b1086d9f

in src/rpcwallet.cpp: in 801be244dd outdated

397@@ -513,6 +398,7 @@ Value getbalance(const Array& params, bool fHelp)
398         nMinDepth = params[1].get_int();
399 
400     if (params[0].get_str() == "*") {
401+    	// TODO: Why is there a "different way" if both "should always return the same number"?

sipa commented at 8:31 pm on 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.

mikegogulski commented at 8:34 pm on December 25, 2012:

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.

BitcoinPullTester commented at 9:08 pm on December 25, 2012: none

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

tests for SetCompact and GetCompact of CBigNum 895e618227

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.

087d971294

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.

3e12b3930f

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.

714fc57a2d

o Added AnnotatedMixin which adds locking annotations to the mutex

API, compatible with clang's -Wthread-safety

e622ef1e57

o Annotated lock-like functions in net.h.

o Removed unused function EndMessageAbortIfEmpty

637075a8a5

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.

000f859afa

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

1cb1882847

Add "checkpoints" option, to permit disabling of checkpoint logic. b13fab57a5

New 'checkpoints' option should default to true. 68247322ad

Enable script verification for reorganized mempool tx 31f532bcae

Only send reorged txn to mempool after checkpoint 0d4c380729

FlushBlockFile(): check for valid FILE pointer

- don't call FileCommit() and fclose() if no valid FILE pointer was
  returned by OpenBlockFile()

2334194a51

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.

0a94022f8c

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

a70ce53bcd

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.

eec557b012

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

f80420348e

added build instructions for Ubuntu >= 12.04 7fbec986b1

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

46ec79cdd9

remove unneeded flag from MSG_INFORMATION and fix an indentation 6496a83f5a

rework ThreadSafeAskFee() / askFee() functions

- remove unused parameter from ThreadSafeAskFee(), which also results in
  the removal of an orphan translation-string

a761c8c507

Update src/makefile.mingw

With MinGW we use .a not .lib

1ead321ad1

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()

39bc56d151

Add GetTimeMicros() for ore accurate benchmarking f1e482a503

Add -benchmark for reporting block processing times d32c91a50c

Reconstruct coins/ from scratch when missing. 5c80b21e41

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.

982502f15b

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.

77f72362b5

add rescan bool to importprivkey to control whether to do a rescan after import 0c556cb3eb

Checkpoint at first 25-btc-reward block (210,000) 6a421ca65b

OptionsModel now has MapPortUPnP=false if UPNP is not supported 6b4776977c

Change timestamps to use ISO8601 formatting 969a8ff194

Compile c/objective-c code max compatiblity when RELEASE b3a18c1821

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

13d773fdcb

LevelDB: build_detect_platform fix NATIVE_WINDOWS indentation

- fix some indentation issues

992d36570b

Fix two typos in main.h

Break one long comment down into 3 lines so it's readable.

013d36f7b0

Split off hash.h from util.h c235cc1e16

Convert fRescan argument to importprivkey to bool 1709ddf52f

mikegogulski commented at 2:23 pm on December 26, 2012: none

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

Diapolo commented at 3:53 pm on December 26, 2012: none

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

mikegogulski closed this on Dec 26, 2012

mikegogulski commented at 8:53 pm on December 26, 2012: none

Closing this down in favor of #2130

sipa commented at 9:10 pm on December 26, 2012: member

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

mikegogulski commented at 9:14 pm on December 26, 2012: none

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.

HashUnlimited referenced this in commit 7fb06fad78 on Jun 4, 2018

DrahtBot locked this on Sep 8, 2021

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