Properly initialize CWallet::nTimeFirstKey #4254

tm314159 commented at 12:10 AM on May 30, 2014: contributor

Fixed unitialized variable in wallet.

Remove unused imports in macdeploy script d16f6f87e1

doc: Clarify wording about testing in README.md

Weaken and clarify the wording a bit, it currently implies that we get
more pull requests than we can ever handle which discourages
contribution.

4665b1ab6f

Properly initialize CWallet::nTimeFirstKey cd791d3873

tm314159 commented at 12:26 AM on May 30, 2014: contributor

nTimeFirstKey is an int64_t, so I think it requires a long 0? I can change to a regular 0 if people are more comfortable with it.

jgarzik commented at 12:29 AM on May 30, 2014: contributor

Ah, viewing artifact ('l' not '1'). You probably want 0LL, but it doesn't really matter, as the compiler will DTRT at compile time.

Though, in general, those zero-inits could probably all be removed.

tm314159 commented at 12:33 AM on May 30, 2014: contributor

It really does need the zero init. Here's the reason why I added the init:

tm@...:~/bitcoind/bitcoin/src$ valgrind ./bitcoind ==2337== Memcheck, a memory error detector ==2337== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. ==2337== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info ==2337== Command: ./bitcoind ==2337== ==2337== Conditional jump or move depends on uninitialised value(s) ==2337== at 0x319176: CWallet::LoadKeyMetadata(CPubKey const&, CKeyMetadata const&) (wallet.cpp:110) ==2337== by 0x33645A: ReadKeyValue(CWallet_, CDataStream&, CDataStream&, CWalletScanState&, std::string&, std::string&) (walletdb.cpp:509) ==2337== by 0x3374F0: CWalletDB::LoadWallet(CWallet_) (walletdb.cpp:623) ==2337== by 0x3218FD: CWallet::LoadWallet(bool&) (wallet.cpp:1485) ==2337== by 0x157F16: AppInit2(boost::thread_group&) (init.cpp:958) ==2337== by 0x140142: AppInit(int, char**) (bitcoind.cpp:143) ==2337== by 0x13649E: main (bitcoind.cpp:180) ==2337==

brandondahler commented at 2:06 AM on May 30, 2014: contributor

I would say either change it to 0LL or just plain 0. Using a lowercase 'L' looks incorrect and using just a single 'L' would cause it to be implicitly casted to 64 bits on a 32-bit compiler and explicitly defined to 64 bits on a 64-bit compiler (in most cases, because long is 64-bits on 64-bit compiler).

laanwj commented at 6:41 AM on May 30, 2014: member

@jgarzik No, the zero inits can not be removed. Not sure what makes you say so. C++ gives no guarantee about zeroing POD types before use. That said, please remove the lower-case l, it looks weird.

Merge pull request #4187

d16f6f8 Remove unused imports in macdeploy script (Federico Bond)

b73dc953a8

Merge pull request #4252

c21c74b osx: Fix missing dock menu with qt5 (Cory Fields)

97ab93f50b

Merge pull request #4218

4665b1a doc: Clarify wording about testing in README.md (Wladimir J. van der Laan)

32a9ddfc33

Change nTimeFirstKey initialization to regular 0 instead of 0l 36015dbe0a

tm314159 commented at 6:02 PM on May 30, 2014: contributor

Ok. Changed the nTimeFirstKey initialization to 0 and committed/pushed.

laanwj commented at 6:12 PM on May 30, 2014: member

Thanks! Please squash the two commits into one, then this can be merged.

Properly initialize CWallet::nTimeFirstKey

Change nTimeFirstKey initialization to regular 0 instead of 0l

Properly initialize CWallet::nTimeFirstKey to zero.

ca3775558d

Merge branch 'master' of https://github.com/tm314159/bitcoin 23e3cb9693

tm314159 commented at 9:12 PM on May 30, 2014: contributor

OK. Tried to squash the changes. Hope I did it correctly - not that familiar with git.

jgarzik commented at 10:05 PM on May 30, 2014: contributor

ACK code change @tm314159 What you need to do is a "rebase" -- rewrite your branch from scratch, containing only the one commit. Then "git push --force" to your branch associated with this github pull req. github will notice the branch was updated, and update the pull request.

Properly initialize CWallet::nTimeFirstKey 1477fa3c9c

Change nTimeFirstKey initialization to regular 0 instead of 0l 3b17800139

Merge branch 'master' of https://github.com/tm314159/bitcoin

rebased from upstream.

d9ac99d7fd

BitcoinPullTester commented at 11:36 PM on May 30, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d9ac99d7fd7a5afd93f1d2adf5298477e15f7a7a 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.

tm314159 commented at 11:59 PM on May 30, 2014: contributor

OK done. If this isn't correct, maybe I should just delete the branch and start all over, since it's just a one-liner.

jgarzik commented at 2:48 AM on May 31, 2014: contributor

Yes. "rewrite your branch from scratch, containing only the one commit" containing just that one liner. There should be one and only one commit on that entire branch. You are not appending a commit to an existing branch, you should be creating a new branch.

At a high level, you are creating a "clean history" rather than a bunch of commits in a sequence such as

my feature
fix my feature
fix the fix
merge the fix into my other development branch
etc.

None of that git history is needed in the main bitcoin repository. Just need a single commit.

laanwj closed this on Jun 3, 2014

laanwj commented at 6:56 AM on June 3, 2014: member

(merged as #4273)

MarcoFalke locked this on Sep 8, 2021