Fix cli build command #2995

Jimbo4350 · 2021-07-30T12:44:16Z

The build command was underestimating tx fees

cardano-api/src/Cardano/Api/Fees.hs

JaredCorduan · 2021-07-30T17:16:19Z

cardano-api/src/Cardano/Api/Fees.hs

    txbody1 <- first TxBodyError $ -- TODO: impossible to fail now
               makeTransactionBody txbodycontent1 {
-                 txFee = TxFeeExplicit explicitTxFees 0
+                 txFee  = TxFeeExplicit explicitTxFees $ Lovelace (2^(32 :: Integer)),


I guess this is set so that the this field takes up the maximal bytes for size calculation, right?

Yes, though I think this is a hack and we've not understood something. I thought we would only need a 4 byte integer, i.e. 2^32-1. Jordan's example was then off by 2 bytes, and switching from 2^32-1 to 2^32 is basically a hack to add in an extra 4 bytes.

The fee cannot possibly be that high, indeed in Jordan's example the fee would actually fit into a 2 byte integer (i.e. it's less than 2^16-1).

It'd be preferable to track down where the missing bytes are. It looks like in Jordan's example we're actually off by exactly 4 bytes, since the validation error was for a fee for 2 bytes (2 * 44), and the fee field was already overly generous by 2 bytes.

Also using 2^32 is a bit misleading. It'd be better to be more explicit that we're reserving 8 bytes by using 2^64-1

dcoutts

The fact that we've had to set both fields to be 2^32 rather than 2^32-1 means we've got our accounting off by a few bytes. Using 8 byte integer fields here is a hack. We should figure out where we're out.

JaredCorduan · 2021-08-02T20:30:41Z

cardano-api/src/Cardano/Api/Fees.hs

-                 txFee = TxFeeExplicit explicitTxFees 0
+                 txFee  = TxFeeExplicit explicitTxFees $ Lovelace (2^(31 :: Integer)),
+                 txOuts = TxOut changeaddr
+                                (lovelaceToTxOutValue $ Lovelace (2^(31 :: Integer)))


we need to reserve more space for the change address. it could potentially take up 8 bytes. reserving 4 bytes for the fee is great, though.

dcoutts · 2021-08-03T10:12:04Z

cardano-api/src/Cardano/Api/Fees.hs

    txbody1 <- first TxBodyError $ -- TODO: impossible to fail now
               makeTransactionBody txbodycontent1 {
-                 txFee = TxFeeExplicit explicitTxFees 0
+                 txFee  = TxFeeExplicit explicitTxFees $ Lovelace (2^(31 :: Integer) - 1),


Should be (2^(32 :: Integer) - 1), i.e. 32 not 31

dcoutts

Looking good. Just one problem I spot, that we regressed on the balance check while adding a min utxo check for the ordinary user-specified outputs.

dcoutts · 2021-08-03T11:44:32Z

cardano-api/src/Cardano/Api/Fees.hs

+   balanceCheck :: Lovelace -> Either TxBodyErrorAutoBalance ()
+   balanceCheck balance


This no longer checks that the balance is over the min utxo value. The check earlier

mapM_ (`checkMinUTxOValue` pparams) $ txOuts txbodycontent1

does not cover the case of the balance, because the change output with the balance is not included in txbodycontent1.

I would keep the original check

| balance < minUTxOValue = Left (TxBodyErrorAdaBalanceTooSmall balance)

I would also suggest splitting the error case into two because they are different errors with different causes, different solutions and should have different error messages. The TxBodyErrorAdaBalanceTooSmall should be for the case of the balance being too small.

We should have a separate error for the other user-specified outputs being too small, i.e. used by checkMinUTxOValue in

mapM_ (`checkMinUTxOValue` pparams) $ txOuts txbodycontent1

dcoutts

We're still misreporting the balance < min utxo issue.

dcoutts · 2021-08-03T13:23:55Z

cardano-api/src/Cardano/Api/Fees.hs

+   balanceCheck :: TxOutValue era -> Either TxBodyErrorAutoBalance ()
+   balanceCheck balance
+    | txOutValueToLovelace balance < 0 =
+        Left . TxBodyErrorAdaBalanceNegative $ txOutValueToLovelace balance
+    | otherwise =
+        checkMinUTxOValue (TxOut changeaddr balance TxOutDatumHashNone) pparams


It's not a good idea to merge the balance < minUTxOValue check here into the general checkMinUTxOValue.

Note how the code now never uses the TxBodyErrorAdaBalanceTooSmall error constructor and so will report the wrong error message.

I actually don't see why we need to change the balanceCheck code at all. It does not need to be given the full TxOutValue. As far as I can see the Lovelace only was fine.

The minUTxO value is calculated differently depending on the era, which is why I put checkMinUTxOValue in balanceCheck.

Co-authored-by: Duncan Coutts <duncan@well-typed.com>

dcoutts

So I still think the balanceCheck and checkMinUTxOValue is ugly and overly complicated, but I'm not going to block merging the PR over that now.

cardano-api/src/Cardano/Api/Fees.hs

Jimbo4350 · 2021-08-04T10:56:00Z

bors r+

2995: Fix cli build command r=Jimbo4350 a=Jimbo4350 The `build` command was underestimating tx fees Co-authored-by: Jordan Millar <jordan.millar@iohk.io>

iohk-bors · 2021-08-04T11:10:48Z

Build failed:

ci/hydra-eval

Jimbo4350 · 2021-08-04T11:29:53Z

bors r+

iohk-bors · 2021-08-04T12:11:12Z

Build succeeded:

Jimbo4350 requested review from DavidEichmann, dcoutts, erikd and newhoggy as code owners July 30, 2021 12:44

dcoutts reviewed Jul 30, 2021

View reviewed changes

cardano-api/src/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved

cardano-api/src/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch from 317fce9 to a83e7f7 Compare July 30, 2021 17:06

JaredCorduan reviewed Jul 30, 2021

View reviewed changes

dcoutts reviewed Jul 30, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch from a83e7f7 to 682ddfb Compare August 2, 2021 09:16

JaredCorduan reviewed Aug 2, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch from 7825393 to e35f13c Compare August 3, 2021 08:30

dcoutts reviewed Aug 3, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch 2 times, most recently from 9e4da6b to d10dd95 Compare August 3, 2021 11:42

dcoutts requested changes Aug 3, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch 2 times, most recently from f0c8229 to 7a74ecc Compare August 3, 2021 13:16

dcoutts requested changes Aug 3, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch 8 times, most recently from 063de69 to 1b5672b Compare August 4, 2021 09:53

Fix cli build command as it was underestimating the tx fee

260c2ee

Co-authored-by: Duncan Coutts <duncan@well-typed.com>

Jimbo4350 force-pushed the jordan/fix-cli-build-cmd branch from 1b5672b to 260c2ee Compare August 4, 2021 10:24

dcoutts approved these changes Aug 4, 2021

View reviewed changes

cardano-api/src/Cardano/Api/Fees.hs Show resolved Hide resolved

Jimbo4350 mentioned this pull request Aug 4, 2021

Clean up transaction build command #3016

Closed

iohk-bors bot added a commit that referenced this pull request Aug 4, 2021

Merge #2995

849dcb3

2995: Fix cli build command r=Jimbo4350 a=Jimbo4350 The `build` command was underestimating tx fees Co-authored-by: Jordan Millar <jordan.millar@iohk.io>

iohk-bors bot merged commit cb11587 into master Aug 4, 2021

iohk-bors bot deleted the jordan/fix-cli-build-cmd branch August 4, 2021 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cli build command #2995

Fix cli build command #2995

Jimbo4350 commented Jul 30, 2021

JaredCorduan Jul 30, 2021

dcoutts Jul 30, 2021

dcoutts Jul 30, 2021

dcoutts left a comment

JaredCorduan Aug 2, 2021

dcoutts Aug 3, 2021 •

edited

Loading

dcoutts left a comment

dcoutts Aug 3, 2021

dcoutts left a comment

dcoutts Aug 3, 2021

Jimbo4350 Aug 3, 2021

dcoutts left a comment

Jimbo4350 commented Aug 4, 2021

iohk-bors bot commented Aug 4, 2021

Jimbo4350 commented Aug 4, 2021

iohk-bors bot commented Aug 4, 2021

		balanceCheck :: Lovelace -> Either TxBodyErrorAutoBalance ()
		balanceCheck balance

Fix cli build command #2995

Fix cli build command #2995

Conversation

Jimbo4350 commented Jul 30, 2021

JaredCorduan Jul 30, 2021

Choose a reason for hiding this comment

dcoutts Jul 30, 2021

Choose a reason for hiding this comment

dcoutts Jul 30, 2021

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

JaredCorduan Aug 2, 2021

Choose a reason for hiding this comment

dcoutts Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts Aug 3, 2021

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts Aug 3, 2021

Choose a reason for hiding this comment

Jimbo4350 Aug 3, 2021

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

Jimbo4350 commented Aug 4, 2021

iohk-bors bot commented Aug 4, 2021

Jimbo4350 commented Aug 4, 2021

iohk-bors bot commented Aug 4, 2021

dcoutts Aug 3, 2021 •

edited

Loading