udb: add upgradeToVersion12 #1126

kLkA · 2018-05-03T19:54:55Z

In this upgrade function we do check all MSOs for related
tx existence. If TX doesn't exist then we drop MSO

jrick · 2018-05-04T13:40:01Z

Few issues:

A long time ago we had several different components (address manager, tx store, stake store) all being saved in the database, but they were versioned separately and one component could not use data from another because it could not assume the data format. This was fixed by moving everything under the udb package and merging all of the components under a single database version. The old upgrade code for each individual component was still necessary to bring it up to the state where the merge could be performed, but following that, no more upgrades are made to the components individually. See upgrades.go, upgrades_test.go, and the testdata directory for the new upgrade mechanism and testing infrastructure.

Second, you can not delete or put values when using a cursor or within a ForEach function (which also wraps a cursor). This invalidates the cursor and further cursor usage can silently cause database corruption. Read all values which need changes first and then modify them at the end.

jrick · 2018-05-04T13:47:46Z

Also, be aware of golang/go#18007 when generating the testdata files. I have needed to rename testdata to something else just to go run the generators.

kLkA · 2018-05-04T19:09:42Z

I didn't get why to generate new testdata since it is only data inconsistency fix. It should be fully compatible with v7 version, isn't it?

jrick · 2018-05-07T13:50:40Z

wallet/udb/upgrades.go

+	// removeMultisigOutsWithoutTransactions is the tenth version of the database.
+	// It fixes issue with MultisigOuts without related transactions. Simply find all
+	// detached MultisigOutputs and remove
+	removeMultisigOutsWithoutTransactionsVersion = 10


This name is a bit long. removeExtraMultisigOutsVersion reads better to me. The comment should also describe why they were an issue (failure to insert transactions due to multisig output double spend checks, because they were not always correctly removed in previous database code). And please don't use "simply" when describing the fix, just describe how the fix is performed.

jrick · 2018-05-07T13:50:51Z

wallet/udb/upgrades.go

+	metadataBucket := tx.ReadWriteBucket(unifiedDBMetadata{}.rootBucketKey())
+	txmgrBucket := tx.ReadWriteBucket(wtxmgrBucketKey)
+
+	// Assert this function is only called on version 8 databases.


jrick · 2018-05-07T13:52:25Z

wallet/udb/upgrades.go

+
+	var multisigOutsMarkedForDelete [][]byte
+	multisigOutBucket := txmgrBucket.NestedReadWriteBucket([]byte("ms"))
+	multisigOutBucket.ForEach(func(k, v []byte) error {


I'd prefer a cursor to be used here for consistency, instead of a ForEach. See the other upgrade functions, which all use cursors.

jrick · 2018-05-07T13:52:57Z

wallet/udb/upgrades_test.go

@@ -15,6 +15,7 @@ import (
 	"path/filepath"
 	"testing"

+	"errors"


move this to the above group with all of the stdlib packages

jrick · 2018-05-07T13:53:53Z

wallet/udb/upgrades_test.go

@@ -34,6 +35,7 @@ var dbUpgradeTests = [...]struct {
 	// No upgrade test for V7, it is a backwards-compatible upgrade
 	{verifyV8Upgrade, "v7.db.gz"},
 	// No upgrade test for V9, it is a fix for V8 and the previous test still applies
+	{verifyV10Upgrade, "v7.db.gz"},


If we are testing a 9 -> 10 upgrade, we should have a version 9 database with the error.

jrick · 2018-05-07T14:01:26Z

After upgrading, the database will be compatible with the older db test artifacts, however those won't expose the problem we are trying to fix here.

kLkA · 2018-05-07T21:25:28Z

Hi @jrick . I was exploring methods to create mock objects in multisigOut bucket and didn't find any public functions for it. Could you clarify that thing and maybe suggest some approach you would like to see?

kLkA · 2018-05-08T21:31:54Z

@jrick Travis CI error log doesn't seem like related to new code

jrick · 2018-05-09T14:55:24Z

The error is fixed on master now.

As for creating the test db, you can just write keys/values directly to the DB. This is fine for this case since we are intentionally trying to create an invalid DB that will be fixed during the upgrade.

kLkA · 2018-05-09T16:04:57Z

Is it ok If I will leave more complex solution for invalid DB creation? I didn't manage to create valid value of multisig record without using multisig methods

jrick · 2018-05-11T18:27:15Z

The invalid DB for the test can be created in a more simpler way by using keys and values created from the keyMultisigOut and valueMultisigOut functions. Since these are unexported, you can copy them into v9.go. The best test would contain multisig outputs with and without the associated transaction, so we can test that only those outputs without the transaction are removed.

kLkA · 2018-05-11T21:21:39Z

Changed approach to use keyMultisigOut and valueMultisigOut functions. Added multisig outputs with and without associated transaction

wallet/udb/testdata/v9.go

wallet/udb/upgrades_test.go

wallet/udb/testdata/v9.go

jrick · 2018-07-03T16:18:41Z

wallet/udb/upgrades.go

+			Height: int32(mso.BlockHeight),
+		})
+		if txVal == nil {
+			multisigOutsMarkedForDelete = append(multisigOutsMarkedForDelete, k)


to be on the safe side, make a copy of the key. bolt is unfortunately ridden with bugs related to cursor invalidation

@jrick could you check this change in fresh version. Did I make it right?

In this upgrade function we do check all MSOs for related tx existence. If TX doesn't exist we drop MSO

kLkA · 2018-10-11T19:27:26Z

Done

jrick reviewed May 7, 2018

View reviewed changes

jrick requested changes May 17, 2018

View reviewed changes

kLkA force-pushed the force_db_upgrade_to_remove_mso_without_tx_exist branch from eba6b21 to 3e900aa Compare May 24, 2018 15:07

jrick requested changes Jul 3, 2018

View reviewed changes

kLkA force-pushed the force_db_upgrade_to_remove_mso_without_tx_exist branch 3 times, most recently from e57e14c to 4072ab6 Compare September 29, 2018 15:25

kLkA changed the title ~~udb: add upgradeToVersion4~~ udb: add upgradeToVersion12 Sep 29, 2018

kLkA force-pushed the force_db_upgrade_to_remove_mso_without_tx_exist branch 4 times, most recently from 6bde0c1 to 17d79ba Compare September 29, 2018 15:44

udb: add upgradeToVersion12

0ce1ec7

In this upgrade function we do check all MSOs for related tx existence. If TX doesn't exist we drop MSO

kLkA force-pushed the force_db_upgrade_to_remove_mso_without_tx_exist branch from 17d79ba to 0ce1ec7 Compare September 29, 2018 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

udb: add upgradeToVersion12 #1126

udb: add upgradeToVersion12 #1126

kLkA commented May 3, 2018

jrick commented May 4, 2018

jrick commented May 4, 2018

kLkA commented May 4, 2018

jrick May 7, 2018

jrick May 7, 2018

jrick May 7, 2018

jrick May 7, 2018

jrick May 7, 2018

jrick commented May 7, 2018

kLkA commented May 7, 2018

kLkA commented May 8, 2018

jrick commented May 9, 2018

kLkA commented May 9, 2018

jrick commented May 11, 2018

kLkA commented May 11, 2018

jrick Jul 3, 2018

kLkA Sep 29, 2018

kLkA commented Oct 11, 2018

udb: add upgradeToVersion12 #1126

Are you sure you want to change the base?

udb: add upgradeToVersion12 #1126

Conversation

kLkA commented May 3, 2018

jrick commented May 4, 2018

jrick commented May 4, 2018

kLkA commented May 4, 2018

jrick May 7, 2018

Choose a reason for hiding this comment

jrick May 7, 2018

Choose a reason for hiding this comment

jrick May 7, 2018

Choose a reason for hiding this comment

jrick May 7, 2018

Choose a reason for hiding this comment

jrick May 7, 2018

Choose a reason for hiding this comment

jrick commented May 7, 2018

kLkA commented May 7, 2018

kLkA commented May 8, 2018

jrick commented May 9, 2018

kLkA commented May 9, 2018

jrick commented May 11, 2018

kLkA commented May 11, 2018

jrick Jul 3, 2018

Choose a reason for hiding this comment

kLkA Sep 29, 2018

Choose a reason for hiding this comment

kLkA commented Oct 11, 2018