Move important UTxO state transition functions to the top-level and test them

[sevanspowell]

This work is intended to allow future modifications to the UTxO state transition function (i.e. collateral) to be tested in isolation, at the unit level.

My changes are primarily to the nested function "applyTx". I have tried to keep my changes minimal:

• Use more verbose variable names in that function to make the purpose of each variable clear.
• More clearly delineate between transaction outputs that are unspent and transaction outputs "we know about" but may possibly be spent.
• Make some effort to separate the ideas of "knowledge of a UTxO" from "ownership of a UTxO" to make testing easier.
• Extract out the core state transition function from the existing "do lots" "applyTx" function and move it to the top-level.
• Move other useful functions to the top-level for the same reason.
• Test important functions in wallet model spec.
• Re-implement hasKnownInput/hasKnownOutput to make their implementations a little more clear and symmetric.

Comments

These changes make testing collateral much easier, we can just test the "applyTxToUTxO" function at the unit level, and adjust existing model tests as necessary.

Issue Number

ADP-1092

[jonathanknowles]

Suggestion: move this difference function to the UTxO module.

Justification: it's a function that operates purely on UTxO values, and could viewed as analogous to these difference functions:

• TokenBundle.difference
• TokenMap.difference
• Set.difference
• Map.difference
• ...

[sevanspowell]

Absolutely 👍

[jonathanknowles]

@sevanspowell and I had a discussion about the difference function.

We believe it might make more sense if this function were to have the following signature:

- difference :: UTxO -> UTxO     -> UTxO
+ difference :: UTxO -> Set TxIn -> UTxO

Reasons:

• The current implementation relies on finding the difference between two TxOut values in the event that a key appears in both UTxO sets.
• But it's not clear what useful answer we can obtain from differencing a pair of TxOut values. For example, if the pair of TxOut values have different addresses, then which address should we pick, and why?
• In all situations that we currently deal with, there should only be one possible TxOut for a given TxIn. (We shouldn't ever have a state where there are two possible TxOut values for a given TxIn.)
• If we assume that there is only one unique mapping for any given TxIn, it makes sense to constrain the type so that we're only subtracting a set of TxIn values (so the function does not need to deal with the odd case where TxOut values differ) from the key set of our UTxO parameter.

However, after making this simplification, we can see there is already a pre-existing function with these semantics:

excluding :: UTxO -> Set TxIn ->  UTxO
excluding (UTxO utxo) =
    UTxO . Map.withoutKeys utxo

Hence, in all places that currently use UTxO.difference, it might make more sense to call UTxO.excluding with just the set of TxIn values from the second map.

[jonathanknowles]

Hi Sam

I've already made a few comments. But to add to those:

1. I reckon it would be useful to add some coverage checks to the following properties:

   • prop_applyTxToUTxO_balance
   • prop_applyTxToUTxO_entries
   • prop_applyTxToUTxO_spendTx_utxoFromTx
   • prop_filterByAddress_balance_applyTxToUTxO
   • prop_spendTx_balance_inequality
   • prop_spendTx_balance
   • prop_spendTx_filterByAddress
   • prop_spendTx_isSubset
   • prop_spendTx
   • prop_utxoFromTx_balance
   • prop_utxoFromTx_is_unspent

2. There are several properties where we use forAll with genX and shrinkX for some type X, where we don't modify the generator in any way.

   I think we could simplify the code a bit by introducing Arbitrary instances (within ModelSpec) for UTxO, Tx, and so on. For example: https://github.com/input-output-hk/cardano-wallet/pull/2848/files#r700738862. Though I reckon it's a good idea to do this only after adding coverage checks, in case we need to alter the size parameter for any of the generators.

[jonathanknowles]

LGTM!

I've run the test suite with --qc-max-success 10000, and all the tests pass.

[jonathanknowles]

bors r+

[iohk-bors]

Build failed:

• ci/hydra-build:required

This one looks like a networking issue:

    BYRON_TX_LIST_03 - Shelley CLI does not list Byron wallet transactions (220ms)
    BYRON_TRANS_DELETE - Cannot delete tx on Byron wallet using shelley CLI (200ms)
    BYRON_TRANS_CREATE / BYRON_TRANS_ESTIMATE - Cannot create/estimate tx on Byron wallet using shelley CLI
      create (657ms)
UnexpectedFailure (MissingBlockError (EmptySlot (RealPoint (SlotNo 4724) d189e4debd0a5da70dbf6542a05920234532be37462a6ec2887573dd904714fa)))

cardano-node: The ImmutableDB got corrupted, full validation will be enabled for the next startup      fees (445ms)
  SHELLEY_CLI_WALLETS
retry: expected: Status {statusCode = 201, statusMessage = "Created"}
retry:  but got: Status {statusCode = 500, statusMessage = "Internal Server Error"}
retry:
retry: from the following response: Left (HttpException (ConnectionFailure Network.Socket.connect: <socket: 20>: does not exist (Connection refused)))
retry: Retrying failed test.
retry: Test failed again in the same way.

[sevanspowell]

bors retry

[iohk-bors]

Build failed:

• ci/hydra-build:required

Test failure:

            , tip = ApiBlockReference
                 { absoluteSlotNumber = ApiT
                     ( SlotNo 4574 )
                 , slotId = ApiSlotId
                     { epochNumber = ApiT
                         ( EpochNo
                             { unEpochNo = 45 }
                         )
                     , slotNumber = ApiT
                         ( SlotInEpoch
                             { unSlotInEpoch = 74 }
                         )
                     }
                 , time = 2021 - 09 - 03 08 : 33 : 40.8 UTC
                 , block = ApiBlockInfo
                     ( Quantity 2201 )
                 }
             }

       Waited longer than 90s to resolve action: "Wallet balance is as expected".
       expected: Quantity 999978
        but got: Quantity 0

  To rerun use: --match "/CLI Specifications/SHELLEY_CLI_HW_WALLETS/HW_WALLETS_01x - Restoration from account public key preserves funds/"

Randomized with seed 2141484065

I tried this locally, and it passes. So will retry.

Looks like:
#2855

[jonathanknowles]

bors retry

[iohk-bors]

Build succeeded:

• ci/hydra-build:required
• buildkite/cardano-wallet