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

Commit vs rollbacks #827

Merged
merged 50 commits into from
May 9, 2023
Merged

Commit vs rollbacks #827

merged 50 commits into from
May 9, 2023

Conversation

pgrange
Copy link
Contributor

@pgrange pgrange commented Apr 19, 2023

Fixes #784

☕ Introduces rollbackAndForward on mock chain to simulate rollbacks during model spec executions.

☕ Adds ADR23 to provide a rationale for the following changes.

☕ Removes the high-level handling of rollbacks in HeadLogic, but keeps a LocalChainState in Chain.Direct which is rolled back and kept up-to-date to do consistent chain following. This gets rid of the problematic race condition between setChainState and chainCallback. The chainState in headState is kept for persistency reasons.

☕ Adds tests to HandlersSpec which do cover the newly moved-into-chain-layer rollback logic.

☕ Drops the RolledBack server output as clients cannot do anything about it.

☕ Small refactorings across MockChain, BehaviorSpec, Model modules.


  • CHANGELOG updated
  • Documentation updated
  • Added and/or updated haddocks
  • No new TODOs introduced or explained herafter

@pgrange pgrange changed the title Commit vs rollback Commit vs rollbacks AGAIN Apr 19, 2023
@pgrange pgrange force-pushed the commit_vs_rollback branch 5 times, most recently from f367420 to 55dad07 Compare April 20, 2023 06:54
@pgrange pgrange changed the title Commit vs rollbacks AGAIN Commit vs rollbacks Apr 20, 2023
@github-actions
Copy link

github-actions bot commented Apr 20, 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-05-09 08:49:14.972216807 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial e5cc20df4a2216e706b3d00b59fbb15a7cf12dbd28d271d4a8cf6d04 4336
νCommit 47c102d5f95a0648b4065f2b8bff59d3e34536a82ee7b0d42df73123 2124
νHead 9fe3a5c4d826f9475368e1e24c15bf22f4df19893cce2689d3c0564a 9492
μHead 7bec671467e923281c92e94257931913be106e217270a7b97076cb9b* 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 4743 14.82 5.84 0.52
2 4944 17.61 6.91 0.56
3 5151 18.27 7.09 0.58
5 5562 22.99 8.88 0.64
10 6586 33.25 12.69 0.80
37 12121 97.46 36.92 1.74

Cost of Commit Transaction

Currently only one UTxO per commit allowed (this is about to change soon)

UTxO Tx size % max Mem % max CPU Min fee ₳
1 600 15.41 5.98 0.34

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 817 27.76 10.82 0.49
2 114 1139 43.24 16.99 0.67
3 170 1460 60.60 23.97 0.88
4 226 1783 81.71 32.47 1.12

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 639 18.88 8.43 0.39
2 804 19.98 9.56 0.42
3 969 21.37 10.82 0.44
5 1299 24.16 13.33 0.50
10 2124 31.43 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 675 24.35 10.47 0.45
2 840 26.06 11.84 0.48
3 1015 28.20 13.38 0.52
5 1337 31.20 15.95 0.58
10 2169 40.20 22.97 0.74
44 7771 98.46 69.54 1.79

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 4857 22.19 9.33 0.61
2 5037 32.31 13.55 0.73
3 5360 48.74 20.71 0.93
4 5823 72.84 31.45 1.22
5 6010 89.93 38.73 1.42

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 4765 8.67 3.57 0.46
5 1 57 4805 10.06 4.39 0.47
5 5 285 4944 15.64 7.69 0.55
5 10 568 5119 22.61 11.82 0.64
5 20 1138 5482 36.57 20.08 0.83
5 30 1706 5839 50.53 28.34 1.02
5 40 2276 6202 64.49 36.60 1.21
5 50 2844 6565 78.46 44.87 1.40
5 65 3700 7102 99.42 57.28 1.68

@github-actions
Copy link

github-actions bot commented Apr 20, 2023

Test Results

