Skip to content

Fix obseration of deposits to be settled #1978

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

ch1bo
Copy link
Member

@ch1bo ch1bo commented Apr 30, 2025

This is will be the last PR related to deposits that also fixes the "deposit used too early" problem identified in course of #1951 and related discussions. With all the prior work already done in #1969, #1974 and the refactorings of #1977 this was fairly straight forward.

⏰ Breaking change on the network as I needed to change the ReqSn message.

⏰ Needed to substantially increase the defaultTTL of re-enqueing inputs. This is needed because the ReqSn handling will wait for the deposit to become active (passes the --deposit-period) and needs to act on the ReqSn only then. This is problematic because the input queue is not (yet?) persisted and restarting the node will lose that state -> the deposit will become active, but an AckSn would not be sent. See #1999 for a follow-up to address this.

⏰ The fact that we observe a SlotNo in DepositObservation requires a TimeHandle to convert it further to a UTCTime (in convertObservation). As the hydra-chain-observer does not have a TimeHandle, I decided to switch to using HeadObservation (the type from hydra-tx) instead of OnChainTx (the type from hydra-node) for the chain observer / hydra-explorer interface. This results in quite a lot of (backwards compatible) changes and this companion PR: cardano-scaling/hydra-explorer#47

TODO:

  • spec changes
  • make chain observers not break the explorer API

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

@ch1bo ch1bo added the stacked Stacked diff. Review it, but leave merging to the author. label Apr 30, 2025
@ch1bo ch1bo added this to the 0.22.0 milestone Apr 30, 2025
@ch1bo ch1bo self-assigned this Apr 30, 2025
Copy link

github-actions bot commented Apr 30, 2025

Transaction cost differences

No cost or size differences found

@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from c595e1d to 6b3dbf0 Compare April 30, 2025 21:55
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from c5206a0 to 21ac957 Compare April 30, 2025 21:56
@ch1bo ch1bo linked an issue Apr 30, 2025 that may be closed by this pull request
Copy link

github-actions bot commented Apr 30, 2025

Transaction 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 2025-05-16 12:39:32.503122602 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial c8a101a5c8ac4816b0dceb59ce31fc2258e387de828f02961d2f2045 2652
νCommit 61458bc2f297fff3cc5df6ac7ab57cefd87763b0b7bd722146a1035c 685
νHead be6ebc744208c660bf0fdc1cfbb5157477cd305de5b1777e575cbb4c 14665
μHead 1f47a42d1d6edc32ccd834acb19d5db3b2a5232f0bd7eaa8908dc519* 5284
νDeposit ae01dade3a9c346d5c93ae3ce339412b90a0b8f83f94ec6baa24e30c 1102
  • The minting policy hash is only usable for comparison. As the script is parameterized, the actual script is unique per head.

Init transaction costs

Parties Tx size % max Mem % max CPU Min fee ₳
1 5836 10.35 3.28 0.51
2 6037 12.79 4.06 0.55
3 6238 15.02 4.77 0.58
5 6640 18.85 5.96 0.64
10 7646 28.97 9.13 0.79
43 14281 98.73 30.85 1.80

Commit transaction costs

This uses ada-only outputs for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 561 2.44 1.16 0.20
2 742 3.38 1.73 0.22
3 923 4.36 2.33 0.24
5 1282 6.41 3.60 0.28
10 2183 12.13 7.25 0.40
54 10065 98.61 68.52 1.88

CollectCom transaction costs

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 525 24.43 7.12 0.42
2 114 636 34.24 9.85 0.53
3 170 747 43.81 12.53 0.63
4 226 858 52.43 15.00 0.72
5 282 969 63.05 17.94 0.83
6 337 1081 66.04 19.08 0.87
7 394 1192 75.42 21.86 0.97
8 452 1307 96.42 27.16 1.18

Cost of Increment Transaction

Parties Tx size % max Mem % max CPU Min fee ₳

Cost of Decrement Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 618 22.46 7.26 0.41
2 689 22.55 7.92 0.42
3 930 26.92 9.83 0.48
5 1244 31.03 12.31 0.54
10 1926 38.35 17.68 0.67
42 6697 98.15 55.64 1.64

Close transaction costs

