V2 migration #655

emmanueldenloye · 2021-06-03T19:44:05Z

No description provided.

If we find old noderefs (e.g. us1.testnet.chainweb.com), we remove them and put the correct uri (api.testnet.chainweb.com)

jmininger · 2021-06-04T19:52:48Z

You should make sure that the tests in frontend/tests/Frontend/VersionedStore* are changed over from V1 to V2 (or that V2 is added?)

jmininger · 2021-06-04T20:08:53Z

frontend/src/Frontend/VersionedStore/V2.hs

+    newNetworks = (\nets -> StoreFrontend_Network_Networks :=> Identity (convertNodeRefs $ V0.unNetworkMap $ runIdentity nets))
+      <$> DMap.lookup V0.StoreNetwork_Networks v0
+
+    -- This will regenerate the missing key. Desktop will recover the key with


This is something we really need to look out for with the upcoming chainweaver changes. Might be ok in the immediate future, but we can't copy the same code over for a V2 --> V3 if this ever happens in the future, because then we would risk wiping out users' keys

jmininger · 2021-06-04T20:25:55Z

frontend/src/Frontend/VersionedStore/V2.hs

+
+deriving instance Show (StoreFrontend key a)
+
+upgradeFromV0 :: (Monad m, HasCrypto key m) => DMap (V0.StoreFrontend key) Identity -> m (DMap (StoreFrontend key) Identity)


Might it make more sense to just compose V1.upgradeFromV0 and V2.upgradeFromV1 so that we aren't stuck copying code over for each migration and having to make sure that we have all of the "previous stuff" from the intermediary migration as well as all of the new migration stuff?

Hmmmm, interesting point. I think we probably should compose the migrations otherwise the logical conclusion just won't be scalable. That's typically the way I've done it with DB migrations.

jmininger · 2021-06-04T20:41:11Z

frontend/src/Frontend/VersionedStore.hs

@@ -161,3 +168,4 @@ versionedStorage = VersionedStorage
          liftIO $ Api._transactionLogger_rotateLogFile txLogger
          pure ()
        Just (StoreFrontendVersion_1 :=> _) -> pure ()
+        Just (StoreFrontendVersion_2 :=> _) -> pure ()


Reminder to fix this

just replace the entries for the network names "Testnet" and "Mainnet"

also fixed upgrade procedure

these new test cases will investigate how chainweaver recovers from a migration that fails at some point before completion.

mightybyte · 2021-06-09T21:53:14Z

frontend/src/Frontend/Storage/Class.hs

