Interleaved wallet #384

ch1bo · 2022-06-02T16:04:32Z

I couldn't help but try out how a synchronous TinyWallet would look like & work. This Draft PR tries to drive a discussion whether this is something we want or we should stick with the asynchronous interface for balancing & signing transactions produced by the Chain.Direct layer.

Draft a TinyWallet with reset and update methods to query the latest state and handle individual blocks received via the main ChainSync client.
query UTxO (and other stuff) from ChainPoint ?
hydra-cluster tests pass
~~Tests using MockServer like WalletSpec and DirectSpec~~
No timeouts and retry anymore
Further simplified the back channel of mkChain to be a simple SubmitTx

Seems to work and I wonder if it maybe even fixes #367 ?

github-actions · 2022-06-02T16:20:28Z

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	2022-06-03 17:40:08.80266863 UTC
Max. memory units	14000000.00
Max. CPU units	10000000000.00
Max. tx size (kB)	16384

Cost of Init Transaction

Parties	Tx size	% max Mem	% max CPU
1	4738	32.48	19.08
2	4937	15.69	8.61
3	5136	23.24	13.01
5	5538	24.27	13.21
10	6537	27.77	14.30
30	10540	75.84	39.43
40	12538	96.65	50.02

Cost of Commit Transaction

Currently only one UTxO per commit allowed (this is about to change soon)

UTxO	Tx size	% max Mem	% max CPU
1	5861	17.94	8.78

Cost of CollectCom Transaction

Parties	Tx size	% max Mem	% max CPU
1	13518	22.04	11.96
2	14469	44.72	24.95
3	13999	55.65	30.55
4	14876	88.41	49.31

Cost of Close Transaction

Parties	Tx size	% max Mem	% max CPU
1	9149	10.19	5.42
2	9389	12.24	6.51
3	9488	12.29	6.55
5	10001	16.80	8.96
10	11148	25.74	13.80
30	15034	60.54	32.58
55	15817	67.18	35.28

Cost of Contest Transaction

Parties	Tx size	% max Mem	% max CPU
1	9197	10.84	5.76
2	9243	10.16	5.41
3	9357	10.67	5.67
5	9896	15.24	8.14
10	11131	26.64	14.29
30	14770	53.53	28.74
37	15968	61.49	32.96

Cost of Abort Transaction

Parties	Tx size	% max Mem	% max CPU

Cost of FanOut Transaction

Involves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.

UTxO	Tx size	% max Mem	% max CPU
3	13069	13.59	7.87
10	13240	23.15	13.36
60	14921	97.71	56.24

ch1bo · 2022-06-03T08:09:59Z

hydra-node/src/Hydra/Chain/Direct/Wallet.hs

          case coverFee_ pparams systemStart epochInfo lookupUTxO walletUTxO partialTx of
            Left e ->
              pure (Left e)
            Right (walletUTxO', balancedTx) -> do
-              _ <- swapTMVar utxoVar (walletUTxO', pparams, systemStart, epochInfo)
+              writeTVar utxoVar (walletUTxO', pparams, systemStart, epochInfo)


Do we still want to do this? This is optimistically removing used utxos when balancing. But there is no guarantee that transaction will ever be included in the chain?

Thinking out loud: the reason we would need this is because we want to prevent trying to double-spend some UTxO. Double-spending would suppose that we try to submit more than one transaction at the time.

✔️ We can't commit until after Init has been observed

✔️ We don't try to collect-com until after all commits have been observed

✔️ We can't close until after collect-com has been observed

✔️ We can't contest until after a close has been observed

❌ We might try to fanout right after we submit a contest.

Indeed, we allow to contest right up until the deadline; which may lead in situation where the fanout is posted while a contest is pending. This is pretty bad because depending on how transactions gets re-organised due to chain fork, the fanout may end up being processed before / in stead of the contest. If we make sure that the utxo used for fanout depends on the one used for contest, then we at least limit this kind of issues.

As a side-note, we wouldn't be able to fanout either because in normal scenarios, we have only one fuel UTxO. Thus, if a contest is pending, no UTxO is available and fanout becomes impossible. But I highly prefer failing for a "lack of payment input" than later having to handle a rejected transaction from the network because of a double-spending (which is precisely the kind of case we want to treat as adversarial and for which there's no point retrying transactions).

One thing we ought to implement IMO is predicted UTxO. Indeed, at this very moment, we know perfectly well what the transaction is and thus, what would be the resulting UTxO if included. We can thus, instead of removing the consumed UTxO, replaced it with the produced ones as if we had already observed the transaction. This doesn't change much the first 4 cases above, but it allows us to successfully submit a fanout on top of a pending contestation.

Wait.. what will happen if a transaction fails to be submitted? e.g. the fanout because it was tried before the deadline? Will the UTxO be "made available" again? If not.. that is even a bug and we would NOT want to optimistically consume the UTxO at this location.

Yup.

That's why we need validity intervals on transactions submitted as part of the Hydra protocol. Say 1h. If after 1h you haven't seen the transaction in a block, then you can legitimately "unlock" the UTxO that was "optimistically consume".

In the end, consuming the UTxO (as in, erasing it completely) is kind of a bad idea. Ideally we would keep two sets. One being the sets of UTxO currently part of pending transactions.

Wouldn't it be a much simpler solution to just not bother and have the ledger sort out potential double spends? i.e. the corner case where two tx were balanced, signed and submitted using the same UTxO because of a race between observing the first tx and constructing the second tx?

After all, our own local cardano-node would not approve a double spend and if our application (or the client in proxy) wants to spam the chain with fanout txs it's our/their fault.

I did not follow all the details, doing something else, but I would rather rely on retrying tx and some timeout based on actual observations than maintaining yet another state on-chain. If I understood correctly what's proposed. Or even better: Leave those details to a proper wallet implementation and just define some simpler "protocol" interaction between chain poster and attached wallet.

hydra-node/src/Hydra/Chain/Direct/Wallet.hs

KtorZ · 2022-06-03T08:38:17Z

I will spend time on this PR tomorrow @ch1bo , happy to give it a look.

The tiny wallet is not allocating/freeing any resource anymore so we don't need to use the with pattern.

This should make this easier to test and maybe even ditch the MockServer

Craft mock `queryUTxO` handles suitabel for the test at hand instead

This makes it more convincing to not explicitly test this and tests of applyBlock to be sufficient.

We arrived at a point where we would need to be fixing the MockServer because it seems to rely on some behavior not present in the "new" TinyWallet behavior anymore. But the identical test against a real cardano-node works. So we decided we do not want to bother.

ch1bo · 2022-06-03T10:06:23Z

@KtorZ worked with @abailly-iohk on this and we think it's ready for your review :)

github-actions · 2022-06-03T10:20:06Z

Unit Test Results

224 tests - 1 222 ✔️ - 1 6m 29s ⏱️ -5s
  82 suites - 1     2 💤 ±0
    5 files ±0     0 ❌ ±0

Results for commit 26e1b30. ± Comparison against base commit 9acd79d.

This pull request removes 3 and adds 2 tests. Note that renamed tests count towards both.

Hydra.Chain.Direct ‑ can init and abort a head given nothing has been committed
Hydra.Chain.Direct.Wallet/withTinyWallet ‑ connects to server and returns UTXO in a timely manner
Hydra.Chain.Direct.Wallet/withTinyWallet ‑ tracks UTXO correctly when payments are received

Hydra.Chain.Direct.Wallet/newTinyWallet ‑ initialises wallet by querying UTxO
Hydra.Chain.Direct.Wallet/newTinyWallet ‑ re-queries UTxO from the reset point

♻️ This comment has been updated with latest results.

ch1bo · 2022-06-03T13:50:28Z

Note to myself (cc @abailly-iohk): we forgot to actually query from point and maybe there is a somewhat okayisch way to do the queries in a single connection without too much of ouroboros fancyness.

ghost · 2022-06-03T15:03:33Z

@ch1bo Do you want to do this here, or in another PR?

ch1bo · 2022-06-03T17:12:04Z

@ch1bo Do you want to do this here, or in another PR?

@abailly-iohk Did it in this PR now. Otherwise the semantics wouldn't have been complete. Only thing left (for a follow-up PR) is the fact that it's still using cardano-ledger types and maybe we can use some balancing / cost estimation maintained by someone else :)

The queryXXX functions of the includec CardanoClient do use the local state query protocol, which can acquire some specific point on the chain when passed `Just ChainPoint`, so we do configure these functions to take a `QueryPoint`. We do use this to query at the point to which the chain rolled back in TinyWallet workflow.

Following the previous commit we need to specify now explicitly that we are interested in querying the latest point / tip.

KtorZ · 2022-06-05T05:32:03Z

hydra-node/src/Hydra/Chain/Direct.hs

-  eraHistory <- queryEraHistory networkId socketPath
+  tip@(ChainPoint slotNo _) <- queryTip networkId socketPath
+  systemStart <- querySystemStart networkId socketPath (QueryAt tip)
+  eraHistory <- queryEraHistory networkId socketPath (QueryAt tip)


This is probably how we should be querying most things generally speaking -> first get a point, and then run all subsequent queries on that point. It's nothing really crucial though because we explicitly said Hydra heads would need to be terminated in case of protocol parameter updates thus in principle, the case where we get different parameters across several nearby queries does not happen.
Still.. I'd prefer doing things properly from the get-go, so that the day we decide to do something more elaborate regarding hard-forks or protocol updates, we have one less problem to think of.

KtorZ · 2022-06-05T05:42:30Z

hydra-node/src/Hydra/Chain/Direct/Handlers.hs

@@ -189,12 +165,8 @@ finalizeTx TinyWallet{sign, getUTxO, coverFee} headState partialTx = do
    Right validatedTx -> do
      pure $ sign validatedTx



Note that, while we're at it, this CannotCoverFees error is quite misleading here because it actually wraps several cases, some of which have nothing to do with covering fees so-to-speak:

= ErrNoAvailableUTxO | ErrNotEnoughFunds ChangeError | ErrNoPaymentUTxOFound | ErrScriptExecutionFailed (RdmrPtr, ScriptFailure StandardCrypto)

KtorZ

Nice clean-up. Glad to see the MockServer & DirectSpec going to oblivion. This is indeed simpler and dictates the kind of interface we would expect from an external wallet 👍

ch1bo requested review from KtorZ and a user June 2, 2022 16:04

ch1bo commented Jun 3, 2022

View reviewed changes

ghost force-pushed the ch1bo/interleaved-wallet branch from 25d9a93 to 996878c Compare June 3, 2022 09:36

ch1bo and others added 15 commits June 3, 2022 12:05

Try to interleave wallet init/update within a single chainsync

bf76a73

Invoke wallet reset/update in chain sync client

86ac575

Use a TVar and remove IOManager from withTinyWallet

0aa872d

Remove chain clients from TinyWallet

1540509

Convert withTinyWallet handle to newTinyWallet

f6a7a0d

The tiny wallet is not allocating/freeing any resource anymore so we don't need to use the with pattern.

Not retry on finalize, remove timeout and rebase mistakes

d196b68

Change 'TxPoster' to an even simpler 'SubmitTx'

0028e52

Factor out querying utxo and other things from the TinyWallet

34c1c1b

This should make this easier to test and maybe even ditch the MockServer

[wip] Remove use of mock-server in Wallet tests

e488416

Craft mock `queryUTxO` handles suitabel for the test at hand instead

Always write utxoVar in update and trace ApplyBlock

a3e97ce

This makes it more convincing to not explicitly test this and tests of applyBlock to be sufficient.

Draft a test for reset from point

0595204

Query chain at point of reset

4d77d46

Small refactor on WalletSpec

04fd556

Use InitializingWallet trace and fix logs api spec

200b373

ch1bo force-pushed the ch1bo/interleaved-wallet branch from 613c0d4 to c5190eb Compare June 3, 2022 10:05

ch1bo marked this pull request as ready for review June 3, 2022 10:06

ch1bo force-pushed the ch1bo/interleaved-wallet branch from f91ecec to 3326d98 Compare June 3, 2022 17:13

Update call sites of queryXXX functions in hydra-cluster

26e1b30

Following the previous commit we need to specify now explicitly that we are interested in querying the latest point / tip.

ch1bo force-pushed the ch1bo/interleaved-wallet branch from d5a74ae to 26e1b30 Compare June 3, 2022 17:30

KtorZ reviewed Jun 5, 2022

View reviewed changes

KtorZ approved these changes Jun 5, 2022

View reviewed changes

ch1bo merged commit c98cc2c into master Jun 7, 2022

ch1bo deleted the ch1bo/interleaved-wallet branch June 7, 2022 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interleaved wallet #384

Interleaved wallet #384

ch1bo commented Jun 2, 2022 •

edited

Loading

github-actions bot commented Jun 2, 2022 •

edited

Loading

ch1bo Jun 3, 2022

KtorZ Jun 5, 2022

ch1bo Jun 7, 2022

KtorZ Jun 7, 2022

ch1bo Jun 7, 2022

ghost Jun 7, 2022

KtorZ commented Jun 3, 2022

ch1bo commented Jun 3, 2022

github-actions bot commented Jun 3, 2022 •

edited

Loading

ch1bo commented Jun 3, 2022

ghost commented Jun 3, 2022

ch1bo commented Jun 3, 2022 •

edited

Loading

KtorZ Jun 5, 2022

KtorZ Jun 5, 2022

KtorZ left a comment

		@@ -189,12 +165,8 @@ finalizeTx TinyWallet{sign, getUTxO, coverFee} headState partialTx = do
		Right validatedTx -> do
		pure $ sign validatedTx

Interleaved wallet #384

Interleaved wallet #384

Conversation

ch1bo commented Jun 2, 2022 • edited Loading

github-actions bot commented Jun 2, 2022 • edited Loading

Transactions Costs

Cost of Init Transaction

Cost of Commit Transaction

Cost of CollectCom Transaction

Cost of Close Transaction

Cost of Contest Transaction

Cost of Abort Transaction

Cost of FanOut Transaction

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Jun 3, 2022

ch1bo commented Jun 3, 2022

github-actions bot commented Jun 3, 2022 • edited Loading

Unit Test Results

ch1bo commented Jun 3, 2022

ghost commented Jun 3, 2022

ch1bo commented Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

ch1bo commented Jun 2, 2022 •

edited

Loading

github-actions bot commented Jun 2, 2022 •

edited

Loading

github-actions bot commented Jun 3, 2022 •

edited

Loading

ch1bo commented Jun 3, 2022 •

edited

Loading