304 tests  +1   298 ✔️ +1   17m 47s ⏱️ -27s
101 suites +1       6 💤 ±0 
    5 files   ±0       0 ±0 

Results for commit c0c14d2. ± Comparison against base commit 586eb27.

This pull request removes 7 and adds 8 tests. Note that renamed tests count towards both.
Hydra.Behavior/rolling back & forward ‑ does work for rollbacks past init
Hydra.Behavior/rolling back & forward ‑ does work for rollbacks past open
Hydra.Chain.Direct.Handlers ‑ roll forward fails with outdated TimeHandle
Hydra.Chain.Direct.Handlers ‑ roll forward results in Tick events
Hydra.Chain.Direct.Handlers ‑ yields observed transactions rolling forward
Hydra.Chain.Direct.Handlers ‑ yields rollback events onRollBackward
Hydra.HeadLogic/Coordinated Head Protocol ‑ notify user on rollback
Hydra.Behavior/rolling back & forward does not make the node crash ‑ does work for rollbacks past init
Hydra.Behavior/rolling back & forward does not make the node crash ‑ does work for rollbacks past open
Hydra.Chain.Direct.Handlers/LocalChainState ‑ can resume from chain state
Hydra.Chain.Direct.Handlers/chainSyncHanlder ‑ observes transactions onRollForward
Hydra.Chain.Direct.Handlers/chainSyncHanlder ‑ roll forward fails with outdated TimeHandle
Hydra.Chain.Direct.Handlers/chainSyncHanlder ‑ roll forward results in Tick events
Hydra.Chain.Direct.Handlers/chainSyncHanlder ‑ rollbacks state onRollBackward
Hydra.Model ‑ check head opens if all participants commit

♻️ This comment has been updated with latest results.

@ch1bo ch1bo force-pushed the commit_vs_rollback branch 2 times, most recently from 089e8c5 to 1ec9668 Compare April 20, 2023 17:43
ch1bo added a commit that referenced this pull request Apr 22, 2023
@pgrange
Copy link
Contributor Author

pgrange commented Apr 25, 2023

Commit Problems with rollback managemen mainly remove a lot of rollback logic from the head logic. It has an impact on what's been done in #184

This were the requirements from #184:

✅ The hydra-node does not crash when a rollback occurs

This is still ensured by the "rolling back & forward" tests from BehaviorSpec:
We make the chain roll back and forward and then check that we can keep using the node without problem.

It is also ensured by the ModelSpec and the fact that the MockChain now rollbacks and the tests pass.

❌ A ServerOutput is emitted to the client when a rollback occurs

We discovered that the rollback server output is of no use as it is right now. As a client, we get the message rollback but, really, we have no idea what to do with that.

We could easily keep that message if we want but we decided to remove it for now as it's a sort of noise the client wouldn't no how to use. #185

✅ When a not-yet-open head is rolled back, clients can continue opening (i.e. commit, close or abort) the head after the rollback occurred (safety)

The property check head opens if all participants commit in ModelSpec exercises this and proves it's possible for a head to be open in presence of rollbacks.

Although note that we only exercise rollbacks and forward were all the seen transactions will happen again in the fork, we do not consider situations were a transaction will disappear from the fork.

Also, the chainState being rolled back, I could imagine a situation were the head logic would post a collecCom transaction when, from the chainState point of view, not everybody has committed yet. But for some reasons I was not able to reproduce this in the test... maybe I'm missing something here but even if it happens in real life, it should not introduce any security risk for the user. Worst case: a node crashing when trying to open the head?

✅ When an already-open head is rolled back "crossing opening point", it is acceptable that the off-chain communication state is reset and we could continue or contest (no liveliness)

I don't see how this requirement would have been impacted by this change.

@ch1bo
Copy link
Collaborator

ch1bo commented Apr 26, 2023

@pgrange Interesting proposal of not doing rollbacks at all on the HeadState!

The hydra-node does not crash when a rollback occurs

