channeldb/kvdb: introduce new KV-store database abstraction #3833

Roasbeef · 2019-12-13T02:37:18Z

In this PR, we create a new package kvdb, which is meant to serve as the basis for any future database abstractions within lnd. Rather than directly use the walletdb package (which we base off of), we instead use a series of type-aliases to re-type the fundamental types/interfaces of the walletdb package. This lets us type kvdb.RwTx instead of walletdb.ReadWriteTransaction everywhere. Additionally, our usage of type-aliases is also intended to create an easy pathway in the future wherein we can gradually re-defined or re-implement these types to wean off of the walletdb package.

Cross-Over Considerations

One thing that we'll need to reconcile is that the CreateTopLevelBucket method for walletdb will return an error if the bucket doesn't exist, rather than creating the bucket just as CreateBucketIfNotExists for bbolt does. We can either make a small wrapper around the old interface to fix this, or just modify walletdb itself.

One other question is if we should go back and modify all the legacy migrations to use this new interface or not. Once new backends are added, there'll also come a time where we need to persist additional state to ensure that users can't arbitrarily move between backends (assuming they're replicated, a manual port would be needed).

What's Missing?

This PR is meant to just be a PoC (hence its marking as a draft PR) to demonstrate how we can easily introduce a new KV-like interface without limiting the future possibilities of a split database, or more high-level object methods (which would allow SQL based backends more flexibility w.r.t table structure).

Other things this PR needs to properly compile:

Finishing porting over all the main channeldb types to use the new kvdb package types.
Port over all the straggling instances of database usage outside the channeldb package
Figure out how we want to handle database upgrades (a two-level version maybe?), and the upgrade path to replicated databases.

Roasbeef · 2020-01-10T02:56:11Z

We have achieved full compilation everywhere!

Roasbeef · 2020-01-10T02:57:02Z

Before this lands some form of this PR will need to land in btcwallet: btcsuite/btcwallet#671

Roasbeef · 2020-01-16T01:12:35Z

The btcwallet PR has been merged, this PR is now ready for final review!

Roasbeef · 2020-01-17T03:23:05Z

Fixed an issue that broke the tests due to passing in the wrong type (pretty subtle) into the DB within the contractcourt. The rule going forward is that if you're using the Batch call, then you need to pass in the kvdb.Backend instance rather than the channeldb.DB struct which embeds_ that interface.

joostjager

Is there a (PoC) implementation of the abstract interface for a different backend, to validate that this is an abstraction that is going to work before merging it?

bhandras · 2020-01-17T11:02:56Z

For context: I'm going to work on an etcd backend. Aggree with @joostjager fully. It would be nice to still treat this PR as a PoC as there are a large differences between etcd and bbolt (no buckets, cursors are different, transactions have limitations).

channeldb/kvdb/interface.go

bhandras

Continuing the review now that some of the previous questions are proven to work.

channeldb/kvdb/interface.go

bhandras · 2020-02-19T07:42:51Z

channeldb/kvdb/interface.go

+
+// RwBucket represents a bucket (a hierarchical structure within the database)
+// that is allowed to perform both read and write operations.
+type RwBucket = walletdb.ReadWriteBucket


IMO, we should either use Rw or ReadWrite as a prefix for read-write APIs, but not mix them. I'm still in favor of reusing names from walletdb vs renaming, but Rw is indeed shorter (though looks inconsistent next to the Read prefix)

The type aliases can only modify top level exported types, and not methods as well.

What I meant is that there's a mix of Rw and ReadWrite prefixes in the type aliases which feels inconsistent.

bhandras · 2020-02-19T07:44:27Z

channeldb/kvdb/interface.go

+	_ "github.com/btcsuite/btcwallet/walletdb/bdb" // Import to register backend.
+)
+
+// Update opens a database read/write transaction and executes the function f


One thing that bothers me a bit is that the comment refers to the function signature, yet we can't see it here.

True we can't see it here, but it'll show up as normal on the rendered godoc (I think).

channeldb/kvdb/interface.go

contractcourt/briefcase.go

cfromknecht

completed a full pass, overall changes look good. i agree with @bhandras's comments regarding bbolt's method names being easier/simpler to use, esp. re TopLevel and NestedBucket. only other comments are about mainly about naming for interface types, preferring R* and RW* due to brevity and consistency

cfromknecht · 2020-01-22T15:22:44Z

channeldb/kvdb/interface.go

+
+// ReadTx represents a database transaction that can only be used for reads. If
+// a database update must occur, use a RwTx.
+type ReadTx = walletdb.ReadTx


naming suggestion: RTx and, similarly, RWTx? similar for cursor, bucket, etc. little strange that one type uses the full name while the other uses an abbreviation

channeldb/channel.go

cfromknecht · 2020-01-22T15:30:34Z

go.mod

@@ -44,6 +44,7 @@ require (
 	github.com/rogpeppe/fastuuid v1.2.0 // indirect
 	github.com/tv42/zbase32 v0.0.0-20160707012821-501572607d02
 	github.com/urfave/cli v1.18.0
+	go.etcd.io/bbolt v1.3.3


no go.sum change?

still no go.sum change? is there a reason?

Roasbeef · 2020-03-18T03:47:19Z

Rebased to the latest version of master.

bhandras

I think this is ready. My only complaint is the mixing of names (Rw vs ReadWrite).
LGTM

bhandras · 2020-03-18T14:11:01Z

channeldb/kvdb/interface.go

+
+// RwBucket represents a bucket (a hierarchical structure within the database)
+// that is allowed to perform both read and write operations.
+type RwBucket = walletdb.ReadWriteBucket


What I meant is that there's a mix of Rw and ReadWrite prefixes in the type aliases which feels inconsistent.

Roasbeef · 2020-03-18T18:39:43Z

Build is now green.

cfromknecht

overall i think the naming could improve, but looks correct. LGTM as long as we commit to doing a rename at a later point. only one lingering question

cfromknecht · 2020-03-18T21:38:16Z

go.mod

@@ -44,6 +44,7 @@ require (
 	github.com/rogpeppe/fastuuid v1.2.0 // indirect
 	github.com/tv42/zbase32 v0.0.0-20160707012821-501572607d02
 	github.com/urfave/cli v1.18.0
+	go.etcd.io/bbolt v1.3.3


still no go.sum change? is there a reason?

In this commit, we create a new package `kvdb`, which is meant to serve as the basis for any future database abstractions within `lnd`. Rather than directly use the `walletdb` package (which we base off of), we instead use a series of type-aliases to re-type the fundamental types/interfaces of the `walletdb` package. This lets us type `kvdb.RwTx` instead of `walletdb.ReadWriteTransaction` everywhere. Additionally, our usage of type-aliases is also intended to create an easy pathway in the future wherein we can gradually re-defined or re-implement these types to wean off of the `walletdb` package.

In this commit, we migrate all the code in `channeldb` to only reference the new `kvdb` package rather than `bbolt` directly. In many instances, we need to add two version to fetch a bucket as both read and write when needed. As an example, we add a new `fetchChanBucketRw` function. This function is identical to `fetchChanBucket`, but it will be used to fetch the main channel bucket for all _write_ transactions. We need a new method as you can pass a write transaction where a read is accepted, but not the other way around due to the stronger typing of the new `kvdb` package.

The explicit `bbolt` dep is gone, as we depend on `kvdb`, which is actually `walletdb`, which has its own module that defines the proper `bbolt` version.

Roasbeef · 2020-03-19T02:35:57Z

still no go.sum change? is there a reason

If I build, then go go mod tidy, I get this diff:

diff --git a/go.mod b/go.mod
index 55e4fea1e..812f824fa 100644
--- a/go.mod
+++ b/go.mod
@@ -14,7 +14,6 @@ require (
 	github.com/btcsuite/btcwallet/walletdb v1.2.0
 	github.com/btcsuite/btcwallet/wtxmgr v1.0.0
 	github.com/btcsuite/fastsha256 v0.0.0-20160815193821-637e65642941
-	github.com/coreos/bbolt v1.3.3
 	github.com/davecgh/go-spew v1.1.1
 	github.com/go-errors/errors v1.0.1
 	github.com/golang/protobuf v1.3.1
@@ -45,7 +44,6 @@ require (
 	github.com/rogpeppe/fastuuid v1.2.0 // indirect
 	github.com/tv42/zbase32 v0.0.0-20160707012821-501572607d02
 	github.com/urfave/cli v1.18.0
-	go.etcd.io/bbolt v1.3.3
 	golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67
 	golang.org/x/net v0.0.0-20190206173232-65e2d4e15006
 	golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
diff --git a/go.sum b/go.sum
index c2773b2e5..b8991183e 100644
--- a/go.sum
+++ b/go.sum
@@ -185,9 +185,6 @@ golang.org/x/crypto v0.0.0-20170930174604-9419663f5a44/go.mod h1:6SG95UA2DQfeDnf
 golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
 golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67 h1:ng3VDlRp5/DHpSWl02R4rM9I+8M2rhmsuLwAMmkLQWE=
 golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
-golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
-golang.org/x/crypto v0.0.0-20200109152110-61a87790db17 h1:nVJ3guKA9qdkEQ3TUdXI9QSINo2CUPM/cySEvw2w8I0=
-golang.org/x/crypto v0.0.0-20200109152110-61a87790db17/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
 golang.org/x/lint v0.0.0-20180702182130-06c8688daad7/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
@@ -199,8 +196,6 @@ golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73r
 golang.org/x/net v0.0.0-20181220203305-927f97764cc3/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20190206173232-65e2d4e15006 h1:bfLnR+k0tq5Lqt6dflRLcZiz6UaXCMt3vhYJ1l4FQ80=
 golang.org/x/net v0.0.0-20190206173232-65e2d4e15006/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
-golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3 h1:0GoQqolDA55aaLxZyTzK/Y2ePZzZTUrRacwib7cNsYQ=
-golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
@@ -213,8 +208,6 @@ golang.org/x/sys v0.0.0-20181107165924-66b7b1311ac8/go.mod h1:STP8DvDyc/dI5b8T5h
 golang.org/x/sys v0.0.0-20181116152217-5ac8a444bdc5/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190209173611-3b5209105503 h1:5SvYFrOM3W8Mexn9/oA44Ji7vhXAZQ9hiP+1Q/DMrWg=
 golang.org/x/sys v0.0.0-20190209173611-3b5209105503/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
-golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
-golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20190904154756-749cb33beabd h1:DBH9mDw0zluJT/R+nGuV3jWFWLFaHyYZWD4tOT+cjn0=
 golang.org/x/sys v0.0.0-20190904154756-749cb33beabd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=

See the last commit for the rationale.

Updated the types to always use Rw for ReadWrite.

Roasbeef added database Related to the database/storage of LND golang/build system Related to the go language and compiler incomplete WIP PR, not fully finalized, but light review possible labels Dec 13, 2019

Roasbeef force-pushed the kv-abstraction branch from ee9f91d to 9d9c73e Compare January 10, 2020 02:55

Roasbeef marked this pull request as ready for review January 10, 2020 02:55

Roasbeef requested review from cfromknecht, halseth, joostjager and wpaulino as code owners January 10, 2020 02:55

Roasbeef requested review from bhandras and removed request for halseth, joostjager and wpaulino January 10, 2020 02:56

Roasbeef force-pushed the kv-abstraction branch 2 times, most recently from 33509e6 to d54fd5e Compare January 10, 2020 07:06

Roasbeef added this to the 0.10.0 milestone Jan 14, 2020

Roasbeef force-pushed the kv-abstraction branch from d54fd5e to a3a9bab Compare January 16, 2020 01:11

Roasbeef force-pushed the kv-abstraction branch from 3495b68 to 6e8cde7 Compare January 16, 2020 02:57

Roasbeef added the v0.10 label Jan 17, 2020

joostjager reviewed Jan 17, 2020

View reviewed changes

bhandras reviewed Jan 17, 2020

View reviewed changes

channeldb/kvdb/interface.go Show resolved Hide resolved

bhandras reviewed Jan 17, 2020

View reviewed changes

channeldb/kvdb/interface.go Show resolved Hide resolved

bhandras reviewed Jan 17, 2020

View reviewed changes

channeldb/kvdb/interface.go Show resolved Hide resolved

bhandras requested changes Feb 19, 2020

View reviewed changes

cfromknecht reviewed Mar 2, 2020

View reviewed changes

andrerfneves mentioned this pull request Mar 3, 2020

[WIP] Add post for socratic 102 BitDevsNYC/BitDevsNYC.github.io#50

Merged

Roasbeef requested review from cfromknecht and bhandras March 18, 2020 03:47

Roasbeef force-pushed the kv-abstraction branch from 6e8cde7 to daba17a Compare March 18, 2020 03:47

bhandras approved these changes Mar 18, 2020

View reviewed changes

cfromknecht reviewed Mar 18, 2020

View reviewed changes

Roasbeef added 14 commits March 18, 2020 19:34

contractcourt: convert to use new kvdb abstraction

320101d

htlcswitch: convert to use new kvdb abstraction

4e68914

routing: convert to use new kvdb abstraction

28bbaa2

watchtower: convert to use new kvdb abstraction

557b930

macaroons: convert to use new kvdb abstraction

852b238

discovery: covert to use new kvdb abstraction

ace7a78

sweep: convert to use new kvdb abstraction

46cec2f

lnwallet: convert to use new kvdb abstraction

98170e0

chainntnfs: convert to use new kvdb abstraction

f196308

autopilot: convert to use new kvdb abstraction

21521ff

lnd: convert to use new kvdb abstraction

071c7cb

build: silence new linter errors, tidy modules

a4e3990

The explicit `bbolt` dep is gone, as we depend on `kvdb`, which is actually `walletdb`, which has its own module that defines the proper `bbolt` version.

Roasbeef force-pushed the kv-abstraction branch from daba17a to a4e3990 Compare March 19, 2020 02:36

Roasbeef merged commit 4897b34 into lightningnetwork:master Mar 19, 2020

cfromknecht mentioned this pull request May 6, 2020

multi: rename kvdb Read* types to R* #4252

Merged

guggero mentioned this pull request Jul 27, 2020

travis: add integration test on ARM #4408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb/kvdb: introduce new KV-store database abstraction #3833

channeldb/kvdb: introduce new KV-store database abstraction #3833

Roasbeef commented Dec 13, 2019 •

edited by bhandras

Loading

Roasbeef commented Jan 10, 2020

Roasbeef commented Jan 10, 2020

Roasbeef commented Jan 16, 2020

Roasbeef commented Jan 17, 2020

joostjager left a comment

bhandras commented Jan 17, 2020

bhandras left a comment

bhandras Feb 19, 2020

Roasbeef Mar 18, 2020

bhandras Mar 18, 2020

bhandras Feb 19, 2020

Roasbeef Mar 18, 2020

cfromknecht left a comment

cfromknecht Jan 22, 2020

cfromknecht Jan 22, 2020

cfromknecht Mar 18, 2020

Roasbeef commented Mar 18, 2020

bhandras left a comment

bhandras Mar 18, 2020

Roasbeef commented Mar 18, 2020

cfromknecht left a comment

cfromknecht Mar 18, 2020

Roasbeef commented Mar 19, 2020

channeldb/kvdb: introduce new KV-store database abstraction #3833

channeldb/kvdb: introduce new KV-store database abstraction #3833

Conversation

Roasbeef commented Dec 13, 2019 • edited by bhandras Loading

Cross-Over Considerations

What's Missing?

Roasbeef commented Jan 10, 2020

Roasbeef commented Jan 10, 2020

Roasbeef commented Jan 16, 2020

Roasbeef commented Jan 17, 2020

joostjager left a comment

Choose a reason for hiding this comment

bhandras commented Jan 17, 2020

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Mar 18, 2020

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Mar 18, 2020

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Mar 19, 2020

Roasbeef commented Dec 13, 2019 •

edited by bhandras

Loading