Use LevelDB by default for the transaction database #1619

mikehearn commented at 6:47 PM on July 21, 2012: contributor

This patch set imports the Google LevelDB 1.5 database, refactors the transaction database code a little and then lets you pick between LevelDB or BDB via a build flag (qmake USE_LEVELDB=- to go back).

On test machines, LevelDB seems to be significantly faster than BDB due to the fact that the bulk of the IO work is handled by a separate thread. The main thread just appends data to the end of a log file when writing, and the IO thread then turns logs into sorted tables and compacts them together.

When indexing the block chain Bitcoin appears to be CPU bound on Windows and Linux machines, even during migration when signature checking is disabled. On my SSD based MacBook Pro the performance isn't as good and it still seems to be IO bottlenecked, which is the opposite of what I'd expect.

This code instructs LevelDB to use a 10-bit bloom filter per block. This expands the size of the database quite a bit, but allows a reduction in disk seeks. I haven't tested different bloom filter sizes to see what works better. It is an area for future work.

LevelDB is statically included because:

Upstream is not widely packaged and the build system does not support "make install", so it's annoying to link against
We need custom patches to make it work on Windows.
It's important that all Bitcoin users are running on a db engine tested by us. We don't want some fraction of the network nodes to be "upgraded" but still running on BDB just because of who built their binaries.

luke-jr commented at 6:56 PM on July 21, 2012: member

LevelDB should not be forked unless absolutely necessary; try to get Win32 support upstream first
If it must be forked, the fork should have its own git repository, which can be embedded using git submodules (or just pulled in by gitian for Win32)
Dynamic linking to OS-provided LevelDB should be supported, especially since the fork is only needed for Win32

luke-jr commented at 6:59 PM on July 21, 2012: member

Also, does LevelDB support in-memory db environments for the test suite?

justmoon commented at 11:04 PM on July 21, 2012: contributor

If it must be forked, the fork should have its own git repository, which can be embedded using git submodules (or just pulled in by gitian for Win32)

I'm maintaining the branch here, feel free to fork/clone: https://github.com/justmoon/leveldb-mingw

It should be pointed out that the "fork" doesn't actually modify a single line of code in LevelDB. LevelDB runs in Chromium (env_chromium.cc) on Windows and is heavily tested there, but of course those APIs are Chrome specific. For Mac, Linux etc. it comes with an env_posix.cc. For Windows, Edouard wrote an env_boost.cc. All I did then was to make the MSVC specific stuff MingW compatible. (diff) @mikehearn: Let me know if/how we should go about promoting this upstream. We'd probably have the best chances once we also test native compilation on Windows with both GCC and MSVC - which is on my todo list.

Dynamic linking to OS-provided LevelDB should be supported, especially since the fork is only needed for Win32

I agree, there are many bad, but also a few good reasons why people might want to link to their own LevelDB build, so there should be a clean, canonical way of doing it without having to edit build files.

mikehearn commented at 11:10 PM on July 21, 2012: contributor

I'll wait for Gavin to give his opinion on this before doing any work.

Honestly, I don't care about trying to get the Windows support into upstream. The authors/maintainers of LevelDB are some of the most senior engineers at Google. They are, to put it mildly, very busy. Nor do they particularly care about making LevelDB useful outside the Chrome context. As forks go, this one is likely to not be much of a hassle, but upstreaming the Windows/MinGW support likely is.

I also don't see any benefit in depending on some other git repository via gitian, or making it easy for people to link against a dynamic libleveldb. There aren't any reasons why you'd have leveldb on your system. If somebody wants to modify the leveldb code, they could just as easily patch the version in the source tree.

I think the whole libraries/dynamic/static/forking thing is a distraction. LevelDB is just code. There are no reasons you'd want to have it split out from the main codebase. Nobody should be upgrading the version of LevelDB Bitcoin uses unless they have a good understanding of both codebases, it's definitely not something that should just be randomly pulled in via an apt-get upgrade.

Diapolo commented at 11:26 PM on July 21, 2012: none

So should this compile fine on Windows with MinGW and the default Qt SDK GCC (which is currently still 4.4) as it is? Just interested, how much longer takes a compilation with this patch?

mikehearn commented at 12:03 AM on July 22, 2012: contributor

Try it and find out :-)

LevelDB is a small library. It should only take a minute or two more.

justmoon commented at 8:25 AM on July 22, 2012: contributor

So should this compile fine on Windows with MinGW and the default Qt SDK GCC (which is currently still 4.4) as it is?

LevelDB's build system uses a shell script (build_detect_platform) to create a build_config.mk file. To build natively on Windows, we would need some other way to generate that file. Other than that, the code should work with MSVC and MingW-GCC - though I haven't tested it yet, so there may a few minor issues to fix.

Just interested, how much longer takes a compilation with this patch?

On my Ubuntu system, Bitcoin-Qt takes 312.8s to compile (which doesn't change significantly with this patch) and LevelDB takes 13.5s to compile. So the overall increase of the compile time is about 4-5% for me.

Note that LevelDB is not currently included in a make clean, so you don't rebuild it very often. Maybe it should be though?

mikehearn commented at 10:24 AM on July 22, 2012: contributor

make clean should work. If you look at the makefile diffs then I tried to ensure it'd be run at the right times, at least for the makefiles in src/. Is there a case where that doesn't work?

justmoon commented at 10:59 AM on July 22, 2012: contributor

moon@clymene:/home/ubuntu/bitcoin$ make clean
[...]
moon@clymene:/home/ubuntu/bitcoin$ ls src/leveldb-1.5.0/libleveldb.a 
src/leveldb-1.5.0/libleveldb.a

justmoon commented at 11:09 AM on July 22, 2012: contributor

Oh I see what you mean - I think you added it to the makefiles, but not to bitcoin-qt.pro, so when using qmake if you do a make clean, LevelDB won't be included.

mikehearn commented at 10:02 AM on July 30, 2012: contributor

I've just seen the same issue as Stefan reported, where sometimes hashBestChain seems to go missing from the DB. I haven't had time to investigate yet.

justmoon commented at 10:16 AM on July 30, 2012: contributor

For reference, this is the error Mike is talking about. After a clean shutdown, the next start goes like this:

Bitcoin version v0.6.1-523-g2af483b-dirty-beta (2012-07-02 17:41:31 +0200)
Startup time: 07/24/12 08:40:11
Default data directory C:\Users\moon\AppData\Roaming\Bitcoin
Used data directory C:\Users\moon\AppData\Roaming\Bitcoin
Bound to [::]:8333
Bound to 0.0.0.0:8333
Loading block index...
Opening LevelDB in C:\Users\moon\AppData\Roaming\Bitcoin\txleveldb
Opened LevelDB sucessfully
ERROR: CTxDB::LoadBlockIndex() : hashBestChain not found in the block index
 block index             243ms
Loading wallet...
dbenv.open LogDir=C:\Users\moon\AppData\Roaming\Bitcoin\database
ErrorFile=C:\Users\moon\AppData\Roaming\Bitcoin\db.log
nFileVersion = 69900
Error loading block index database
[...]

sipa commented at 2:03 PM on July 31, 2012: member

I'd like to have all database-breaking changing merged at once, as I don't we'd like to either support many different combinations of database environments, or force users to regularly rebuild or at least reindex their database.

I'm specifically referring to Jeff's attempts to split the database in several parts, and my "ultraprune" branch (though we should first do benchmarks to check the performance of combining both).

Also, @luke-jr, ultraprune has an abstract class to represent the coin database, and I've already implemented an std::map-backed implementation, so whether or not the database itself can be memory-backed is not much of an issue anymore.

mikehearn commented at 7:57 AM on August 1, 2012: contributor

My concern with that is we need better performance right now, because miners have been observed dropping transactions to ensure fast block propagation.

Pruning is a more complex change with unresolved questions like how to ensure we have a good enough collection of archival nodes, how they are found, etc. I guess it also improves performance (maybe?) due to the smaller working set - combining these branches and getting some performance metrics would be useful.

Jeffs work I think is subsumed by this because leveldb is structured as lots of small files already. On 31 Jul 2012 16:03, "Pieter Wuille" < reply@reply.github.com> wrote:

I'd like to have all database-breaking changing merged at once, as I don't we'd like to either support many different combinations of database environments, or force users to regularly rebuild or at least reindex their database.

I'm specifically referring to Jeff's attempts to split the database in several parts, and my "ultraprune" branch (though we should first do benchmarks to check the performance of combining both).

Also, @luke-jr, ultraprune has an abstract class to represent the coin database, and I've already implemented an std::map-backed implementation, so whether or not the database itself can be memory-backed is not much of an issue anymore.

Reply to this email directly or view it on GitHub: #1619 (comment)

luke-jr commented at 8:03 AM on August 1, 2012: member

IMO, the solution to block propagation is to relay before doing any of the heavy per-transaction lifting anyway. Anything else just reduces the problem, but doesn't eliminate it.

Diapolo commented at 8:03 AM on August 1, 2012: none

As I still had not the time to test it another question. This does not replace the blk000x.dat files, which are no "database", right?

sipa commented at 10:29 AM on August 1, 2012: member

@mikehearn The reason I'm pushing ultraprune is exactly because of the performance improvements. If I disable signature checking, importing the entire blockchain (using -loadblock, a few weeks ago) took 7 minutes on my laptop (6 minutes when on tmpfs instead of disk, 5 minutes when I use an std::map-backed store instead of BDB). It was originally intended to reduce storage requirements and permit pruning, but because of the much smaller working set, there is a very significant speedup. Pruning isn't even implemented right now (but would be trivial), and I need a bit more time to do tests to verify that it works exactly as the current validation engine, but apart from that, it is mostly functional. I hope to have a pull request soon.

justmoon commented at 1:39 PM on August 1, 2012: contributor

As I still had not the time to test it another question. This does not replace the blk000x.dat files, which are no "database", right?

Thats correct.

BitcoinPullTester commented at 11:20 PM on August 8, 2012: none

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

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not merge cleanly onto current master
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32

mikehearn commented at 2:44 PM on August 15, 2012: contributor

I rebased and fixed up the unit tests that no longer worked. It uses an in-memory leveldb for the tests now. Also made qmake clean work, but it relies on a horrible hack that assumes things about how qmake works internally. I didn't find a better way to do it, unfortunately.

AFAIK this now resolves all issues except for Lukes comments, but as mentioned before, I don't plan on doing anything about that unless Gavin requires it because it seems like a lot of work for no benefit.

luke-jr commented at 2:53 PM on August 15, 2012: member

No benefit? I'd think improved maintainability and socially acceptable distribution of the client, not to mention the benefit of Win32 support for other projects if LevelDB accepts the patches upstream, are pretty obvious benefits. It shouldn't be much work, either.

BitcoinPullTester commented at 2:55 PM on August 15, 2012: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/9211106f282e05b345e77fee608e5c832e775923 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32

mikehearn commented at 3:19 PM on August 15, 2012: contributor

I think Jenkins was building an earlier version. I did two forced pushes fairly close to each other. Hopefully it'll try again soon.

The main reason I don't see much benefit is I don't really expect leveldb to change from this point onwards. If upstream doesn't change then having it be a separate library doesn't achieve much.

The patch we're using wouldn't be accepted upstream, I already asked. Sanjay wants raw Win32 usage, not via boost. We already depend on boost so it makes no difference for us. But the env files would have to be rewritten and tested by a Windows developer.

jgarzik commented at 3:45 PM on August 15, 2012: contributor

Most of the @luke-jr requests are not reasonable, but I do agree about wanting to avoid forking Windows support.

Is there a technical reason why straight Win32 API cannot be used? I looked through env_boost.cc and port_win.cc, and it seems like a doable task.

Several of the functions in env_boost.cc would become quite a bit shorter, possibly just one line, if you just used the standard Windows APIs.

IMO get Windows support upstream, then clone. We don't want to maintain a fork I don't think.

laanwj commented at 3:48 PM on August 15, 2012: member

I think it's OK to maintain our own database within the bitcoin source base. This gives more control, and prevents unexpected data format changes and incompatibility (as we have seen with BerkelyDB...).

justmoon commented at 4:05 PM on August 15, 2012: contributor

Is there a technical reason why straight Win32 API cannot be used? I looked through env_boost.cc and port_win.cc, and it seems like a doable task.

Yes it is doable as evidenced by the fact that it has been done:

http://code.google.com/p/leveldbwin/source/browse/trunk/leveldb/util/env_win32.cc?r=3

This file is woefully out of date I'm afraid. But it should allow even somebody with very lacking Win32 API skills to complete this project in a reasonable amount of time.

Since I want a Windows port of LevelDB for the Windows version of BitcoinJS and since we do not have a dependency on Boost, I shall have a go at it.

sipa commented at 4:14 PM on August 15, 2012: member

I have no problem with keeping a fork in our codebase for now. The code is small, and as mentioned, it makes us safe(r) from incompatible database file changes.

If a Win32 support gets merged upstream, we can still consider switching back.

BitcoinPullTester commented at 4:24 PM on August 15, 2012: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/ad6ba92a3b5f86c2c11b282bb98aea85371feabd for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32

BitcoinPullTester commented at 5:57 PM on August 15, 2012: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/39f12e99f7699f4ec0e3e26051dda1571de6cf84 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32

luke-jr commented at 7:12 PM on August 15, 2012: member

@jgarzik Why is it unreasonable to keep a separate git repository for a separate codebase? It's trivial (would take about 15 minutes to setup), harmless, and has much to gain (upstream can merge it, directly). If @mikehearn doesn't want to spend 15 minutes on it, I wouldn't object to doing it myself...

jgarzik commented at 7:36 PM on August 15, 2012: contributor

A separate git tree is a messy headache... but hey, LLVM does it with clang.

sipa commented at 7:45 PM on August 15, 2012: member

Oh I was just commenting on whether or not to maintain our own LevelDB branch. I don't mind doing it in a separate git repository, though I don't have a preference.

laanwj commented at 6:51 AM on August 16, 2012: member

I'd prefer to keep everything that is needed to build bitcoin together in one repository. I've worked with git submodules before, and it's a pain. I'm sure if it's a pain for me it's especially for new contributors that don't know the system. Splitting up the repository is useful for really large and modular projects, neither applies to bitcoin.

We can always remove it again if upstream ever decides to merge it (which they will not do until a rewrite so...).

luke-jr commented at 7:41 AM on August 16, 2012: member

Guess we better import libstdc++, G++, etc...

Upstream doesn't even have to merge anything for most builders. The LevelDB fork is only needed for Windows - which is already stupidly difficult without gitian, and gitian can easily pull in any repo it needs.

laanwj commented at 9:48 AM on August 16, 2012: member

If it is decided to include leveldb:

We should not hardcode the version number (1.5.0) inside the repository, and thus rename the directory to 'src/leveldb'
I'm not sure building the .a separately is needed. It makes the qmake file (at least) pretty ugly. Is it possible to include the .cc files that we need in the build project like we do with spirit-json? I'm not sure this is a good idea either but I agree with @luke-jr that the current "nested build" setup is pretty ugly

jgarzik commented at 3:03 PM on August 16, 2012: contributor

Building internal foo.a files is not ugly -- it is normal for large projects. And it usually helps improve link times. I'm certain Qt can handle it somehow.

laanwj commented at 3:06 PM on August 16, 2012: member

This has nothing to do with qt; most bigger qt projects use cmake... qmake is pretty much a hack itself

BitcoinPullTester commented at 1:23 PM on August 21, 2012: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/ae98492e87de56f5e6c1869b1a7bb80f7b79c579 for test log.

This pull does not merge cleanly onto current master

Import LevelDB 1.5, it will be used for the transaction database. 20ce7aaa56

Implement LevelDB support in Bitcoin.

This accelerates the tx index database significantly. See the
discussion on bitcoin-dev for details.

19939a86d5

Implement migration to leveldb from bdb.

490 seconds on my machine for 185k blocks.

f2113d66ec

Leveldb Windows port by Edouard Alligand, adapted for MingW by me. 2861d84088

Ugly hack to make qmake clean work 265b5d6845

Also log the transactions/second in AcceptBlock, for easier performance comparisons. 9437e19fb1

Fix linux-mingw makefile. f0ca3b0576

Another fix for makefile.linux-mingw 9ab78efc90

BitcoinPullTester commented at 3:12 PM on August 30, 2012: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/9ab78efc90dc8ac7380e23f6f53d5a9b71666af9 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in http://jenkins.bluematt.me/pull-tester/files/FullBlockTestGenerator.java)

mikehearn commented at 4:04 PM on September 20, 2012: contributor

Closing this as it should be merged as part of ultraprune.

mikehearn closed this on Sep 20, 2012

suprnurd referenced this in commit 5f4362cb82 on Dec 5, 2017

DrahtBot locked this on Sep 8, 2021