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

Prevent empty decommits #1502

Closed
ch1bo opened this issue Jul 12, 2024 · 0 comments · Fixed by #1534
Closed

Prevent empty decommits #1502

ch1bo opened this issue Jul 12, 2024 · 0 comments · Fixed by #1534
Assignees
Labels
follow-up Leftover work from a bigger item, ideally done in short succession.
Milestone

Comments

@ch1bo
Copy link
Collaborator

ch1bo commented Jul 12, 2024

Follow-up of #1057

Why

After making incremental decommits possible in #1057, we realized that the original specification is not preventing "empty decommit" requests. That means, that anyone can request to decommit nothing. While this is no safety problem, it can lead to annoying situations where one participant spams the head with empty decrements (on the L1).

What

Instead of detecting the spam, we want to prevent this situation altogether by rejecting ReqDec requests whose transactions don't have any outputs.

How

This is a BehaviorSpec test that covers this:

      it "prevents empty decommits" $
        shouldRunInSim $ do
          withSimulatedChainAndNetwork $ \chain ->
            withHydraNode aliceSk [bob] chain $ \n1 ->
              withHydraNode bobSk [alice] chain $ \n2 -> do
                openHead chain n1 n2

                send n1 (Decommit $ SimpleTx 1 mempty mempty)
                waitUntilMatch [n1] $ \case
                  DecommitInvalid{headId} | headId == testHeadId -> True
                  _ -> False

We might want to ensure though that also requests of other nodes are rejected.

@ch1bo ch1bo mentioned this issue Jul 12, 2024
4 tasks
@v0d1ch v0d1ch self-assigned this Jul 16, 2024
@ch1bo ch1bo added this to the 0.18.0 milestone Jul 22, 2024
@ch1bo ch1bo added the follow-up Leftover work from a bigger item, ideally done in short succession. label Jul 23, 2024
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
This was referenced Jul 24, 2024
@ch1bo ch1bo closed this as completed in 2f4ac12 Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up Leftover work from a bigger item, ideally done in short succession.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants