Implement ADR 18 #579
Merged
Implement ADR 18 #579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ch1bo
force-pushed
the
ch1bo/adr-18-for-real
branch
3 times, most recently
from
a0a2154 to
128c156
Compare
ch1bo
commented
Oct 25, 2022
hydra-node/src/Hydra/Chain.hs
Outdated
| deriving instance IsTx tx => Eq (PostTxError tx) | ||
| deriving instance IsTx tx => Show (PostTxError tx) | ||
| -- TODO: document | ||
| advanceSlot :: a -> a |
There was a problem hiding this comment.
This could/should be moved to a test-specific type class for chain states as it is only required in the simulatedChainAndNetwork of the BehaviorSpec (Alternative: make that SimpleTx specific.)
ch1bo
commented
Oct 25, 2022
| @@ -362,7 +362,7 @@ estimateScriptsCost pparams systemStart epochInfo utxo tx = do | |||
| Left translationError -> | |||
| Left $ ErrTranslationError translationError | |||
| Right units -> | |||
| Map.traverseWithKey (\ptr -> left $ ErrScriptExecutionFailed . (ptr,)) units | |||
| traverse (left ErrScriptExecutionFailed) units | |||
There was a problem hiding this comment.
Hahah.. @ffakenz, I happened to remove it here as well 😅. I should re-add the redeemer pointer.
ch1bo
force-pushed
the
ch1bo/adr-18-for-real
branch
from
0317dc2 to
1ccbe50
Compare
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Cost of Init Transaction
Cost of Commit TransactionCurrently only one UTxO per commit allowed (this is about to change soon)
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort TransactionSome variation because of random mixture of still initial and already committed outputs.
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
|
ch1bo
force-pushed
the
ch1bo/adr-18-for-real
branch
from
1ccbe50 to
1027391
Compare
2 tasks
ch1bo
commented
Oct 27, 2022
| } | ||
| readTVarIO headState >>= save | ||
| mapM_ (callback . Observation) onChainTxs | ||
| forM_ receivedTxs $ \tx -> |
There was a problem hiding this comment.
NOTE: previously these have been reversed
| Just Observation{newChainState} -> atomically $ writeTVar stateVar newChainState | ||
| let handler = chainSyncHandler nullTracer callback (pure timeHandle) | ||
| -- Simulate some chain following | ||
| run $ mapM_ (onRollForward handler) blocks |
There was a problem hiding this comment.
Could: not test roll forward when testing the roll backward (it's not needed by the implementation)
Test Results267 tests +2 261 ✔️ +2 16m 6s ⏱️ + 1m 36s Results for commit 3613cee. ± Comparison against base commit c8f99f9. This pull request removes 8 and adds 10 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
ch1bo
commented
Nov 2, 2022
| IsChainState tx | ||
| where | ||
| -- | Types of what to keep as L1 chain state. | ||
| type ChainStateType tx = c | c -> tx |
There was a problem hiding this comment.
Nice, didn't know one can use functional dependencies here!
There was a problem hiding this comment.
I remembered it from having read something about injective type families. My first try was to use a data family but the changes were much more involved and annoying, esp. with parameterised types.
…irectChain This is required to "tie the knot" in the main. We need a starting state before we can define the callback (which needs access to some state).
Also move the ledger directly into HydraNode
This is a bit annoying as we need to thread the chain state through postTx and waitCallback (or derivatives).
Only this way we can avoid orphan instances and cyclic module dependencies.
This pairs a ChainState with a slot. We need to know the latter to be able to rollback correctly.
It's sufficient that we check correct yielding of events now. The actual rollback logic is in the HeadLogic and tested there.
This is an interesting one. Should we have unit tests for this? i.e. asserting the chain state used on effects?
The ChainContext (in the chain state) needs to be consistent for correctly minting and distributing tokens on InitTx.
This is necessary to have the latest chainState when observing multiple transactions without processing events in HeadLogic, e.g. concurrent commits. It will also make chainState update redundant in the HeadLogic, which in turn increases complexity of "where things are happening".
Also clean up run options logging test
This would be quite awkward to define and means that either the simulatedChainAndNetwork is not correctly implemented to be used with real Tx (non-SimpleTx), or that we actually want to have a different test driver for the model based tests (we will also want to construct & validate real transactions).
We only use it in simulatedChainAndNetwork, hence moving it there.
Only relying on this modifyHeadState in 'Main' would mean that mocks like the simulatedChainAndNetwork would become even more complicated.
Instead of an (open) external type family I have turned it into an injective associated type family, using the TypeFamilyDependencies option. It seems to me this simplify the definition and the use by tying both the transactions and the state to the tx type.
ch1bo
force-pushed
the
ch1bo/adr-18-for-real
branch
from
ee45583 to
ed3bcdd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🍁 Based on previous work done with @ffakenz, @v0d1ch and @pgrange
🍁 Changed interface of
Hydra.Chainby adding aChainStateType txandIsChainStateto modifychainStatein aheadStateabstractlyChainCallbackis taking a continuation now which will yield an event🍁 Removed chain-local persistence and in-memory TVar.
🍁 Rollbacks are now fully handled in the
HeadLogic. The rollback event will contain a rollbackChainSlotand we can get thechainStateSlotviaIsChainStateto unwind theHeadState txaccordingly.🍁 Tying the knot between chain state updates & providing the callback with the latest state in the
hydra-nodeMainmodule now (see TODO below)🍁 Needed to update the
simulatedChainAndNetworkquite a bit. Not fully satisfied with the result, especially as chain states are something node-specific and they are shared between all testHydraNodes in the simulation. This might be even problematic for theModeltests should they ever usechainState.🍁 Rewritten
DirectChainSpectest driver. It needs to keep & updatechainStatenow.🍁 Lowered scope of the
HandlersSpectests on rollbacks as they cannot cover full rollback behavior anymore. Only correct yielding ofRollbackevents is in scope now.🍁 Clear chain state persistence in some E2E tests now as it would be loaded consistently with the heads state now.
🍁 Moved state & transaction generators from
Hydra.Chain.Direct.ContextintoHydra.Chain.Direct.Stateto avoid orphan instances and additional modules to break cyclic dependencies (theIsChainStateis annoying).🍁 Consolidated
ReceivedTxsintoRolledForwardlog entry.🍁 Renamed the
HydraHeadhandle toNodeStateTODO
simulatedChainAndNetworkforModel/ModelSpecRemovechainStatefromChainEvent,ChainEffect& also not thread it throughHeadLogicBehaviorSpeceven more..