-
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
Remove protocol parameters from hydra-cluster #1214
Conversation
e502616
to
6818374
Compare
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Script summary
Cost of Init Transaction
Cost of Commit TransactionThis is using ada-only outputs for better comparability.
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.
End-To-End Benchmark ResultsThis page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes. Generated at 2023-12-24 19:19:48.929435015 UTC Baseline Scenario
Baseline Scenario
|
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 like this PR is removing some cruft from the code repository, and this highlights some further refactoring
hydra-cluster/src/HydraNode.hs
Outdated
@@ -215,11 +214,12 @@ withHydraCluster :: | |||
[SigningKey HydraKey] -> | |||
-- | Transaction id at which Hydra scripts should have been published. | |||
TxId -> | |||
ProtocolParameters -> |
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.
This list of parameters is starting to get quite large and unwieldy, perhaps an opportunity for more refactoring later?
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.
Yes definitely
hydra-cluster/src/HydraNode.hs
Outdated
@@ -236,10 +236,11 @@ withConfiguredHydraCluster :: | |||
TxId -> | |||
-- | Modifies the `ChainConfig` passed to a node upon startup | |||
(Int -> ChainConfig -> ChainConfig) -> | |||
ProtocolParameters -> |
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.
ditto
hydra-cluster/src/HydraNode.hs
Outdated
@@ -293,11 +295,12 @@ withHydraNode :: | |||
[Int] -> | |||
-- | Transaction id at which Hydra scripts should have been published. | |||
TxId -> | |||
ProtocolParameters -> |
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.
ditto
6818374
to
8b2f44d
Compare
b6ab1d9
to
59b2d18
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.
This is a good start and I think we should just avoid using ProtocolParameters
again and use PParams
right away.
cardano-cli query protocol-parameters --testnet-magic 42 | jq '.txFeeFixed = 0 |.txFeePerByte = 0 | .executionUnitPrices.priceMemory = 0 | .executionUnitPrices.priceSteps = 0' \ | ||
> protocol-parameters.json | ||
``` | ||
|
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.
Should: instead of explaining this we could just pull those from the network in seed-devnet.sh
?
This would keep the demo as streamlined as before.
I like this section though and we should merge it with / replace the one in the quickstart: https://hydra.family/head-protocol/docs/getting-started/quickstart/#ledger-parameters
hydra-cluster/src/CardanoNode.hs
Outdated
, networkId | ||
, protocolParameters = | ||
zeroPParams $ | ||
Api.fromLedgerPParams Api.ShelleyBasedEraBabbage pparams |
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.
Must: not use ProtocolParameters
here and just leave them as ledger PParams
.
We know that this type will go away from the cardano-api
or is just a wrapping of the ledger PParams
anyway.
This will remove this zeroPParams
and conversion. We can do this because the only thing we do in hydra-cluster
with these parameters is to write them to disk anyhow.
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.
Good idea. GHC has problems finding the json instance no matter what I try 2d4309e#diff-2192a085c660665079395143368226091c3c14c9943ad234dcedafb32d69630cR85
putTextLn "Seeding network" | ||
let hydraTracer = contramap FromHydraNode tracer | ||
hydraScriptsTxId <- seedNetwork node dataset (contramap FromFaucet tracer) | ||
let contestationPeriod = UnsafeContestationPeriod 10 | ||
withHydraCluster hydraTracer workDir nodeSocket startingNodeId cardanoKeys hydraKeys hydraScriptsTxId contestationPeriod $ \(leader :| followers) -> do | ||
withHydraCluster hydraTracer workDir nodeSocket startingNodeId cardanoKeys hydraKeys hydraScriptsTxId protocolParameters contestationPeriod $ \(leader :| followers) -> do |
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.
Could: Change withHydraCluster
to take a RunningNode
Test Results388 tests ±0 383 ✅ ±0 22m 7s ⏱️ +45s Results for commit ac73f6b. ± Comparison against base commit f5b1e4f. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
05605e0
to
60b5c76
Compare
10fdcb2
to
c2358f1
Compare
The actuall file is needed just for the hydra offline mode. In the rest of the codebase we query the pparams from carano-node.
c2358f1
to
32676cd
Compare
fix #1124