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

Incremental decommits off-chain #1223

Merged
merged 51 commits into from
Jan 26, 2024

Conversation

v0d1ch
Copy link
Contributor

@v0d1ch v0d1ch commented Dec 22, 2023

This PR introduces changes related to off-chain decommit protocol logic.
Now user has two options to request a decommit of some UTxO from the open Head:

  • Issue a http request to /decommit endpoint
  • Issue a client input Decommit

Decommit happens in sequence and you can't ask for a decommit while another one is in flight.

It is the user's responsibility to craft a decommit transaction which should be applicable to the local ledger state on L2.

If the transaction applies then the ReqDec network effect is issued and the next Snapshot that is to be signed specifies
which UTxO is to be decommitted from the Head.

On next AckSn all nodes need to agree/sign the snapshot that excludes the decommit UTxO from the ledger state.

Once everybody signs the snapshot decommit tx is issued and upon observing it on L1 it is removed from the local state and another decommit can be processed.


  • 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 Dec 22, 2023
@ch1bo ch1bo self-requested a review January 2, 2024 10:42
@ch1bo ch1bo self-assigned this Jan 2, 2024

-- | Infix version of 'difference'.
(\\) :: UTxO' out -> UTxO' out -> UTxO' out
a \\ b = difference a b
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not used.

feels weird to have to define this function twice.

i would keep only the infix version and define difference in the where clause.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We thought infix version might come in handy. We can certainly remove it if you want.

hydra-cluster/src/Hydra/Cluster/Scenarios.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/API/ServerOutput.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/Chain/Direct/Handlers.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Close.hs Outdated Show resolved Hide resolved
@@ -377,6 +377,42 @@ spec = parallel $ do

waitUntil [n1] $ GetUTxOResponse testHeadId (utxoRefs [2, 42])

it "can request decommit" $
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this test is redundant as it is being covered by the below.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so too so we could remove it.

hydra-node/src/Hydra/Snapshot.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/HeadLogicSnapshotSpec.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/NodeSpec.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@abailly-iohk abailly-iohk left a comment

Choose a reason for hiding this comment

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

I only did a surface review and did not dive deeply in the decommit logic. It overall makes sense to me but take my comments with a grain of salt.

@@ -66,7 +67,7 @@ spec = do
NetworkEffect ReqSn{} -> True
_ -> False
)
let snapshot1 = Snapshot testHeadId 1 mempty []
let snapshot1 = Snapshot testHeadId 1 mempty [] mempty
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we want a smart constructor here :)

@@ -121,7 +122,7 @@ spec = do

it "sends ReqSn when leader and there are seen transactions" $ do
headState <- runEvents bobEnv simpleLedger (inOpenState threeParties simpleLedger) $ do
step (NetworkEvent defaultTTL alice $ ReqSn 1 [])
step (NetworkEvent defaultTTL alice $ ReqSn 1 [] Nothing)
Copy link
Contributor

Choose a reason for hiding this comment

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

and another smart constructor there I think?

hydra-node/test/Hydra/HeadLogicSpec.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/API/ServerOutput.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/API/ServerOutput.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch 2 times, most recently from 6a551d2 to bda2115 Compare January 12, 2024 21:15
@v0d1ch v0d1ch force-pushed the feature/incremental-decommit branch from c00dd75 to f04183a Compare January 14, 2024 19:32
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch 2 times, most recently from 97b2a69 to 815c2f1 Compare January 14, 2024 22:12
@abailly-iohk abailly-iohk mentioned this pull request Jan 16, 2024
16 tasks
@ch1bo ch1bo removed their assignment Jan 17, 2024
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from 43a575f to eefba04 Compare January 17, 2024 18:55
Copy link

github-actions bot commented Jan 17, 2024

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 2024-01-24 12:01:36.355624796 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 985245919fcc6c0c5cd116023cd2c947c43e80dcbb5075fe12433fbb 4072
νCommit 7cb20fa71eb4c563ca283566ebe0aa65859d96c3f8cba35c52c181fd 2043
νHead c23d4bf0cf9f3bc8c4461ca49bcf2ef46e5fb31d6f13cb2bfba5d207 8815
μHead c1491c68117a7fa173c91f39ed21e7aa4c979acb3194cec5f308b244* 3851
  • 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 4378 10.40 4.01 0.46
2 4576 12.68 4.88 0.49
3 4776 14.94 5.74 0.52
5 5179 19.12 7.31 0.59
10 6182 30.43 11.61 0.75
41 12418 99.02 37.59 1.77

Cost of Commit Transaction

This is using ada-only outputs for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 531 11.37 4.44 0.30
2 721 15.04 6.07 0.35
3 912 18.85 7.75 0.40
5 1285 26.90 11.27 0.51
10 2211 49.55 20.97 0.80
19 3905 99.43 41.75 1.43

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 480 21.80 8.52 0.41
2 113 590 32.24 12.75 0.53
3 169 700 44.56 17.78 0.67
4 225 814 60.25 24.15 0.85
5 283 920 78.13 31.46 1.05
6 339 1031 93.32 37.86 1.22

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 568 12.75 5.57 0.32
2 671 13.57 5.94 0.33
3 930 15.34 7.10 0.37
5 1295 17.86 8.59 0.41
10 1520 24.17 12.23 0.50
50 8811 71.09 38.60 1.39

Cost of Contest Transaction

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

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 4307 18.86 8.11 0.55
2 4395 27.08 11.57 0.65
3 4686 47.94 21.03 0.90
4 4732 59.68 26.07 1.03
5 5018 86.92 38.38 1.35

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 4206 8.27 3.46 0.43
5 1 57 4239 9.37 4.17 0.44
5 5 285 4376 14.20 7.18 0.51
5 10 567 4542 20.14 10.91 0.59
5 20 1139 4884 32.65 18.62 0.76
5 30 1710 5227 44.95 26.25 0.93
5 40 2275 5562 57.34 33.92 1.10
5 50 2847 5904 69.78 41.61 1.27
5 74 4212 6717 98.93 59.78 1.68

End-To-End Benchmark Results

This page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest master code.

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 2024-01-24 12:04:18.035711122 UTC

Baseline Scenario

Number of nodes 3
Number of txs 9000
Avg. Confirmation Time (ms) 24.330726389
P99 117.79472145000008ms
P95 33.5762009ms
P50 21.3332945ms
Number of Invalid txs 0

Baseline Scenario

Number of nodes 1
Number of txs 3000
Avg. Confirmation Time (ms) 4.325800727
P99 7.135714899999987ms
P95 5.190308049999995ms
P50 4.1685385ms
Number of Invalid txs 0

@v0d1ch v0d1ch force-pushed the feature/incremental-decommit branch from f04183a to 4381adb Compare January 17, 2024 19:21
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from eefba04 to 1051875 Compare January 17, 2024 19:22
@v0d1ch v0d1ch force-pushed the feature/incremental-decommit branch from 4381adb to b873b97 Compare January 18, 2024 20:45
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from ce1ce0a to 54d84a8 Compare January 18, 2024 20:46
Copy link

github-actions bot commented Jan 19, 2024

Test Results

421 tests   414 ✅  17m 2s ⏱️
138 suites    7 💤
  5 files      0 ❌

Results for commit 73d5576.

♻️ This comment has been updated with latest results.

@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch 2 times, most recently from 834735d to 14d950d Compare January 22, 2024 08:03
@v0d1ch v0d1ch force-pushed the feature/incremental-decommit branch from b873b97 to cc4aba5 Compare January 22, 2024 08:40
ch1bo and others added 8 commits January 22, 2024 09:41
Remove some fixmes but also add one more in HeadLogic to remind us
to change the response to tx instead of UTxO.
Once we figure out exact type we want to return we will resolve FIXME in
the api schema
Add HeadParameters to DecrementTx
Stub out the decrementTx with appropriate params
Rename maybeEmitDecommitApproved to maybeEmitDecrementTx
decrementTx is missing the head redeemer for decrement so this is moving
into on-chain territory a bit.

TODO: Add Decrement redeemer
TODO: How to construct the correct UTxO hash

Only emit DecrementTx if leader
Construct the Decrement redeemer using provided signatures and
utxoHash using the UTxO from confirmed Snapshot.
We want better observability when something goes wrong
It makes sense to have one type for all possible invalid
outcomes.
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from 643c00b to fb69705 Compare January 23, 2024 17:04
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from 56e6d40 to 0dc0e7b Compare January 23, 2024 19:31
We only need to emit ReqSn if we are the leader, other events
should be published
When we receive ReqDec it doesn't make sense to see
DecommitAlreadyInFlight if we didn't request the decommit
in the first place.
Since we hash the utxo to decommit separately we also need another
argument to the verification function.

Also rename Decrement redeemer field to match the others
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from fc080ff to 7dba1f8 Compare January 24, 2024 12:06
Copy link
Collaborator

@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.

I was VERY close to approve this. Only the head logic was slightly off. It's a must change as I think we introduce these changes with this PR and make it behave similar to NewTx / TxInvalid behavior (+ avoid boolean blindness tech debt right away).

hydra-node/src/Hydra/API/ServerOutput.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/API/ServerOutput.hs Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/json-schemas/api.yaml Outdated Show resolved Hide resolved
hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
On ReqDec if there is a decommit tx present in state
just issue RequirementFailure DecommitTxInFlight
@v0d1ch v0d1ch force-pushed the incremental-decommits-off-chain branch from a831ed1 to 32c8fb5 Compare January 24, 2024 14:29
hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/Snapshot.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/Chain/Direct/Tx.hs Show resolved Hide resolved
@v0d1ch v0d1ch merged commit 8f502fc into feature/incremental-decommit Jan 26, 2024
17 of 20 checks passed
@v0d1ch v0d1ch deleted the incremental-decommits-off-chain branch January 26, 2024 09:53
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.

4 participants