@@ -169,6 +169,7 @@ restoreLocalStorage
  :: forall storeKeys m
  . ( HasStorage m
    , Monad m
+    , MonadIO m


What was the reason for these new MonadIO constraints? Is it left over from debugging?

Yeah, I'm pretty sure that's the reason. I'll take them out.

mightybyte · 2021-06-09T22:13:48Z

frontend/src/Frontend/VersionedStore/V2.hs

+
+deriving instance Show (StoreFrontend key a)
+
+upgradeFromV0 :: (Monad m, HasCrypto key m) => DMap (V0.StoreFrontend key) Identity -> m (DMap (StoreFrontend key) Identity)


Hmmmm, interesting point. I think we probably should compose the migrations otherwise the logical conclusion just won't be scalable. That's typically the way I've done it with DB migrations.

mightybyte · 2021-06-09T22:17:36Z

frontend/tests/Frontend/VersionedStoreSpec.hs

@@ -292,7 +374,169 @@ test_v0ToV1Upgrade = testCaseSteps "V0 to V1 Upgrade" $ \step -> do
  where
    path = "tests" </> "Frontend" </> "VersionedStoreSpec.files" </> "V0"

+fail_test_v0ToV2Upgrade :: FailStorageState -> TestTree


If we use compositional migrations as mentioned above, we might be able to get this test to become more like a property, i.e.

migrate a (a+2) == (migrate a (a+1) >> migrate (a+1) (a+2))

Not sure if that will be too invasive of a change though.

I actually really like this idea. Also adding new tests for new versions will be a lot easier to make.

mightybyte · 2021-06-09T23:01:50Z

frontend/src/Frontend/VersionedStore/V2.hs

+convertNodeRefs :: Map NetworkName [NodeRef] -> Map NetworkName [NodeRef]
+convertNodeRefs = Map.mapWithKey migrate
+  where
+    migrate = \case


Hmmmm...I'm contemplating the possibility of doing the replacement no matter what the network name is. I think I've seen instances where it's lowercase "mainnet" rather than "Mainnet". We would have to keep track of which of the node refs were actually found to know which new ref to add. But that would work for these other cases.

jmininger · 2021-06-10T15:27:24Z

frontend/src/Frontend/VersionedStore/V2.hs

+
+upgradeFromV0 :: (Monad m, HasCrypto key m) => DMap (V0.StoreFrontend key) Identity -> m (DMap (StoreFrontend key) Identity)
+upgradeFromV0 v0 = do
+  (newKeysList, newAccountStorage) <- foldMapM splitOldKey oldKeysList


splitOldKey really concerns me, for the web version. Its relying on the keys' index in a list, and then using that index to "regenerate" them instead of saving /copying them somewhere. This code is so easy to accidentally copy over, for, say the next migration. We should either rewrite it or at the very least use CPP to make it a no-op on ghcjs. Not quite sure its in the scope of this task, but it could so easily go really bad really quickly if it isn't taken care of imo

In essence, we don't care what networks the user has either created, modified or left alone, we filter out undesired noderefs in that network name.

It is equivalent to "These" except that we take out the "This" constructor. With it gone, we still get the same applicative operations from "These" (sans "This") but we don't have to account for the "This" constructor in situations where we know it is impossible to produce it.

jmininger · 2021-06-11T12:07:20Z

frontend/src/Frontend/VersionedStore/V2.hs

@@ -152,16 +155,19 @@ fromMultiSet = ($ []) . Map.foldrWithKey (\k i -> (.) (dlrep k i)) id
      | otherwise = (v:) . dlrep v (n - 1)

 convertNodeRefs :: Map NetworkName [NodeRef] -> Map NetworkName [NodeRef]
-convertNodeRefs = Map.mapWithKey migrate
+convertNodeRefs = fmap migrate
  where


Is the nested where necessary ?

The datatype we really want is something equivalent to Either () a but with a different Applicative instance.

emilypi · 2021-06-11T15:45:48Z

frontend/src/Frontend/VersionedStore/V2.hs

+-- the "This" constructor in situations where we know it is impossible to
+-- produce it. This is also morally equivalent to Either () b with a
+-- different applicative instance
+data Deez b = Dis b | Dat b


slightly misleading comment.

These a b ~ a + b + a*b

But

These () a ~ 1 + a + 1*a ~ 1 + a + a ~ Maybe (Either a a)

and

Either () b ~ 1 + b ~ Maybe b

You want a newtype for Either a a here to get the modified Applicative.

make "Deez" a newtype

emmanueldenloye added 2 commits June 3, 2021 15:43

init commit

40ff87e

1st cut of migration

72e76f3

If we find old noderefs (e.g. us1.testnet.chainweb.com), we remove them and put the correct uri (api.testnet.chainweb.com)

mightybyte requested review from mightybyte and jmininger June 4, 2021 17:04

jmininger reviewed Jun 4, 2021

View reviewed changes

emmanueldenloye added 8 commits June 5, 2021 13:00

compose migrations

b837e7b

bump version on restoreBackup

ac59840

don't care about power users

1d53d87

just replace the entries for the network names "Testnet" and "Mainnet"

add V1->V2 test, fix V0->V2 test

eaa6f84

also fixed upgrade procedure

actually run both upgrade tests

10051b8

mocking new test cases

40ced78

these new test cases will investigate how chainweaver recovers from a migration that fails at some point before completion.

add OverloadedStrings pragma

1b59cc9

finished previously mocked tests (last commit)

17588f2

mightybyte reviewed Jun 9, 2021

View reviewed changes

jmininger reviewed Jun 10, 2021

View reviewed changes

emmanueldenloye added 3 commits June 10, 2021 14:50

take out unnecessary MonadIO constraints

1594e25

unconditionally alter refs for networks in local storage

a5c3e57

In essence, we don't care what networks the user has either created, modified or left alone, we filter out undesired noderefs in that network name.

jmininger reviewed Jun 11, 2021

View reviewed changes

Use an even simpler datatype for processing the map merge

5167b87

The datatype we really want is something equivalent to Either () a but with a different Applicative instance.

emilypi reviewed Jun 11, 2021

View reviewed changes

emmanueldenloye added 2 commits June 11, 2021 13:04

another simple refactor

e5d7d24

make "Deez" a newtype

for JM

84eaa4b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V2 migration #655

V2 migration #655

emmanueldenloye commented Jun 3, 2021

jmininger commented Jun 4, 2021

jmininger Jun 4, 2021 •

edited

Loading

jmininger Jun 4, 2021

mightybyte Jun 9, 2021

jmininger Jun 4, 2021

mightybyte Jun 9, 2021

emmanueldenloye Jun 9, 2021

mightybyte Jun 9, 2021

mightybyte Jun 9, 2021

emmanueldenloye Jun 9, 2021

mightybyte Jun 9, 2021

jmininger Jun 10, 2021

jmininger Jun 11, 2021

emilypi Jun 11, 2021 •

edited

Loading


		deriving instance Show (StoreFrontend key a)

		upgradeFromV0 :: (Monad m, HasCrypto key m) => DMap (V0.StoreFrontend key) Identity -> m (DMap (StoreFrontend key) Identity)

V2 migration #655

Are you sure you want to change the base?

V2 migration #655

Conversation

emmanueldenloye commented Jun 3, 2021

jmininger commented Jun 4, 2021

jmininger Jun 4, 2021 • edited Loading

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

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

emilypi Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

jmininger Jun 4, 2021 •

edited

Loading

emilypi Jun 11, 2021 •

edited

Loading