-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Model state for tracking stake keys #2636
Conversation
caa1b00
to
a6a27a1
Compare
describe "Dropped or re-ordered transactions" $ do | ||
it "chain valid to ledger => activeKeys == regs ledger" $ property $ \cmds -> do | ||
let chain = chainFromCmds cmds | ||
let showTxs txs = | ||
"\nAccepted txs:\n" <> unlines (map show txs) <> "\n" | ||
forAllShow (sublistOf chain) showTxs $ \subChain -> do | ||
let s = apply subChain s0 | ||
case (applyLedger subChain initialLedger) of | ||
Right (Ledger regs) -> counterexample | ||
"valid chain => expecting the wallet's activeKeys \ | ||
\to match all registered keys in the ledger" $ | ||
Set.fromList (fmap toRewardAccount (activeKeys s)) | ||
=== regs | ||
Left _ -> label "invalid chain" True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can now show that rollbacks are a problem!
1) Mock ledger, Dropped or re-ordered transactions, chain valid to ledger => activeKeys == regs ledger
Falsified (after 13 tests and 6 shrinks):
[CmdSetPortfolioOf 1,CmdSetPortfolioOf 2]
Accepted txs:
Tx [RegisterKey (RewardAccount "1"),Delegate (RewardAccount "1")]
valid chain => expecting the wallet's activeKeys to match all registered keys in the ledger
fromList [] /= fromList [RewardAccount "1"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sublistOf
cannot re-order elements. Not sure if this matters. Maybe 🤔 Should either fix or add a comment at least.
90cf0eb
to
f807128
Compare
-- NOTE: For convenience we treat ix=-1 as no keys, as ix=0 means the first | ||
-- key (at index 0) is registered and delegating. | ||
go i chain tx (cmd:rest) = case cmd of | ||
CmdSetPortfolioOf n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that this will never generate delegation certificates on their own, which should be pretty important to test.
f118402
to
fe40dc1
Compare
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/DelegationSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
-- it for the pointer. | ||
-- | ||
-- TODO: Is this actually bad though? Or is it good because it will | ||
-- then be sent back as change? Any other problems? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also something I stumble upon when updating the proposal which I dismissed by requiring that the pointer can only be a UTxO that is part of a transaction which also contains delegation certificate or key de-registration certificates. There may be other UTxO at the reward address, but they should not count as pointer (and the wallet should be free to ignore them or redeem them, whatever is the simplest).
It however forces the wallet to "follow the pointer" in a way, and simply querying the LSQ isn't sufficient to figure out whether a pointer exist. I think that following the pointer was however already necessary in order to spend it right (we need to have the exact TxIn for that) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by requiring that the pointer can only be a UTxO that is part of a transaction which also contains delegation certificate or key de-registration certificates
Now done.
It however forces the wallet to "follow the pointer" in a way, and simply querying the LSQ isn't sufficient to figure out whether a pointer exist. I think that following the pointer was however already necessary in order to spend it right (we need to have the exact TxIn for that) ?
I haven't put much thought about using LSQ here, but I don't see why it would help. Now we do need to store a TxIn
, but it's not that complicated.
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
applyTx (Tx cs _ins outs) h s0 = | ||
let | ||
s = foldl (flip applyCert) s0 cs | ||
isOurOut i (TxOut addr _b) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To identify if a transaction is relevant to the DelegationState
, I think it would suffice to check for delegation or key de-registration certificate containing either the next key index, or the last active key index. We don't really need to worry about output. The CIP explicitly requires that the first output of a such a transaction is the pointer output;
Thus, I would do it the other way around (also because it may be much faster that way): lookup for certificates relevant to us, figure out from the certificate what is the last active key index and then, have a guard / invariant checking that the first output does indeed match.
In case it doesn't, that is an invariant violation and the wallet behavior is undefined. Crashing seems excessive although., beyond this point we really do not know what the delegation state of the wallet is 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I changed it to assume the pointer only changes based on the delegation certificates.
The CIP explicitly requires that the first output of a such a transaction is the pointer output
Wouldn't that be an unnecessary loss of flexibility? 🤔 (Not that I know what we'd use it for, maybe changing portfolio of multiple accounts at the same time 🤷♂️ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that be an unnecessary loss of flexibility? thinking (Not that I know what we'd use it for, maybe changing portfolio of multiple accounts at the same time man_shrugging )
More of a convention (perhaps a useless one?). In the end, since the wallet tracks the UTxO it's quite easy to locate it in the input set.
In response to #2636 (comment) It makes things neater, and should work as well.
In response to #2636 (comment) It makes things neater, and should work as well.
6e9c4f8
to
5bd7057
Compare
-> Maybe (Index 'Soft 'AddressK) | ||
pointerIx 0 = Nothing | ||
pointerIx 1 = Nothing | ||
pointerIx n = Just $ toEnum n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have
[0,1], 1 ada in held by key 2
because I previously allowed
[], 1 ada in held by key 0
Now the latter state is not possible. So we could have the pointer correspond to to the lastActive
rather than the nextUnused
ix. I.e.
[0,1], 1 ada in held by key 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little bit confused by this function at the moment, maybe it will get clearer in the rest of the review. However, I would expect the pointer to be created during the transition [0] -> [0,1] (as you state in comment) and that, pointerIx n = Just n
; It feels more intuitive to me for the pointer to track the highest active key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels more intuitive to me for the pointer to track the highest active key
Yeah, I was starting to feel the same. Might try that. Here it would be pointerIx n = Just $ pred $ toEnum n
though, as n
is the number of stake keys you want.
That said, there should be some small fee-tradeoffs from this choice (if I understand correctly)
With the pointer tracking nextKeyIx
:
- Adding stake keys: we need to spend the pointer at
nextKeyIx
, and delegate withnextKeyIx
, so we save a witness. - Removing stake keys: no savings
With the pointer tracking lastActiveIx
:
- Adding stake keys: no savings
- Removing stake keys: we need to spend the pointer at
lastActiveIx
, and deregister withlastActiveIx
, so we save a witness.
But I guess the difference is so small it's not even worth thinking about with 44 lovelace/byte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, a argument in the other direction I didn't think of until recently:
Assuming
- re-delegation of existing stake-keys requires spending and re-creating the pointer
- the pointer uses
lastActiveIx
Then we save a witness when changing the delegation at lastActiveIx
.
PointerUTxO | ||
-- ^ pointer utxo that need to be spent when changing state. | ||
!Bool | ||
-- ^ Is key 0 still registered? For compatibility with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little newtype or sum-type would be better 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with
-- | Is key 0 still registered? For compatibility with
-- single-stake-key wallets, we need to track this.
--
-- >>> activeKeys (More (toEnum 3) p ValidKey0)
-- [0, 1, 2]
--
-- >>> activeKeys (More (toEnum 3) p MissingKey0)
-- [1, 2]
--
-- (pseudocode; requires a bit more boilerplate to compile)
--
-- See the implementaiton of @applyTx@ for how it is used.
data Key0Status = ValidKey0 | MissingKey0
deriving (Eq, Show, Generic)
-> Maybe (Index 'Soft 'AddressK) | ||
pointerIx 0 = Nothing | ||
pointerIx 1 = Nothing | ||
pointerIx n = Just $ toEnum n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little bit confused by this function at the moment, maybe it will get clearer in the rest of the review. However, I would expect the pointer to be created during the transition [0] -> [0,1] (as you state in comment) and that, pointerIx n = Just n
; It feels more intuitive to me for the pointer to track the highest active key.
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
|
||
changeStateTx :: Maybe Tx | ||
changeStateTx = txWithCerts $ case compare (toEnum n) (nextKeyIx s) of | ||
GT -> deleg [nextKeyIx s .. toEnum (n - 1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me some time to convince myself that the next index was inded toEnum (n - 1)
partly because it feels to me that the logic of nextKeyIx
is sort of "leaking out". That's the kind of moment where I think functions like scanl / iterate
fit nicely to avoid silly mistake, imagine a function nextState :: DelegationState -> DelegationState
, such that it can then be written as:
take n $ iterate nextState s
This reads much better to me as in "give me the next n
states"; and the same approach can be done when going in the other direction using a predState
analogous function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding
nextState :: (Index 'Soft 'AddressK -> pointer) -> State pointer -> State pointer
prevState :: (Index 'Soft 'AddressK -> pointer) -> State pointer -> State pointer
to be able to advance the State
with a TxOut
as a pointer, rather than a TxIn
, and also re-use them in applyTx
.
But there's also the problem of:
- Do we call
nextState
orprevState
, and when do we stop? - How to yield certificates, which might need to be modified based on
isReg
.
It seemed like a iterate
-approach would end up very complicated, potentially with many foldl
, scanl
and take
steps. And boundary-cases of takeWhile
seems just as complicated as .. toEnum (n - 1)
.
So without an "aha" idea… I think being somewhat imperative here is fine… The tests should cover a lot of things, and give us confidence.
where | ||
mkTxIn (PointerUTxO i c) = (i, c) | ||
-- Note: If c > minUTxOVal we need to rely on the wallet to return the | ||
-- difference to the user as change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be interesting to incorporate in the coin selection and is similar to some of the needs of the Plutus application backend. The wallet will need to be able to "continue" the construction of a transaction from a pre-filled one.
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
repairKey0IfNeededTx <> changeStateTx | ||
where | ||
repairKey0IfNeededTx :: Maybe Tx | ||
repairKey0IfNeededTx = case repairKey0 $ state s of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment to clarify what this does would be nice. If I understand correctly, this re-register the initial key in case where it has been de-registered by some other mean? Although I do think the code is correct, I am also wondering why this is necessary ? I am generally quite against doing anything on the behalf of users without explicit consent. Imagine the scenario where a user has restored the same wallet in Daedalus and Yoroi. She then de-registers her first stake key in Yoroi which allows it because it doesn't implement this proposal yet. Then, some weeks later while she re-delegate to new pools, the first stake key gets re-registered without her asking anything.
I don't think the first key should actually be handled so specially. Rather, the multi-stake key state should really only care about keys beyond the first one. If the first key has been de-registered, then so be it 🤷 ... Checking this before trying to de-register it (as you do below) should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, some weeks later while she re-delegate to new pools, the first stake key gets re-registered without her asking anything.
setPortfolioOf n
currently doesn't care about the pool-choice of each key. It couldn't just silently delegate key 0 without knowing what to delegate to (hadn't completely realised this though, so good that you brought it up). A more concrete scenario:
A user delegates to pools A, B, C with stake keys 0, 1 and 2 respectively.
Yoroi shows her wallet is delegating to A. She undelegates.
According to Future Daedalus she is… in some broken state?… delegating to B, C.
I imagine
setPortfolio [A,B,C]
would repair the state by re-registering and delegating key 0setPortfolio [B,C]
would shift{1 -> B, 2 -> C}
to{ 0 -> C, 1 -> B }
, by de-registering key 2, and re-registering key 0- De-registering keys is a problem we need to solve in general. If she just recently de-registered key 0, the rewards she loses from key 2 might be offset by re-covering the rewards in key 0 though.
To me it seems much easier if we always use key 0 in addresses, some percent of the time, and treat a missing key 0, as a invalid, temporary state.
When the user un-delegated using the old wallet, the change addresses used key 0. Same for all txs using the old wallet.
If both wallets can agree that key 0 should be used for some addresses, there's less rebalancing/fighting to be made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(added a comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's some very nice testing. I like the approach and it gives a lot of confidence about what's being done.
Some general remarks: the code is sometimes hard to follow. Many short identifiers where they could be a bit more explicit, and some functions / local bindings would benefits from type signatures to guide the reader. It'd be very nice to iterate a bit on the code itself to try to make it crystal clear since the problem that it solves is already rather intricate.
(presentableKeys s3) `shouldBe` | ||
[toEnum 0, toEnum 1, toEnum 2] | ||
it "usableKeys == [0, 1]" $ do | ||
usableKeys s3 `shouldBe` [toEnum 0, toEnum 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/DelegationSpec.hs
Outdated
Show resolved
Hide resolved
-- Properties | ||
-- | ||
|
||
prop_keysConsequtive :: (DelegationState StakeKey' -> [StakeKey]) -> [Cmd] -> Property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Tx cs [] [_o] -> Delegate (acct 1) `elem` cs | ||
Tx cs [_i] [] -> DeRegisterKey (acct 1) `elem` cs | ||
Tx cs [_i] [_o] -> any ((> 1) . ixFromCert) cs | ||
Tx cs [] [] -> all ((< 1) . ixFromCert) $ filter notReg cs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
counterexample "\nstate /= state without adversarial cmds" $ do | ||
let usableKeys' = usableKeys . wallet . applyCmds env0 | ||
usableKeys' cmds | ||
=== usableKeys' (filter (not . isAdversarial) cmds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
prop_canAlwaysGoTo0 cmds = do | ||
let env = applyCmds env0 (cmds ++ [CmdSetPortfolioOf 0]) | ||
counterexample (pretty env) $ | ||
activeKeys (wallet env) === activeKeys (wallet env0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
counterexample (pretty env) $ | ||
length (activeKeys $ wallet env) === n | ||
.&&. allActiveKeysRegistered env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
-- shouldn't be affected negatively from it. | ||
| CmdMimicPointerOutput RewardAccount | ||
-- ^ Someone could send funds to the same UTxO | ||
deriving (Generic, Eq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👌
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/DelegationSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/DelegationSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Delegation.hs
Outdated
Show resolved
Hide resolved
ad8eaed
to
ce5a371
Compare
I created https://jira.iohk.io/browse/ADP-938 for a future opportunity to revise the design and align with the CIP if needed. Before merging, will
|
ce5a371
to
8d5b36a
Compare
bors r+ |
6735d55
to
2b449d4
Compare
Canceled. |
a398277
to
c2762a1
Compare
bors r+ |
2636: Model state for tracking stake keys r=Anviking a=Anviking # Issue Number ADP-724 # Overview - [x] Add concepts of - `usableKeys` - keys to be used as part of addresses - `activeKeys` - keys that are registered and delegating - `presentableKeys` - keys that can be displayed to the user (superset of the other two) - [x] Some simple golden tests for understanding - [x] Property tests using mock user-actions and chain data - [x] Add pointer UTxO - [x] Add notion of mock ledger to test resistance to rollbacks - [x] Implement the pointer UTxO - [x] Ensure it works - [x] Look to test interactions with old wallets which don't know about the pointer, nor other stake keys - [x] There's some failure at least - [x] Improve counterexample - [x] Fix - [ ] Look to test concurrent wallet-actions (might be slightly different from txs that could be dropped or re-ordered) - Think we're ok without it - [ ] We need a lookup-map for delegation certificates - Shouldn't affect the testing, so not crucial. # Comments ``` Goldens initialDelegationState presentableKeys is [0] usableKeys is [0] registering and delegating key 0 presentableKeys == [0, 1] usableKeys is still [0] registering and delegating key 1 presentableKeys == [0, 1, 2] usableKeys == [0, 1] Impossible gaps in stake keys (shouldn't happen unless someone manually constructs txs to mess with the on-chain state) presentableKeys == [0, 1, 2] (doesn't find 5) usableKeys == [0, 1] (usableKeys s) are consequtive +++ OK, passed 100 tests. (presentableKeys s) are consequtive +++ OK, passed 100 tests. (activeKeys s) are consequtive +++ OK, passed 100 tests. pointer is only created and destroyed in specific cases +++ OK, passed 100 tests. adversaries can't affect usableKeys +++ OK, passed 100 tests. adversaries can't affect pointer ix +++ OK, passed 100 tests. (apply (cmds <> CmdSetPortfolioOf 0) s0) === s0 +++ OK, passed 100 tests. no rejected txs, normally +++ OK, passed 100 tests. can recover from dropped transactions +++ OK, passed 2000 tests. ``` <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: IOHK <[email protected]>
Build failed:
|
bors r+ |
Build succeeded: |
Issue Number
ADP-724
Overview
usableKeys
- keys to be used as part of addressesactiveKeys
- keys that are registered and delegatingpresentableKeys
- keys that can be displayed to the user (superset of the other two)Comments