Monad attacks #154

carlhammann · 2022-09-02T20:26:05Z

This PR defines

newtype Attack a = Attack (MockChainSt -> TxSkel -> [(TxSkel, a)])

and uses the fact that this type is a monad with branching and failure capabilities to rework our attack language. A good first example to see this in action is probably the simplest of the "big" attacks: The token duplication attack.

This is not yet completely finished, in particular:

I still need to update the tests for the double satisfaction attack.
The attacks in Cooked.Attack.AddConstraints are heavily biased towards input constraints, because that's what I needed so far. This should be made more symmetric.
The same holds for Cooked.Tx.Constraints.Optics
Make cooked export all of the LTL and Attack framework.

However, it's ready for feedback and review!

If merged, this PR will close #143 .

… to cope with UntypedAttack

facundominguez

I looked at the first 6 files today. So far the interface looks approachable.

cooked-validators/src/Cooked/Attack/AddConstraints.hs

cooked-validators/src/Cooked/Attack/Common.hs

facundominguez

Reviewed one more file.

cooked-validators/src/Cooked/Attack/DoubleSat.hs

facundominguez · 2022-09-07T19:15:54Z

cooked-validators/src/Cooked/Attack/DoubleSat.hs

+  --   combinations where /at most/ three (one for each focus) extra constraints
+  --   are added. One of these combinations is of course the one where nothing
+  --   is added, and that one is omitted, which brings the grand total down to
+  --   71 options.


I can't figure out from this paragraph what combination of constraints are applied in each modified transaction.

In a similar vein, making sure I'm following... you get 72 because you try 0, 1, or 2 additional SpendsScript constraints for the first PaysScript constraint, then 0, 1, 2, or 3 for the second, etc.? Hence (2+1)(3+1)(5+1)

I'll enumerate them explicitly.

cooked-validators/src/Cooked/Attack/DatumHijacking.hs

cooked-validators/src/Cooked/Attack/DoubleSat.hs

cooked-validators/tests/Cooked/AttackSpec/AddConstraints.hs

cooked-validators/src/Cooked/Attack/DupToken.hs

facundominguez

I finished reviewing all the files. I payed full attention to the documentation, and some to the implementation.

On the documentation side, I think the PR is a net improvement, despite of the requests for clarification that I left in comments.

The PR is larger than ideal, but I don't think it is worth splitting it. If we were to question fundamental assumptions, a lot of it might need to be redone. As possible, it would be better to submit pieces of the work as they are created, even if some parts need amends later on.

That said, I like the direction of the changes which gives more opportunity to reuse the code that defines the attacks (so called composability). We would need to collect some custom attacks to show the reuse in action, which I guess would be the substance of motivating the change. Maybe it is already possible to build the case with the implemented attacks, I'm not sure.

cooked-validators/src/Cooked/MockChain/Monad/Staged.hs

cooked-validators/tests/Cooked/AttackSpec/Common.hs

cooked-validators/tests/Cooked/AttackSpec/DoubleSat.hs

cooked-validators/tests/Cooked/LtlSpec.hs

cooked-validators/tests/Cooked/TestUtils.hs

facundominguez · 2022-09-09T22:25:08Z

I edited my review to add nothing less than the positive feedback. Please, excuse me for the hiccup.

mmontin

To me, the PR is too large to be merged directly. The ideal timeline would be :

a PR that presents the new Attack type as well as the direct abstract attacks deduced from it
a PR with single steps modifications working on concrete elements such as paysScripts
a PR with higher scale modification covering for example all misc constraints
a PR with the new implementation for each of our "real" attacks such as DoubleSat
a PR with new examples / updated examples

This way, it would be a lot easier to review, a lot easier to implement changes based on the previous reviews and, overall, this would lead to a result that will more likely suit each of us. Not saying, of course, that the current result is not suitable, just that a better process in such big PRs can go a long way into improving our work, getting better feedback and ultimately coming up with a refined and improved final result.

On another note, this is a great work, we get most of the features we wanted as well as a nice flow to build attacks based on smaller steps modifications. I'm eager to see these changes merged once the process leading to the full merge has been improved.

I suggest to stash the modifications, checkout the current main version and integrate the stashed changes following the outline presented at the beginning of this review comment.

cooked-validators/src/Cooked/Attack/AddConstraints.hs

mmontin · 2022-09-25T11:19:29Z

cooked-validators/src/Cooked/Attack.hs

-    module Cooked.Attack.TamperDatum,
-  )
-where
+module Cooked.Attack (module X) where


I am still unsure that the single steps modifications should be called attacks. Why not

call the single steps modifications "Modifications" although there might be conflict there with the meaning of modifications in LTL

call actual aggregations of modifications toward a specific goal "Attacks"

cooked-validators/src/Cooked/Attack/AddConstraints.hs

cooked-validators/src/Cooked/Attack/Common.hs

0xd34df00d

I have mostly nitpicking and bikeshedding comments. Feel free to ignore!

Also, I've seen from left to right in the docs in a couple of places where indexing is mentioned. I'm not sure there's a natural order to constraints/etc (that's not defined in a circular way on indexing).

cooked-validators/src/Cooked/Attack/DupToken.hs

cooked-validators/src/Cooked/Attack/AddConstraints.hs

cooked-validators/src/Cooked/Tx/Constraints/Type.hs

cooked-validators/src/Cooked/Attack/Common.hs

0xd34df00d · 2022-10-06T21:43:21Z

cooked-validators/src/Cooked/Attack/Common.hs

+
+instance MonadPlus Attack
+
+-- * Simple attacks


This is largely bikeshedding, but how do you reckon renaming the attacks to drop the Attack word everywhere (and renaming what clashes with optics here)? I'd rather import this module qualified instead of having Attack everywhere, especially when writing more complex ones. As for these simple attacks, we could adopt either MonadState or optics lingo, and for the former we could rename mcstAttack → get, viewAttack → gets, and so on.

After all, the Attack monad itself seems out to be not just about attacks, but rather more generic and about state (that just happens to be MockChainSt in our case) and non-determinism over it.

But, again, this is very much a personal preference, so please feel free to leave it as is it now.

cooked-validators/src/Cooked/Attack/Common.hs

Co-authored-by: 0xd34df00d <georg.rudoy@tweag.io>

carlhammann · 2022-10-07T08:32:11Z

So, let me summarise the state of this PR:

There are several voices that do not like the "attack" nomenclature any more. I guess I'm one of them, by now. There are these ideas at the moment (which are independent):
- Always import the attack modules qualified, which alllows renaming all functions like xAttack to x.
- Change the language of the comments from "attack" to "modification" or something similar.
- Rename the AddConstraints module to PushPopConstraints.
The role of functions like mkAccumLAttack that rely on optics to build attacks is not clear enough.
- In particular, what is "left to right"? (It depends on your optic, so is there a better wording?)
There are some questions about details left:
- Is the documentation of the doubleSatAttack understandable now? In particular, is it clear what it does with TryCombinations?
- Do we agree that sameConstraints is a sensible "semantic" equality relation on Constraints?
  - (How) should the functions in Attack.AddConstraints respect this equality?

carlhammann · 2022-10-07T12:46:50Z

I think that my last question is interesting enough to warrant its own issue, which I opened: #165. For the purposes of this PR, I think we should leave sameConstraints as it is.

carlhammann · 2022-10-14T11:58:31Z

In my perception, after talking to many of you, all unanswered questions are now recorded as issues, and only these two remain:

Is the documentation of the doubleSatAttack understandable now? @facundominguez and @lucaspena were the ones who were, rightfully, most unhappy with it.
Do we have a better word than "Attack"? Both @mmontin and @0xd34df00d were unhappy with it, and I'm as well. I'd like to rename everything apart from the "real" attacks like token duplication, double satisfaction,..., to something else. For our group, it's clear by now that an attack is just something that changes a transaction, but for outsiders, I think that the word comes with too many expectations attached to it. That is, what should the name of the Attack type become? I quite like Tweak: It sounds like a small adjustment, which arguably is what most of the "xAttack" functions do, and a combination of tweaks with malicious intent is an attack.

facundominguez · 2022-10-14T14:33:55Z

Is the documentation of the doubleSatAttack understandable now?

I think is much better. Thanks.

Do we have a better word than "Attack"?

Tweak is good. Should it be plural? An attack can produce more than one modification, right?
A long name would be TransactionModifications which could be compressed to TxMods perhaps.

* Start experimenting with attacks in an arbitraty monad * Try Attack = MockChainSt -> TxSkel -> Maybe (TxSkel, a) * Rewrite the dupTokenAttack as a combination of smaller attacks * Add tests for the modified dupTokenAttack * Rewrite the datumHijackingAttack in the new language * Better rewriting of the datumHijackingAttack * Make attacks return lists, revert the LTL module to main * Move 'somewhere' and 'everywhere' to MockChain.Monad.Staged, in order to cope with UntypedAttack * Tests for datumHijackingAttack; weird behaviour in mkSelectAttack * Found the culprit in mkSelectAttack, added regression test * Redefine mkAttack and the datumTamperingAttack * Rewrite permutOutAttack and its tests * Reintroduce non-attack tests, remove tests for '<>' on 'Attack's * Status save on doubleSatAttack * Make an extra module for attacks that add/remove constraints * Rewrote the doubleSatAttack * Some better comments at the double satisfaction attack * Rewrite tests for doubleSatAttack * Test the addConstraintsAttack * Adapt the examples to the new attacks * Some attacks to add and remove OutConstraints * Make cooked export everything * Address Facundo's and Lucas' comments, first half * Rework the test for the doubleSatAttack * Address more comments from Facundo an Mathieu * Make some tests more structured, as suggested by Facundo * Incorporate suggestions from Georg * Update cooked-validators/src/Cooked/Attack/AddConstraints.hs Co-authored-by: 0xd34df00d <georg.rudoy@tweag.io> * Cleanup from the last commit * Repair 'sameConstraints', yet again * Rename Attack -> Tweak Co-authored-by: 0xd34df00d <georg.rudoy@tweag.io>

carlhammann and others added 23 commits August 26, 2022 19:41

Start experimenting with attacks in an arbitraty monad

65a472d

Try Attack = MockChainSt -> TxSkel -> Maybe (TxSkel, a)

8884a28

Rewrite the dupTokenAttack as a combination of smaller attacks

e285b24

Add tests for the modified dupTokenAttack

efc7eb9

Rewrite the datumHijackingAttack in the new language

396cf7b

Better rewriting of the datumHijackingAttack

41244bd

Make attacks return lists, revert the LTL module to main

400c50b

Move 'somewhere' and 'everywhere' to MockChain.Monad.Staged, in order…

0c545fe

… to cope with UntypedAttack

Tests for datumHijackingAttack; weird behaviour in mkSelectAttack

22c747f

Found the culprit in mkSelectAttack, added regression test

095e22e

Redefine mkAttack and the datumTamperingAttack

5b0d8f3

Rewrite permutOutAttack and its tests

ed86ecb

Reintroduce non-attack tests, remove tests for '<>' on 'Attack's

23e19f7

Status save on doubleSatAttack

2f2797e

Make an extra module for attacks that add/remove constraints

418c254

Rewrote the doubleSatAttack

067a794

Some better comments at the double satisfaction attack

801e8ab

Rewrite tests for doubleSatAttack

6790089

Test the addConstraintsAttack

a06d47e

Adapt the examples to the new attacks

1bc6e25

Some attacks to add and remove OutConstraints

048d77e

Make cooked export everything

0e65615

Merge branch 'main' into monad-attacks

d762ca2

facundominguez reviewed Sep 6, 2022

View reviewed changes

facundominguez reviewed Sep 7, 2022

View reviewed changes

lucaspena reviewed Sep 8, 2022

View reviewed changes

cooked-validators/src/Cooked/Attack/DatumHijacking.hs Outdated Show resolved Hide resolved

lucaspena reviewed Sep 8, 2022

View reviewed changes

cooked-validators/src/Cooked/Attack/DoubleSat.hs Outdated Show resolved Hide resolved

lucaspena reviewed Sep 8, 2022

View reviewed changes

cooked-validators/tests/Cooked/AttackSpec/AddConstraints.hs Outdated Show resolved Hide resolved

facundominguez reviewed Sep 8, 2022

View reviewed changes

cooked-validators/src/Cooked/Attack/DupToken.hs Show resolved Hide resolved

facundominguez approved these changes Sep 9, 2022

View reviewed changes

mmontin requested changes Sep 25, 2022

View reviewed changes

carlhammann added 2 commits October 5, 2022 14:06

Address Facundo's and Lucas' comments, first half

44d7020

Rework the test for the doubleSatAttack

e3ab421

carlhammann mentioned this pull request Oct 6, 2022

Make Transactions have a list of labels instead of Maybe a label (low priority) #164

Closed

carlhammann added 3 commits October 6, 2022 10:59

Address more comments from Facundo an Mathieu

f3389ac

Make some tests more structured, as suggested by Facundo

faae62a

Merge branch 'main' into monad-attacks

9038925

0xd34df00d approved these changes Oct 7, 2022

View reviewed changes

carlhammann and others added 3 commits October 7, 2022 10:07

Incorporate suggestions from Georg

8f7193d

Update cooked-validators/src/Cooked/Attack/AddConstraints.hs

37a2a60

Co-authored-by: 0xd34df00d <georg.rudoy@tweag.io>

Cleanup from the last commit

00301d1

Repair 'sameConstraints', yet again

2ea04ff

carlhammann mentioned this pull request Oct 10, 2022

Can we start doing releases? #167

Closed

Merge branch 'main' into monad-attacks

a90c621

carlhammann added 2 commits October 14, 2022 18:39

Rename Attack -> Tweak

614d0ac

Merge remote-tracking branch 'origin/monad-attacks' into monad-attacks

4d11868

mmontin approved these changes Oct 17, 2022

View reviewed changes

mmontin merged commit ed5e8c9 into main Oct 17, 2022

mmontin deleted the monad-attacks branch October 17, 2022 06:25

carlhammann mentioned this pull request Oct 17, 2022

Explicit sanity checks for Transaction Skeletons #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monad attacks #154

Monad attacks #154

carlhammann commented Sep 2, 2022 •

edited

Loading

facundominguez left a comment

facundominguez left a comment

facundominguez Sep 7, 2022

lucaspena Sep 8, 2022

carlhammann Oct 5, 2022 •

edited

Loading

facundominguez left a comment •

edited

Loading

facundominguez commented Sep 9, 2022

mmontin left a comment •

edited

Loading

mmontin Sep 25, 2022

0xd34df00d left a comment

0xd34df00d Oct 6, 2022

carlhammann commented Oct 7, 2022

carlhammann commented Oct 7, 2022

carlhammann commented Oct 14, 2022 •

edited

Loading

facundominguez commented Oct 14, 2022 •

edited

Loading

Monad attacks #154

Monad attacks #154

Conversation

carlhammann commented Sep 2, 2022 • edited Loading

facundominguez left a comment

Choose a reason for hiding this comment

facundominguez left a comment

Choose a reason for hiding this comment

facundominguez Sep 7, 2022

Choose a reason for hiding this comment

lucaspena Sep 8, 2022

Choose a reason for hiding this comment

carlhammann Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

facundominguez left a comment • edited Loading

Choose a reason for hiding this comment

facundominguez commented Sep 9, 2022

mmontin left a comment • edited Loading

Choose a reason for hiding this comment

mmontin Sep 25, 2022

Choose a reason for hiding this comment

0xd34df00d left a comment

Choose a reason for hiding this comment

0xd34df00d Oct 6, 2022

Choose a reason for hiding this comment

carlhammann commented Oct 7, 2022

carlhammann commented Oct 7, 2022

carlhammann commented Oct 14, 2022 • edited Loading

facundominguez commented Oct 14, 2022 • edited Loading

carlhammann commented Sep 2, 2022 •

edited

Loading

carlhammann Oct 5, 2022 •

edited

Loading

facundominguez left a comment •

edited

Loading

mmontin left a comment •

edited

Loading

carlhammann commented Oct 14, 2022 •

edited

Loading

facundominguez commented Oct 14, 2022 •

edited

Loading