simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage #2112

KtorZ · 2020-09-04T08:52:05Z

Issue Number

#2051

Overview

I've added scenarios for Byron and Icarus keys
I've added a 'within' clause to some scenarios in order to make sure that the properties are executed within a reasonable time.

Comments

This didn't really improve execution time significantly, but it is still useful to keep.

Anviking

Looks good; a few comments

Anviking · 2020-09-04T09:58:50Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

-        upperBound_ !n | isTooBig n = n
-                       | otherwise  = upperBound_ (n*growingFactor)
+    findMax :: Word8 -> Word8
+    findMax inf


findMax minBound first looks deceptively simple... until you start looking at it. Can we give give an overview about what is going on here? E.g. Find the largest amount of inputs that doesn't make the tx too big. Tries in sequence from 0 and upward.

Also, btw, why did you decide to remove the bisection? Just simplicity?

Also I think it might make sense to take the condition explicitly (see other comment).

Also, btw, why did you decide to remove the bisection? Just simplicity?

I originally suspected that the bisection could have been causing some slowness, taking to long to find a fix point. I actually measured the time and observed some very long delays until I realized I was measuring the wrong thing. In the meantime, I had changed the implementation to findMax which turned out to be slightly master but also much simpler so I kept it.

Anviking · 2020-09-04T10:00:41Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

-                       | otherwise  = upperBound_ (n*growingFactor)
+    findMax :: Word8 -> Word8
+    findMax inf
+        | inf == maxBound    = 0


What does inf == maxBound mean? If 1 input would be too much, findMax 0 should return 0. But it also returns 0 if all possibilities pass? That seems like an error. (If the max tx size is really big...)

Actually you're right, this should just return maxBound 👍

Anviking · 2020-09-04T10:01:14Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    findMax inf
+        | inf == maxBound    = 0
+        | isTooBig (inf + 1) = inf
+        | otherwise          = findMax (inf + 1)

    isTooBig nInps = size > fromIntegral maxSize


What would you think about

Calling this txTooBig

Passing it explicitly to the findMax function

Sounds good.

Okay, but why ? What's the added benefit in this context?

I think findMax :: Word8 -> Word8 is a kind-of deceptive name and type-signature. It sounds abstract (at first glance), but it is not. I believe it is meant more as a helper of estimateMaxNumberOfInputs.

I think it should either be abstract, or have a more specialised name.

I think explicitly showing the dependency on the txTooBig condition might be clearer in both cases, but in particular, it would make findMax abstract (assuming no other details specific to this use-case).

Modulo, if the type-signature becomes too ugly, perhaps.

Anviking · 2020-09-04T10:19:21Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

-    :: Cardano.NetworkId
+    :: forall k. (TxWitnessTagFor k, Typeable k)
+    => Cardano.NetworkId
+    -> [(Word8, Word8)]


Doc comment? 🙏

I think a name like estimateMaxInputsGivenOutputsTests would make the call-site tuples more easy to understand too.

I think a name like estimateMaxInputsGivenOutputsTests would make the call-site tuples more easy to understand too.

Ok, I see estimateMaxInputsTests is not only goldens. Then I'm less sure...

estimateMaxInputsGivenOutputsTests

Estimating max inputs is always given some outputs though.

Yes, but I think these tests should be easily understood without being 100% sure about that.

Adding a doc comment to [(Word8, Word8)] definitely helps.

But my idea with estimateMaxInputsGivenOutputsTests was to hint the same, already at the call-site, similar to the impossible [(outputs: 1, maxInputs: 2)], or the perhaps overkill EstimateMaxInputsGolden { outputs, maxInputs :: Word8 }.

Anviking · 2020-09-04T10:25:01Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

@@ -162,23 +183,25 @@ spec = do
        res `shouldBe` Right (FeeEstimation 165413 165413)

 estimateMaxInputsTests
-    :: Cardano.NetworkId
+    :: forall k. (TxWitnessTagFor k, Typeable k)


Anviking · 2020-09-04T10:27:28Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

-        _estimateMaxNumberOfInputs @ShelleyKey net size Nothing nOuts
+prop_moreOutputsMeansLessInputs net size nOuts
+    = withMaxSuccess 1000
+    $ within 100000


I am slightly worried about Windows here which it notably slower. I can go down to 50000 on my machine without issue, so I took an order of magnitude higher just to be sure.

KtorZ · 2020-09-07T13:01:50Z

bors r+

2112: simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage r=KtorZ a=KtorZ # Issue Number  #2051 # Overview  - I've added scenarios for Byron and Icarus keys - I've added a 'within' clause to some scenarios in order to make sure that the properties are executed within a reasonable time. # Comments  This didn't really improve execution time significantly, but it is still useful to keep.  Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-09-07T13:16:20Z

Canceled.

KtorZ · 2020-09-07T13:16:28Z

bors r+

2112: simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage r=KtorZ a=KtorZ # Issue Number  #2051 # Overview  - I've added scenarios for Byron and Icarus keys - I've added a 'within' clause to some scenarios in order to make sure that the properties are executed within a reasonable time. # Comments  This didn't really improve execution time significantly, but it is still useful to keep.  Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-09-07T14:28:42Z

Build failed:

buildkite/cardano-wallet

KtorZ · 2020-09-07T16:02:29Z

bors r+

2112: simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage r=KtorZ a=KtorZ # Issue Number  #2051 # Overview  - I've added scenarios for Byron and Icarus keys - I've added a 'within' clause to some scenarios in order to make sure that the properties are executed within a reasonable time. # Comments  This didn't really improve execution time significantly, but it is still useful to keep.  Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-09-07T16:17:47Z

Build failed:

buildkite/cardano-wallet

…erage - I've added scenarios for Byron and Icarus keys - I've added a 'within' clause to some scenarios in order to make sure that the properties are executed within a reasonable time.

KtorZ · 2020-09-08T08:41:34Z

bors r+

iohk-bors · 2020-09-08T10:59:43Z

Build succeeded:

KtorZ requested a review from Anviking September 4, 2020 08:52

KtorZ self-assigned this Sep 4, 2020

KtorZ added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Sep 4, 2020

KtorZ force-pushed the KtorZ/2051/investigate-slow-fee-estimation branch from 7c6e87e to 8df8b86 Compare September 4, 2020 08:56

Anviking reviewed Sep 4, 2020

View reviewed changes

KtorZ force-pushed the KtorZ/2051/investigate-slow-fee-estimation branch from 8df8b86 to 2427e60 Compare September 7, 2020 12:48

Anviking approved these changes Sep 7, 2020

View reviewed changes

KtorZ force-pushed the KtorZ/2051/investigate-slow-fee-estimation branch from 692d376 to dba03b6 Compare September 7, 2020 16:02

KtorZ added 2 commits September 8, 2020 09:07

simplify implementation of 'estimateMaxNumberOfInputs' & increase cov…

5f84ee9

…erage - I've added scenarios for Byron and Icarus keys - I've added a 'within' clause to some scenarios in order to make sure that the properties are executed within a reasonable time.

review wording and add comments to clarify estimateMaxNumberOfInputs

0ea0242

KtorZ force-pushed the KtorZ/2051/investigate-slow-fee-estimation branch from dba03b6 to 5d7e3c6 Compare September 8, 2020 07:46

remove unicode character causing Windows to go crazy

0b58616

KtorZ force-pushed the KtorZ/2051/investigate-slow-fee-estimation branch from 5d7e3c6 to 0b58616 Compare September 8, 2020 08:38

iohk-bors bot merged commit 3feb1d0 into master Sep 8, 2020

iohk-bors bot deleted the KtorZ/2051/investigate-slow-fee-estimation branch September 8, 2020 10:59

Anviking mentioned this pull request Oct 1, 2020

Estimating fees on empty Shelley wallet takes 5-6s #2170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage #2112

simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage #2112

KtorZ commented Sep 4, 2020

Anviking left a comment

Anviking Sep 4, 2020 •

edited

Loading

Anviking Sep 4, 2020

KtorZ Sep 4, 2020

Anviking Sep 4, 2020 •

edited

Loading

KtorZ Sep 4, 2020

Anviking Sep 4, 2020

KtorZ Sep 4, 2020

Anviking Sep 4, 2020 •

edited

Loading

Anviking Sep 4, 2020

Anviking Sep 4, 2020

KtorZ Sep 4, 2020

Anviking Sep 4, 2020 •

edited

Loading

Anviking Sep 4, 2020

Anviking Sep 4, 2020

KtorZ Sep 4, 2020 •

edited

Loading

KtorZ commented Sep 7, 2020

iohk-bors bot commented Sep 7, 2020

KtorZ commented Sep 7, 2020

iohk-bors bot commented Sep 7, 2020

KtorZ commented Sep 7, 2020

iohk-bors bot commented Sep 7, 2020

KtorZ commented Sep 8, 2020

iohk-bors bot commented Sep 8, 2020

simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage #2112

simplify implementation of 'estimateMaxNumberOfInputs' & increase coverage #2112

Conversation

KtorZ commented Sep 4, 2020

Issue Number

Overview

Comments

Anviking left a comment

Choose a reason for hiding this comment

Anviking Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

KtorZ commented Sep 7, 2020

iohk-bors bot commented Sep 7, 2020

KtorZ commented Sep 7, 2020

iohk-bors bot commented Sep 7, 2020

KtorZ commented Sep 7, 2020

iohk-bors bot commented Sep 7, 2020

KtorZ commented Sep 8, 2020

iohk-bors bot commented Sep 8, 2020

Anviking Sep 4, 2020 •

edited

Loading

Anviking Sep 4, 2020 •

edited

Loading

Anviking Sep 4, 2020 •

edited

Loading

Anviking Sep 4, 2020 •

edited

Loading

KtorZ Sep 4, 2020 •

edited

Loading