Make estimateFee faster for large wallets

[Anviking]

Issue Number

ADP-692

Overview

• Unify selectAssets and selectAssetsNoOutputs
  • Idea was to allow estimateFee to call both readUTxOIndex and selectAssets — simplifying the call-sites — but I didn't end up doing this now. This seems right to me still, unless I have missed something?
• Separate out a readUTxOIndex from selectAssets
• Make sure readUTxOIndex is called only once, when repeating the coin selection 100x to estimate fees.

Comments

• Viewing diff commit-per-commit may be helpful

Instead of 260s, we get 5s estimateFee time:

[bench-restore:Info:7] [2021-02-04 14:17:40.76 UTC] Restoring: [still restoring (4.01%)]
restoration: 736.4 s

Running read wallet
read wallet: 4.359 s

Running utxo statistics
utxo statistics: 28.54 ms

Running list addresses
list addresses: 1.010 s

Running list transactions
list transactions: 25.22 s

Running estimate tx fee
estimate tx fee: 5.091 s

[wallet:Error:60] [2021-02-04 14:18:18.44 UTC] Unexpected error following the chain: StatementAlreadyFinalized "BEGIN"
[wallet:Notice:60] [2021-02-04 14:18:18.44 UTC] Destroying cursor connection at ThreadId 61
restore: StatementAlreadyFinalized "BEGIN"


All results:
BenchSeqResults:
  benchName: 90.0-percent-seq
  restoreTime: 736.4 s
  readWalletTime: 4.359 s
  listAddressesTime: 1.010 s
  listTransactionsTime: 25.22 s
  estimateFeesTime: 5.091 s
  walletOverview:
     number of addresses: 54498
     number of transactions: 34810
     = Total value of 25008452939161007 lovelace across 28241 UTxOs
      ... 10                18
      ... 100               142
      ... 1000              133
      ... 10000             153
      ... 100000            280
      ... 1000000           1674
      ... 10000000          1302
      ... 100000000         1369
      ... 1000000000        1768
      ... 10000000000       3139
      ... 100000000000      4682
      ... 1000000000000     9737
      ... 10000000000000    3634
      ... 100000000000000   202
      ... 1000000000000000  6
      ... 10000000000000000 2
      ... 45000000000000000 0

1,186,372,795,296 bytes allocated in the heap
 187,306,453,352 bytes copied during GC
     739,022,840 bytes maximum residency (626 sample(s))
      37,791,752 bytes maximum slop
             704 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0     1082118 colls, 1082118 par   541.175s  136.483s     0.0001s    0.0040s
  Gen  1       626 colls,   625 par   58.612s  14.913s     0.0238s    0.0730s

  Parallel GC work balance: 10.33% (serial 0%, perfect 100%)

  TASKS: 17 (1 bound, 16 peak workers (16 total), using -N4)

  SPARKS: 0(0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.002s  (  0.004s elapsed)
  MUT     time  582.358s  (638.775s elapsed)
  GC      time  599.788s  (151.396s elapsed)
  RP      time    0.000s  (  0.000s elapsed)
  PROF    time    0.000s  (  0.000s elapsed)
  EXIT    time    0.000s  (  0.001s elapsed)
  Total   time  1182.148s  (790.176s elapsed)

  Alloc rate    2,037,187,147 bytes per MUT second

  Productivity  49.3% of total user, 80.8% of total elapsed

Benchmark restore: FINISH
Completed 2 action(s).

[KtorZ]

Arg. Maybe I should have picked a better name in the first place because, the fact that there are no outputs is not actually what drives the body of this function. That selection really is selecting with delegation / key de-registration in mind. So it is more like selectAssetsForDelegation... Conflating this with the selectAssets function itself feels wrong and confusing :/

What exactly is the rationale for merging these?

[Anviking]

Unify selectAssets and selectAssetsNoOutputs
Idea was to allow estimateFee to call both readUTxOIndex and selectAssets — simplifying the call-sites — but I didn't end up doing this now. This seems right to me still, unless I have missed something?

[KtorZ]

I don't think the call site would be much more complicated by keeping them separate, no ?

[Anviking]

Right now we do:

wal <- unsafeRunExceptT $ W.readUTxOIndex @_ @s @k w wid
let runSelection = W.selectAssets @_ @s @k w wal txCtx (out :| []) getFee
runExceptT $ withExceptT show $ W.estimateFee runSelection

But I was thinking we could get to:

runExceptT $ withExceptT show $ W.estimateFee @_ @s @k w txCtx [out] getFee

With NoOutputs and Outputs represented by the list argument, we don't need to pass in the right variant of selectAssets. estimateFee could then call readUTxOIndex directly (but outside the loop).

[KtorZ]

But choosing the right variant of selectAssets is actually quite crucial because, it is the caller who knows what the right variant is. So this:

wal <- unsafeRunExceptT $ W.readUTxOIndex @_ @s @k w wid
let runSelection = W.selectAssets @_ @s @k w wal txCtx (out :| []) getFee
runExceptT $ withExceptT show $ W.estimateFee runSelection

Is much better.

[KtorZ]

(modulo the unsafeRunExceptT 😬 ...)

[rvl]

Looks good!

[rvl]

Suggested change

	guardWithdrawal pending' = do
	case Set.lookupMin $ Set.filter hasWithdrawal pending' of
	guardPendingWithdrawal = case Set.lookupMin $ Set.filter hasWithdrawal pending of

[rvl]

bors r+

[iohk-bors]

Build failed (retrying...):

• ci/hydra-build:required

      readPoolProduction should give pools with descending slots after consecutive 1-slot-depth rollbacks
        +++ OK, passed 100 tests.
      readPoolProduction should never give pools with no slots after rollback - arbitrary N-slot-depth rollbacks
        +++ OK, passed 100 tests (100% number of slots <= 10).
      readStakeDistribution . putStakeDistribution == pure
        +++ OK, passed 100 tests (4% Empty distributions).
building of '/nix/store/7aplr1wzf4vxrrh8gpzrf58by0m1rqvd-cardano-wallet-core-test-unit-2021.1.28-check' timed out after 900 seconds of silence

#2472

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:387:59:
  | 1) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_ESTIMATE_09 - Invalid amount, 1.5
  | uncaught exception: IOException of type ResourceVanished
  | fd:248: hFlush: resource vanished (Broken pipe)
  |  
  | To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_ESTIMATE_09 - Invalid amount/1.5/"

[Anviking]

bors r+

[rvl]

It's not a good day for merging so far ...

[iohk-bors]

Build succeeded:

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