This is not tested by the BehaviorSpec as it only covers the business logic and nothing on the chain layer. The ModelSpec with the MockChain including about 80% of the Direct chain should give higher confidence here.

When a not-yet-open head is rolled back, clients can continue opening [...] Also, the chainState being rolled back, I could imagine a situation were the head logic would post a collecCom transaction when, from the chainState point of view, not everybody has committed yet.

These are the interesting cases and this can certainly happen. I do not think the node will crash though and expect more an error when posting a tx, but this is also happening today when everyone tries to collect after the last commit (which is the behavior on master).

Example scenario:

  • Two party head
  • Rollforward, Init tx -> observed, chain state: Initializing [] -> head state: Initializing []
  • A submits commit tx
  • Rollforward, Commit tx A -> observed, chain state: Initializing [A committed] -> head state: Initializing [A committed]
  • B submits a commit tx
  • Rollback -> chain state: Initializing []
    • Important: Head state unchanged
  • Rollforward, Commit tx B -> observed, chain state: Initializing [B committed] -> head state: Initializing [A committed, B committed]
    • node submits a collect tx
    • submission error because the chain layer cannot construct a collect tx from current state (or the resulting tx is just not valid)
  • Rollforward, Commit tx A -> observed, chain state: Initializing [B committed, A committed] -> head state: Initializing [A committed, B committed]
    • Important: logic needs to be able to handle Commit A even though it already thinks A committed

When an already-open head is rolled back "crossing opening point", it is acceptable that the off-chain communication state is reset and we could continue or contest (no liveliness)

This will be still true, depending in how we will implement this.

  • Before, the logic rolled back the HeadState via the previousRecoverableState to the fork point.
  • Without rollback handling in the logic layer, it depends in how we deal with a OnCommitTx observation when we are in Open head state.
    • A natural way to do interpret this is "The chain is always right", so we would reset the the head state to be Initializing.
      • In this case it is unclear what exactly is in this Initializing state? Shall we try to submit a collect tx right away?
    • Alternatively we could "Hope all head transactions are on the chain" and just do nothing (which does not drive the life cycle forward)

@pgrange pgrange force-pushed the commit_vs_rollback branch 4 times, most recently from 644d8ba to 2bd6fa3 Compare April 26, 2023 17:06
@ghost ghost force-pushed the commit_vs_rollback branch 2 times, most recently from c502618 to 48a5ead Compare April 27, 2023 09:12
@pgrange pgrange marked this pull request as ready for review April 27, 2023 10:35
v0d1ch and others added 25 commits May 9, 2023 10:23
We currently have the slot and the chain state in the rollback event. If
we only use the chain slot and never use the included chainState, the
computation for rolling it back will never be forced and practically the
'rollback' function is not tested that way.
This hides the fact that internally a list of 'ChainStateAt' values is
kept and narrows down the interface of getting the latest chain state,
pushing a new one and rolling back to a previous one.
This is redundant to the already included ChainStateType as every such
chain state needs have a ChainSlot via the IsChainState type class.
- We want to be able to keep the history of chain states.
We may see a rollback by 0 observable blocks, which is okay.
This will correctly allow it to resume from the provided 'ChainStateAt'
even in presence of rollbacks.
ConnectToChain -> SimulatedChainNetwork
TestHydraNode -> TestHydraClient
This will simplify any further refactoring of the mocked chain and
connected nodes are managed internally.
Also refactor flushQueue and inline block creation.
The comment was already saying that this is good practice and indeed in
the given context 'm' can be any IO-ish monad, so we keep the 'threads'
to clean them up in 'stopTheWorld'.
@ch1bo ch1bo merged commit 5642332 into master May 9, 2023
@ch1bo ch1bo deleted the commit_vs_rollback branch May 9, 2023 11:01
@pgrange pgrange added this to the 0.10.0 milestone May 11, 2023
@ch1bo ch1bo removed this from the 0.10.0 milestone May 11, 2023
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.

Commits vs. rollbacks
5 participants