Parties Tx size % max Mem % max CPU Min fee ₳
1 657 29.00 9.25 0.48
2 740 30.15 10.16 0.50
3 914 32.59 11.68 0.54
5 1340 35.42 14.19 0.60
10 2090 45.13 20.75 0.76
35 5560 96.29 53.45 1.56

Contest transaction costs

Parties Tx size % max Mem % max CPU Min fee ₳
1 678 33.70 10.50 0.53
2 769 35.02 11.47 0.55
3 997 38.37 13.36 0.60
5 1320 42.98 16.20 0.67
10 2004 53.77 22.90 0.84
30 5086 99.74 51.04 1.55

Abort transaction costs

There is some variation due to the random mixture of initial and already committed outputs.

Parties Tx size % max Mem % max CPU Min fee ₳
1 5719 26.89 9.00 0.68
2 5794 34.67 11.56 0.77
3 5927 41.05 13.70 0.84
4 6202 55.86 18.76 1.01
5 6319 65.07 21.81 1.11
6 6385 70.19 23.54 1.17
7 6534 83.54 28.08 1.32
8 6636 91.70 30.74 1.40
9 6696 93.19 31.16 1.42

FanOut transaction costs

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 ₳
10 0 0 5834 18.31 6.11 0.60
10 5 285 6005 29.36 10.43 0.73
10 10 568 6172 39.08 14.30 0.84
10 20 1138 6512 59.13 22.23 1.07
10 30 1708 6854 79.83 30.38 1.32
10 39 2220 7159 98.55 37.75 1.53

End-to-end benchmark results

This page is intended to collect the latest end-to-end benchmark results produced by Hydra's continuous integration (CI) system from the latest master code.

Please note that these results are approximate as they are currently produced from limited cloud VMs and not controlled hardware. Rather than focusing on the absolute results, the emphasis should be on relative results, such as how the timings for a scenario evolve as the code changes.

Generated at 2025-05-16 12:42:02.620486007 UTC

Baseline Scenario

Number of nodes 1
Number of txs 300
Avg. Confirmation Time (ms) 4.260731913
P99 7.124907779999992ms
P95 5.13926845ms
P50 4.092054ms
Number of Invalid txs 0

Memory data

Time Used Free
2025-05-16 12:40:45.656502164 UTC 747M 6720M
2025-05-16 12:40:50.65640435 UTC 854M 6507M
2025-05-16 12:40:55.656417267 UTC 858M 6503M
2025-05-16 12:41:00.656490512 UTC 860M 6501M
2025-05-16 12:41:05.65635288 UTC 863M 6497M
2025-05-16 12:41:10.656477236 UTC 862M 6497M

Three local nodes

Number of nodes 3
Number of txs 900
Avg. Confirmation Time (ms) 27.668660647
P99 42.23505003ms
P95 35.9825574ms
P50 26.752069499999998ms
Number of Invalid txs 0

Memory data

Time Used Free
2025-05-16 12:41:24.603506932 UTC 784M 6611M
2025-05-16 12:41:29.603940939 UTC 1008M 6308M
2025-05-16 12:41:34.605206206 UTC 1068M 6191M
2025-05-16 12:41:39.603790745 UTC 1079M 6129M
2025-05-16 12:41:44.603782638 UTC 1085M 6123M
2025-05-16 12:41:49.603897424 UTC 1085M 6121M
2025-05-16 12:41:54.603829743 UTC 1086M 6120M
2025-05-16 12:41:59.603782597 UTC 1090M 6116M

@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from 6b3dbf0 to 6c3443b Compare May 5, 2025 09:21
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 21ac957 to 7c5396a Compare May 5, 2025 09:22
Copy link
Contributor

@v0d1ch v0d1ch left a comment

Choose a reason for hiding this comment

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

It feels like we should make sure we all understand the impact of these recent changes especially if you will not be around so much in the following days. I find it personally hard to review large PRs (previous stacked PR) without having a dedicated session with the author (which is fine since you plan to do that today)

@@ -102,7 +102,7 @@ import Hydra.Tx.Party (Party (vkey))
import Hydra.Tx.Snapshot (ConfirmedSnapshot (..), Snapshot (..), SnapshotNumber, SnapshotVersion, getSnapshot)

defaultTTL :: TTL
defaultTTL = 5
defaultTTL = 10000
Copy link
Member Author

Choose a reason for hiding this comment

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

Must: only have one defaultTTL

waitDelay :: DiffTime
waitDelay = 0.1
waitDelay = 1
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not good, we should at least make this wait configurable different for waits "on L2 stuff" vs. the wait for deposits to become active.

However, we should address the situation of protocol logic needing to wait in the first place!

@ch1bo
Copy link
Member Author

ch1bo commented May 6, 2025

In course of a discussion session we identified that even on master we have an issue with too low wait times in at least this situation:

image

When a party that is not the snapshot leader requests a decrement via ReqDec, there must not be a commit/increment $U_\alpha$ pending. Even without the changes of this PR, increments may take several seconds (~20s blocks on mainnet) while the defaultTTL and delay between retries upon a wait outcome results in a maximum wait time of 0.5s.

Hence this PR does not worsen the situation, but we should make the Wait outcome parameterizable so we can indicate how to long keep an event in the queue. Also, a follow-up issue / PR could investigate whether switching the waitDeposit in ReqSn handling deliberately to a require and retry snapshot requests on each Tick would be a path forward.

In general, the protocol logic feels more and more contrived and sending requests that may need to be re-enqueued is not very failure resistant / good practice in distributed systems.

@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from 6c3443b to 7d2ba58 Compare May 12, 2025 06:42
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 0b947c4 to ea1843f Compare May 12, 2025 06:44
@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from 7d2ba58 to b8d4bd0 Compare May 12, 2025 06:51
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from ea1843f to 7fbdf00 Compare May 12, 2025 06:51
@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from b8d4bd0 to f1b65fe Compare May 12, 2025 07:05
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 7fbdf00 to 8848846 Compare May 12, 2025 07:05
@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from f1b65fe to 6a4b5b4 Compare May 12, 2025 07:11
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 8848846 to 8a91c86 Compare May 12, 2025 07:12
@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from 6a4b5b4 to 6d6eff1 Compare May 12, 2025 12:42
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 8a91c86 to dc51e30 Compare May 12, 2025 12:45
@ch1bo ch1bo force-pushed the deposit-bonus-contestation-period branch from 6d6eff1 to c0c67f8 Compare May 12, 2025 12:48
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch 2 times, most recently from bf66f8f to ed63321 Compare May 12, 2025 14:08
ch1bo added 10 commits May 15, 2025 12:07
This will mark the creation of the deposit.
This changes the convertObservation function to require a TimeHandle and
will make chain observers need to use a different type than OnChainTx.
This handles the created time on Deposit to determine whether it is
still inactive or can be requested / signed upon a snapshot request.

Importantly, the snapshot signing must be able to wait for the deposit
to become active and then submit snapshot acknowledgement. To that end,
the TTL and time between re-enqueuing waits is greatly increased now.
This was helpful during debugging (until we have a proper GET /deposits).
This will allow chain observers to use these valueu directly without
further conversion to 'OnChainTx'. The latter will be the hydra-node
specific type which contains already converted time fields.

The change here is incomplete as HeadObservation is missing several
instances and breaks the explorer API.
The note about observing without using resolved inputs was outdated and
the approach now was deemed fine in the past (contrary to the wording
used). See commit 91d48d8
The Direct.State module is lots of dead code. The OpenState was not
actually using the OpenThreadOutput other than for getKnownUTxO.
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch 2 times, most recently from 9c785aa to af53252 Compare May 15, 2025 10:50
Base automatically changed from deposit-bonus-contestation-period to master May 15, 2025 12:41
@ch1bo ch1bo removed the stacked Stacked diff. Review it, but leave merging to the author. label May 15, 2025
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from efe0f7d to 2f999a5 Compare May 15, 2025 20:20
ch1bo added 9 commits May 15, 2025 22:21
They are now only used as parameters to close and contest transaction
creation respectively.
Turns out the contestedThreadOutput was not used at all
This allows us to define a backwards compatible JSON parser only using
the HeadObservation type.
We want a serialization that resembles OnChainTx so the hydra-explorer
is backwards compatible (tested there).
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 2f999a5 to cfa4a0e Compare May 15, 2025 20:21
@ch1bo ch1bo requested a review from v0d1ch May 15, 2025 20:22
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from cfa4a0e to 15b6372 Compare May 15, 2025 20:26
@ch1bo ch1bo force-pushed the deposit-protocol-fix branch from 15b6372 to 68acfb5 Compare May 16, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress 🕐
Development

Successfully merging this pull request may close these issues.

2 participants