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

Enforce (on-chain) that Hydra transitions are authenticated by participants #292

Merged
merged 18 commits into from
Mar 28, 2022

Conversation

KtorZ
Copy link
Collaborator

@KtorZ KtorZ commented Mar 25, 2022

Fixes #284

@abailly-iohk There's currently one failing test in hydra-cluster

--match "/Test.LocalCluster/should produce blocks, provide funds, and send Hydra OCV transactions/"

but, I do think that the failing assertion on this test does really not have its place here. I left a comment in the last commit, I let you judge of that 😊


  • 📍 Inject signer mutation for CollectCom Check all Hydra txs signed by a Head participant #284

  • 📍 Sketch validation of CollectCom signers Check all Hydra txs signed by a Head participant #284

  • 📍 Extract tokens from outputs and implement signatory verification.

  • 📍 Try to generate key for signing CollectCom tx Check all Hydra txs signed by a Head participant #284

  • 📍 Extract 'signWith' from TinyWallet and use it to authenticate Collectcom with party's credentials.

  • 📍 Add signer's vk to the explicit required signers list when signing.

  • 📍 Add 'mutate required signer' to Abort. Mutation tests fail.

  • 📍 Refactor head contract to re-use logic for checking signatories on-chain.

  • 📍 Add signers to the health abort transaction
    Moved some logic around to make it possible.

  • 📍 Add mutation mutation to close tx about required signers.

  • 📍 Implement signature verification on-chain for close transition.

  • 📍 Repair healthy close tx, and correctly gather signatures on-chain.

  • 📍 Fix finding Head currency symbol.
    The initial version of the function was fine for pre-CollectCom transitions, but after the collect-com has happened, PTs are all aggregated under one policy and thus, there may be many assets locked by the head script.

  • 📍 Provide required signers when constructing Hydra on-chain transactions.
    The rationale for doing this is double:

    (a) So far, we've been deferring signing up to inside the wallet and,
    requiring signature to happen on the transaction templates from
    Direct.Tx breaks a bit this abstraction. Moreso, we initially went
    that way because we thought that signing a transaction would result in
    the signatories being available on-chain in the Plutus context. Turns
    out that only explicitly specified signatories (in requiredSigners)
    end up in the Plutus context after all.

    (b) The 'State' abstraction we use internally is already tailored to
    one party and hold its 'ownVerificationKey'. So the Cardano signer key
    is already readily available and it is therefore much easier to
    provide it along the way. As a nice benefits, it makes it also clear
    from the signature of the low-level Direct.Tx functions that they now
    require to be explictly signed by a party.

  • 📍 Actually sign transaction AFTER modifying its body content
    😬 ... now that're changing the 'requiredSigners', we need to make sure to sign the modified body, otherwise, we produce an invalid witness.

  • 📍 Revert adding required signers during 'signWith'
    This is in fact no longer needed and had become error-prone. Now that the Direct.Tx primitives take care of adding the required signer, there's no need to fiddle with the tx body during signing anymore.

  • 📍 Leave note about failing (albeit wrong) integration test.

@KtorZ KtorZ requested a review from abailly-iohk March 25, 2022 11:58
@KtorZ KtorZ self-assigned this Mar 25, 2022
abailly-iohk and others added 17 commits March 28, 2022 06:19
  Moved some logic around to make it possible.
  The initial version of the function was fine for pre-CollectCom transitions, but after the collect-com has happened, PTs are all aggregated under one policy and thus, there may be many assets locked by the head script.
  The rationale for doing this is double:

  (a) So far, we've been deferring signing up to inside the wallet and,
  requiring signature to happen on the transaction templates from
  Direct.Tx breaks a bit this abstraction. Moreso, we initially went
  that way because we thought that signing a transaction would result in
  the signatories being available on-chain in the Plutus context. Turns
  out that only explicitly specified signatories (in requiredSigners)
  end up in the Plutus context after all.

  (b) The 'State' abstraction we use internally is already tailored to
  one party and hold its 'ownVerificationKey'. So the Cardano signer key
  is already readily available and it is therefore much easier to
  provide it along the way. As a nice benefits, it makes it also clear
  from the signature of the low-level Direct.Tx functions that they now
  require to be explictly signed by a party.
  😬 ... now that're changing the 'requiredSigners', we need to make sure to sign the modified body, otherwise, we produce an invalid witness.
  This is in fact no longer needed and had become error-prone. Now that the Direct.Tx primitives take care of adding the required signer, there's no need to fiddle with the tx body during signing anymore.
@abailly-iohk
Copy link
Contributor

@KtorZ There's even the following comment in the code:

-- TODO(AB): remove when DirectChainSpec is refactored to use cardano-api
failAfter 30 $ assertCanCallInitAndAbort cluster

This was something I did back in October when you were busy with wallet stuff we had barely started work on Direct tx submission, and I wanted to better understand how txs were built and submitted through the API.

* Rename file to match what we are testing, eg. CardanoCluster and not
LocalCluster
* remove failing assertion that's not relevant anymore: This used to
test we could construct basic Hydra transactions "by hand" but it's
getting more complicated and pretty much useless as it's thoroughly
tested elsewhere
@github-actions
Copy link

Unit Test Results

    5 files  ±0    71 suites  ±0   7m 2s ⏱️ -25s
191 tests ±0  189 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit afea828. ± Comparison against base commit b66d451.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
Test.LocalCluster ‑ should produce blocks, provide funds, and send Hydra OCV transactions
Test.CardanoCluster ‑ should produce blocks, provide funds, and send Hydra OCV transactions

@abailly-iohk abailly-iohk merged commit 46fecfa into master Mar 28, 2022
@abailly-iohk abailly-iohk deleted the ensemble/284 branch March 28, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check all Hydra txs signed by a Head participant
2 participants