Jörmungandr binary roundtrips

[Anviking]

Issue Number

#219

Overview

• I've added roundtrips testing decode (encode signedTx) === signedTx and the same for addresses.

Comments

[KtorZ]

The two functions above are dodgy :s ... What's wrong with using runGet ?

[Anviking]

What's wrong with using runGet

Not sure what you're referring to here, but runGet throws exceptions. And it seemed easier to not have to use the different api of Test.QuickCheck.Monadic when I was starting this. Hence the unsafePerformIO and Control.Exception.try.

There is actually an Either-returning version of runGet. But none for runPut.

[KtorZ]

Seems like we are re-encoding code source logic into tests which is a sign that either of them are wrong. If the source can't be easily tested, that's a problem. Or maybe, the test isn't testing at the right level of abstraction.

[Anviking]

Yes! I had a thought about remaking this function to

validateSignedTx :: (Tx, [TxWitness]) -> Either ErrSignedTxValidation (Tx, [TxWitness])

, moving it and calling it in mkStdTx. We could test that decode (encode signedTx) throws iff validateSignedTx returns Left.

Although then it would be tempting to have jormungandr-specific types:

data SignedTxIn = SignedTxIn Address Word8 Coin TxWitness

data SignedTx = SignedTx
    { signedInputs :: [SignedTxIn]
    , outputs :: [TxOut]

and make the encoders work with them… 🤔

[KtorZ]

I am not sure this function should even exist (cf comments on the other PR). Instead:

• generators for property-based tests should generate valid values
• there should be some "negative tests" showing that, for invalid transaction types, the serialization fails.

[Anviking]

To begin with:
upon further reflection, I think there are two important properties:

-- Note: tx :: (Tx, [TxWitness])) below

-- **Jörmungandr-valid transactions roundtrip**
forall tx. valid tx ==>  decode (encode tx) === tx

-- **We must not be able to encode a jörmungandr-invalid transaction
-- to valid binary**. "Valid binary" means that we can decode it.
--
-- So while we don't care about general Tx round-tripping, we care about 
-- the result either 1) not being a 'Tx' or 2) being Jörmungandr-valid, in which
-- case it must be the same tx as we started with:
forall tx. case (decode (encode tx)) of
                   Right tx' -> tx' == tx
                   Left _ -> True -- we don't care

Sketch on the topic:

[Anviking]

And well, then we need the definition of a [jörmungandr-]valid transaction in one way or another. @KtorZ thoughts?

[Anviking]

Oh I forgot one point. By labelling the errors - either via error messages in the Binary module or in the valid function we can know that we are generating interesting enough jörmungandr-invalid transactions.

[KtorZ]

Hmmm... valid tx ==> decode (encode tx) is an invalid use-case for the pre-condition. Just make your generator valid in the first place! If you want to test invalid transaction, then make a generator which generates invalid ones... though, this is a bit counter-productive with having a type-system in a first place.

The idea of a strongly typed system is to make sure that, having something of a given type gives us some guarantees and invariant about this type. So, by considering a Tx, we are assuming that it is well-formed, otherwise, we are better of having a ([TxIn], [TxOut]) and makes no assumption about the number of inputs and outputs.

We have properties checking that our coin selection preserve such invariant (and only construct valid txs). We can assume something similar for transaction coming out of Jörmungandr. However, there's nothing wrong with expressing those invariants in code and have some explicit assertions which would loudly fail in case they are violated. It's usually better to encode in the type system whatever we can, although here, we are really deserializing bytes from a trusted source, and there's not much we could do apart from crashing if a tx was invalid.

[Anviking]

I guess I was paranoid about the "strong typed system" being too weak and the safetynet failing us 🤔

Anyhow --> #476 (comment)

[KtorZ]

Not sure if the shrinker buys us anything here. Bytes are just ... bytes, there's no sensible to shrink them (in that context at least).

[Anviking]

I wanted the failing test cases to be less noisy by shrinking to all zeros, and I didn't want to generate only zeros.

[KtorZ]

Not sure if genericArbitrary does preserve Bounded instances, does it ?

[Anviking]

Not sure, but I wrote a custom one now.

[Anviking]

Generators now only generate valid transactions. Potential remaining points for objections:

• Use of unsafePerformIO instead of monadicIO
• Shrinkers - we could remove them

[KtorZ]

I am not sure to get the need for this one? The formers:

        [SignedTx (Tx ins' outs, wits')
        | (ins', wits') <- unzip <$> shrinkList' (zip ins wits)] ++
        [SignedTx (Tx ins outs', wits) | outs' <- shrinkList' outs ]

will already shrink outputs to very reasonable counter examples, won't they?

[Anviking]

shrinkList' doesn't shrink the elements of the lists. The way it is now that's not guaranteed either, but it might shrink the elements 😬

So we… could remove most of the other shrinkers and we'd be kind of fine, keep them as is, or we could make them perfect 🤔

[KtorZ]

Have you tried the shrinker with only:

        [SignedTx (Tx ins' outs, wits')
        | (ins', wits') <- unzip <$> shrinkList' (zip ins wits)] ++
        [SignedTx (Tx ins outs', wits) | outs' <- shrinkList' outs ]

and observed the output ?

[Anviking]

Yes…

coin = Coin {getCoin = 34180938945809574}},TxOut {address = Address {unAddress = "\ETXEx\\'>v56F\vx\NUL\EOT,Zn~\SOre<]\DC1Q8\DC4\\\DLEw\ETB\t\DLE"}, coin = Coin {getCoin = 44999999999999985}},TxOut {address = Address {unAddress = "\ETXx\vl\DC3&Z\vX1]S\ETXeo_A\CAN]Y\DC3\DC2=v\SOebqzO-QK"}, coin = Coin {getCoin = 15}},TxOut {address = Address {unAddress = "\ETX#\SOHi_:\NUL>Of\STXrc.\NAK??\EMqVk2_1\SO|1jM]~\128\RS"}, coin = Coin {getCoin = 44999999999999906}},TxOut {address = Address {unAddress = "\ETXF\SYN\CAN\STXZFp\r$<X\SI\ACKME6\DC34!)\ESCD\STX\SO0-\DEL8[yE\a"}, coin = Coin {getCoin = 57}},TxOut {address = Address {unAddress = "\ETXAyxP[2\DLENY7|q[<T@\NUL0\RSI&\ETB\USj[.N$\EM\ETX\128\FS"}, coin = Coin {getCoin = 3773066673805617}},TxOut {address = Address {unAddress = "\ETXUv\US\FSVRG$e\DEL\EOTXm3N~u!]\EMR3\f\RSY|#<*O'\FS"}, coin = Coin {getCoin = 44}},TxOut {address = Address {unAddress = "\ETX\DC2\\\fxeSH\DC3EqIt!\NAK*l/$R\128cP\128}\NAKDFV\ACK_*\EM"}, coin = Coin {getCoin = 44999999999999929}},TxOut {address = Address {unAddress = "\ETXE\DEL23u\SOS HAKuf9\USS\DEL[>D~\ENQqSP=\STX\GS U6f"}, coin = Coin {getCoin = 44999999999999965}},TxOut {address = Address {unAddress = "\ETX\b'?r!!*&<y>K1\EOT#9G=BrW\SYN=;\DC3D+P\DC2#\EM\""}, coin = Coin {getCoin = 75}},TxOut {address = Address {unAddress = "\ETX6gC~U+\\a06\SOi#\atOsp\ETX#H`\fEwZzc\br\"6"}, coin = Coin {getCoin = 6}},TxOut {address = Address {unAddress = "\ETXO\NAKf\SUB5\128Js!3 \SOH\DLEv|^\DC21~\ETXx+m7vLV`Z\DC3@\n"}, coin = Coin {getCoin = 869739039304620}},TxOut {address = Address {unAddress = "\ETX2#\v.Fo]A8Mn<n;.\SYN`\vy\STXkf$'\ENQ?M0wbs7"}, coin = Coin {getCoin = 90}},TxOut {address = Address {unAddress = "\ETX\DC1.dTDX_}x1 x\SI'i\USLbTieB`&\DEL\"\DC3@\DC4=.C"}, coin = Coin {getCoin = 93}},TxOut {address = Address {unAddress = "\ETXZp\128&\DC2`\n`T\ACKW0O\"R\bdv}%:\GS,9\nMSG\n\rf\ESC"}, coin = Coin {getCoin = 43970056871992574}},TxOut {address = Address {unAddress = "\ETXB-,+f9#p]Z\DC1A*x\ENQYG`\DC26nioWa\RSnv2k\SOHG"}, coin = Coin {getCoin = 20}},TxOut {address = Address {unAddress = "\ETXQ\b\\+\SO{BoCJ\ETB\STX8g\SIsJg\"G4npxB\r\DLEqJ\NUL\NAK)"}, coin = Coin {getCoin = 17218733216125888}},TxOut {address = Address {unAddress = "\ETX\ENQNbp0\EMf\NUL\ETBTj\US\CAN\NULav_\128\&2~N/O\ACKc|_\\=b\a\CAN"}, coin = Coin {getCoin = 3022692135060161}},TxOut {address = Address {unAddress = "\ETXFNF6zh\DC3\SYN\DC1|g{ +7\FS#Z\EOT[#68W\NULrWmD\ETXYw"}, coin = Coin {getCoin = 44999999999999987}},TxOut {address = Address {unAddress = "\ETXgG\tv\v-XkL\DC4MbWW>\f^hR\ap\NAK`\a\FS(I\US1-1\US"}, coin = Coin {getCoin = 44999999999999933}},TxOut {address = Address {unAddress = "\ETX/x\SYN03\DC1C.Wv\ENQ\ACKK.\CAN~u\SIZ\SYNN:\DC41\FSjS`c\SIMD"}, coin = Coin {getCoin = 86}},TxOut {address = Address {unAddress = "\ETXX;> \fupz\SI#>&(6\RSFD]\DLE\USi\CAN\SYN\GSA`\DC3.ds9\DC4"}, coin = Coin {getCoin = 34000846781260719}},TxOut {address = Address {unAddress = "\ETXRvi$y\SOH\DC1^\DEL~sbF8[:7zcD`\EOT\ACK\128&ZQ(}+\SOH)"}, coin = Coin {getCoin = 16227796498456957}},TxOut {address = Address {unAddress = "\ETX\nX\NAK\EM\GS&\US\ACK\CAN0v(9$0DVw-\f\b\b\\\NULF?!Ig\v?K"}, coin = Coin {getCoin = 44999999999999904}},TxOut {address = Address {unAddress = "\ETX`MIo[ZI}kO\STX Y>\SUBCW\ESCR=Jw&t'0Hh*\DC2f&"}, coin = Coin {getCoin = 2975782956370087}},TxOut {address = Address {unAddress = "\ETXP5+Xal\ACKm4\DELI$TCS6 MNEpT4$\DEL(u=C`P*"}, coin = Coin {getCoin = 35297643722518077}},TxOut {address = Address {unAddress = "\ETXi+eK\DLE.6:NpX<\fg$\SI\DC4LA\SUByE\DC3\SUBJ\DEL*~\fJ#h"}, coin = Coin {getCoin = 44999999999999961}},TxOut {address = Address {unAddress = "\ETXwM{k\r\ENQ<<$\v\NULC\b2D^\t\SI\SYN2TG!6T|z\EMK\ETX%\ETB"}, coin = Coin {getCoin = 49}},TxOut {address = Address {unAddress = "\ETXK\"*@\NAKl\STXL4\SYNcLr!\ENQp?y#\DLE3[AB\GS1+s\DC1j\DC4\t"}, coin = Coin {getCoin = 1612748889065292}},TxOut {address = Address {unAddress = "\ETX[^\128\NAK{\FSL)7p\CAN8ZcL@Nra~/G\SYNcyc.\DC3E\b~~"}, coin = Coin {getCoin = 34149941902569273}},TxOut {address = Address {unAddress = "\ETXw(wr\GS0\n<\ENQp\DEL\EMofs\CAN\RS_*F\NUL4\SO'yX\SIfz]&\a"}, coin = Coin {getCoin = 9478177015978517}},TxOut {address = Address {unAddress = "\ETXx%{WU\SI\SO\ACK\\\US%\"yB\EM\nEBMsE` EyE^2O\SUBBT"}, coin = Coin {getCoin = 21}},TxOut {address = Address {unAddress = "\ETXXiQj>rW\ETBdI';}\t\EM<\DELX/:8\"=4\ETBZ\v'H\ETX;\NAK"}, coin = Coin {getCoin = 6205270086029695}},TxOut {address = Address {unAddress = "\ETXm\ETB\\!f\ESC\RS\ETX\DC1Sop\DELQo\a\EMO`@\US\n]FGSFbb\rx\DC4"}, coin = Coin {getCoin = 44999999999999930}},TxOut {address = Address {unAddress = "\ETXI?\ESC\ACKht01u;U\DEL,\EOT\SOV+9\ETX&G\ACK\SI\tS6S\EMFwN\DC1"}, coin = Coin {getCoin = 8858930130137839}},TxOut {address = Address {unAddress = "\ETX6XO\aK(\EOT*N\ETX>p/vYE-9D@WU\CAN\NAKM9\DC2R*W,C"}, coin = Coin {getCoin = 52}},TxOut {address = Address {unAddress = "\ETX(bHo\NUL_S9%|#!vl+c\DC3\DC3G,(=s\NULKceg\r\RS]`"}, coin = Coin {getCoin = 89}},TxOut {address = Address {unAddress = "\ETX0\128(N|!0x*2X\EMd6Z\"xL\"znIH7}D\SOHEN-o\ESC"}, coin = Coin {getCoin = 44999999999999909}},TxOut {address = Address {unAddress = "\ETX,<CY\SO\b4/\FSkUj\SI\DC4_\ETB\USM/MU\EOTj-\DC2O,)g\DC3s\SOH"}, coin = Coin {getCoin = 19498067338884164}},TxOut {address = Address {unAddress = "\ETX\CAN\RS\SI\SYNkRi p.9a~S>jKR\DC3\DELB\DELM3\DELpn[\ETX\DC4o-"}, coin = Coin {getCoin = 1748180663868147}},TxOut {address = Address {unAddress = "\ETX~\rN!\t?EM'4s\ETXgR|T,>EX\"D`6mRBbN4B\DC3"}, coin = Coin {getCoin = 88}},TxOut {address = Address {unAddress = "\ETX`3'=\DLEJ}z`\NAKvbpi[\DLE\f\CANTJ-W|\DC1P\DC38?w6AI"}, coin = Coin {getCoin = 79}},TxOut {address = Address {unAddress = "\ETX\ENQylp*\ETB\\Nzh\vg$;\DLE\EOTDy4\SUB\USG!g\\\NAKM\DC2(+7\FS"}, coin = Coin {getCoin = 44999999999999944}},TxOut {address = Address {unAddress = "\ETXJJ\a,uu}C?x[AW\SOH*mR\EM\ETXF:FHXM <\ETXC\EMa("}, coin = Coin {getCoin = 3}},TxOut {address = Address {unAddress = "\ETXq15Drmo\CAN#hU=F)NZ{E\f7//3r\ACK8\STXi5ts-"}, coin = Coin {getCoin = 20}},TxOut {address = Address {unAddress = "\ETXb\t\"w\ACK5q=\SYNHX\DC1\t>SmWW\SI\t6oG\STX\FSFbh:7\DC4 "}, coin = Coin {getCoin = 48}},TxOut {address = Address {unAddress = "\ETXQE?|?w&i!h\SIN\DC2L'RZ!}m\DC3~\ETBu= \CAN~&wvb"}, coin = Coin {getCoin = 44999999999999905}},TxOut {address = Address {unAddress = "\ETX853-\DLE\SOH\nU\"\SUB8=djlT@KYa*m/h\DC1\US\DC3/g[\SI@"}, coin = Coin {getCoin = 33174635739604593}},TxOut {address = Address {unAddress = "\ETXc{@a'2Qoxv<MQ\DC3'\ESC\DC28#zj\ETB J\128[\128\RS\ETXRB`"}, coin = Coin {getCoin = 44999999999999959}},TxOut {address = Address {unAddress = "\ETX74}d}{\ETX*\\smsA*X\EM^\NAK\ACK\DC1`T*5\ESCD\128\\\FSYn\ENQ"}, coin = Coin {getCoin = 11}},TxOut {address = Address {unAddress = "\ETXAZ\SOX'l[0\DC2P\EOTK\SUB.\STX\DLE.\fX\SO!y(2;\SOHFVz\GS.\ACK"}, coin = Coin {getCoin = 33876087739400020}},TxOut {address = Address {unAddress = "\ETX\v}\DC1ivJ\RS;{\GS;_`:B\CANI`0,\a+\ESC_uM_T7,!4"}, coin = Coin {getCoin = 44999999999999973}},TxOut {address = Address {unAddress = "\ETX%aUl~\\\v-;/#,MQ\128\EM\DC2\SYN\ENQys\SI\fWs}K9\SOH}-\f"}, coin = Coin {getCoin = 44999999999999919}},TxOut {address = Address {unAddress = "\ETX\CANs\v*?>u(wc^|It[\128}O}F/1u\t.oA\b*c\v\NAK"}, coin = Coin {getCoin = 44999999999999990}},TxOut {address = Address {unAddress = "\ETX>G`C\SO5\DEL[J\DC1 7D|h*\NUL^)ii]\DEL2!/N\ACK.Y\RSm"}, coin = Coin {getCoin = 0}},TxOut {address = Address {unAddress = "\ETX\SUB\STXI(\DC4\asz,\DLEG&\DC1|\FSe]V\DEL~1 hI\ACKFU[#\SUB<r"}, coin = Coin {getCoin = 22611204891216084}},TxOut {address = Address {unAddress = "\ETX;m&\SO8-\DC2NQ\t\SOHX\SI\FS Z>wIB1+g>P\t^O5$)$"}, coin = Coin {getCoin = 67}},TxOut {address = Address {unAddress = "\ETXH0|U\GS4]\EOTU2h9\ENQ\DC2\NAK\STX>A@EA\NULW\FSl\rB\ETXA\SIsE"}, coin = Coin {getCoin = 42296183007052279}},TxOut {address = Address {unAddress = "\ETXg+z\SYN\r\RS\DLEG_uRx$\DC3\EM\"V\SYNPIA9g)\EM\ENQ\DC4\SI=w\NULo"}, coin = Coin {getCoin = 52}},TxOut {address = Address {unAddress = "\ETX\ACK\SOHqyn^\128\a\NAKh}y[M\EOT\NAK\CANC\ETB=Y\FS\SOH7N\EOTo-,<\FS\NUL"}, coin = Coin {getCoin = 30784557493257846}},TxOut {address = Address {unAddress = "\ETX^O3\EM\DC3QG\"2\DC4\DLE;KIuja6\ETBR(\GS\f/UL\SOH\DEL\r\r\EOT\n"}, coin = Coin {getCoin = 83}},TxOut {address = Address {unAddress = "\ETXw\128@PFSTi~*S0,5G \GS/*i\FS2\ETX\RSi\DC1UAF#x\EM"}, coin = Coin {getCoin = 44999999999999954}},TxOut {address = Address {unAddress = "\ETX\CANq~\SYNvixw\DC4\ETBv|\SO&\SO6t B8L\GS,\EOT|y\NAKx4\SUB\\9"}, coin = Coin {getCoin = 50}},TxOut {address = Address {unAddress = "\ETX\EOTKx\DC3-\vDO3~YUOyD_m7)vA\a9,UTC71 \FSv"}, coin = Coin {getCoin = 25}},TxOut {address = Address {unAddress = "\ETX\\U4f\EOT\DC1\t6=\tpPOB(bjQP;*s]\SUB\EOT4\CANl\DC3\ACKsX"}, coin = Coin {getCoin = 44999999999999960}},TxOut {address = Address {unAddress = "\ETX\SI-\ENQt\"9\\zh,!\bdU\\LT\DEL4\STXyZ\\awW\a8\n\v>-"}, coin = Coin {getCoin = 44999999999999922}}]},[TxWitness {unWitness = "\ETB*K\bb\US=\128Y\FS\ETB88V:)\n\STXPR\SYN\FSh60\ESC\ESCYH_!\DC3&\NUL\SOr61 E\STX\ah;za\SUB\DC1DxS\ETXD7'sL\RSC\EMc/jz"}])

Definition of shrinkList':

        shrinkList' xs  = filter (not . null)
            [ take n xs | Positive n <- shrink (Positive $ length xs) ]

[Anviking]

Vanilla shrinkList works, but it is very slow. With a slight tweak it seems perfect though!:

        shrinkList' xs  =
            (shrinkHeadAndReplicate shrink xs) ++
            (shrinkList shrink xs)

        -- Try shrinking the 'head' of the list and replace the elements in
        -- the 'tail' with the exact same element. If the failure is related
        -- to the size of the list, this makes the shrinking much faster.
        shrinkHeadAndReplicate f (x:xs) =
            f x >>= \x' -> return $ x':(map (const x') xs)
        shrinkHeadAndReplicate _f [] = []

[KtorZ]

I mean, the point of a shrinker is to be applied repeatedly... The shrinkList above does a fair job at shrinking...

let shrinkOne = fmap fst . uncons . shrink
let shrinks = take 10 $ iterate (>>= shrinkOne) (pure tx)
TIO.putStrLn $ pretty (blockListF shrinks)

- ~> 2nd 101445f5...4f64892e
  ~> 2nd 70677272...436a0003
  ~> 1st ac184923...db215619
  ~> 2nd 6e567f73...1e3056fc
  ~> 1st 0a58186e...3c556037
  <~ 63086 @ 41444452...44523034
  <~ 22649 @ 41444452...44523034
  <~ 59611 @ 41444452...44523033
  <~ 88300 @ 41444452...44523031
  <~ 95915 @ 41444452...44523033

- ~> 2nd 101445f5...4f64892e
  ~> 2nd 70677272...436a0003
  ~> 1st ac184923...db215619
  <~ 63086 @ 41444452...44523034
  <~ 22649 @ 41444452...44523034
  <~ 59611 @ 41444452...44523033
  <~ 88300 @ 41444452...44523031
  <~ 95915 @ 41444452...44523033

- ~> 2nd 101445f5...4f64892e
  ~> 2nd 70677272...436a0003
  <~ 63086 @ 41444452...44523034
  <~ 22649 @ 41444452...44523034
  <~ 59611 @ 41444452...44523033
  <~ 88300 @ 41444452...44523031
  <~ 95915 @ 41444452...44523033

- ~> 2nd 101445f5...4f64892e
  <~ 63086 @ 41444452...44523034
  <~ 22649 @ 41444452...44523034
  <~ 59611 @ 41444452...44523033
  <~ 88300 @ 41444452...44523031
  <~ 95915 @ 41444452...44523033

- ~> 2nd 101445f5...4f64892e
  <~ 63086 @ 41444452...44523034
  <~ 22649 @ 41444452...44523034
  <~ 59611 @ 41444452...44523033

- ~> 2nd 101445f5...4f64892e
  <~ 63086 @ 41444452...44523034
  <~ 22649 @ 41444452...44523034

- ~> 2nd 101445f5...4f64892e
  <~ 63086 @ 41444452...4452303

It's true that it requires quite a few steps. If you have really big transactions, it could be nice to make it slightly faster.

[KtorZ]

Why not simply runPut and runGet? Since you're testing happy path.. let it throw in case of unexpected error. unsafePerformIO is not really something we want to use when we can avoid it and here clearly, there's no need.

Note that (since you mentioned it somewhere) using catch and a monadicIO property isn't much more work:

property $ \(SignedTx signedTx) -> monadicIO $ liftIO $ do
    return (runPut ...) `catch` (\MyException -> ....)

[Anviking]

🤔 Don't we lose failing-test-case-output without === ?

I guess this is still a roundtrip so we would be using like assert (tx' == tx) still?

[KtorZ]

Hmmm.. why would we loose them? In case of failure, we can call something like expectationFailure with a clear error message.

[Anviking]

shouldBe seemed to work well, except for hanging for ~40 s when printing large outputs. I suspect it's trying to find the minimal-edit-distance.

So went with:

                if tx' == Right signedTx
                then return ()
                else expectationFailure $
                    "tx /= decode (encode tx) == " ++ show tx'

resulting in the following output: