From 76a1d893caf88e84fe6982b92deae7ca38f1ece9 Mon Sep 17 00:00:00 2001 From: Franco Testagrossa Date: Tue, 18 Jul 2023 10:18:56 +0200 Subject: [PATCH 1/9] Refactor tests to use update instead of emitSnapshot --- hydra-node/hydra-node.cabal | 2 +- hydra-node/src/Hydra/HeadLogic.hs | 12 +- .../test/Hydra/HeadLogicSnapshotSpec.hs | 244 ++++++++++++++++++ hydra-node/test/Hydra/HeadLogicSpec.hs | 12 +- hydra-node/test/Hydra/SnapshotStrategySpec.hs | 137 ---------- 5 files changed, 260 insertions(+), 147 deletions(-) create mode 100644 hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs delete mode 100644 hydra-node/test/Hydra/SnapshotStrategySpec.hs diff --git a/hydra-node/hydra-node.cabal b/hydra-node/hydra-node.cabal index e04b2290409..e559a6b6eea 100644 --- a/hydra-node/hydra-node.cabal +++ b/hydra-node/hydra-node.cabal @@ -299,7 +299,7 @@ test-suite tests Hydra.OptionsSpec Hydra.PartySpec Hydra.PersistenceSpec - Hydra.SnapshotStrategySpec + Hydra.HeadLogicSnapshotSpec Paths_hydra_node Spec Test.Hydra.Fixture diff --git a/hydra-node/src/Hydra/HeadLogic.hs b/hydra-node/src/Hydra/HeadLogic.hs index ce7338b05bc..17bbbc2708f 100644 --- a/hydra-node/src/Hydra/HeadLogic.hs +++ b/hydra-node/src/Hydra/HeadLogic.hs @@ -355,7 +355,7 @@ data RequirementFailure tx | InvalidMultisignature {multisig :: Text, vkeys :: [VerificationKey HydraKey]} | SnapshotAlreadySigned {knownSignatures :: [Party], receivedSignature :: Party} | AckSnNumberInvalid {requestedSn :: SnapshotNumber, lastSeenSn :: SnapshotNumber} - | SnapshotDoesNotApply {requestedSn :: SnapshotNumber, txid :: TxIdType tx, error :: ValidationError } + | SnapshotDoesNotApply {requestedSn :: SnapshotNumber, txid :: TxIdType tx, error :: ValidationError} deriving stock (Generic) deriving instance (Eq (TxIdType tx)) => Eq (RequirementFailure tx) @@ -427,6 +427,16 @@ collectWaits = \case Effects _ -> [] Combined l r -> collectWaits l <> collectWaits r +-- FIXME should return Maybe? +collectState :: Outcome tx -> [HeadState tx] +collectState = \case + NoOutcome -> [] + Error _ -> [] + Wait _ -> [] + NewState s -> [s] + Effects _ -> [] + Combined l r -> collectState l <> collectState r + -- * The Coordinated Head protocol -- ** Opening the Head diff --git a/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs b/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs new file mode 100644 index 00000000000..874274a4bb7 --- /dev/null +++ b/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs @@ -0,0 +1,244 @@ +{-# LANGUAGE DuplicateRecordFields #-} +{-# LANGUAGE LambdaCase #-} +{-# OPTIONS_GHC -Wno-unused-do-bind #-} + +module Hydra.HeadLogicSnapshotSpec where + +import Hydra.Prelude hiding (label) +import Test.Hydra.Prelude + +import qualified Data.List as List +import qualified Data.Map.Strict as Map +import Hydra.Chain (HeadParameters (..)) +import Hydra.Crypto (sign) +import Hydra.HeadLogic ( + CoordinatedHeadState (..), + Effect (..), + Environment (..), + Event (NetworkEvent), + HeadState (..), + OpenState (OpenState), + SeenSnapshot (..), + collectEffects, + collectState, + coordinatedHeadState, + defaultTTL, + isLeader, + update, + ) +import Hydra.HeadLogicSpec (inOpenState, inOpenState', runEvents, step) +import Hydra.Ledger (Ledger (..), txId) +import Hydra.Ledger.Simple (SimpleTx (..), aValidTx, simpleLedger, utxoRef) +import Hydra.Network.Message (Message (..)) +import Hydra.Options (defaultContestationPeriod) +import Hydra.Party (deriveParty) +import Hydra.Snapshot (ConfirmedSnapshot (..), Snapshot (..), getSnapshot) +import Test.Hydra.Fixture (alice, aliceSk, bob, bobSk, carol, carolSk) +import Test.QuickCheck (Property, counterexample, forAll, oneof, (==>)) +import Test.QuickCheck.Monadic (monadicST, pick) + +spec :: Spec +spec = do + parallel $ do + let threeParties = [alice, bob, carol] + Ledger{initUTxO} = simpleLedger + envFor signingKey = + let party = deriveParty signingKey + in Environment + { party + , signingKey + , otherParties = List.delete party threeParties + , contestationPeriod = defaultContestationPeriod + } + + let coordinatedHeadState = + CoordinatedHeadState + { seenUTxO = initUTxO + , allTxs = mempty + , seenTxs = mempty + , confirmedSnapshot = InitialSnapshot initUTxO + , seenSnapshot = NoSeenSnapshot + } + let sendReqSn = + isJust + . find + ( \case + NetworkEffect ReqSn{} -> True + _ -> False + ) + let snapshot1 = Snapshot 1 mempty [] + + let ackFrom sk vk = NetworkEvent defaultTTL vk $ AckSn (sign sk snapshot1) 1 + + describe "Generic Snapshot property" $ do + prop "there's always a leader for every snapshot number" prop_thereIsAlwaysALeader + + describe "On ReqTx" $ do + prop "always emit ReqSn given head has 1 member" prop_singleMemberHeadAlwaysSnapshotOnReqTx + + it "sends ReqSn when leader and no snapshot in flight" $ do + let tx = aValidTx 1 + outcome = update (envFor aliceSk) simpleLedger (inOpenState' [alice, bob] coordinatedHeadState) $ NetworkEvent defaultTTL alice $ ReqTx tx + + collectEffects outcome + `shouldContain` [NetworkEffect (ReqSn 1 [txId tx])] + + it "Do NOT send ReqSn when we are NOT the leader even if no snapshot in flight" $ do + let tx = aValidTx 1 + st = coordinatedHeadState{seenTxs = [tx]} + outcome = update (envFor bobSk) simpleLedger (inOpenState' [alice, bob] st) $ NetworkEvent defaultTTL bob $ ReqTx tx + + collectEffects outcome `shouldNotSatisfy` sendReqSn + + it "Do NOT send ReqSn when we are the leader but snapshot in flight" $ do + let tx = aValidTx 1 + sn1 = Snapshot 1 initUTxO mempty :: Snapshot SimpleTx + st = coordinatedHeadState{seenSnapshot = SeenSnapshot sn1 mempty} + outcome = update (envFor aliceSk) simpleLedger (inOpenState' [alice, bob] st) $ NetworkEvent defaultTTL alice $ ReqTx tx + + collectEffects outcome `shouldNotSatisfy` sendReqSn + + it "update seenSnapshot state when sending ReqSn" $ do + let tx = aValidTx 1 + st = inOpenState' threeParties coordinatedHeadState + outcome = update (envFor aliceSk) simpleLedger st $ NetworkEvent defaultTTL alice $ ReqTx tx + + let st' = + inOpenState' threeParties $ + coordinatedHeadState + { seenTxs = [tx] + , allTxs = Map.singleton (txId tx) tx + , seenUTxO = initUTxO <> utxoRef (txId tx) + , seenSnapshot = RequestedSnapshot{lastSeen = 0, requested = 1} + } + + collectState outcome `shouldContain` [st'] + + describe "On AckSn" $ do + it "sends ReqSn when leader and there are seen transactions" $ do + let + tx = aValidTx 1 + bobEnv = + Environment + { party = bob + , signingKey = bobSk + , otherParties = [alice, carol] + , contestationPeriod = defaultContestationPeriod + } + + headState <- runEvents bobEnv simpleLedger (inOpenState threeParties simpleLedger) $ do + step (NetworkEvent defaultTTL alice $ ReqSn 1 []) + step (NetworkEvent defaultTTL carol $ ReqTx tx) + step (ackFrom carolSk carol) + step (ackFrom aliceSk alice) + + let outcome = update bobEnv simpleLedger headState $ ackFrom bobSk bob + collectEffects outcome `shouldSatisfy` sendReqSn + + it "do NOT send ReqSn when we are the leader but there are NO seen transactions" $ do + let + bobEnv = + Environment + { party = bob + , signingKey = bobSk + , otherParties = [alice, carol] + , contestationPeriod = defaultContestationPeriod + } + + headState <- runEvents bobEnv simpleLedger (inOpenState threeParties simpleLedger) $ do + step (NetworkEvent defaultTTL alice $ ReqSn 1 []) + step (ackFrom carolSk carol) + step (ackFrom aliceSk alice) + + let outcome = update bobEnv simpleLedger headState $ ackFrom bobSk bob + collectEffects outcome `shouldNotSatisfy` sendReqSn + + it "Do NOT send ReqSn when we are NOT the leader but there are seen transactions" $ do + let + tx = aValidTx 1 + notLeaderEnv = + Environment + { party = carol + , signingKey = carolSk + , otherParties = [alice, bob] + , contestationPeriod = defaultContestationPeriod + } + + let initiateSigningASnapshot actor = + step (NetworkEvent defaultTTL actor $ ReqSn 1 []) + newTxBeforeSnapshotAcknowledged = + step (NetworkEvent defaultTTL carol $ ReqTx tx) + + headState <- runEvents notLeaderEnv simpleLedger (inOpenState threeParties simpleLedger) $ do + initiateSigningASnapshot alice + step (ackFrom carolSk carol) + newTxBeforeSnapshotAcknowledged + step (ackFrom aliceSk alice) + let everybodyAcknowleged = update notLeaderEnv simpleLedger headState $ ackFrom bobSk bob + collectEffects everybodyAcknowleged `shouldNotSatisfy` sendReqSn + + it "update seenSnapshot state when sending ReqSn" $ do + let + tx = aValidTx 1 + bobEnv = + Environment + { party = bob + , signingKey = bobSk + , otherParties = [alice, carol] + , contestationPeriod = defaultContestationPeriod + } + + headState <- runEvents bobEnv simpleLedger (inOpenState threeParties simpleLedger) $ do + step (NetworkEvent defaultTTL alice $ ReqSn 1 []) + step (NetworkEvent defaultTTL carol $ ReqTx tx) + step (ackFrom carolSk carol) + step (ackFrom aliceSk alice) + + let outcome = update bobEnv simpleLedger headState $ ackFrom bobSk bob + + case collectState outcome of + [Open OpenState{coordinatedHeadState = CoordinatedHeadState{seenSnapshot = actualSnapshot}}] -> + actualSnapshot `shouldBe` RequestedSnapshot{lastSeen = 1, requested = 2} + other -> expectationFailure $ "Expected to be in open state: " <> show other + +prop_singleMemberHeadAlwaysSnapshotOnReqTx :: ConfirmedSnapshot SimpleTx -> Property +prop_singleMemberHeadAlwaysSnapshotOnReqTx sn = monadicST $ do + seenSnapshot <- + pick $ + oneof + [ pure NoSeenSnapshot + , LastSeenSnapshot <$> arbitrary + ] + tx <- pick $ aValidTx <$> arbitrary + let + aliceEnv = + let party = alice + in Environment + { party + , signingKey = aliceSk + , otherParties = [] + , contestationPeriod = defaultContestationPeriod + } + st = + CoordinatedHeadState + { seenUTxO = mempty + , allTxs = mempty + , seenTxs = [] + , confirmedSnapshot = sn + , seenSnapshot + } + outcome = update aliceEnv simpleLedger (inOpenState' [alice] st) $ NetworkEvent defaultTTL alice $ ReqTx tx + Snapshot{number = confirmedSn} = getSnapshot sn + nextSn = confirmedSn + 1 + pure $ + ( collectEffects outcome + `shouldContain` [NetworkEffect (ReqSn nextSn [txId tx])] + ) + & counterexample (show outcome) + +prop_thereIsAlwaysALeader :: Property +prop_thereIsAlwaysALeader = + forAll arbitrary $ \sn -> + forAll arbitrary $ \params@HeadParameters{parties} -> + not (null parties) + ==> any (\p -> isLeader params p sn) parties diff --git a/hydra-node/test/Hydra/HeadLogicSpec.hs b/hydra-node/test/Hydra/HeadLogicSpec.hs index b2380de8c52..f8e9cf8c206 100644 --- a/hydra-node/test/Hydra/HeadLogicSpec.hs +++ b/hydra-node/test/Hydra/HeadLogicSpec.hs @@ -45,6 +45,7 @@ import Hydra.HeadLogic ( SeenSnapshot (NoSeenSnapshot, SeenSnapshot), WaitReason (..), collectEffects, + collectState, collectWaits, defaultTTL, update, @@ -635,14 +636,9 @@ assertNewState outcome = -- NewState is about to be superseded when we implement event-sourced persistency -- See https://github.com/input-output-hk/hydra/issues/913 -- In the meantime, we are expecting for an Outcome to only contain one single NewState. - case collectStateChanges outcome of - Nothing -> Hydra.Test.Prelude.error $ "Expecting one single newState in outcome: " <> show outcome - Just newState -> pure newState - where - collectStateChanges = \case - NewState st -> Just st - Combined l r -> collectStateChanges l <|> collectStateChanges r - _ -> Nothing + case collectState outcome of + [newState] -> pure newState + _ -> Hydra.Test.Prelude.error $ "Expecting one single newState in outcome: " <> show outcome assertEffects :: (HasCallStack, IsChainState tx) => Outcome tx -> IO () assertEffects outcome = hasEffectSatisfying outcome (const True) diff --git a/hydra-node/test/Hydra/SnapshotStrategySpec.hs b/hydra-node/test/Hydra/SnapshotStrategySpec.hs deleted file mode 100644 index f2d3429023b..00000000000 --- a/hydra-node/test/Hydra/SnapshotStrategySpec.hs +++ /dev/null @@ -1,137 +0,0 @@ -{-# LANGUAGE DuplicateRecordFields #-} - -module Hydra.SnapshotStrategySpec where - -import Hydra.Prelude hiding (label) -import Test.Hydra.Prelude - -import qualified Data.List as List -import Hydra.Chain (HeadParameters (..)) -import Hydra.HeadLogic ( - CoordinatedHeadState (..), - Effect (..), - Environment (..), - NoSnapshotReason (..), - Outcome (..), - SeenSnapshot (..), - SnapshotOutcome (..), - emitSnapshot, - isLeader, - newSn, - ) -import Hydra.HeadLogicSpec (inOpenState') -import Hydra.Ledger (Ledger (..)) -import Hydra.Ledger.Simple (SimpleTx (..), aValidTx, simpleLedger) -import Hydra.Network.Message (Message (..)) -import Hydra.Options (defaultContestationPeriod) -import Hydra.Party (deriveParty) -import Hydra.Snapshot (ConfirmedSnapshot (..), Snapshot (..), getSnapshot) -import Test.Hydra.Fixture (alice, aliceSk, bob, bobSk, carol, cperiod) -import Test.QuickCheck (Property, counterexample, forAll, label, oneof, (===), (==>)) -import Test.QuickCheck.Monadic (monadicST, pick) -import qualified Prelude - -spec :: Spec -spec = do - parallel $ do - let threeParties = [alice, bob, carol] - Ledger{initUTxO} = simpleLedger - envFor signingKey = - let party = deriveParty signingKey - in Environment - { party - , signingKey - , otherParties = List.delete party threeParties - , contestationPeriod = defaultContestationPeriod - } - - let params = HeadParameters cperiod threeParties - - let coordinatedHeadState = - CoordinatedHeadState - { seenUTxO = initUTxO - , allTxs = mempty - , seenTxs = mempty - , confirmedSnapshot = InitialSnapshot initUTxO - , seenSnapshot = NoSeenSnapshot - } - - describe "New Snapshot Decision" $ do - it "sends ReqSn given is leader and no snapshot in flight and there's a seen tx" $ do - let tx = aValidTx 1 - st = coordinatedHeadState{seenTxs = [tx]} - newSn (envFor aliceSk) params st `shouldBe` ShouldSnapshot 1 [tx] - - prop "always ReqSn given head has 1 member and there's a seen tx" prop_singleMemberHeadAlwaysSnapshot - - prop "there's always a leader for every snapshot number" prop_thereIsAlwaysALeader - - it "do not send ReqSn when we aren't leader" $ do - let tx = aValidTx 1 - st = coordinatedHeadState{seenTxs = [tx]} - newSn (envFor bobSk) params st `shouldBe` ShouldNotSnapshot (NotLeader 1) - - it "do not send ReqSn when there is a snapshot in flight" $ do - let sn1 = Snapshot 1 initUTxO mempty :: Snapshot SimpleTx - st = coordinatedHeadState{seenSnapshot = SeenSnapshot sn1 mempty} - newSn (envFor aliceSk) params st `shouldBe` ShouldNotSnapshot (SnapshotInFlight 1) - - it "do not send ReqSn when there's no seen transactions" $ do - newSn (envFor aliceSk) params coordinatedHeadState - `shouldBe` ShouldNotSnapshot NoTransactionsToSnapshot - - describe "Snapshot Emission" $ do - it "update seenSnapshot state when sending ReqSn" $ do - let tx = aValidTx 1 - st = inOpenState' threeParties coordinatedHeadState{seenTxs = [tx]} - st' = - inOpenState' threeParties $ - coordinatedHeadState - { seenTxs = [tx] - , seenSnapshot = RequestedSnapshot{lastSeen = 0, requested = 1} - } - - emitSnapshot (envFor aliceSk) (NewState st) - `shouldBe` Combined (NewState st') (Effects [NetworkEffect $ ReqSn 1 [1]]) - -prop_singleMemberHeadAlwaysSnapshot :: ConfirmedSnapshot SimpleTx -> Property -prop_singleMemberHeadAlwaysSnapshot sn = monadicST $ do - seenSnapshot <- - pick $ - oneof - [ pure NoSeenSnapshot - , LastSeenSnapshot <$> arbitrary - ] - let tx = aValidTx 1 - aliceEnv = - let party = alice - in Environment - { party - , signingKey = aliceSk - , otherParties = [] - , contestationPeriod = defaultContestationPeriod - } - st = - CoordinatedHeadState - { seenUTxO = mempty - , allTxs = mempty - , seenTxs = [tx] - , confirmedSnapshot = sn - , seenSnapshot - } - params = HeadParameters cperiod [alice] - decision = newSn aliceEnv params st - Snapshot{number} = getSnapshot sn - pure $ - decision - === ShouldSnapshot (succ number) [tx] - & counterexample ("decision: " <> show decision) - & label (Prelude.head . Prelude.words . show $ sn) - -prop_thereIsAlwaysALeader :: Property -prop_thereIsAlwaysALeader = - forAll arbitrary $ \sn -> - forAll arbitrary $ \params@HeadParameters{parties} -> - length parties - > 0 - ==> any (\p -> isLeader params p sn) parties From b40aff9a3f094e25bf580cb7581e7bc669f31ada Mon Sep 17 00:00:00 2001 From: Franco Testagrossa Date: Tue, 18 Jul 2023 10:24:50 +0200 Subject: [PATCH 2/9] Inline emit snapshot handling on AckSn and ReqTx handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Removed tautology (sˆ = ¯s) introduced to make the emit snapshot reusable during AckSn handling - Removed tautology (T ̸ ˆ = ∅) introduced to make the emit snapshot reusable during ReqTx handling With this in place, now we do not need to compute the next head state to decide wheter or not to emit a snapshot. This enables to refactor, more easily, the head logic towards event-sourcing (ADR#24) --- hydra-node/src/Hydra/HeadLogic.hs | 182 +++++++++++++----------------- 1 file changed, 80 insertions(+), 102 deletions(-) diff --git a/hydra-node/src/Hydra/HeadLogic.hs b/hydra-node/src/Hydra/HeadLogic.hs index 17bbbc2708f..c39a02395a7 100644 --- a/hydra-node/src/Hydra/HeadLogic.hs +++ b/hydra-node/src/Hydra/HeadLogic.hs @@ -657,35 +657,67 @@ onOpenNetworkReqTx env ledger st ttl tx = case applyTransactions currentSlot seenUTxO [tx] of Left (_, err) | ttl <= 0 -> - NewState (Open st{coordinatedHeadState = untrackTxInState}) - `Combined` Effects [ClientEffect $ TxInvalid headId seenUTxO tx err] + NewState (Open st{coordinatedHeadState = untrackTxInState}) + `Combined` Effects [ClientEffect $ TxInvalid headId seenUTxO tx err] | otherwise -> NewState (Open st{coordinatedHeadState = trackTxInState}) `Combined` Wait (WaitOnNotApplicableTx err) Right utxo' -> - NewState - ( Open - st - { coordinatedHeadState = - trackTxInState - { seenTxs = seenTxs <> [tx] - , seenUTxO = utxo' + Effects [ClientEffect $ TxValid headId tx] + `Combined` if isLeader parameters party nextSn && not snapshotInFlight + then + NewState + ( Open + st + { coordinatedHeadState = + trackTxInState + { seenTxs = seenTxs' + , seenUTxO = utxo' + , seenSnapshot = + RequestedSnapshot + { lastSeen = seenSnapshotNumber seenSnapshot + , requested = nextSn + } + } } - } - ) - `Combined` Effects [ClientEffect $ TxValid headId tx] - & emitSnapshot env + ) + `Combined` Effects [NetworkEffect (ReqSn nextSn (txId <$> seenTxs'))] + else + NewState + ( Open + st + { coordinatedHeadState = + trackTxInState + { seenTxs = seenTxs' + , seenUTxO = utxo' + } + } + ) where Ledger{applyTransactions} = ledger - CoordinatedHeadState{allTxs, seenTxs, seenUTxO} = coordinatedHeadState + Environment{party} = env + + CoordinatedHeadState{allTxs, seenTxs, seenUTxO, confirmedSnapshot, seenSnapshot} = coordinatedHeadState - OpenState{coordinatedHeadState, headId, currentSlot} = st + Snapshot{number = confirmedSn} = getSnapshot confirmedSnapshot + + OpenState{coordinatedHeadState, headId, currentSlot, parameters} = st trackTxInState = coordinatedHeadState{allTxs = Map.insert (txId tx) tx allTxs} untrackTxInState = coordinatedHeadState{allTxs = Map.delete (txId tx) allTxs} + snapshotInFlight = case seenSnapshot of + NoSeenSnapshot -> False + LastSeenSnapshot{} -> False + RequestedSnapshot{} -> True + SeenSnapshot{} -> True + + nextSn = confirmedSn + 1 + + seenTxs' = seenTxs <> [tx] + -- | Process a snapshot request ('ReqSn') from party. -- -- This checks that s is the next snapshot number and that the party is @@ -821,7 +853,7 @@ onOpenNetworkAckSn :: -- | Snapshot number of this AckSn. SnapshotNumber -> Outcome tx -onOpenNetworkAckSn env openState otherParty snapshotSignature sn = +onOpenNetworkAckSn Environment{party} openState otherParty snapshotSignature sn = -- TODO: verify authenticity of message and whether otherParty is part of the head -- Spec: require s ∈ {ŝ, ŝ + 1} requireValidAckSn $ do @@ -834,21 +866,35 @@ onOpenNetworkAckSn env openState otherParty snapshotSignature sn = -- Spec: σ̃ ← MS-ASig(k_H, ̂Σ̂) let multisig = aggregateInOrder sigs' parties let allTxs' = foldr Map.delete allTxs confirmed + let nextSn = sn + 1 requireVerifiedMultisignature multisig snapshot $ - NewState - ( onlyUpdateCoordinatedHeadState $ - coordinatedHeadState - { confirmedSnapshot = - ConfirmedSnapshot - { snapshot - , signatures = multisig + Effects [ClientEffect $ SnapshotConfirmed headId snapshot multisig] + `Combined` if isLeader parameters party nextSn && not (null seenTxs) + then + NewState + ( onlyUpdateCoordinatedHeadState $ + coordinatedHeadState + { seenSnapshot = + RequestedSnapshot + { lastSeen = seenSnapshotNumber $ LastSeenSnapshot (number snapshot) + , requested = nextSn + } } - , seenSnapshot = LastSeenSnapshot (number snapshot) - , allTxs = allTxs' - } - ) - `Combined` Effects [ClientEffect $ SnapshotConfirmed headId snapshot multisig] - & emitSnapshot env + ) + `Combined` Effects [NetworkEffect (ReqSn nextSn (txId <$> seenTxs))] + else + NewState + ( onlyUpdateCoordinatedHeadState $ + coordinatedHeadState + { confirmedSnapshot = + ConfirmedSnapshot + { snapshot + , signatures = multisig + } + , seenSnapshot = LastSeenSnapshot (number snapshot) + , allTxs = allTxs' + } + ) where seenSn = seenSnapshotNumber seenSnapshot @@ -891,21 +937,22 @@ onOpenNetworkAckSn env openState otherParty snapshotSignature sn = RequireFailed $ InvalidMultisignature{multisig = show multisig, vkeys} - vkeys = vkey <$> parties -- XXX: Data structures become unwieldy -> helper functions or lenses onlyUpdateCoordinatedHeadState chs' = Open openState{coordinatedHeadState = chs'} - CoordinatedHeadState{seenSnapshot, allTxs} = coordinatedHeadState - OpenState - { parameters = HeadParameters{parties} + { parameters , coordinatedHeadState , headId } = openState + CoordinatedHeadState{seenSnapshot, allTxs, seenTxs} = coordinatedHeadState + + HeadParameters{parties} = parameters + -- ** Closing the Head -- | Client request to close the head. This leads to a close transaction on @@ -1113,77 +1160,8 @@ update env ledger st ev = case (st, ev) of -- * Snapshot helper functions -data SnapshotOutcome tx - = ShouldSnapshot SnapshotNumber [tx] -- TODO(AB) : should really be a Set (TxId tx) - | ShouldNotSnapshot NoSnapshotReason - deriving (Eq, Show, Generic) - -data NoSnapshotReason - = NotLeader SnapshotNumber - | SnapshotInFlight SnapshotNumber - | NoTransactionsToSnapshot - deriving (Eq, Show, Generic) - isLeader :: HeadParameters -> Party -> SnapshotNumber -> Bool isLeader HeadParameters{parties} p sn = case p `elemIndex` parties of Just i -> ((fromIntegral sn - 1) `mod` length parties) == i _ -> False - --- | Snapshot emission decider -newSn :: Environment -> HeadParameters -> CoordinatedHeadState tx -> SnapshotOutcome tx -newSn Environment{party} parameters CoordinatedHeadState{confirmedSnapshot, seenSnapshot, seenTxs} = - if - | not (isLeader parameters party nextSn) -> - ShouldNotSnapshot $ NotLeader nextSn - | -- NOTE: This is different than in the spec. If we use seenSn /= - -- confirmedSn here, we implicitly require confirmedSn <= seenSn. Which - -- may be an acceptable invariant, but we have property tests which are - -- more strict right now. Anyhow, we can be more expressive. - snapshotInFlight -> - ShouldNotSnapshot $ SnapshotInFlight nextSn - | null seenTxs -> - ShouldNotSnapshot NoTransactionsToSnapshot - | otherwise -> - ShouldSnapshot nextSn seenTxs - where - nextSn = confirmedSn + 1 - - snapshotInFlight = case seenSnapshot of - NoSeenSnapshot -> False - LastSeenSnapshot{} -> False - RequestedSnapshot{} -> True - SeenSnapshot{} -> True - - Snapshot{number = confirmedSn} = getSnapshot confirmedSnapshot - --- | Emit a snapshot if we are the next snapshot leader. 'Outcome' modifying --- signature so it can be chained with other 'update' functions. -emitSnapshot :: IsTx tx => Environment -> Outcome tx -> Outcome tx -emitSnapshot env outcome = - case outcome of - NewState (Open OpenState{parameters, coordinatedHeadState, chainState, headId, currentSlot}) -> - case newSn env parameters coordinatedHeadState of - ShouldSnapshot sn txs -> do - let CoordinatedHeadState{seenSnapshot} = coordinatedHeadState - NewState - ( Open - OpenState - { parameters - , coordinatedHeadState = - coordinatedHeadState - { seenSnapshot = - RequestedSnapshot - { lastSeen = seenSnapshotNumber seenSnapshot - , requested = sn - } - } - , chainState - , headId - , currentSlot - } - ) - `Combined` Effects [NetworkEffect (ReqSn sn (txId <$> txs))] - _ -> outcome - Combined l r -> Combined (emitSnapshot env l) (emitSnapshot env r) - _ -> outcome From 6939591270a4521910c0a44e30ea9da3606068d4 Mon Sep 17 00:00:00 2001 From: Pascal Grange Date: Tue, 18 Jul 2023 10:28:08 +0200 Subject: [PATCH 3/9] Update the spec --- spec/fig_offchain_prot.tex | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/fig_offchain_prot.tex b/spec/fig_offchain_prot.tex index 56f99edef71..8c76de72e5d 100644 --- a/spec/fig_offchain_prot.tex +++ b/spec/fig_offchain_prot.tex @@ -79,7 +79,7 @@ $\hatmL \gets \hatmL\applytx\tx$ % % issue snapshot if we are leader - \If{$\hats = \bars \land \hpLdr(\bars + 1) = i \land \hatmT \neq \emptyset$}{% + \If{$\hats = \bars \land \hpLdr(\bars + 1) = i$}{% \Multi{} $(\hpRS,\bars+1,\hatmT)$ \;% } } @@ -144,7 +144,7 @@ \If{$\forall k \in [1..n]: (k,\cdot) \in \hatSigma$}{ % - % TODO: MS-ASig used different than in the preliminaries + % TODO: MS-ASig used different than in the preliminaries $\msCSig \gets \msComb(\hydraKeys^{setup}, \hatSigma)$ \; % $\eta' \gets (\hats, \combine(\hatmU))$ \; % @@ -157,8 +157,8 @@ $\forall \tx \in \mT_{res} : \Out (\hpConf,\tx)$ \; % % issue snapshot if we are leader - \If{$\hats = \bars \land \hpLdr(\bars + 1) = i \land \hatmT \neq \emptyset$}{% - \Multi{} $(\hpRS,\bars+1,\hatmT)$ \;% + \If{$\hpLdr(s+1) = i \land \hatmT \neq \emptyset$}{% + \Multi{} $(\hpRS,s+1,\hatmT)$ \;% } } } } From 79edb09836e41c92a57c571f48215e366c38ba8a Mon Sep 17 00:00:00 2001 From: Pascal Grange Date: Tue, 18 Jul 2023 18:10:21 +0200 Subject: [PATCH 4/9] We're happy with it that way for now This will be changed with the event-sourced approach anyway. --- hydra-node/src/Hydra/HeadLogic.hs | 1 - 1 file changed, 1 deletion(-) diff --git a/hydra-node/src/Hydra/HeadLogic.hs b/hydra-node/src/Hydra/HeadLogic.hs index c39a02395a7..09ea08a9606 100644 --- a/hydra-node/src/Hydra/HeadLogic.hs +++ b/hydra-node/src/Hydra/HeadLogic.hs @@ -427,7 +427,6 @@ collectWaits = \case Effects _ -> [] Combined l r -> collectWaits l <> collectWaits r --- FIXME should return Maybe? collectState :: Outcome tx -> [HeadState tx] collectState = \case NoOutcome -> [] From 5dd957cee6a2654e75336dc9dd18f6162ac46b27 Mon Sep 17 00:00:00 2001 From: Pascal Grange Date: Tue, 18 Jul 2023 18:19:51 +0200 Subject: [PATCH 5/9] Pick the snapshot number right from the function's parameter --- hydra-node/src/Hydra/HeadLogic.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hydra-node/src/Hydra/HeadLogic.hs b/hydra-node/src/Hydra/HeadLogic.hs index 09ea08a9606..8289be0206a 100644 --- a/hydra-node/src/Hydra/HeadLogic.hs +++ b/hydra-node/src/Hydra/HeadLogic.hs @@ -875,7 +875,7 @@ onOpenNetworkAckSn Environment{party} openState otherParty snapshotSignature sn coordinatedHeadState { seenSnapshot = RequestedSnapshot - { lastSeen = seenSnapshotNumber $ LastSeenSnapshot (number snapshot) + { lastSeen = sn , requested = nextSn } } @@ -890,7 +890,7 @@ onOpenNetworkAckSn Environment{party} openState otherParty snapshotSignature sn { snapshot , signatures = multisig } - , seenSnapshot = LastSeenSnapshot (number snapshot) + , seenSnapshot = LastSeenSnapshot sn , allTxs = allTxs' } ) From cd7043c96f305b15493051217cd44676637afa68 Mon Sep 17 00:00:00 2001 From: Pascal Grange Date: Tue, 18 Jul 2023 18:22:17 +0200 Subject: [PATCH 6/9] Fix grammar in tests description --- hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs b/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs index 874274a4bb7..4fa38776f35 100644 --- a/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs +++ b/hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs @@ -83,14 +83,14 @@ spec = do collectEffects outcome `shouldContain` [NetworkEffect (ReqSn 1 [txId tx])] - it "Do NOT send ReqSn when we are NOT the leader even if no snapshot in flight" $ do + it "does NOT send ReqSn when we are NOT the leader even if no snapshot in flight" $ do let tx = aValidTx 1 st = coordinatedHeadState{seenTxs = [tx]} outcome = update (envFor bobSk) simpleLedger (inOpenState' [alice, bob] st) $ NetworkEvent defaultTTL bob $ ReqTx tx collectEffects outcome `shouldNotSatisfy` sendReqSn - it "Do NOT send ReqSn when we are the leader but snapshot in flight" $ do + it "does NOT send ReqSn when we are the leader but snapshot in flight" $ do let tx = aValidTx 1 sn1 = Snapshot 1 initUTxO mempty :: Snapshot SimpleTx st = coordinatedHeadState{seenSnapshot = SeenSnapshot sn1 mempty} @@ -98,7 +98,7 @@ spec = do collectEffects outcome `shouldNotSatisfy` sendReqSn - it "update seenSnapshot state when sending ReqSn" $ do + it "updates seenSnapshot state when sending ReqSn" $ do let tx = aValidTx 1 st = inOpenState' threeParties coordinatedHeadState outcome = update (envFor aliceSk) simpleLedger st $ NetworkEvent defaultTTL alice $ ReqTx tx @@ -135,7 +135,7 @@ spec = do let outcome = update bobEnv simpleLedger headState $ ackFrom bobSk bob collectEffects outcome `shouldSatisfy` sendReqSn - it "do NOT send ReqSn when we are the leader but there are NO seen transactions" $ do + it "does NOT send ReqSn when we are the leader but there are NO seen transactions" $ do let bobEnv = Environment @@ -153,7 +153,7 @@ spec = do let outcome = update bobEnv simpleLedger headState $ ackFrom bobSk bob collectEffects outcome `shouldNotSatisfy` sendReqSn - it "Do NOT send ReqSn when we are NOT the leader but there are seen transactions" $ do + it "does NOT send ReqSn when we are NOT the leader but there are seen transactions" $ do let tx = aValidTx 1 notLeaderEnv = @@ -177,7 +177,7 @@ spec = do let everybodyAcknowleged = update notLeaderEnv simpleLedger headState $ ackFrom bobSk bob collectEffects everybodyAcknowleged `shouldNotSatisfy` sendReqSn - it "update seenSnapshot state when sending ReqSn" $ do + it "updates seenSnapshot state when sending ReqSn" $ do let tx = aValidTx 1 bobEnv = From c638ba8c64314d2fca0f484bace6d4e061c50d5c Mon Sep 17 00:00:00 2001 From: Sebastian Nagel Date: Wed, 19 Jul 2023 10:58:28 +0200 Subject: [PATCH 7/9] Fix state update on confirming snapshot This was not covered by a test and hence we have created a bit more higher level a behavior spec which asserts that snapshots are created while there are still transactions seen and not in snapshots yet. --- hydra-node/src/Hydra/HeadLogic.hs | 7 ++++- hydra-node/test/Hydra/BehaviorSpec.hs | 40 ++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/hydra-node/src/Hydra/HeadLogic.hs b/hydra-node/src/Hydra/HeadLogic.hs index 8289be0206a..5612031ee24 100644 --- a/hydra-node/src/Hydra/HeadLogic.hs +++ b/hydra-node/src/Hydra/HeadLogic.hs @@ -873,7 +873,12 @@ onOpenNetworkAckSn Environment{party} openState otherParty snapshotSignature sn NewState ( onlyUpdateCoordinatedHeadState $ coordinatedHeadState - { seenSnapshot = + { confirmedSnapshot = + ConfirmedSnapshot + { snapshot + , signatures = multisig + } + , seenSnapshot = RequestedSnapshot { lastSeen = sn , requested = nextSn diff --git a/hydra-node/test/Hydra/BehaviorSpec.hs b/hydra-node/test/Hydra/BehaviorSpec.hs index d159817bf09..2f7791c36b6 100644 --- a/hydra-node/test/Hydra/BehaviorSpec.hs +++ b/hydra-node/test/Hydra/BehaviorSpec.hs @@ -258,7 +258,7 @@ spec = parallel $ do withHydraNode bobSk [alice] chain $ \n2 -> do openHead n1 n2 - send n1 (NewTx (aValidTx 42)) + send n1 (NewTx $ aValidTx 42) waitUntil [n1, n2] $ TxValid testHeadId (aValidTx 42) let snapshot = Snapshot 1 (utxoRefs [1, 2, 42]) [42] @@ -268,6 +268,44 @@ spec = parallel $ do send n1 Close waitForNext n1 >>= assertHeadIsClosedWith 1 + it "snapshots are created as long as transactions to snapshot exist" $ + shouldRunInSim $ + withSimulatedChainAndNetwork $ \chain -> + withHydraNode aliceSk [bob] chain $ \n1 -> + withHydraNode bobSk [alice] chain $ \n2 -> do + openHead n1 n2 + + -- Load the "ingest queue" of the head enough to have still + -- pending transactions after a first snapshot request by + -- alice. Note that we are in a deterministic simulation here. + send n1 (NewTx $ aValidTx 40) + send n1 (NewTx $ aValidTx 41) + send n1 (NewTx $ aValidTx 42) + + -- Expect alice to create a snapshot from the first requested + -- transaction right away which is the current snapshot policy. + waitUntilMatch [n1, n2] $ \case + SnapshotConfirmed{snapshot = Snapshot{number, confirmed}} -> + number == 1 && confirmed == [40] + _ -> False + + -- Expect bob to also snapshot what did "not fit" into the first + -- snapshot. + waitUntilMatch [n1, n2] $ \case + SnapshotConfirmed{snapshot = Snapshot{number, confirmed}} -> + -- NOTE: We sort the confirmed to be clear that the order may + -- be freely picked by the leader. + number == 2 && sort confirmed == [41, 42] + _ -> False + + -- As there are no pending transactions and snapshots anymore + -- we expect to continue normally on seeing just another tx. + send n1 (NewTx $ aValidTx 44) + waitUntilMatch [n1, n2] $ \case + SnapshotConfirmed{snapshot = Snapshot{number, confirmed}} -> + number == 3 && confirmed == [44] + _ -> False + it "depending transactions stay pending and are confirmed in order" $ shouldRunInSim $ withSimulatedChainAndNetwork $ \chain -> From 6d1215fdd9a07c52468c282a56504f34c88973aa Mon Sep 17 00:00:00 2001 From: Sebastian Nagel Date: Wed, 19 Jul 2023 11:00:11 +0200 Subject: [PATCH 8/9] Drop the pending test in BehaviorSpec This is now covered by the recently added test about snapshotting there. --- hydra-node/test/Hydra/BehaviorSpec.hs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/hydra-node/test/Hydra/BehaviorSpec.hs b/hydra-node/test/Hydra/BehaviorSpec.hs index 2f7791c36b6..749aa002a0f 100644 --- a/hydra-node/test/Hydra/BehaviorSpec.hs +++ b/hydra-node/test/Hydra/BehaviorSpec.hs @@ -379,25 +379,6 @@ spec = parallel $ do TxInvalid{transaction} -> transaction == tx'' _ -> False - it "multiple transactions get snapshotted" $ do - pendingWith "This test is not longer true after recent changes which simplify the snapshot construction." - shouldRunInSim $ do - withSimulatedChainAndNetwork $ \chain -> - withHydraNode aliceSk [bob] chain $ \n1 -> - withHydraNode bobSk [alice] chain $ \n2 -> do - openHead n1 n2 - - send n1 (NewTx (aValidTx 42)) - send n1 (NewTx (aValidTx 43)) - - waitUntil [n1] $ TxValid testHeadId (aValidTx 42) - waitUntil [n1] $ TxValid testHeadId (aValidTx 43) - - let snapshot = Snapshot 1 (utxoRefs [1, 2, 42, 43]) [42, 43] - sigs = aggregate [sign aliceSk snapshot, sign bobSk snapshot] - - waitUntil [n1] $ SnapshotConfirmed testHeadId snapshot sigs - it "outputs utxo from confirmed snapshot when client requests it" $ shouldRunInSim $ do withSimulatedChainAndNetwork $ \chain -> From 8f567f20d3bba44f50f2b837676adbbc65c5b368 Mon Sep 17 00:00:00 2001 From: Pascal Grange Date: Wed, 19 Jul 2023 16:15:11 +0000 Subject: [PATCH 9/9] FIX performance issue Forgot to purge the allTxs list when transaction is included in a snapshot. Added a test to ensure that we check that both, when we're supposed to send a ReqSn and when we're not. This is not pretty as we duplicate one test so that we can check for every branch but it's safer now that we're explicit about this and we can, hopefully, improve that when we migrate to event-sourced approach. --- hydra-node/src/Hydra/HeadLogic.hs | 1 + hydra-node/test/Hydra/HeadLogicSpec.hs | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/hydra-node/src/Hydra/HeadLogic.hs b/hydra-node/src/Hydra/HeadLogic.hs index 5612031ee24..43bce207023 100644 --- a/hydra-node/src/Hydra/HeadLogic.hs +++ b/hydra-node/src/Hydra/HeadLogic.hs @@ -883,6 +883,7 @@ onOpenNetworkAckSn Environment{party} openState otherParty snapshotSignature sn { lastSeen = sn , requested = nextSn } + , allTxs = allTxs' } ) `Combined` Effects [NetworkEffect (ReqSn nextSn (txId <$> seenTxs))] diff --git a/hydra-node/test/Hydra/HeadLogicSpec.hs b/hydra-node/test/Hydra/HeadLogicSpec.hs index f8e9cf8c206..c9e9f30d7d8 100644 --- a/hydra-node/test/Hydra/HeadLogicSpec.hs +++ b/hydra-node/test/Hydra/HeadLogicSpec.hs @@ -174,6 +174,27 @@ spec = (Open OpenState{coordinatedHeadState = CoordinatedHeadState{allTxs}}) -> txId t1 `notMember` allTxs _ -> False + it "removes transactions from allTxs when included in a acked snapshot even when emitting a ReqSn" $ do + let t1 = SimpleTx 1 mempty (utxoRef 1) + pendingTransaction = SimpleTx 2 mempty (utxoRef 2) + reqSn = NetworkEvent defaultTTL alice $ ReqSn 1 [1] + snapshot1 = Snapshot 1 (utxoRefs [1]) [1] + ackFrom sk vk = NetworkEvent defaultTTL vk $ AckSn (sign sk snapshot1) 1 + + sa <- runEvents bobEnv ledger (inOpenState threeParties ledger) $ do + step $ NetworkEvent defaultTTL alice $ ReqTx t1 + step reqSn + step (ackFrom carolSk carol) + step (ackFrom aliceSk alice) + + step $ NetworkEvent defaultTTL alice $ ReqTx pendingTransaction + + step (ackFrom bobSk bob) + + sa `shouldSatisfy` \case + (Open OpenState{coordinatedHeadState = CoordinatedHeadState{allTxs}}) -> txId t1 `notMember` allTxs + _ -> False + it "rejects last AckSn if one signature was from a different snapshot" $ do let reqSn = NetworkEvent defaultTTL alice $ ReqSn 1 [] snapshot = Snapshot 1 mempty []