[Wallet] replace BDB with internal append only (logdb) backend #5686

This pull request is in early stage but discussions could help driving this into the right direction.

It will basically remove bdb (currently not on compile level) and make use of a internal key/value store. Logdb (the internal key/value store) is a append only store where every key/value-frame contains a sha256 checksum all available frames. Consistency is guaranteed.

The current implementation passes all tests but has lack of recover/rewrite possibilities.

Why this

• IMO BerkleyDB is oversized for a "reference wallet implementation". This append-only file-store could prevent from corruptions and relieves compile stress.
• wallet flush threads are no longer required.
• remove another dependency and get more control over our stack

What's next / ToDos

• add compact Rewrite() function (save whole memory map atomically to a clean file): this is required after encrypting the wallet (added 2015/01/22)
• add thread safe / concurrency tests for logdb
• add Recover() function: scan through a corrupt wallet file and try to recover keys even when some key/value-frames and/or checksums are corrupted
• add more tests
• add wallet encryption test

This is on my internal roadmap:

• remove bdb

• transform account to labels
• implement bip32
• add spv mode

Currently there is no migration tool from bdb to logdb. Because it would be good to avoid Bdb in our binaries, a python tool for wallet migration could do the job.

IMO the version code 0.1x should make clear that such required manual migrations are still possible.

@jonasschnelli Agree that a migration tool is necessary before rolling this out.

There are a few complications with python that would need double-checking:

• Are python installs guaranteed capable of reading BDB 4.8-format data?
• For people using --with-incompatible-bdb configure option -- like me and some of the other devs -- conversion becomes even more annoying.

I would prefer a C++ migration tool. Eventually you could split it off into a separate project or repository if the continued BDB link-time dependency remains annoying.

@jgarzik a C++ tool would probably get better results. Agreed. It would just help the main bitcoin repository to not have to much legacy stuff. There is also some other things that can be removed from the code like the bdb wallet migration and features-checking (example: wallet feature for compressed key, etc.).

Moving the c++ migration tool to separate repository makes sense. There we could also support the --with-incompatible-bdb option so users could make sure to use the same version as they used while creating/manipulating their wallets.

Are python installs guaranteed capable of reading BDB 4.8-format data?

Yes - Python is generally linked against the system's BDB, which on modern systems is always >4.8. Newer BDB versions have no problem reading old databases. The problem is the other way around. After the file is "touched" it won't be backwards compatible anymore. Making a temporary copy then reading that would be a strategy to keep the origin file as-is.

Apart form that: nice work! I agree with your internal roadmap.

I think it makes sense to do this work on a 'newwallet' branch so that other people can cooperate, and we can merge it into master when the work is complete, which will likely be no sooner than 0.12.

+1 @laanwj

Concept ACK for the wallet changes.

@jonasschnelli Yes. I go back and forth between "separate repo now" and "separate repo later"

For the initial conversion, it makes everything easier if the migration tool is in bitcoin/bitcoin.git and is built with the current build system. That is the path of least resistance.

Build system & install Just Works. You automatically get the migration tool, even if you didn't know you needed it until (oops, right now!)

I try to be extra-conservative with Real Money(tm), which is what we're dealing with when it comes to the "legacy wallet" we're maintaining.

It may be a good idea to have 0.11 still compatible with bdb wallets without upgrading them - note that all our wallet upgrades have been opt-in so far, and you can still use a 0.4.0 wallet with 0.10 while retaining backward compatibility.

Supporting both (bdb and logdb) wallet formats (at runtime) would require some work. I'm also aware of the missing backward compatibility. Supporting bdb wallets after a possible switch to logdb would require resources better spent at bip32, accounting overhaul, etc. IMO a clean cut would be worth it. Remove all the migration, min-version feature-compatibility legacy stuff.

I agree with not letting users in the dark and support a migration tool (c++, included in bitcoind for 0.11 or 0.12, but slowly extract migration tool to different repository). First "logdb" release could still include bdb as dependency, used for compiling the bitcoin migration tool. If you miss the library, it will build bitcoin-core without this tool (like #5680).

Migration of a wallet is a risky process and it's probably better to do this outside of bitcoind as a conscious decision from the user.

On IRC I've explicitly argued against dual database support, and I'm going to do so here again. Having an intermediate phase where both databases are supported means legacy concerns (such as accounts) have to ooze into the new wallet interface. It also complicates testing. Are you going to verify that the new wallet will keep working with 0.3 wallets at every step of development?

An alternative option I've thought of is to leave the old wallet untouched. Move wallet,walletdb,rpcwallet (and others as needed) to src/legacywallet.

Then build this new wallet with new database, new interface in src/wallet/. As well as a migration tool.

Using a configure option, one can then either build against the new wallet or the old wallet. This gives the 'conservative' choice to stick with the old wallet (for however long we're willing to carry it) with all its quirks, but without intermediate complications for the new code, and having to carry any of that over.

In the beginning the legacywallet will be the default and the new wallet will be experimental-only.

This is basically the "fork the wallet and develop it separately" approach, but a bit improvised as it's all in one repository - for now. It means we can merge this soon(TM) and experiment with it, instead of delaying that to a further future 'when it's ready'.

(the only complication here I see is with regard to the UI, which has to interface with both, but a similar thing can be done there; UI that only applies to the old wallet can be moved to a legacy directory)

Sounds reasonable, but if we can't build with both supported, how will binaries go out? Probably not too much more effort to just keep the class names unique and switch between them at runtime startup, is it?

I like laanwjs idea of supporting both formats with a compile option.

Two concerns I see:

• the new wallet needs some changes in init.cpp. I could try to make the init.cpp changes flexible in case of the backend database. Even rpcwallet.cpp could support both. But this needs work.

The 2nd concern is more a "conceptual" one. If we support both wallet formats, why should one upgrade his wallet? Couldn't that not lead to a need of supporting both formats? And if we once drop support of BDB, how is the migration process different now from the one in future (the user has to do it once anyway)? Maybe we could force people to upgrade and save some - already rare - development resources. I mean it's not the network, it's the wallet.

The original idea I discussed w/ @sipa on IRC was dropping support for BDB from bitcoind entirely, plus an auto-launched migration tool. Still prefer that arrangement. From bitcoind's standpoint, BDB support is deleted. No dual mode codebase.

@jonasschnelli

• Yes, initialization/deinitalization should be moved to the wallet module(s) themselves and only called from init. This is long due anyhow, and mostly low-impact move-only.
• My idea is not about supporting two formats, but supporting two different wallets. The old wallet will stay exactly as it is now (barring critical fixes). The new wallet will see new development, and come with new features (as well as get rid of old cruft), and an interface that supports e.g. multiwallet. It just means that the transition process is more smooth.

More daring users can use the new wallet, whereas more conservative users can stay with the old wallet for say a major release or two until it is completely axed (but the new one, as well as the conversion process, much better tested). @luke-jr Building a monster executable with both wallet implementations could be possible, with e.g. an command line option to switch. Not sure if this is a good idea in practice, but I don't want to exclude the possibility upfront. We'll see when we get there I suppose. In the beginning this won't be an issue as the new wallet will be experimental-only so requiring build-time invocations is a good thing.

@laanwj hmmm, that is an interesting way to transition. Let's think through the user making that old wallet / new wallet choice. That plan would indeed make for a nice, clean separation.

I like the roadmap with multiwallet support and a legacy wallet / new wallet selectable. Maybe add a migrate button that only appears when the legacy wallet is selected. Maybe we want to support this wallet selection (and the migration button) as a build option disabled by default until it gets more testing as @laanwj also suggests. Then enable it by default or just remove the option and enable it always and eventually, some day, drop support for the legacy wallet entirely. The "alternate repo for the migration tool" would just be the version previous to dropping the support. It could be as short as:

• 0.11 multiwallet with migration tool
• 0.12 drop support for legacy wallet.

I did some brainwork in how we could handle the wallet code. Supporting both type of wallets at runtime or as compile option would be possible but some has drawbacks:

• if we support both wallets, things like multiwallet or bip32 needs implementation in both worlds. Even if only one wallet type can handle things like multiwallet support, the interface needs adaption for supporting this somehow.
• also how would we handle unit-tests, rpc-tests? Run twice for both wallets? IMO this makes no sense.

I also did a simple implementation of a possible command line arg to switch the wallet type. Abstract CWallet, implement CLogDBWallet, CBDBWallet. But it just feels after the wrong way.

I strongly suggest supporting only one wallet type and supply a perfect working migration tool. A clean cut in implementation. IMO this sounds after good invested time/work into the right things.

There's no rush in merging the new wallet into the master.

@jonasschnelli What about

void CLegacyWallet::Bip32Interface(args)
{
    throw "CWallet::Bip32Interface() not implemented by CLegacyWallet";
}

? For testing you can test both for the common interface and only the new one for the new features. Of course, I agree it's all simpler if no new features are added until the legacy wallet is removed, which I think should be the next release just after the one with the migration tool.

Another option to avoid having multi-wallet support at all would be to have a release with the new wallet and the migration tool in the form of an "import old wallet from file" option. But I'm not sure how other people will feel about that type of backards support.

I believe separating a CWallet interface from a CLegacyWallet implementation would already be a gain and a good first step for all cases. I'm not sure if you're arguing against that, against multi-wallet support, or only against supporting several types of wallets at the same time.

I thought about the implementation once more. Unless somebody else convince me for another way i think i start with this (maybe this is more or less the original idea of @laanwj):

• Replace the wallet code inside of the #ifdef WALLET_ENABLE with flexible hooks. Goal: get rid of wallet-code inside of the bitcoind's general code. We can keep the ifdefs or we could provide a empty "no-wallet" instance to allow the user to run without a wallet.
• Move everything wallet related to src/wallet/ (rpc dispatch table, unit-test, etc.)
• Create a clean, more or less abstract, wallet interface outside or src/wallet where wallets can inherit from
• Add support for the legacy bdb wallet (update to the new interface)
• Add support for the already more or less done logdb wallet
• Add a feature flags to wallet-interface so bitcoin-qt can check if a wallet support a such feature like multiwallet, backup-capabilities, bip32, etc.

This would be more effort then i originally thought, but will finally cut out the wallet-code and place it behind a clean interface.

Any concerns when going into this direction?

It really depends on how ugly the code becomes, how large the maintenance burden becomes, to support two wallets in parallel. It sounds like there will be a lot of "creeping #ifdefs" and semi-duplicated code in an effort to keep the legacy wallet unchanging.

@jgarzik The legacy-wallet needs mostly move-only operation (move stuff from init.cpp, rpcmisc, etc.) to src/legacywallet/wallet.cpp (which then should provide a new class(name) CLegacyWallet [or similar]). I would see most wallet #ifdefs gone, unless people would find it better to have ifdefs then a CWallet conform "no-wallet" implementation.

But yes. The legacy-bdb-wallet needs a lot of moving/changes to fit in a new flexible wallet implementation. The drawbacks are time and risks.

@jonasschnelli : Sounds good to me. Some notes:

provide a empty "no-wallet" instance to allow the user to run without a wallet..

Don't call it a no-wallet. As with the NotificationInterface, just give it a general name, like ModuleInterface. In this case it's used to plug in a wallet, but the same mechanism could be used to plug in another module that provides e.g. RPC calls and hooks into the init/shutdown.

This is not just a hack to support two wallets, but a step toward more modularity. Not sure if you ever saw #3440, but it was a plan in the same direction.

if we support both wallets, things like multiwallet or bip32 needs implementation in both worlds. Even if only one wallet type can handle things like multiwallet support, the interface needs adaption for supporting this somehow.

My point was to avoid a common interface by completely separating the implementations. E.g. move the old wallet and RPC code that uses it out of the way.

If this turns out to not be possible/practical, of course you could decide to not do this.

I'm just afraid this will mean it will take a long time until the new wallet can be merged. It means it needs to be suitable for all users of the current wallet, instead of initially just experimental users. It also means it will get a lot less testing until it is merged. This has shown to be a problem for pulls that make large changes to the wallet before (like the last round of multi-wallet changes), eventually resulting in the owner giving up and them never being merged. That's why I proposed this, not to give you extra work.

But yes. The legacy-bdb-wallet needs a lot of moving/changes to fit in a new flexible wallet implementation.

OK, my idea was that this would not be needed. Every change potentially introduces bugs. If you need many changes to the legacy wallet, this defeats the purpose of having a stable, unchanged legacy wallet.

Because this is getting to big, i try to form independent easy-reviewable PRs to slowly come towards the new wallet. I first like to move the wallet into a module by decoupling init/RPC/tests and remove the ifdef ENABLE_WALLET (only one ifdef ENABLE_WALLET for loading the module).

Sounds like a good plan to me. Can you reference those PRs from here as you create them?

@jtimon currently it's #5752, #5758, #5744 But also have a look at the "status" page: #5761.

A suggestion for a more robust frame format.

uint64_t Unique Magic per log file
uint64_t Counter
uint64_t Length
    0xffffffffffffffff indicates length not specified
uint64_t Unique Magic per Record
uint8_t[Length] Body
uint64_t Unique Magic per Record
SHA256 hash of Counter|Length|Unique Magic per Record|Body

• on length-not-specified, seems like the majority of cases (100%?) would use 0xffff..
• it can sometimes be useful to pad the body to a 64-bit etc. alignment

@pstratem but wouldn't the 2nd and 3rd element (uint64_t Counter and uint64_t Length) break the advantage of a append only format?

@jgarzik

on length-not-specified, seems like the majority of cases (100%?) would use 0xffff..

The purpose of allowing an unspecified length is extremely long records, those simply do not exist in a bitcoin wallet.

• Keys
• Transactions

it can sometimes be useful to pad the body to a 64-bit etc. alignment

eh... yeah I guess sometimes, but that should be defined as part of the format for the Body, note everything else is 64 bit aligned

@jonasschnelli

but wouldn't the 2nd and 3rd element (uint64_t Counter and uint64_t Length) break the advantage of a append only format?

you're gonna have to explain that one

but wouldn't the 2nd and 3rd element (uint64_t Counter and uint64_t Length) break the advantage of a append only format?

you're gonna have to explain that one

Ah now i see! I first thought the counter and length is at the beginning of the file and contains the combined length/counter of all frames within the filestore. But as i read again i saw that you propose the counter and length per frame.

This kind of stuff is the reason for the frame to be more robust. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg886512.html

Leaning towards closing as Work In Progress. General IRC consensus has been "we want this" for a long time, so concept ACK from most developers seems implied, myself included.

However, this seems more of a longer term project resulting in a long lived PR. Recommend storing this on a branch and opening a mailing list thread to follow development.

Closing. This is included in my core wallet fork an will be further developed there with a chance of allowing a merge to this repository if it's once is stable and tested enough.

I started an append only key/value file format in pure C, based on @sipa concept, that aims to be storage format for wallets (https://github.com/libbtc/libbtc/pull/41), suitable for MCUs, smartphones, desktop, etc.

Just in case someone wants to contribute.

What's the status on this? It's been over a year now, is this idea dead?