Coin Selection Better Coverage + Fix Fee Calculation #196
Conversation
| + BS.length bytes | ||
| + sizeOf (CBOR.encodeWord32 $ crc32 bytes) | ||
| + sizeOfCoin c | ||
| 1 + BS.length bytes + sizeOfCoin c |
There was a problem hiding this comment.
So, turns out those 5 bytes + the crc are already part of the bytes payload! So we don't need to count them twice!
| -- We always go for the higher bound for change address payload's size, | ||
| -- so, we may end up with up to 4 extra bytes per change address in our | ||
| -- estimation. | ||
| margin = 4 * fromIntegral (length $ CS.change sel) | ||
| realFeeSup = ceiling (a + b*(fromIntegral size + margin)) | ||
| realFeeInf = ceiling (a + b*(fromIntegral size)) | ||
| in | ||
| property (calcFee >= realFeeInf && calcFee <= realFeeSup) | ||
| (calcFee >= realFeeInf && calcFee <= realFeeSup, encodedTx) | ||
| === (True, encodedTx) |
There was a problem hiding this comment.
No change in the test logic here, but we'll now display the full transaction binary representation upon failure which is quite handy for debugging!
| mainnetA <- genAddress (39, 43) | ||
| testnetA <- genAddress (46, 50) | ||
| mainnetA <- genAddress (33, 33) | ||
| testnetA <- genAddress (40, 40) |
There was a problem hiding this comment.
This ought to be only the internal payload of the address. So, same confusion as within the fee calculation but, on the generation this time. The error was hard to catch because this one actually compensate for the previous one in the implementation. So, only in rare cases (1 tests out of 33k!), the double-counting would not be compensated by the wrong generation here and trigger an error!
| @@ -299,7 +300,7 @@ spec = do | |||
|
|
|||
| describe "Fee Estimation properties" $ do | |||
| it "Estimated fee is the same as taken by encodeSignedTx" | |||
| (withMaxSuccess 1000 $ property propFeeEstimation) | |||
| (withMaxSuccess 2500 $ property propFeeEstimation) | |||
Turns out the CBOR surrounding encoding is actually already included in the payload size! So we don't need to count it twice.
KtorZ
force-pushed
the
KtorZ/coin-selection-better-coverage
branch
from
2fea66c
to
6f4155b
Compare
It will be removed anyway: cardano-foundation/cardano-wallet#196
Issue Number
Overview
Comments
@piotr-iohk This is the best I can do in terms of coverage in a reasonable amount of time. There's still one particular overflow case that is hard to instrument in practice (impossible to meet 馃 ?) but that is there in the code as a safe "gatekeeper" I suppose.