Skip to content

Conversation

jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Sep 26, 2012

Hopefully this is done in a database-independent manner, with minimal interference to the upcoming ultraprune.

@BitcoinPullTester
Copy link

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

src/main.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wouldn't delete bdb database files yourself. Use the dbenv methods to do so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with sipa, dbenv seemed to get very confused when I renamed database files behind its back for my corrupt wallet.dat handling code.

@sipa
Copy link
Member

sipa commented Sep 28, 2012

Surprising how little code changes are necessary for this. Ultraprune already has a CDiskBlockPos however, which is also used inside CTxBlockPos, so maybe you can use that instead? I think you can just copy it, and remove the IMPLEMENT_SERIALIZE.

@sipa
Copy link
Member

sipa commented Oct 1, 2012

I also wonder about this: what if the current best chain happens to be known in the new/repained blockchain? There should be no need to rebuild the entire index in that case, only the block positions.

EDIT: right, you can't, as in the current database structure, transactions also contain disk positions. It's something that can be done post-ultraprune though.

@jgarzik
Copy link
Contributor Author

jgarzik commented Oct 3, 2012

Updated with the following changes:

  • uses CDiskBlockPos from sipa's ultraprune branch
  • calls our CDBEnv to perform database removal

@sipa
Copy link
Member

sipa commented Oct 7, 2012

Have you tested this?

$ ./bitcoind -reindex
DB_ENV->dbremove: method not permitted before handle's open method

@gavinandresen
Copy link
Contributor

I wrote up a 4-step test plan that I've been working through.

I also have a branch (not published) to reconcile this with my corrupt wallet handling pull; @jgarzik see https://github.com/gavinandresen/bitcoin-git/commits/BDB_RECOVER

The testplan I was working through:

  1. Run new testnet3-in-a-box instance, let it IBD, shut it down. Remove testnet3/database/ directory.
    EXPECT: automatic re-index of blk0001.dat on startup.
  2. Same a (1), but run with -detachdb. Corrupt blkindex.dat by truncating it by a few bytes, then restart.
    EXPECT: automatic re-index of blk0001.dat on startup.
  3. Same as (2), but corrupt some bytes instead of truncating the file.
    EXPECT: automatic re-index.
  4. Compile a bitcoind against a later version of BDB (e.g. BDB 5 instead of 4). Run without -detachdb.
    Then startup a bitcoind compiled against the earlier BDB version.
    EXPECT: Something Reasonable-- not a BDB_RUNRECOVERY uncaught exception/crash.
  5. Same as (4), run with -detachdb.
    EXPECT: Same something reasonable as (4).

I haven't tested 4/5 yet, and don't know enough about BDB major version compatibility to know what is reasonable to expect.

@jgarzik
Copy link
Contributor Author

jgarzik commented Oct 9, 2012

Doing the database remove in the environment definitely creates a super-large removal transactions, yet does not actually remove blkindex.dat. blk0002.dat is growing here, which should not be the case. Further debugging is needed.

@gavinandresen
Copy link
Contributor

Instead of: dbenv.dbremove(NULL, strFile.c_str(), "main", DB_AUTO_COMMIT); you might try dbenv.dbremove(NULL, strFile.c_str(), NULL, DB_AUTO_COMMIT);
... then re-create blkindex.dat using TxDb("cr")

I think passing NULL as the third argument removes the file.

Caveat: a new genesis block is added to blkNNNN.dat with each run,
thanks to LoadBlockIndex() creating one, when it creates a blank block index.
@jgarzik
Copy link
Contributor Author

jgarzik commented Oct 10, 2012

Fixed a few bugs, and rebased on top of @gavinandresen 's ::RemoveDb(). blkindex.dat is now properly removed, and reindex proceeds as expected. Things appear to be working now.

Caveat: a genesis block is newly stored to blkNNNN.dat, for each -reindex invocation. LoadBlockIndex() adds a new one to the (it thinks) newly created block file. All other blocks are properly read and processed in-place.

-reindex highlights the poor speed of BDB indexing alone... and should provide a useful apples-to-apples basis for comparing the old BDB system with ultraprune.

@sipa
Copy link
Member

sipa commented Oct 20, 2012

Needs rebase.

@BitcoinPullTester
Copy link

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

@jgarzik
Copy link
Contributor Author

jgarzik commented Oct 21, 2012

Superceded by #1943, closing

@jgarzik jgarzik closed this Oct 21, 2012
@jgarzik jgarzik deleted the reindex branch August 24, 2014 04:19
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
626b0d9 incrementalmerkletree file added to cmake. (furszy)
4368a00 incremental merkle tree test: resolve invalid deserialization/serialization data equality check. (furszy)
9cfb0a7 rename sapling util file to not shadow src/util file. (furszy)
68d494d Incremental Merkle Tree + merkletree tests back port (furszy)

Pull request description:

  This is coming from bitcoin#1798, focused on include the incremental merkle tree primitive with all of its unit tests.
  This base primitive class is almost one to one with upstream.

  Decoupled the following commits:

  * Incremental Merkle Tree + merkletree tests back port —> 762d5f1
  * Rename sapling util file to not shadow src/util file --> cae89d3

ACKs for top commit:
  random-zebra:
    ACK 626b0d9
  Fuzzbawls:
    utACK 626b0d9

Tree-SHA512: 9bae913aa553ec42864b5f5f252f8ce6bd9fd6fd89011c63c31e1de06cba820dc99feeaabe38801f012a54abc81b8213aee0334695c4e0673591e48dc07d5954
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
b11b357 removing global namespace usage for noteencryption and sapling_note unit tests (furszy)
52da4e0 [Sapling] Note Encryption unit tests back ported. (furszy)
4d2f01a [Sapling] change cm() to cmu() in SaplingNote class (furszy)

Pull request description:

  Another decoupling from bitcoin#1798. Similar to bitcoin#1870, these changes are part of the primitives unit test coverage back port work.
  Commits included:

  * change cm() to cmu() in SaplingNote class —> d437276
  * Note Encryption unit tests back ported. —> e4c1bbf

ACKs for top commit:
  Fuzzbawls:
    ACK b11b357
  random-zebra:
    re-utACK b11b357

Tree-SHA512: 56914dba65ee239f7e6713a111abca5bd1a4e684fadcdbed4314295f499f3b6689271fa7999c7f182dcba069ebe29299bed944e7ee6b1bcb3aad987884811242
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants