Leveldb subtree at 1 12 0 #2702

Performs the following:

• Brings the LevelDB code base up to 1.12.0
• Incorporates an upstream deadlock fix for Windows condition variables
• Incorporates upstream fix #178: cannot resize a level 0 compaction set
• Limits the rate of compaction to prevent runaway resource usage

Here's the upstream issue regarding the deadlock: http://code.google.com/p/leveldb/issues/detail?id=149

And the compaction issue: http://code.google.com/p/leveldb/issues/detail?id=178

This is the commitlog of the subtree: https://github.com/ripple/leveldb/commits/ripple-fork

Here's a decent comparison highlighting the benefits of git-subtree over submodules: http://blogs.atlassian.com/2013/05/alternatives-to-git-submodule-git-subtree/

This blog post describes my experiences with submodules: http://codingkilledthecat.wordpress.com/2012/04/28/why-your-company-shouldnt-use-git-submodules/

This pull request still contains a "Remove leveldb in preparation for git-subtree", which was already done.

Can you rebase this, and only include relevant commits?

Code changes look good to me otherwise.

Roger that, I have rewritten the branch. It will need to pass through the automated testing again, although in theory the files are still the same. But don't take my word for it! I rewrote public history.

As per sipa's comments, if you want individual leveldb commits to appear in the bitcoin commit log instead of a squash, that can be done. But the price is that first the entire leveldb commit log must be merged into bitcoin's commit log once. After that, new commits to leveldb will appear incrementally.

I'd prefer to have access to the individual changes that were made to the leveldb subtree.

Ideally, the leveldb changes appear as individual commits in our repository, but that may be hard (I'm not familiar enough with git-subtree yet). An alternative is having a leveldb branch (or separate) repository under the bitcoin project, and occasionally pull changes from there through such a squashed commit as exists in this pullreq now. Other devs opinions? @gavinandresen @jgarzik @gmaxwell @laanwj?

Updated the version of leveldb to 1.12.

LGTM. I didn't notice this pull also updated the version, my mistake. The diff is identical to what I got doing it the old fashioned way, so it seems like the upgrade is a no-brainer.

To copy what I wrote before, the fix for CompactRange() would be security sensitive if we ever used that call anywhere. Given the risk of someone introducing a call to that in future, we should ensure this upgrade does take place.

I don't care how the commits are represented one way or another. LevelDB is stable and doesn't change much. It hardly seems worth worrying about the optimal arrangement given that fact.

I vote for upgrading to the latest LevelDB also, as we seem to have quite a few strange related DB-corruption issues.

Has anyone done the necessary audit to be sure there aren't some uncontroversial bugfixes which could affect Bitcoin network behaviour?

The repository for Ripple and Bitcoin's fork of LevelDB has been moved to the Ripple organization:

https://github.com/ripple/leveldb

@gavinandresen @jgarzik @laanwj @gmaxwell Are you in favor of having a bitcoin/leveldb repository?

/me rather liked the in-tree attributes. That was one of the selling points in moving to leveldb from BDB: it would be in-tree, so no crazy version troubles due to strange linking.

@jgarzik Yes, note that what you have now is "in-tree." What @sipa is asking is if you want to also have as a public repository, your fork of leveldb. @sipa I say, why not add bitcoin/leveldb? There's no downside to it. You can either fork the one from ripple, or create your own empty repository and push to it like this:

    # From your local bitcoin repository

    # add a remote for bitcoin's fork of leveldb
    git add leveldb git@github.com:bitcoin/leveldb.git 

    # add a remote for ripple's fork
    git add ripple git@github.com:ripple/leveldb.git

    # bring the ripple fork's ref into the local repo, so subtree-split can find it
    git fetch ripple

    # split out the commits to leveldb into its own branch
    git subtree split -P src/leveldb -b leveldb

    # push the recreated branch to bitcoin's repository.
    # it will be called bitcoin-fork
    git push leveldb leveldb:bitcoin-fork

@sipa You should try these steps anyway. You can always throw away the branch or repository. Get some practice with git-subtree, it's great!

@jgarzik No one suggests changing that. This is using git-subtree, so a copy of the LevelDB source is still inside our code, but there's a separate repository where development of our LevelDB tree can happen (it has significant changes, including a non-upstream Windows port with some patches by us). This way, the LevelDB code can live somewhat independently, but we can easily synchronize the in-repo copy.

One of the nice things about git-subtree is that it doesn't matter where that extra repository is maintained - it's only identified using commit ids. In a previous patch, we 'switched' to a tree that @vinniefalco extracted from our LevelDB history, and this commits pulls in changes that have been made there. My question is whether we wouldn't rather have such a LevelDB tree under github.com/bitcoin.

@vinniefalco No need to yell.

@sipa Yeah yeah, I forgot to mark those commands as "code" in GitHub flavored markdown so they became section headers in large bold text.

Anyway, here's a simple improvement for bringing in LevelDB that lets you trim it from the Makefile or whatever and also will speed up your build:

https://gist.github.com/vinniefalco/5880905

At some point I will make it work without the fancy macros and put it in our fork so anyone can use it.

To answer Luke, yes, I audited the changes and didn't see anything that could cause desynchronization or other security issues. The FD limit was raised but we did that already, I think.

On Fri, Jun 28, 2013 at 12:21 AM, Vinnie Falco notifications@github.comwrote:

@sipa https://github.com/sipa Yeah yeah, I forgot to mark those commands as "code" in GitHub flavored markdown.

Anyway, here's a simple improvement for bringing in LevelDB that lets you trim it from the Makefile or whatever and also will speed up your build:

https://gist.github.com/vinniefalco/5880905

— Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2702#issuecomment-20159155 .

I've amended the pull request to include Vaclav's changes from 6/12/2013: Added GNU/kFreeBSD kernel name (TARGET_OS)

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

Should I rebase this? It's been 2 months...

It seems like a simple merge, not sure why it's not done yet. There's no changes in the new leveldb that could cause problems.

FYI, in Ripple we are trying out HyperLevelDB and it is looking pretty awesome.

https://github.com/rescrv/HyperLevelDB

Okay, I did a coinstate rebuild at height 248116 with this code and got a bit identical UTXO set at the end.

I had a little concern about the level 0 compaction fix because it looked like under some corner case conditions (and not just making a compact call) that it could screw up and lose some modifications to the database. Even if thats true, that kind of corruption hasn't happened for at least my node.

I do think we should just setup a bitcoin/leveldb and subtree that though. It looks like reparenting it is kind of obnoxious (creates a big diff), so when we do need to apply some leveldb patch it would better be a tree we control. Am I missing something here?

I think this code has had sufficient review, so I'm merging this.

I've since setup a bitcoin/leveldb repository, but I'm not familiar enough with git-subtree to set up things properly.

You have my contact info, whenever you want I can teach you

On Sun, Jul 28, 2013 at 4:01 AM, Pieter Wuille notifications@github.comwrote:

I think this code has had sufficient review, so I'm merging this.

I've since setup a bitcoin/leveldb repository, but I'm not familiar enough with git-subtree to set up things properly.

— Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2702#issuecomment-21681833 .

Follow me on Github: https://github.com/vinniefalco

@gmaxwell To apply a patch to leveldb all you need to do is split the subtree out of Bitcoin using "git subtree split", and it will become an orphaned branch in your local repo (a branch that doesn't share any history with bitcoin). Then you apply the level db patches to that branch. Finally, use "git subtree pull" or "git subtree merge" (preferably with --ff-only) on your local orphaned leveldb branch to bring the changes back in.

Once you have split your leveldb branch using "git subtree split" you can push it to your own remote leveldb repository and maintain it as a fork. Or not. Whatever you want to do. The files will still always exist as normal files in the bitcoin repo.

As usual contact me if you have questions.

I had a little concern about the level 0 compaction fix because it looked like under some corner case conditions (and not just making a compact call) that it could screw up and lose some modifications to the database.

I had the same concern when I saw it, so I checked with Sanjay and he said it could only occur if you manually used CompactRange. I guess he'd know, so I left it at that.

Note that the changes in this pull request have been superced by #2907 .