Track stake key registrations independently from delegation #1851

Anviking · 2020-07-03T18:20:00Z

Issue Number

Overview

Keep track of stake key registrations such that we don't try to register a key twice
TODO: model impl
There was a integration test, which I saw passing at some point, but I've not rebased it to this PR
Some DB model-like tests would have been nice, but no time now

Comments

To delegate in shelley we need to know whether we need to register the
stake key, or if it already exists. Trying to register the same key
twice will cause the delegation to fail.

In shelley there are three delegation certs:

reg
dereg
delegate

We need to keep track of two things:

Delegation status
Is the stake key registered or not? (new in this commit)

Also new in this commit:

Stake key de-registration inserts an uptdate to the delegation status
table that says that it is not delegating.

Anviking · 2020-07-03T18:23:19Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+                            (DelegationCertificate wid sl Nothing)
+                        pure <$> repsert
+                            (StakeKeyCertificateKey wid sl)
+                            (StakeKeyCertificate wid sl False)


Here we insert "deregistration" into both tables

lib/core/src/Cardano/Wallet.hs

KtorZ · 2020-07-03T18:33:31Z

Stake key de-registration inserts an uptdate to the delegation status
table that says that it is not delegating.

This seems wrong. Stake key de-registration should be tracked separately as well, and the delegation status should be inferred from the database current status (as it is now).

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

lib/core/src/Cardano/Wallet.hs

KtorZ · 2020-07-03T19:13:00Z

lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs

+StakeKeyCertificate
+    stakeKeyCertWalletId             W.WalletId     sql=wallet_id
+    stakeKeyCertSlot                 W.SlotId       sql=slot
+    stakeKeyCertIsReg                Bool           sql=is_reg


I'd recommend using something more explicit than a boolean here. Boolean convey very little information and they are easy to misinterpret (like I did on my first review...). I'd suggest something like Registration | Deregistration, stored as plain text lowercased in the database, so it's also readable when debugging / inspecting.

KtorZ · 2020-07-03T19:17:38Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+                    [Desc StakeKeyCertSlot]
+
+            return $ case val of
+                Nothing -> Left $ ErrNoSuchWallet wid


Hmmm. No. There could be a wallet but no key. This should return negatively about the existing of a key, but not return an error. To know whether there's a wallet, this should use the same pattern as for other functions:

selectWallet wid >>= \case Nothing -> pure $ Left $ ErrNoSuchWallet wid Just{} -> {- ... do actual stuff ... -}

KtorZ · 2020-07-03T19:18:24Z

lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs

+    stakeKeyCertSlot                 W.SlotId       sql=slot
+    stakeKeyCertIsReg                Bool           sql=is_reg
+
+    Primary stakeKeyCertWalletId stakeKeyCertSlot


KtorZ · 2020-07-03T19:18:47Z

lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs

@@ -676,7 +676,8 @@ fromShelleyDelegationCert = \case
    SL.DCertDeleg (SL.DeRegKey credentials) ->
        Just $ W.CertDelegateNone (fromStakeCredential credentials)

-    SL.DCertDeleg SL.RegKey{} -> Nothing
+    SL.DCertDeleg (SL.RegKey cred) ->
+        Just $ W.CertRegisterKey $ fromStakeCredential cred


KtorZ · 2020-07-03T20:01:14Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+                        [Desc StakeKeyCertSlot]
+                    return $ case val of
+                        Nothing -> Right False
+                        Just (StakeKeyCertificate _ _ isReg) -> Right isReg


To delegate in shelley we need to know whether we need to register the stake key, or if it already exists. Trying to register the same key twice will cause the delegation to fail. In shelley there are three delegation certs: - reg - dereg - delegate We need to keep track of two things: - Delegation status - Is the stake key registered or not? (new in this commit) Also new in this commit: - Stake key de-registration inserts an uptdate to the delegation status table that says that it is not delegating.

Booleans are error-prone and convey very little information without surrounding context. A sum-type is cheap and much easier to read and debug (even in the database!). Co-authored-by: KtorZ <matthias.benkort@iohk.io>

to allow tests to call it

STAKE_POOLS_JOIN_05 This is a regression test for "Cannot join pool with ITN rewards wallet on shelley testnet"

Anviking · 2020-07-03T21:05:54Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs

+                "passphrase": "Secure Passphrase"
+                } |]
+
+        threadDelay 4000000


Should use eventually I guess

KtorZ · 2020-07-03T21:07:01Z

lib/shelley/test/integration/Main.hs

+
+-- | Pre-register a staking key for the STAKE_POOLS_JOIN_05 test.
+preregisterStakingKeysForTests :: NetworkLayer IO t b -> IO ()
+preregisterStakingKeysForTests nl = do


I am not fond of having this function here but I guess it'll do for now. Ideally, I'd have put that where the rest of the "setup & fixture" work is happening (i.e. in the "Launch" module).

KtorZ · 2020-07-03T21:08:11Z

lib/shelley/test/integration/Main.hs

+            , outputs = []
+            , change = []
+            , deposit = 100000 -- keyDeposit
+            }


CoinSelection is a Monoid, so you can write instead:

mempty { inputs = [(txIn, dummyOut)], deposit = 100000 }

Also, note that the key deposit is available in protocol parameters, would be better than hard-coding it.

key deposit is available in protocol parameters

👍

CoinSelection is a Monoid

Didn't think about it, but in this case I found it useful to explicitly see the new fields of CoinSelection.

Anviking · 2020-07-03T21:08:34Z

lib/core/src/Cardano/Wallet/Primitive/Types.hs

+data StakeKeyCertificate
+    = StakeKeyRegistration
+    | StakeKeyDeregistration
+    deriving (Generic, Show, Read, Eq)


There are so many different kinds of certificate types in here already, for multiple purposes. I'd preferred to keep it closer to the DB.

KtorZ · 2020-07-03T21:10:08Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs

+                "passphrase": "Secure Passphrase"
+                } |]
+
+        threadDelay 4000000


Why the delay?

We try to join immediately when the wallet is created. It has no time to find the existing key reg cert.

Should probably use eventually though. Just sprinkled it in to get it passing.

So that we don't get to look at so many places when trying to figure out how the test cluster is setup. It's also using the cardano-cli for consistency with other fixtures (and because we have nice logging and retrying in place for this in the module already).

KtorZ

Reviewed, partially fixed and partially extended.

KtorZ · 2020-07-03T22:01:23Z

bors r+

1851: Track stake key registrations independently from delegation r=KtorZ a=Anviking # Issue Number #1835 # Overview - [x] Keep track of stake key registrations such that we don't try to register a key twice - [ ] TODO: model impl - There was a integration test, which I saw passing at some point, but I've not rebased it to this PR - Some DB model-like tests would have been nice, but no time now # Comments To delegate in shelley we need to know whether we need to register the stake key, or if it already exists. Trying to register the same key twice will cause the delegation to fail. In shelley there are three delegation certs: - reg - dereg - delegate We need to keep track of two things: - Delegation status - Is the stake key registered or not? (new in this commit) Also new in this commit: - Stake key de-registration inserts an uptdate to the delegation status table that says that it is not delegating.  Co-authored-by: Johannes Lund <johannes.lund@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com>

iohk-bors · 2020-07-03T22:22:44Z

Canceled

KtorZ · 2020-07-03T22:23:08Z

bors r+

1851: Track stake key registrations independently from delegation r=KtorZ a=Anviking # Issue Number #1835 # Overview - [x] Keep track of stake key registrations such that we don't try to register a key twice - [ ] TODO: model impl - There was a integration test, which I saw passing at some point, but I've not rebased it to this PR - Some DB model-like tests would have been nice, but no time now # Comments To delegate in shelley we need to know whether we need to register the stake key, or if it already exists. Trying to register the same key twice will cause the delegation to fail. In shelley there are three delegation certs: - reg - dereg - delegate We need to keep track of two things: - Delegation status - Is the stake key registered or not? (new in this commit) Also new in this commit: - Stake key de-registration inserts an uptdate to the delegation status table that says that it is not delegating.  Co-authored-by: Johannes Lund <johannes.lund@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com>

These tests are quite flaky and often failing for no reason. Plus, they are testing things at the integration level since they require a running node and they are redundant with other integration scenario that are ran as part of the integration test suite

iohk-bors · 2020-07-03T22:50:34Z

Canceled

KtorZ · 2020-07-03T22:55:59Z

bors r+

1851: Track stake key registrations independently from delegation r=KtorZ a=Anviking # Issue Number #1835 # Overview - [x] Keep track of stake key registrations such that we don't try to register a key twice - [ ] TODO: model impl - There was a integration test, which I saw passing at some point, but I've not rebased it to this PR - Some DB model-like tests would have been nice, but no time now # Comments To delegate in shelley we need to know whether we need to register the stake key, or if it already exists. Trying to register the same key twice will cause the delegation to fail. In shelley there are three delegation certs: - reg - dereg - delegate We need to keep track of two things: - Delegation status - Is the stake key registered or not? (new in this commit) Also new in this commit: - Stake key de-registration inserts an uptdate to the delegation status table that says that it is not delegating.  Co-authored-by: Johannes Lund <johannes.lund@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com>

iohk-bors · 2020-07-03T23:01:11Z

Canceled

KtorZ · 2020-07-03T23:08:18Z

bors r+

iohk-bors · 2020-07-03T23:59:48Z

Build succeeded

Anviking self-assigned this Jul 3, 2020

KtorZ self-requested a review July 3, 2020 18:21

KtorZ added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Jul 3, 2020

Anviking commented Jul 3, 2020

View reviewed changes

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved

KtorZ reviewed Jul 3, 2020

View reviewed changes

lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved

Anviking added RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG and removed IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG labels Jul 3, 2020

KtorZ reviewed Jul 3, 2020

View reviewed changes

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved

KtorZ reviewed Jul 3, 2020

View reviewed changes

Anviking and others added 3 commits July 3, 2020 22:09

Fixup ErrNoSuchWallet in readIsStakeKeyRegistered

84f5e15

use an explicit sum-type instead of bool for stake key

eecc482

Booleans are error-prone and convey very little information without surrounding context. A sum-type is cheap and much easier to read and debug (even in the database!). Co-authored-by: KtorZ <matthias.benkort@iohk.io>

KtorZ force-pushed the anviking/1835/track-key-certs branch from 1d3aa95 to 68fbe3b Compare July 3, 2020 20:34

implement database abstract model for state-machine testing.

8a33b1e

KtorZ force-pushed the anviking/1835/track-key-certs branch from 68fbe3b to 8a33b1e Compare July 3, 2020 20:41

Anviking added 3 commits July 3, 2020 22:54

Try to generalize mkTx

b4a8619

Extract mkTx further and export it

f8fbf58

to allow tests to call it

Test that wallet with existing stake key can delegate

0047dcc

STAKE_POOLS_JOIN_05 This is a regression test for "Cannot join pool with ITN rewards wallet on shelley testnet"

Anviking mentioned this pull request Jul 3, 2020

Add pending test for joining pool with pre-existing stake key #1842

Closed

3 tasks

Anviking commented Jul 3, 2020

View reviewed changes

KtorZ reviewed Jul 3, 2020

View reviewed changes

Anviking commented Jul 3, 2020

View reviewed changes

KtorZ reviewed Jul 3, 2020

View reviewed changes

KtorZ approved these changes Jul 3, 2020

View reviewed changes

KtorZ force-pushed the anviking/1835/track-key-certs branch from 734604d to fc77374 Compare July 3, 2020 22:50

Regenerate nix

5e173ea

iohk-bors bot merged commit cc9b576 into master Jul 3, 2020

iohk-bors bot deleted the anviking/1835/track-key-certs branch July 3, 2020 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track stake key registrations independently from delegation #1851

Track stake key registrations independently from delegation #1851

Anviking commented Jul 3, 2020 •

edited

Anviking Jul 3, 2020

KtorZ commented Jul 3, 2020

KtorZ Jul 3, 2020

KtorZ Jul 3, 2020

KtorZ Jul 3, 2020

KtorZ Jul 3, 2020

KtorZ Jul 3, 2020

Anviking Jul 3, 2020

KtorZ Jul 3, 2020

KtorZ Jul 3, 2020

Anviking Jul 3, 2020 •

edited

Anviking Jul 3, 2020

KtorZ Jul 3, 2020

Anviking Jul 3, 2020

KtorZ left a comment

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

Track stake key registrations independently from delegation #1851

Track stake key registrations independently from delegation #1851

Conversation

Anviking commented Jul 3, 2020 • edited

Issue Number

Overview

Comments

Choose a reason for hiding this comment

KtorZ commented Jul 3, 2020

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

Anviking Jul 3, 2020 • edited

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

KtorZ left a comment

Choose a reason for hiding this comment

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

Canceled

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

Canceled

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

Canceled

KtorZ commented Jul 3, 2020

iohk-bors bot commented Jul 3, 2020

Build succeeded

Anviking commented Jul 3, 2020 •

edited

Anviking Jul 3, 2020 •

edited