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

Tune snapshot generation in TxTrace tests #1524

Closed
ch1bo opened this issue Jul 23, 2024 · 1 comment · Fixed by #1695
Closed

Tune snapshot generation in TxTrace tests #1524

ch1bo opened this issue Jul 23, 2024 · 1 comment · Fixed by #1695
Assignees
Labels

Comments

@ch1bo
Copy link
Collaborator

ch1bo commented Jul 23, 2024

Follow-up of #1057

Why

When implementing incremental decommits in #1344, we significantly increased the complexity of snapshots used in the TxTrace tests and switched from a "ahead of time" to a "just in time" generation of snapshots to decrement, close and contest a head with.

This resulted in complicated generation and lots of filtering through preconditions, making the tests less efficient and hard to maintain.

Also, the generated traces are not covering all "interesting" situations within reasonable max successes and we needed to disable checkCoverage on prop_traces.

What

  • Have less actions discarded than actually tested: currently 1.6k discards on 100 runs with only 790 valid actions tested.
  • Ensure interesting cases are generated with lower sizes in prop_traces, re-enable checkCoverage and use the same number of tests in the prop_runActions
  • Reduce "number of moving pieces" and improve maintainability where possible

How

  • Generate snapshots separate from using them, i.e. Decrement, Close and Contest just pick from an available list of snapshots in the model
  • Try to get rid of most predicates that are in both, precondition and validFailingAction -> this reduces discards
@ch1bo ch1bo added the follow-up Leftover work from a bigger item, ideally done in short succession. label Jul 23, 2024
@ch1bo ch1bo added this to the 0.18.0 milestone Jul 23, 2024
@ch1bo ch1bo mentioned this issue Jul 23, 2024
4 tasks
ch1bo added a commit that referenced this issue Jul 23, 2024
This PR adds "incremental decommits" to the Hydra Head protocol, which
allows users to take funds out of an open Head.

- New API endpoint `/decommit` which accepts a "Decommit transaction",
that spends some UTxO and whatever outputs it produces will be made
available on the L1. This can be also done through a new `Decommit`
client input and new server outputs `DecommitRequested`,
`DecommitApproved` and `DecommitFinalized`, as well as `DecommitInvalid`
to inform about status of the decommit.

- Decommits are first approved in a snapshot on L2 via a new network
message `ReqDec`, before a new `decrementTx` can be posted and observed
on-chain.

- Only one decommit can be processed at a given time.

- Update documentation and added how-to about how to use this.

- Acknowledged specification changes by "clearing" of
$\textcolor{red}{\\red}$ areas covered by this implementation in the
specification.

- End-to-end test covering the main scenario of decommitting funds.

- Added mutation tests for Decrement, Close and Contest to cover all
on-chain-verification changes.

- Enhanced `TxTrace` tests to test decrements with various snapshots and
their interaction with close/contest and fanout of a head.

---

* [x] CHANGELOG updated
* [x] Documentation updated
* [x] Haddocks updated
* [x] New TODOs explained hereafter


![image](https://github.com/user-attachments/assets/eed47f06-d519-42cb-a897-98397066fdd9)

  - Two FIXMEs covered by #1524
  - TODO in HeadLogic coverd by #1502
- TODO in tx-cost how we could improve the benchmark output (not
crucial)
- TODO in head logic about rollbacks .. actually something we need to
consider with #199 too
@ch1bo ch1bo removed this from the 0.18.0 milestone Jul 30, 2024
@ch1bo ch1bo added red bin and removed follow-up Leftover work from a bigger item, ideally done in short succession. labels Sep 10, 2024
@ch1bo ch1bo self-assigned this Oct 8, 2024
@ch1bo ch1bo linked a pull request Oct 9, 2024 that will close this issue
4 tasks
@ch1bo
Copy link
Collaborator Author

ch1bo commented Oct 10, 2024

Progressed #1695 today, but not complete yet. Having the snapshots generated as actions and selecting from them to do Close etc. feels more sane to me.

Kept logs in the logbook: https://github.com/cardano-scaling/hydra/wiki/Logbook-2024-H1#sn-on-txtrace-tuning--rework

@ch1bo ch1bo mentioned this issue Oct 27, 2024
4 tasks
v0d1ch added a commit that referenced this issue Oct 30, 2024
Rewrite of the model part and tuning of the TxTrace stateful
property-based test suite comprised of:

* Allow for empty decrements, but identify `DecrementValueNegative`
errors when constructing transactions.

* Added coverage to model test `TxTrace/all valid transactions`

* Changed how snapshots are generated and modeled: Instead of generating
snapshots ad-hoc, we create them in a dedicated `NewSnapshot` action and
only pick from a list of `knownSnapshots` when generating `Close` and
other actions. This resembles better what is actually happening in the
real world, where the adversary could only pick from a list of plausible
snapshots. It also makes the `arbitraryAction` simpler and requires less
`precondition`s. Lastly, this also results in less discarded values as
was the original motivation in #1524

---

<!-- Consider each and tick it off one way or the other -->
* [x] CHANGELOG update not needed
* [x] Documentation update not needed
* [x] Haddocks updated
* [x] No new TODOs introduced or explained herafter
- Three very minor `XXX` comments how further refactoring could be done
in `TxTrace`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant