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

Proposed snapshot should contain only seen transactions #904

Merged
merged 22 commits into from
Jul 7, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 3, 2023

The motivation for this work comes from the idea of packing multiple transactions in a ReqTx (see #900) to guarantee the atomicity of a sequence of txs. Thinking about this feature, I realised that:

  1. To enforce atomicity, we would need the snapshot to contain not only individual tx but actually the ReqTx, plus the proposed new UTxO set
  2. Our current head logic says nothing about the txs part of a snapshot beyond that they should be applicable to the current ledger.
  3. It's therefore impossible to implement the feature and more importantly it's super easy for any node to front-run other nodes: When it's your turn to make a snapshot, just look at the proposed txs pick and chose the one you wants, and add more applicable txs of your liking. In the context of a market, or an auction perhaps, this makes it trivial to cheat as far as I understand it.

Therefore, I initially thought we should augment the protocol to (at least) require that txs in a snapshot have been previously seen in a ReqTx. But this is in essence what #728 implies as this implies the only way to properly apply a transaction from the ReqSn propagating only TxId is to have seen it before and be able to resolve it.

Therefore, the goal of this PR is to fix #728


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

@ghost ghost requested review from ch1bo, ffakenz and pgrange June 3, 2023 09:50
@github-actions
Copy link

github-actions bot commented Jun 3, 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-07-07 08:34:21.373022577 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 2212a4ee618434b9b2f366d7c330dbdfb5c7072e793a850fd0de6ddd 4294
νCommit 69e1ccf9ad73dc6d37a5bc8de5aec86f3c4c1710fe5fd334e0e16b18 2124
νHead 8ae095dca4d14a1b8edffb37faa6c84ec60340fbf389a62f027e0b76 9355
μHead 33642a45c7fbb955ce1704ef09229bb211bf9af9980530db28c6aafe* 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 4739 14.79 5.83 0.52
2 4946 16.80 6.58 0.55
3 5156 19.69 7.68 0.59
5 5562 22.87 8.82 0.64
10 6586 33.70 12.88 0.81
37 12121 96.71 36.60 1.73

Cost of Commit Transaction

This is using ada-only outputs for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 599 14.98 5.74 0.34
2 787 19.66 7.73 0.40
3 972 24.75 9.88 0.46
5 1342 36.15 14.59 0.61
10 2286 71.73 28.85 1.04
13 2844 98.03 39.15 1.35

Cost of CollectCom Transaction

Parties UTxO (bytes) Tx size % max Mem % max CPU Min fee ₳
1 57 815 27.93 10.85 0.49
2 114 1136 43.24 16.93 0.67
3 170 1455 61.98 24.42 0.89
4 227 1783 81.89 32.42 1.12

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 639 18.88 8.42 0.39
2 804 20.27 9.68 0.42
3 969 21.67 10.94 0.45
5 1296 24.16 13.33 0.50
10 2125 31.12 19.62 0.64
50 8726 86.87 69.87 1.74

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 710 24.84 10.83 0.46
2 849 26.06 11.84 0.48
3 1007 27.77 13.21 0.51
5 1335 31.63 16.12 0.58
10 2169 39.77 22.80 0.73
44 7773 98.03 69.37 1.78

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 4855 22.40 9.38 0.61
2 5177 36.68 15.56 0.79
3 5497 53.34 22.83 0.99
4 5676 68.57 29.26 1.16
5 6137 96.09 41.48 1.50

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 4764 8.66 3.57 0.46
5 1 57 4801 10.06 4.39 0.47
5 5 284 4945 15.64 7.69 0.55
5 10 569 5123 22.61 11.82 0.64
5 20 1140 5484 36.56 20.07 0.83
5 30 1708 5845 50.52 28.33 1.02
5 40 2277 6204 64.49 36.60 1.21
5 50 2845 6566 78.46 44.87 1.40
5 65 3701 7106 99.42 57.28 1.68

Copy link
Member

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

Therefore, I think we should augment the protocol to (at least) require that txs in a snapshot have been previously seen in a ReqTx.

In the original protocol a ReqSn was only ever requesting transactions to be snapshotted by id. We (over-)simplified this to have snapshot requests include the transactions directly. This is definitely a performance problem, but maybe you are on to something bigger here? The issue #728 would tackle this.

it's super easy for any node to front-run other nodes

How? Any node would apply the transaction once seen to their seen ledger and not agree with another transaction spending the same input, but differently. Or is this maybe a use case where the thing to be "raced on" is not represented as a UTxO being spent?

hydra-node/test/Hydra/HeadLogicSpec.hs Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jun 5, 2023

The problem I am seeing is not related to performance, only to the fact that a node can receive a snapshot containing arbitrary transactions, even ones it has not yet seen, as long of course as that snapshot makes sense with the latest known one. This makes it possible for a node to "front-run" another node by: 1/ receiving a ReqTx tx that some other party posted then 2/ craft a ReqSn not containing this tx but another tx' that "front-runs" tx, for example by consuming some UTxO with a smart contract for swaps, or a bid for auction, or something else.

Perhaps this is not a real problem? As explained in the PR's description this dawned on me while trying to think about how a node could propose "atomic" transaction sets and how to enforce that in the protocol.

@ch1bo
Copy link
Member

ch1bo commented Jun 5, 2023

Perhaps this is not a real problem?

No, I think you are right and this is a real problem. Which makes #728 much more important!

@pgrange
Copy link
Contributor

pgrange commented Jun 5, 2023

Imagine B want to be evil and sign snapshot with its own not submitted transaction instead of another legit one. Suppose it's B's turn to be the leader. I understand that the concern of this issue would be to prevent the following scenario to occur:

  1. A submits transaction Ta
  2. B ignores Ta
  3. B ask for the signature of a new snapshot including transaction Tb not yet submitted
  4. A signs the snapshot

If we implement this issue, now A will only sign the snapshot if it has seen Tb before receive the snapshot. I can see the following scenario leading to the same result as before:

  1. A submits transaction Ta
  2. B ignores Ta
  3. B submit transaction Tb
  4. B ask for the signature of a new snapshot including transaction Tb just submitted
  5. A signs the snapshot

Are these scenario correct in the context of this issue or did I miss something?

@ghost
Copy link
Author

ghost commented Jun 9, 2023

@pgrange You are right, this issue exposes only part of the problem of the lack of fairness in the Hydra consensus protocol. But a big difference with the previous situation is that if A has seen both Ta and Tb this means they are both compatible with the lates confirmed snapshot so it guarantees that when A turns to sign snapshot comes, it will be able to include Ta.

@ghost ghost mentioned this pull request Jun 11, 2023
4 tasks
@ghost ghost force-pushed the abailly-iohk/snapshot-seen-txs-only branch 2 times, most recently from b92e7da to 6c0f8f4 Compare June 20, 2023 18:12
@ghost ghost marked this pull request as ready for review June 20, 2023 18:20
@ghost ghost requested a review from ch1bo June 20, 2023 18:20
@ghost ghost force-pushed the abailly-iohk/snapshot-seen-txs-only branch from d43b5e8 to 86654d8 Compare June 20, 2023 19:28
@github-actions
Copy link

github-actions bot commented Jun 20, 2023

Test Results

342 tests  +7   336 ✔️ +7   27m 6s ⏱️ + 4m 1s
114 suites +1       6 💤 ±0 
    6 files   ±0       0 ±0 

Results for commit 2511d45. ± Comparison against base commit 14db869.

This pull request removes 1 and adds 8 tests. Note that renamed tests count towards both.
Hydra.HeadLogic/Coordinated Head Protocol ‑ waits if we receive a snapshot with not-yet-seen transactions
Hydra.Crypto/MultiSignature ‑ aggregateInOrder/verifyMultiSignature roundtrip
Hydra.HeadLogic/Coordinated Head Protocol ‑ waits if we receive a snapshot with transaction not applicable on previous snapshot
Hydra.HeadLogic/Coordinated Head Protocol ‑ waits if we receive a snapshot with unseen transactions
Hydra.HeadLogic/Coordinated Head Protocol/Tracks Transaction Ids ‑ keeps transactions in allTxs given it receives a ReqSn
Hydra.HeadLogic/Coordinated Head Protocol/Tracks Transaction Ids ‑ keeps transactions in allTxs given it receives a ReqTx
Hydra.HeadLogic/Coordinated Head Protocol/Tracks Transaction Ids ‑ removes transactions from allTxs when included in a acked snapshot
Hydra.HeadLogic/Coordinated Head Protocol/Tracks Transaction Ids ‑ removes transactions from allTxs when ttl expires
Hydra.Node ‑ signs snapshot even if it has seen conflicting transactions

♻️ This comment has been updated with latest results.

Copy link
Member

@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 think this is a protocol change in the direction where we actually want to be (as also described in #728).

Not sure if it's worthwhile to stop half-way and not do the full switch into ReqSn only holds txId. That would be more intuitive, I guess, as then the resolution from TxId -> Tx is naturally implying the check we do here manually now.

I can approve this change, but would be more comfortable if we would have researchers also think about this (they proposed the only send TxId with lookup of TxId -> Tx in the past, so it should be fine as this is semantically the same though).

Requested change is about updating figure 10 in the spec accordingly.

spec/offchain.tex Outdated Show resolved Hide resolved
@ch1bo ch1bo mentioned this pull request Jun 21, 2023
5 tasks
@ghost
Copy link
Author

ghost commented Jun 21, 2023

After discussing this with @ch1bo it seems to make more sense to bite the bullet and implement the "proper" ReqSn, eg. send only transaction ids

@ghost ghost marked this pull request as draft June 21, 2023 08:04
@ghost ghost self-assigned this Jun 21, 2023
@ghost ghost added the L2 Affect off-chain part of the Head protocol/network label Jun 21, 2023
@ghost ghost force-pushed the abailly-iohk/snapshot-seen-txs-only branch 2 times, most recently from 8283a9c to 524724b Compare June 22, 2023 06:17
@ghost ghost marked this pull request as ready for review June 22, 2023 06:17
@ghost ghost requested a review from ch1bo June 22, 2023 06:17
Copy link
Member

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

Wow this was quick! This only requires updating the spec now IMO (more than the one section I commented)

hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
hydra-node/src/Hydra/HeadLogic.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
spec/fig_offchain_prot.tex Show resolved Hide resolved
spec/offchain.tex Outdated Show resolved Hide resolved
@ghost ghost requested a review from ch1bo June 23, 2023 08:25
@ghost ghost force-pushed the abailly-iohk/snapshot-seen-txs-only branch from 0fb2c1e to f4debad Compare July 6, 2023 09:37
pgrange and others added 6 commits July 6, 2023 21:48
To prepare for #903 we want to be able to combine outcomes
of type `Wait` and `NewState`.

This commit allows for that and provide a more cohesive
Outcome algebra.
This test is currently failing on purpose. The motivation for this
work comes from the idea of packing multiple transactions in a ReqTx
to guarantee the atomicity of a sequence of txs. Thinking about this
feature, I realised that:

1. To enforce atomicity, we would need the snapshot to contain not
only individual tx but actually the ReqTx, plus the proposed new UTxO
set
2. Our current head logic says nothing about the txs part of a
snapshot beyond that they should be applicable to the current ledger.
3. It's therefore impossible to implement the feature and more
importantly it's super easy for any node to front-run other nodes:
When it's your turn to make a snapshot, just look at the proposed txs
pick and chose the one you wants, and add more applicable txs of your
liking. In the context of a market, or an auction perhaps, this makes
it trivial to cheat as far as I understand it.

Therefore, I think we should augment the protocol to (at least)
require that txs in a snapshot have been previously seen in a ReqTx.
@ghost ghost force-pushed the abailly-iohk/snapshot-seen-txs-only branch from 1b5b763 to 2cfe136 Compare July 7, 2023 02:58
@ghost ghost force-pushed the abailly-iohk/snapshot-seen-txs-only branch from 2cfe136 to 62d2bd3 Compare July 7, 2023 06:28
hydra-node/test/Hydra/CryptoSpec.hs Show resolved Hide resolved
hydra-node/src/Hydra/HeadLogic.hs Show resolved Hide resolved
- When trying to apply a tx upon ReqTx, if the tx is not applicable and
TTL has passed we are removing the transaction from the known hydra
state.
@v0d1ch v0d1ch merged commit 2796383 into master Jul 7, 2023
@v0d1ch v0d1ch deleted the abailly-iohk/snapshot-seen-txs-only branch July 7, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L2 Affect off-chain part of the Head protocol/network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReqSn only sends transaction ids
5 participants