#50 Add database versioning

[avemeva]

In this commit the:

• Upgrade mechanism for database was added which makes the current schema rigid and upgradeable.
• Additional bucket metaBucket was added which stores key that house meta-data related to the current/version state of the database.
• Function createChannelDB was modified to create this new bucket+key during initialisation.
• Backup logic was added which makes a complete copy of the current database during migration process and restore the previous version of database if migration failed.

[Roasbeef]

I've completed an initial review of this PR. I haven't yet dug deeply into the tests though.

Correct me if I'm wrong, but I think there's some critical logic missing within the SyncVersions method at the moment: if the process dies (power shutdown, killall, etc), then the database remains in a possibly corrupted state. To fix this, I think within the method, before doing any further processing, the current directory should be checked for existence of the back-up file. If found, then the the file should be restored and the function exiting early. Then in this scenario the user is required to start lnd again in order to ensure proper crash recovery.

Other than what I pointed out above, my comments are relatively minor, great job! With the infrastructure you've set up with this PR doing future migrations should be relatively streamlined in the future once we start doing stable releases.

[Roasbeef]

I think a comment long the lines of this may be a bit more accurate:

Migration is a function which takes a prior outdated version of the database instances and mutates the key/bucket structure to arrive at a more up-to-date version of the database.

[Roasbeef]

Also, I'm not sure this type needs to be externally exported. Database migrations should essentially be silent and obscured from the view outside of this package.

[Roasbeef]

Can you please use the full object initialiaztion syntax here? I think it makes this initialzaion a bit more clear.

So something like:

DBVersions = []Version{
    Version{
        number: 1, 
        migration: nil, // The base DB version requires no migration. 
    }
}

[Roasbeef]

Like the others above, I don't think this variable needs to be publicly exported.

[Roasbeef]

Similar to the comment above, I don't think this struct needs to be publicly exported. One sign of this is that it has no public member variables and is method-less.

[Roasbeef]

Usage of named returns for functions is purposefully avoided across the project.

Can you modify this function to use regular returns? Thanks

[Roasbeef]

In order to make the migration process more robust and fault-tolerant, the entire migration process should take place within a single bolt database transaction.

[Roasbeef]

Doubling down on this statement, I don't think most of the additional logic in this PR is necessary. Additionally, I fear a scenario where a bug in the switchover+backup leads to full database corruption.

With the the single transaction approach, the entire process either is succesful, or fails. If we go with the approach currently implemented (and initially recommend by myself), then we can possibly end up in one of many intermediate states, requiring complex code to recognize and properly restore the state of the database to a stable state.

I think simply relying on bolt db's crash resistance+recovery is a safer, and simpler approach.

[Roasbeef]

Just a minor nit, but alternative syntax which achieves the same thing is:

var migrations []Migration

Alternatively, we could also do something like this which only results in a single allocation:

migrations := make([]Migration, 0, len(versions))

[Roasbeef]

This method is missing a godoc-like comment.

[Roasbeef]

The error from this function is being ignored!

[avemeva]

There is no error from this function, because if db version not exist it's initialised with last db version number.

[Roasbeef]

Minor nit: to save a few lines, the meta variable and err can just be returned directly.

So:

return meta, err

[Roasbeef]

Since no further processing in the function takes place past updating the meta-data state, one can simply do:

return d.store.Update(func(tx *bolt.Tx) error {
      .... 
})

[Roasbeef]

Aside from the minor nits I pointed out in the latest review patch, LGTM!

[Roasbeef]

Minor nit: there's an extra new-line here that can be deleted.

[Roasbeef]

Same nit here about the extra new-line.

[Roasbeef]

Minor nit: I think the variable name openChanBucket in this function and the one below should be renamed to metaBucket as that's the bucket actually being used in this context.

[Roasbeef]

The name in the godoc comment (TestGlobalVersionList) and the actual function name (TestOrderOfMigrations) are mismatched.

[Roasbeef]

It's not clear to me what these if-statements are actually doing.

It seems that they should be replaced with:

if migrations[0].number != 2 {
  ... 
} 

if migrations[1].number != 3 {
  ... 
} 

[avemeva]

@Roasbeef I added additional comment in test. Basically what I am doing in this test is checking the order of migrations that will be applied, getMigrationsToApply function returns array of migration functions.

[Roasbeef]

Nice, LGTM!

This'll prove super useful once we start to roll out stable releases 🚀