Skip to content
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

Non-mandatory fuel #924

Merged
merged 26 commits into from
Jun 20, 2023
Merged

Non-mandatory fuel #924

merged 26 commits into from
Jun 20, 2023

Conversation

v0d1ch
Copy link
Contributor

@v0d1ch v0d1ch commented Jun 12, 2023

  • Deprecates the use of internal commits and the need for marking fuel
  • Adds some documentation about external commits
  • Updates documentation such that marking utxo as fuel is not needed anymore
  • Makes end-to-end tests and benchmarks use the external commit
    • Keeps one test for the internal commit to verify it still works
  • Uses largest UTxO owned by --cardano-signing-key if no marked fuel can be found.

  • CHANGELOG updated or not needed
  • Documentation updated or not needed
  • Haddocks updated or not needed
  • No new TODOs introduced or explained herafter

@v0d1ch v0d1ch self-assigned this Jun 12, 2023
@v0d1ch v0d1ch force-pushed the non-mandatory-fuel branch from d6f48d2 to 578e0ef Compare June 13, 2023 07:20
@ch1bo ch1bo mentioned this pull request Jun 13, 2023
11 tasks
@v0d1ch v0d1ch force-pushed the non-mandatory-fuel branch 3 times, most recently from b656071 to 38d4836 Compare June 15, 2023 14:59
@v0d1ch v0d1ch requested review from ch1bo and pgrange June 15, 2023 15:43
@v0d1ch v0d1ch force-pushed the non-mandatory-fuel branch from 38d4836 to ce2d2bf Compare June 15, 2023 15:44
@github-actions
Copy link

github-actions bot commented Jun 15, 2023

Test Results

314 tests  +2   308 ✔️ +2   21m 47s ⏱️ + 2m 11s
106 suites ±0       6 💤 ±0 
    5 files   ±0       0 ±0 

Results for commit 93201bf. ± Comparison against base commit 6ce7cf1.

This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both.
Test.EndToEnd/End-to-end on Cardano devnet/single party hydra head ‑ commits from external wallet
Test.EndToEnd/End-to-end on Cardano devnet/three hydra nodes scenario ‑ inits a Head and closes it immediately 
Hydra.Chain.Direct.Wallet/newTinyWallet ‑ prefers fueled utxo to pay the fees
Hydra.Chain.Direct.Wallet/newTinyWallet ‑ prefers largest utxo if no fuel available
Test.EndToEnd/End-to-end on Cardano devnet/single party hydra head ‑ commits using fuel
Test.EndToEnd/End-to-end on Cardano devnet/three hydra nodes scenario ‑ inits a Head and closes it immediately

♻️ This comment has been updated with latest results.

@ffakenz ffakenz force-pushed the non-mandatory-fuel branch from ce2d2bf to fcbc8de Compare June 15, 2023 18:05
@v0d1ch v0d1ch force-pushed the non-mandatory-fuel branch from fcbc8de to d8ccdc2 Compare June 16, 2023 12:34
@ch1bo ch1bo force-pushed the non-mandatory-fuel branch 4 times, most recently from 90e0518 to 069d3cb Compare June 16, 2023 14:45
@v0d1ch v0d1ch force-pushed the non-mandatory-fuel branch 5 times, most recently from ce7e061 to d55016f Compare June 19, 2023 10:07
@github-actions
Copy link

github-actions bot commented Jun 19, 2023

Transactions Costs

Sizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using arbitrary values and results are not fully deterministic and comparable to previous runs.

Metadata
Generated at 2023-06-20 15:58:26.960232539 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 2212a4ee618434b9b2f366d7c330dbdfb5c7072e793a850fd0de6ddd 4294
νCommit 69e1ccf9ad73dc6d37a5bc8de5aec86f3c4c1710fe5fd334e0e16b18 2124
νHead 8ae095dca4d14a1b8edffb37faa6c84ec60340fbf389a62f027e0b76 9355
μHead 33642a45c7fbb955ce1704ef09229bb211bf9af9980530db28c6aafe* 4148
  • The minting policy hash is only usable for comparison. As the script is parameterized, the actual script is unique per Head.

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 4739 14.69 5.78 0.52
2 4946 15.54 6.05 0.54
3 5152 16.75 6.47 0.56
5 5561 21.49 8.26 0.63
10 6587 35.77 13.73 0.83
37 12124 98.58 37.38 1.75

Cost of Commit Transaction

This is using ada-only outputs for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 600 14.98 5.74 0.34
2 784 19.57 7.70 0.40
3 972 24.66 9.84 0.46
5 1347 36.15 14.59 0.61
10 2287 71.73 28.85 1.04
13 2836 98.03 39.15 1.35

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 814 27.63 10.73 0.49
2 114 1136 43.25 16.93 0.67
3 171 1456 60.99 24.02 0.88
4 226 1775 81.61 32.31 1.12

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 568 16.51 6.89 0.36
2 805 20.27 9.68 0.42
3 969 21.37 10.82 0.44
5 968 20.47 8.73 0.42
10 2132 31.42 19.73 0.64
50 8725 87.17 69.99 1.74

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 709 24.84 10.83 0.46
2 841 26.49 12.01 0.49
3 1014 27.77 13.21 0.51
5 1328 31.63 16.12 0.58
10 2161 40.20 22.97 0.74
45 7937 99.74 70.74 1.82

Cost of Abort Transaction

Some variation because of random mixture of still initial and already committed outputs.

Parties Tx size % max Mem % max CPU Min fee ₳
1 4853 22.40 9.38 0.61
2 5176 36.59 15.53 0.79
3 5496 53.58 22.93 0.99
4 5675 68.57 29.26 1.16
5 5996 90.55 38.85 1.43

Cost of FanOut Transaction

Involves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.

Parties UTxO UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
5 0 0 4764 8.66 3.57 0.46
5 1 56 4799 10.06 4.39 0.47
5 5 285 4941 15.64 7.69 0.55
5 10 570 5128 22.61 11.82 0.64
5 20 1140 5485 36.56 20.07 0.83
5 30 1709 5846 50.52 28.33 1.02
5 40 2275 6206 64.49 36.60 1.21
5 50 2843 6560 78.46 44.87 1.40
5 65 3698 7097 99.42 57.28 1.68

@v0d1ch v0d1ch force-pushed the non-mandatory-fuel branch from be12bdb to 427c953 Compare June 19, 2023 11:42
@ffakenz ffakenz force-pushed the non-mandatory-fuel branch from d64b022 to 1989cd9 Compare June 19, 2023 13:01
hydra-cluster/src/CardanoNode.hs Show resolved Hide resolved
hydra-node/src/Hydra/Chain/Direct/Wallet.hs Outdated Show resolved Hide resolved
sortingCriteria (_, Babbage.BabbageTxOut _ v _ _) = fst' (gettriples' v)
sortedByValue = sortOn sortingCriteria utxosWithDatum
in case sortedByValue of
findFuelOrLargestUTxO :: Map TxIn TxOut -> Maybe (TxIn, TxOut)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should add a note for when deprecating the Fuel?

@ffakenz ffakenz force-pushed the non-mandatory-fuel branch from 75dacb3 to 17bcbc8 Compare June 19, 2023 15:39
@cardano-scaling cardano-scaling deleted a comment from ffakenz Jun 20, 2023
Copy link
Member

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good, but partial functions in a library is something should avoid strongly. Also there are several "should-do's".


Hydra node allows the users to commit utxo using their own wallet. This removes the need to mark some specific utxo as _fuel_ to drive L1 transactions. Now it is the users obligation to provide enough funds for a key specified with the `--cardano-signing-key` hydra-node flag since this key is used to pay for all L1 transactions.

The node provides the http endpoint at the `/commit` path of the internal api server which can accept multiple user utxos (belonging to public key or script address) to commit to a Head after the `Initialization` phase. Hydra-node sends back a draft transaction which is expected to be submitted to a network by the user. Please take a look at the [api documentation](https://hydra.family/head-protocol/api-reference) and specifically `DraftCommitTxRequest/Response` to get more insights.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The information here is correct, but don't feel afraid in changing/incorporating this more with the Fuel section below!

For example, the explanation for fuel per se is still good to have, only difference is that now it does not need to be marked if we use external commits.

Also, there is a an :::info About commits banner below which feels odd to have if we have this section.

Should: update the Fuel section, and maybe move this after it, only adding additional info

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 241389a

@@ -69,6 +69,10 @@ cardano-cli query utxo --address $(cat ./test-head/Alice/AliceCardano.addr) --te
cardano-cli query utxo --address $(cat ./test-head/Bob/BobCardano.addr) --testnet-magic 2
```

:::warning Fuel is deprecated and will be removed in future Hydra versions.
Please take a look at [external-commits](/head-protocol/docs/getting-started/quickstart#external-commits)
:::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a perfect world we would have updated the tutorial already with external commits. But it may be outdated for other reasons now.

Let's keep this part in mind when we write the follow-up of "actually removing fuel"

hydra-cardano-api/src/Cardano/Api/UTxO.hs Outdated Show resolved Hide resolved
hydra-cardano-api/src/Cardano/Api/UTxO.hs Outdated Show resolved Hide resolved
hydra-cardano-api/src/Cardano/Api/UTxO.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/Chain/Direct/Wallet.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/WalletSpec.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/WalletSpec.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/WalletSpec.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/WalletSpec.hs Outdated Show resolved Hide resolved
@ch1bo ch1bo changed the title Remove Fuel Non-mandatory fuel Jun 20, 2023
@ch1bo ch1bo force-pushed the non-mandatory-fuel branch from 7a4048a to ccbab33 Compare June 20, 2023 09:04
ch1bo and others added 13 commits June 20, 2023 11:31
This is enough as the benchmark does fund the internal signing keys of
each hydra-node at the beginning of the scenario.
Also:
- Improve changelog.
- Enhance fuel warnings in docs.
Also move the function to inner where clause where is needed.

This is also updating the property on finding the largest UTxO and
surprisingly it does not pass (even on the previous version) .
This keeps the same errors as they were originally. We still call the
funds on the internal wallet fuel, but we just don't require marking
them anymore.
@ch1bo ch1bo force-pushed the non-mandatory-fuel branch from 2cda4ff to 2e276a3 Compare June 20, 2023 09:31
ch1bo added 4 commits June 20, 2023 12:40
As we are using req now, we can try to get rid of the retrying on
connection failures.

Also drops the dependency to http-conduit as currently unused.
Also remove details about fuel marking from demo instructions
It's easier to not talk about details in all these tutorials, but just
refer to "fueling the hydra node".
Copy link
Member

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated my change requests. Can approve now.

hydra-cluster/test/Test/EndToEndSpec.hs Outdated Show resolved Hide resolved
(HttpExceptionRequest _ (ConnectionFailure _)) -> threadDelay 100_000 >> cont
e -> throwIO e
(Req.VanillaHttpException _) -> threadDelay 100_000 >> cont
(Req.JsonHttpException _) -> threadDelay 100_000 >> cont
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response <- failAfter 3 $ queryNode hydraNodeId
let respBody = Req.responseBody response
when (Req.responseStatusCode response /= 200) $
failure ("Request for Hydra-node metrics failed :" <> show respBody)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Hydra node allows the users to commit utxo using their own wallet. This removes the need to mark some specific utxo as _fuel_ to drive L1 transactions. Now it is the users obligation to provide enough funds for a key specified with the `--cardano-signing-key` hydra-node flag since this key is used to pay for all L1 transactions.

The node provides the http endpoint at the `/commit` path of the internal api server which can accept multiple user utxos (belonging to public key or script address) to commit to a Head after the `Initialization` phase. Hydra-node sends back a draft transaction which is expected to be submitted to a network by the user. Please take a look at the [api documentation](https://hydra.family/head-protocol/api-reference) and specifically `DraftCommitTxRequest/Response` to get more insights.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 241389a

@ch1bo ch1bo merged commit c60d016 into master Jun 20, 2023
@ch1bo ch1bo deleted the non-mandatory-fuel branch June 20, 2023 18:06
ffakenz added a commit that referenced this pull request Jun 23, 2023
- Rename  to .
- Fix witness parsing for  to be really optional.
This also fixes the golden file for DraftCommitTxRequest.
- Define arbitrary instance for ScriptData.
- Complete arbitrary instance for HashableScriptData.
- Remove singlePartyCommitsFromExternal scenario as it was redundant.
In #924 we have made most tests use external commit of Normal outputs.
- use Cannot in a scenario name as it's easier to read and tell apart from Can.
Cant is easily misread.
- Remove unused data constructor SubmitCommitTx.
ffakenz added a commit that referenced this pull request Jun 23, 2023
- Rename DraftUTxO to TxOutWithWitness.
- Fix witness parsing for TxOutWithWitness to be really optional.
This also fixes the golden file for DraftCommitTxRequest.
- Define arbitrary instance for ScriptData.
- Complete arbitrary instance for HashableScriptData.
- Remove singlePartyCommitsFromExternal scenario as it was redundant.
In #924 we have made most tests use external commit of Normal outputs.
- use Cannot in a scenario name as it's easier to read and tell apart from Can.
Cant is easily misread.
- Remove unused data constructor SubmitCommitTx.
- use $ref: "#/components/schemas/Cbor" for Script datum and redemer in api.yaml
v0d1ch pushed a commit that referenced this pull request Jun 23, 2023
- Rename DraftUTxO to TxOutWithWitness.
- Fix witness parsing for TxOutWithWitness to be really optional.
This also fixes the golden file for DraftCommitTxRequest.
- Define arbitrary instance for ScriptData.
- Complete arbitrary instance for HashableScriptData.
- Remove singlePartyCommitsFromExternal scenario as it was redundant.
In #924 we have made most tests use external commit of Normal outputs.
- use Cannot in a scenario name as it's easier to read and tell apart from Can.
Cant is easily misread.
- Remove unused data constructor SubmitCommitTx.
- use $ref: "#/components/schemas/Cbor" for Script datum and redemer in api.yaml
ffakenz added a commit that referenced this pull request Jun 27, 2023
- Rename DraftUTxO to TxOutWithWitness.
- Fix witness parsing for TxOutWithWitness to be really optional.
This also fixes the golden file for DraftCommitTxRequest.
- Define arbitrary instance for ScriptData.
- Complete arbitrary instance for HashableScriptData.
- Remove singlePartyCommitsFromExternal scenario as it was redundant.
In #924 we have made most tests use external commit of Normal outputs.
- use Cannot in a scenario name as it's easier to read and tell apart from Can.
Cant is easily misread.
- Remove unused data constructor SubmitCommitTx.
- use $ref: "#/components/schemas/Cbor" for Script datum and redemer in api.yaml
pgrange pushed a commit that referenced this pull request Jun 27, 2023
- Rename DraftUTxO to TxOutWithWitness.
- Fix witness parsing for TxOutWithWitness to be really optional.
This also fixes the golden file for DraftCommitTxRequest.
- Define arbitrary instance for ScriptData.
- Complete arbitrary instance for HashableScriptData.
- Remove singlePartyCommitsFromExternal scenario as it was redundant.
In #924 we have made most tests use external commit of Normal outputs.
- use Cannot in a scenario name as it's easier to read and tell apart from Can.
Cant is easily misread.
- Remove unused data constructor SubmitCommitTx.
- use $ref: "#/components/schemas/Cbor" for Script datum and redemer in api.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants