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.

Sign up for GitHub

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

Jump to bottom

Log and send client output when some keys don't match #529

Closed
4 tasks
ch1bo opened this issue Oct 4, 2022 · 3 comments · Fixed by #1143
Closed
4 tasks

Log and send client output when some keys don't match #529

ch1bo opened this issue Oct 4, 2022 · 3 comments · Fixed by #1143
Assignees
@ch1bo
Labels
L1 Affects the on-chain protocol of Hydra task Subtask of a bigger feature. ux Related to user experience

Comments

@ch1bo
Copy link
Member

ch1bo commented Oct 4, 2022 •
edited
Loading

Why

It's very hard to diagnose when a Head initialization was not picked up by one of the hydra-nodes. More specifically, the hydra client does not report anything at all and the hydra-node logs are also not very conclusive. For example, when some keys are not configured correctly there is not even an OnInitTx observation in the logs, just some blocks received.

What

Even when we have not configured our keys correctly, we want to have a log entry about a Head initialization, maybe even an API output. However, we do not want to have the hydra-node "adopt" this Head initialization, as it will not be fine anyways (because we are missing knowledge of hydra and/or cardano keys of the other parties).

Instead of discarding InitObservation when some keys don't match, we want to "discover" all Heads where we are part of. That makes it easier to see a Head has been initialized and if there is a misconfiguration, we can notice by checking Hydra vkeys and the misconfigured party not seeing a head open. (was not informing enough, so we changed the definition of what we want to do here)

How

BLOCKED: This is actually not possible as we don't have any knowledge of cardano keys in the HeadLogic + the chain layer needs to take a decision about a state transition as well (into the L1 onchain state) ADR18 could be a solution to this as we will not be needing to store anything in the chain layer and we could yield the observation of an init transaction into the HeadLogic.

To be able to inform a client about an IgnoredHeadInit we would need to be notified about such a transaction in the head logic! That means, that the chain layer need not filter valid, but unrelated init transactions, but yield a InitObservation always when it is a valid init transaction. No matter whether we are part of this head or not!

  • Do not guard on who is part of a head in Hydra.Chain.Direct.Tx.
  • Filter OnInitTx events in HeadLogic depending on configured Hydra/Cardano keys.
  • Add a ServerOutput for when the filtering deemed to have IgnoredHeadInit.
  • In the tui, explain that a head was ignored and checking logs/configuration is necessary.

TBD

  • There is no concept of layer 1 / cardano keys in the Head logic component so far!
  • The fact that the chain layer needs to take a decision / produce a ChainState limits ourselves in observing init transactions as-is.
@ch1bo ch1bo mentioned this issue Oct 4, 2022
Closed
12 tasks
@ch1bo ch1bo changed the title Hard to diagnose why head initialization was not shown: Should not discard InitObservation when some keys don't match and have higher-level logic log & ignore instead https://github.com/input-output-hk/hydra-poc/blob hydra-poc/hydra-node/src/Hydra/Chain/Direct/Tx.hs Line 559 in c7c28d6 guard $ sort expectedNames == sort actualNames Not discard InitObservation when some keys don't match Oct 4, 2022
@ch1bo ch1bo added the task Subtask of a bigger feature. label Oct 4, 2022
@ffakenz ffakenz self-assigned this Oct 4, 2022
@ffakenz ffakenz mentioned this issue Oct 4, 2022
Closed
@ffakenz
Copy link
Contributor

ffakenz commented Oct 6, 2022 •
edited
Loading

By applying this change, now the misconfigured party can not init a head with others but observes others' initializations.
The problem is that after this observation we can join and open and participate with the initialized head.

A better design would be to keep others' observations but don not be able to join and open a head if any party is misconfigured.

Possibly we display an error in this situations, like: Initialization Discarded/Ignored

ch1bo added a commit that referenced this issue Nov 3, 2022
@ch1bo
Remove propMutationOffChain from Mutation testing
94a2c5d 
This and propTransactionIsNotObserved have always been a crutch because
it would check that a transaction DOES validate, but IS NOT observed. It
was introduced to motivate the filtering of initTx observations which we
now want to actually remove (#529)
ch1bo added a commit that referenced this issue Nov 8, 2022
@ch1bo
Remove propMutationOffChain from Mutation testing
41122da 
This and propTransactionIsNotObserved have always been a crutch because
it would check that a transaction DOES validate, but IS NOT observed. It
was introduced to motivate the filtering of initTx observations which we
now want to actually remove (#529)
ch1bo added a commit that referenced this issue Nov 9, 2022
@ch1bo
Remove propMutationOffChain from Mutation testing
c84e363 
This and propTransactionIsNotObserved have always been a crutch because
it would check that a transaction DOES validate, but IS NOT observed. It
was introduced to motivate the filtering of initTx observations which we
now want to actually remove (#529)
@ch1bo ch1bo added the 💭 idea An idea or feature request label May 16, 2023
@ch1bo ch1bo mentioned this issue Sep 4, 2023
Closed
15 tasks
@ch1bo
Copy link
Member Author

ch1bo commented Sep 5, 2023

Started investigating the second point of the blocking part again

the chain layer needs to take a decision about a state transition as well (into the L1 onchain state)

Although hoped initially ADR18 (now basically superseeded) did not do the trick as the problem is really the fact that the observeInit needs to produce an InitialChainState in order to be able to observe any commits. So we cannot just yield the OnInitTx to the application and not update the state accordingly .. or it would make the code very contrived.

So recently we set out to try to make observe.. functions not need a ChainState to observe and be handed a fully ResolvedTx. While this is possible, the returned ChainState is still required by the construction functions and hence we would need to make the whole transaction creation also less stateful. This is a big change .. and so we have not continued that just yet.

@ch1bo ch1bo changed the title Not discard InitObservation when some keys don't match Log and send client output when some keys don't match Sep 7, 2023
@ghost ghost mentioned this issue Oct 3, 2023
Closed
3 tasks
@ffakenz ffakenz assigned ghost and unassigned locallycompact and ch1bo Oct 24, 2023
@ghost ghost added L1 Affects the on-chain protocol of Hydra ux Related to user experience and removed 💭 idea An idea or feature request labels Oct 24, 2023
@ghost
Copy link

ghost commented Oct 24, 2023

This issue is not only about stateless observation of the chain but also about improving feedback to the users and troubleshooting capabilities of the hydra-node and clients. Errors or notifications in general do not need to go through the HeadLogic as this would clutter this already significantly complex piece of logic, it might make sense to start "channeling" some of the logs to the clients.

@ghost ghost mentioned this issue Oct 25, 2023
Merged
4 tasks
@ch1bo ch1bo assigned ch1bo and unassigned ghost Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ch1bo ch1bo

Labels
L1 Affects the on-chain protocol of Hydra task Subtask of a bigger feature. ux Related to user experience
Projects
None yet
Milestone
No milestone
3 participants
@locallycompact @ch1bo @ffakenz