-
Notifications
You must be signed in to change notification settings - Fork 84
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
Offline mode implementation #1118
Offline mode implementation #1118
Conversation
Do you expect a review now or do you want to wait? |
hydra-node/exe/hydra-node/Main.hs
Outdated
withCardanoLedgerOffline OfflineConfig{ledgerGenesisFile} protocolParams action = do | ||
--TODO(Elaine): double check previous messy branch for any other places where we query node | ||
genesisParameters <- readJsonFileThrow (parseJSON @(Ledger.ShelleyGenesis StandardCrypto)) ledgerGenesisFile | ||
globals <- newGlobals $ fromShelleyGenesis genesisParameters |
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.
We're reading in genesis parameters here, and while I don't think they're a huge obstacle to obtain, I think they're a necessity because of the type of applyTxes. Is this a correct assessment? Is there any way to sidestep this requirement currently?
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.
Not really. You can have a look here: https://github.com/input-output-hk/hydra/blob/e6f25045cb01e74fce3c63a555fb727060c8a4b1/hydra-node/src/Hydra/Chain/Direct/Fixture.hs#L51
In the tests we are using the data structures to prime a Cardano ledger, you could do something similar in off-line mode
hydra-node/exe/hydra-node/Main.hs
Outdated
--TODO(ELAINE): figure out a less strange way to do this | ||
-- | Taken from Cardano.Api.GenesisParameters, a private module in cardano-api | ||
|
||
fromShelleyGenesis :: Shelley.ShelleyGenesis Ledger.StandardCrypto -> GenesisParameters ShelleyEra |
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.
If we really do need genesis parameters, is there a less awkward way to get them than parsing a ShelleyGenesis and running a copied fromShelleyGenesis on it? It's a private function so I'm not sure if this is exposing implementation details
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.
See my answer above
hydra-node/src/Hydra/Chain/Direct.hs
Outdated
@@ -164,6 +178,68 @@ mkTinyWallet tracer config = do | |||
hoistEpochInfo (first show . runExcept) $ | |||
Consensus.interpreterToEpochInfo interpreter | |||
|
|||
withOfflineChain :: | |||
Tracer IO DirectChainLog -> -- TODO(ELAINE): change type maybe ? |
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.
Is it okay to reuse DirectChainLog here, or should we have a different type to indicate this is offline mode?
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.
You would need to add specific constructors for the specific loggging you would do in off-line mode?
A review now I think would be appropriate. Thank you |
406c2e9
to
0c3a59f
Compare
c080e0d
to
4064c81
Compare
This needs new tests to ensure the offline mode functionality but I think should otherwise be ready for review |
6f94fe6
to
d042f6d
Compare
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.
Very nice work so far. As you already mentioned, tests are definitely needed for this and I left a few more remarks on what could be improved.
When you rebase onto the latest master
, you can ensure your changes are formatted correctly (which is checked by the CI now) by simply calling treefmt
from the nix shell.
hydra-node/src/Hydra/Options.hs
Outdated
@@ -161,6 +230,7 @@ data RunOptions = RunOptions | |||
, persistenceDir :: FilePath | |||
, chainConfig :: ChainConfig | |||
, ledgerConfig :: LedgerConfig | |||
, offlineConfig :: Maybe OfflineConfig |
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 see that Hydra.Chain.Offline
should be a drop-in replacement for Hydra.Chain.Direct
. That makes a lot of sense to me.
Concretely, the hydra-node
exe seems to switch a lot on the actual Options
provided and does things differently.
I think it would be worthwhile to model this already her using something like
data ChainConfig
= DirectChainConfig { ... }
| OfflineChainConfig { ... }
along with appropriate Parser ChainConfig
which switches on the mutually exclusive command line options which determine the mode of operation.
My hunch is, that modelling it that way, will allow the main
to separate more cleanly between the two modes.
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.
Please avoid committing test output files. Both, faulty.json
and reencoded.json
, are outputs of the golden test suite which (rightfully) need updating when command line options change.
hydra-node/exe/hydra-node/Main.hs
Outdated
loadGlobalsFromGenesis ledgerGenesisFile | ||
|
||
withCardanoLedger pparams globals $ \ledger -> do | ||
persistence <- createStateChangePersistence (persistenceDir <> "/state") (offlineOptionsNormalizedUtxoWriteBackFilePath =<< leftToMaybe onlineOrOfflineConfig) |
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.
What's the rationale of this piggy-backing onto the standard persistence of StateChanged
events?
It seems like you are interested in storing the latest utxo
at some specific location (in addition to the state
file), but I would like to know the whole user story for this.
This mechanism seems quite orthogonal to the offline mode chain layer and I'd prefer having this in a dedicated PR (in which I would ask for the purpose of this the same way).
Depending in how the downstream usage of this file is, a more modular alternative could be a small utility subscribing to the API (websocket) and writing this file on every SnapshotConfirmed
message.
Another alternative would be to make this "writeBack" mechanism be attached as a consumer of those StateChanged
events here. This would support also an idea we had where the Effects
of the business logic (e.g. sending output to clients on the API) be mere interpretations of the event stream.
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 think the main concern is that in offline mode, we don't have any "Commit" events, so it's decidedly useless: the head opens with an empty UTXO, and so we can't spend any transactions;
So, we added the ability to specify the initial UTXO on the command line, but in a call we had with the team, there was concerns about where to get this initial UTXO file in the first place; as a convenience, we agreed to add something that saved the UTXO state out when the node shut down gracefully, so we could reuse that on the next run.
If it feels a bit messy / arbitrary, we can strip that back out and say that you need to construct the UTXO by hand, and/or fold this into our other ADR (a PR for which is coming soon!)
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 see. But why would you open it empty? Also wouldn't the normal persistence of internal state have the hydra-node
start up in the Open
state with just the same UTxO as on the last run?
In my mind the offline mode could work like the following:
- Always simulates
Init
andCommit
observations based on some configured initial utxo file - When no previous state is loaded, the head logic would interpret that as opening of the head
- Allow normal off-chain transaction processing, evolving the
Open
off-chain head state (which is automatically persisted) - On later starts, the head would already be in an
Open
state as loaded from persistence and any (simulated)Init
andCommit
observations from the offline chain would be ignored
The initial UTxO file would be given by your outer gummiworm protocol I suppose.
OnContestTx{snapshotNumber = number $ getSnapshot confirmedSnapshot}} | ||
FanoutTx{} -> | ||
callback $ Observation { newChainState = cst, observedTx = | ||
OnFanoutTx{}} |
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 an offline chain even open/close a head? I would have expected the offline chain to be in an Open
state right away (constantly yielded Observation
when starting it) and also ignore all commands to CloseTx
it.
withOfflineChain tracer OfflineConfig{ledgerGenesisFile} [email protected]{systemStart} ownHeadId chainStateHistory callback action = do | ||
|
||
|
||
localChainState <- newLocalChainState chainStateHistory |
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.
As you point out in a NOTE further up, we do not need to keep a chain state in the offline chain layer, as it is not needed to construct transactions (in fact there are no L1 transactions). So we can keep it initialChainState = ChainStateAt{chainState = Idle, recordedAt = Nothing}
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.
If you apply my suggestions, this becomes unused.
hydra-node/exe/hydra-node/Main.hs
Outdated
Nothing -> pure () | ||
Just (OfflineConfig {initialUTxOFile})-> do | ||
initialUTxO :: UTxOType Tx <- readJsonFileThrow (parseJSON @(UTxOType Tx)) initialUTxOFile | ||
initializeStateIfOffline chainStateHistory initialUTxO headId party contestationPeriod (putEvent . OnChainEvent) |
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 think this (yielding of constant observations to simulate a head opening) can be moved to withOfflineChain
directly, which would simplify the main function (less casing).
3ba59ac
to
ea06cfd
Compare
147905d
to
8593410
Compare
@cardenaso11 How are things going? Is this PR ready for review, or perhaps another pairing session could be helpful? |
f9ae686
to
e995f48
Compare
I think it is ready for review |
@cardenaso11 Could you rebase the branch on latest master and resolve conflicts? |
e995f48
to
9063377
Compare
I noticed a few tests failing, would you need help on this @cardenaso11 ? |
@abailly-iohk should be fixed now, had some nix changes for dealing with arm64 macos I didn't realize I checked in that got mangled during rebase somehow |
…nChainId" This reverts commit 84adba8.
…, do not merge as is" This reverts commit 6599c0a.
…o have conflicts minimized, warnings and debug code removed. this'll get rebased
Co-authored-by: card <[email protected]>
Co-authored-by: card <[email protected]>
Co-authored-by: card <[email protected]>
Co-authored-by: card <[email protected]>
Co-authored-by: card <[email protected]>
Co-authored-by: card <[email protected]>
77199b8
to
a10c492
Compare
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.
All must issues are resolved now and I can approve merging this.
I will follow-up with a PR that should significantly simplify the Hydra.Options
handling and downstream code duplication in Hydra.Node.Run
and HydraNode
modules.
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.
Some very minor comments
where | ||
logFilePath = workDir </> "logs" </> "hydra-node-" <> show hydraNodeId <.> "log" | ||
|
||
withOfflineHydraNode' :: |
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 don't think we need this separation as withOfflineHydraNode'
is used in only one place. And even the other 2 functions are clunky and should be refactored.
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'll cover this in a refactor PR.
TODO: