-
Notifications
You must be signed in to change notification settings - Fork 87
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
Use transaction ids in snapshots #922
Conversation
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Script summary
Cost of Init Transaction
Cost of Commit TransactionThis is using ada-only outputs for better comparability.
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort TransactionSome variation because of random mixture of still initial and already committed outputs.
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
|
Test Results312 tests - 11 306 ✔️ - 11 20m 17s ⏱️ -14s Results for commit 95948ae. ± Comparison against base commit 4a27e4c. This pull request removes 11 tests.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update the TUI if we want it to work with this change:
hydra-tui: ExceptionInLinkedThread (ThreadId 6) (ClientJSONDecodeError "Error in $.snapshot.confirmedTransactions[0]: parsing TxId failed, expected String, but encountered Object" "{\"headId\":\"b2e49bacf4bd815db4dab65ee9591e918cedf3c3adf24f6add212360\",\"seq\":9,\"signatures\":{\"multiSignature\":[\"fa91a6bd45e2805d4722deac4564d0a1a2077a0506538843edb235596721ddfd2733c916eeaaddddabb4d530043473341efc071086e87a94d77e62d89f6ba502\",\"323298b0c3b13a4f7c856440eefe4b1380519e86fc8ab6edac7d825e03ce0e2ed3614f233c982ba34dc581f8fe922cbb7c8dfd538c7754264e0e84e272fa380a\",\"2e70bfbf59368c1eac077e13f25bab60f6e2b8b2c5990b34b2045aed3789c36be19d9b5ee893880b907d712fd71c315e192a89b31a2d9032232544ad04924a0c\"]},\"snapshot\":{\"confirmedTransactions\":[{\"body\":{\"fees\":0,\"inputs\":[\"ddf1db5cc1d110528828e22984d237b275af510dc82d0e7a8fc941469277e31e#0\"],\"outputs\":[{\"address\":\"addr_test1vqg9ywrpx6e50uam03nlu0ewunh3yrscxmjayurmkp52lfskgkq5k\",\"datum\":null,\"datumhash\":null,\"inlineDatum\":null,\"referenceScript\":null,\"value\":{\"lovelace\":1000000000}}]},\"id\":\"2ee2cb485d7ceda00a1f9f8fc9eac0be46cc03011ec0177c008160389f6145d8\",\"isValid\":true,\"witnesses\":{\"keys\":[\"8200825820eb94e8236e2099357fa499bfbc415968691573f25ec77435b7949f5fdfaa5da0584091d7f0e233c15745a3f793e35bf29f6cd2ed767f1f51032ae8202d7117b839d98d84858607796fbb583933b653911170a6b81b5122cabcdb2651e3d89b6e0001\"]}}],\"snapshotNumber\":1,\"utxo\":{\"2ee2cb485d7ceda00a1f9f8fc9eac0be46cc03011ec0177c008160389f6145d8#0\":{\"address\":\"addr_test1vqg9ywrpx6e50uam03nlu0ewunh3yrscxmjayurmkp52lfskgkq5k\",\"datum\":null,\"datumhash\":null,\"inlineDatum\":null,\"referenceScript\":null,\"value\":{\"lovelace\":1000000000}},\"5c05106772f97eacce0a31ac215ef87aa0746279008bdb3cd07e3abece6d3985#0\":{\"address\":\"addr_test1vqg9ywrpx6e50uam03nlu0ewunh3yrscxmjayurmkp52lfskgkq5k\",\"datum\":null,\"datumhash\":null,\"inlineDatum\":null,\"referenceScript\":null,\"value\":{\"lovelace\":500000000}},\"c4c4804879236033b77292c1cfe44c750a32ac88b998a3ec16533392990daeb6#0\":{\"address\":\"addr_test1vqa25t3aayfmpad20elswmsj94ehmdfjnhc64yz3jg5yl6skf5cck\",\"datum\":null,\"datumhash\":null,\"inlineDatum\":null,\"referenceScript\":null,\"value\":{\"lovelace\":250000000}}}},\"tag\":\"SnapshotConfirmed\",\"timestamp\":\"2023-06-12T15:53:18.840950541Z\"}")
@pgrange How is this failing? I am running all the tests and they pass, and I can't find where this issue comes from. Are you trying to run a TUI from this PR against a hydra-node from an earlier version? It of course can't work out of the box because the change in |
935f867
to
95ade76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good and "feels right". Only the tx transformatin on the API is now not needed and yes, the TUI likely needs updating. The TUI should be fine as it is re-using the types from hydra-node
. Users just need to ensure to use the same version as the node.
(I also just realized that the hydra-tui
tests built via nix now have weird and maybe problematic output as it emulates an interactive terminal already for the nix build
)
@@ -199,7 +199,7 @@ prepareServerOutput ServerOutputConfig{txOutputFormat, utxoInSnapshot} response | |||
handleTxOutput | |||
( key "snapshot" | |||
. key "confirmedTransactions" | |||
.~ toJSON (txToCbor <$> confirmed snapshot) | |||
.~ toJSON (confirmed snapshot) | |||
) | |||
encodedResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: make this now only a encodedResponse
This and the other ConfirmedSnapshot
cases now don't contain transactions and hence do not need to be transformed.
@abailly-iohk I'm running (hopefully) this PR tui against this PR hydra-node and get the following error:
I will triple check my configuration. It might well be the case that I'm not running the good tui. Let's see... |
@abailly-iohk I was, indeed, not using the right version of the TUI. So it works, my bad. (we should add a --version option to the tui, by the way, as it saved my life before with the hydra-node). |
As pascal pointed out, this will be a significant change on the client-side api and we should get feedback from some early adopting users. @uhbif19 @matiwinnetou @Yasuke @hSloan @luigy This PR is removing transactions from |
For sure this is a big change but we can adjust to the API, of course breaking changes will be made for us and app developers. Keeping own transactions is easy, the tricker part is keeping others transactions (also doable). I don't know if any of this is negotiable, if there is an issue and it needs to be done - it needs to be done. I believe you are not breaking the client API contract "for the fun of it" :) |
@pgrange ;) |
@matiwinnetou If you think this makes more sense, we could keep the API as is. |
But I think it generally goes in the sense of a leaner API that we only pass the confirmed txs ids instead of the full transaction: The transactions are observed by clients anyway through the |
We only use Also I still do not understand which responses to client are broadcasted, and which are not. So TxValid/TxInvalid is broadcasted across all Hydra nodes? |
That is a different issue. That requires to do some query |
32cb304
to
6894f2a
Compare
cd0355f
to
10e5791
Compare
@ch1bo I think this change makes sense, the API said that transactions (in a SnapshotConfirmed) could be in a bunch of different shapes originally, and I think the Id is really all that matters here, it is consistent across all forms and is likely more flexible for people who build and submit their transactions in those different shapes. Being able to get a SnapshotConfirmed and check it against the txids that have been submitted sounds like it will 'just work'. So I am for this change. |
367c83f
to
cb1ea22
Compare
Co-authored-by: Pascal Grange <pgrange@users.noreply.github.com>
There is no need to massage the tx output in snapshots anymore
cb1ea22
to
95948ae
Compare
This PR is a first step towards addressing the issue outlined in #904, as it replaces the use of the full transaction with the transaction id in the
Snapshot
body and therefore inSnapshotConfirmed
messages sent to the client.It remains to implement the replacement of full tx with txid in the
ReqSn
message within the protocol.