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

ReqSn only sends transaction ids #728

Closed
4 of 5 tasks
ch1bo opened this issue Feb 17, 2023 · 8 comments · Fixed by #904
Closed
4 of 5 tasks

ReqSn only sends transaction ids #728

ch1bo opened this issue Feb 17, 2023 · 8 comments · Fixed by #904
Labels
green 💚 Low complexity or well understood feature 💬 feature A feature on our roadmap
Milestone

Comments

@ch1bo
Copy link
Member

ch1bo commented Feb 17, 2023

Why

Users expect high throughput of transactions in the Hydra Head.

Transactions are communicated between Hydra participants through the layer two network. Currently the transactions are submitted twice, once in a ReqTx message and gain in the ReqSn message.

The protocol is defined with both kinds of network messages precisely to increase concurrency with minimal increase in bandwidth demands - which is lost if we submit the transactions twice.

What

  • Not send transactions with ReqSn, but only the transaction ids (hashes of transaction bodies).

    This requires additional book-keeping in the Head protocol:

    • Keep a mapping of TxId -> Tx and extend it on every ReqTx
    • ReqSn should only include a [TxId]
    • Resolve TxId -> Tx before checking their applicability on receiving ReqSn
  • Update specification with new behavior Make the spec consistent with #904 #974

  • Do benchmarks of the off-chain performance before and after this change with a discussion / conclusion on this issue or the respective PR. We have a benchmark on a local cluster of hydra-nodes in hydra-cluster.

  • Test with Hydraw which currently leads to deadlocks when it conflicts

  • Create a documentation page about the L2 consensus and how it behaves in prose (core concepts section). This should touch on topics like liveness, fairness, front-running, etc. Add a FAQ entry about:

    • What happens when a hydra-node is offline? (explain liveness constraint)
    • Does the Hydra L2 consensus prevent front-running?

    And reference the specification section 6.4.2 (currently) for details. Could also use the motivation of the PR Proposed snapshot should contain only seen transactions #904 (comment) as input.

How

  • Likely requires not only a seenTxs, but a allTxs :: Map TxId Tx analogous to what is in the spec
  • We want to only use the seenTxs to resolve transactions, no allTxs as this would be an ever growing set (we might explore having this all txs set separately)
  • Ok, definitely use a allTxs :: Map TxId Tx analogous to what is in the spec otherwise the head is very easily stuck in case of conflicting transactions
  • transactions included in a snapshot should be removed from allTxs
@ch1bo ch1bo added the 💭 idea An idea or feature request label Feb 17, 2023
@pgrange
Copy link
Contributor

pgrange commented Feb 21, 2023

We need to run benchmark with/without this improvement and compare performances to ensure it makes a difference.

@pgrange pgrange added 💬 feature A feature on our roadmap green 💚 Low complexity or well understood feature and removed 💭 idea An idea or feature request labels Feb 21, 2023
@ch1bo
Copy link
Member Author

ch1bo commented Mar 8, 2023

We need to also make sure that we keep track of all transactions (T) to properly handle a ReqSn which requests a snapshot using transactions that are conflicting with our local view (T hat).

Roughly the red items from this draft:

Image

@ch1bo
Copy link
Member Author

ch1bo commented Mar 21, 2023

Marked as an idea again as it

  • requires a clear need by a concrete use case
  • we need to measure the status quo and identify transaction submission to be impacting performance.

@ch1bo ch1bo added 💭 idea An idea or feature request and removed 💬 feature A feature on our roadmap green 💚 Low complexity or well understood feature labels Mar 21, 2023
@ch1bo
Copy link
Member Author

ch1bo commented Jun 21, 2023

After having found a reason to wanting a more "complete" set of requested transactions with #904, we decided to continue on this item.

@ch1bo ch1bo added 💬 feature A feature on our roadmap green 💚 Low complexity or well understood feature and removed 💭 idea An idea or feature request labels Jun 21, 2023
@ch1bo ch1bo added this to the 0.12.0 milestone Jun 21, 2023
@ch1bo ch1bo assigned ch1bo and abailly-iohk and unassigned ch1bo Jun 22, 2023
@abailly-iohk
Copy link
Contributor

Ran a couple benchmarks on 0.11.0 baseline and branch from #904 and the results are similar so there is no apparent improvement to be expected now with this change.

@ch1bo
Copy link
Member Author

ch1bo commented Jul 10, 2023

Ran a couple benchmarks on 0.11.0 baseline and branch from #904 and the results are similar so there is no apparent improvement to be expected now with this change.

An improvement is only expected on networked setups. Our benchmarks usually run local nodes, where there is not really a limited network bandwidth. But it's good that there is no regression.

@abailly
Copy link
Contributor

abailly commented Jul 10, 2023

Sure, but we could have expected some slight speedup due to less work being done in serialisation.

@ch1bo ch1bo reopened this Jul 10, 2023
@ch1bo
Copy link
Member Author

ch1bo commented Jul 21, 2023

Also tested with hydraw where two users spammed to spend the same UTxO with different colors/positions which leads to conflicting transactions (metadata is different at least). The Head just worked fine with this:

image

@ch1bo ch1bo closed this as completed Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
green 💚 Low complexity or well understood feature 💬 feature A feature on our roadmap
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants