Use evaluateTransactionFee in calculate-min-fee

[GeorgeFlerovsky]

Cardano.Api.estimateTransactionFee (currently used by Cardano CLI calculate-min-fee) tends to overestimate transaction fees. Furthermore, inspecting the code, estimateTransactionFee appears to only apply the old a * x + b fee formula to the transaction size (augmented to take into account the user-provided shelley and byron witness counts). This is not how fees should be calculated for later eras.

A deprecation note is currently attached to Cardano.Api.estimateTransactionFee, saying that Cardano.Api.evaluateTransactionFee should be used instead. Inspecting the code, evaluateTransactionFee calls the Cardano Ledger's era-aware fee calculation function Cardano.Ledger.Shelley.API.Wallet.evaluateTransactionFee, which in turn calls the EraTx class function getMinFeeTx that is instantiated for every Cardano Ledger era.

https://github.com/IntersectMBO/cardano-api/blob/1c10a287093ea1396c4c0ef0c92460f60bab690f/cardano-api/internal/Cardano/Api/Fees.hs#L190

Therefore, cardano-cli should be updated to use Cardano.Api.evaluateTransactionFee in the handler for calculate-min-fee.

Changelog

- description: |
    Fix calculate-min-fee by switching to use Cardano.Api.evaluateTransactionFee
    instead of the deprecated Cardano.Api.estimateTransactionFee.
    This also deprecates the --mainnet, --testnet-magic, --tx-in-count,
    and --tx-out-count command args, which are no longer necessary.
    They can still be provided, but have no effect.
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - improvement    # QoL changes e.g. refactoring
  - bugfix         # fixes a defect
  - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

https://twitter.com/amw7/status/1740008349905928326

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

I added a golden test to calculate the min fee of a Babbage transaction body with Babbage protocol parameters, comparing against the known minimum fee (below which the transaction gets rejected when submitted to node).

https://github.com/IntersectMBO/cardano-cli/pull/534/files#diff-8d95bc46c5ef4bbab74b92c6b29635f1c3792d1e084d8c7668b0ce75bf9d2b52

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated. See Running tests for more details
• Self-reviewed the diff

[gitmachtl]

hey @GeorgeFlerovsky

i compiled this version to test it. looks like the calculated-fee is correct.

question: can you make the "old" parameters for "mainnet/testnet" and the "tx-in-count/tx-out-count" optional so that it does not directly break existing code?

[GeorgeFlerovsky]

hey @GeorgeFlerovsky

i compiled this version to test it. looks like the calculated-fee is correct.

question: can you make the "old" parameters for "mainnet/testnet" and the "tx-in-count/tx-out-count" optional so that it does not directly break existing code?

Sure, I suppose I can reintroduce them with a deprecation notice. (They won't do anything)

[gitmachtl]

@GeorgeFlerovsky can you answer me a short question. looking at the length of the witnessed txBody in cbor format = txSize. the amount is 1 byte off when it comes to the txFee calculation.

like a txWitnessedBody

"84a40081825820a611a931da1bc763d5e256969ae2565ef17346369829e86a212a09aa33ec017f01018282581d60240382f82dcb3dfc28d8561ca06f2a0969aa875f1555c5c578072b7d1a000d165a82581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b00000002b5b7f5d4a2581c34250edd1e9836f5378702fbf9416b709bc140e04f668cc355208518a2494154414441636f696e19932744746573740f581cf0ff48bbb7bbe9d59a40f1ce90e9e9d0ff5002ec48f232b49ca0fb9aa14e000de14073706f7363726970747301021a000298b5031a02e0eb8fa10081825820742d8af3543349b5b18f3cba28f23b2d6e465b9c136c42e1fae6b2390f5654275840531badccad428c37c08aee33dab83ba4fc4cce4b13ba86dc23ac23316ac3fb08114c81e52ecc7ca69612c2acd071b8987ab3caf308edc87cddc684684bed5e04f5f6"

has a cbor with a size of 337 bytes. using this as the txSize to calculate the txFee = txSize*txFeePerByte + txFeeFixed results in a fee that is 1 byte too high. so is the null at the end shaved off the calculation or is it the 84 array(4) at the start or something else?

[GeorgeFlerovsky]

@GeorgeFlerovsky can you answer me a short question. looking at the length of the witnessed txBody in cbor format = txSize. the amount is 1 byte off when it comes to the txFee calculation.

like a txWitnessedBody

"84a40081825820a611a931da1bc763d5e256969ae2565ef17346369829e86a212a09aa33ec017f01018282581d60240382f82dcb3dfc28d8561ca06f2a0969aa875f1555c5c578072b7d1a000d165a82581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b00000002b5b7f5d4a2581c34250edd1e9836f5378702fbf9416b709bc140e04f668cc355208518a2494154414441636f696e19932744746573740f581cf0ff48bbb7bbe9d59a40f1ce90e9e9d0ff5002ec48f232b49ca0fb9aa14e000de14073706f7363726970747301021a000298b5031a02e0eb8fa10081825820742d8af3543349b5b18f3cba28f23b2d6e465b9c136c42e1fae6b2390f5654275840531badccad428c37c08aee33dab83ba4fc4cce4b13ba86dc23ac23316ac3fb08114c81e52ecc7ca69612c2acd071b8987ab3caf308edc87cddc684684bed5e04f5f6"

has a cbor with a size of 337 bytes. using this as the txSize to calculate the txFee = txSize*txFeePerByte + txFeeFixed results in a fee that is 1 byte too high. so is the null at the end shaved off the calculation or is it the 84 array(4) at the start or something else?

@gitmachtl I think that this question gets fairly deep into the way that cbor gets decoded into Cardano Ledger types. In principle, I could probably track down the answer, but it would be fairly long and tedious across multiple libraries. If I had to guess, I would say that it's caused by the null delimiter being shaved off during deserialization.

[GeorgeFlerovsky]

hey @GeorgeFlerovsky

i compiled this version to test it. looks like the calculated-fee is correct.

question: can you make the "old" parameters for "mainnet/testnet" and the "tx-in-count/tx-out-count" optional so that it does not directly break existing code?

@gitmachtl Done.

[gitmachtl]

Done.

i have recompiled it, looks good to me. not breaking anything anymore.

[smelc]

cc @Jimbo4350

[Jimbo4350]

Thank you for this. It's been on our TODO list for some time.

[Jimbo4350]

I just had a chat with @lehins. This command was created in the early Shelley era days so it does not take into consideration other things that can influence the tx fee. The best solution would be to use setMinFeeTx and modify the calculate-min-fee command to take a witnessed transaction instead of a tx body. Perhaps also including an option to output the fee or the fee updated tx.

Edit: Apologies. calculate-min-fee eventually calls evaluateTransactionFee.

Edit: Settled on implementing item 1 here. 2 and 3 can be done at a later date.

[gitmachtl]

hmm... that would require all current cli tools to "fake" future witnesses. what would be other things that would influence tx fee?

[lehins]

what would be other things that would influence tx fee?

Anything that affects the size of the transaction:

• Redeemers
• AuxData
• Script witnesses, which can't be estimated without providing actual scripts
• the fee amount itself. Increasing the amount can affect the size thus the minimum fee.

So, the only way to accurately compute the fee is to construct a full transaction and call setMinFeeTx on it. The only short-cut I can think of is supplying bogus signatures and public keys, because their validity is not relevant for the fee calculation

[gitmachtl]

The fee amount is most likely always a 5byte cbor representation, that would cover all fees between 65536 and 4294967296 lovelaces. Thats why people should at least use an avg. fee value of f.e. 200000 lovelaces for the dummyTxBody generation imo.

Script witnesses, are you talking about minting/burning scripts - which are already provided in the build-raw process. Or other ones?

Maybe i am confused right now, but when the tx was built with all details via build-raw f.e. and the only thing that is left to do is to assemble witnesses, what can you do to the tx to modify its size at that stage other than providing the witnesses?

[Jimbo4350]

@GeorgeFlerovsky

So this needs some more thought. I propose the following scenarios:

1. The user submits an unsigned tx body specifying the number of byron/shelley witnesses they desire.

   • In the cli we construct dummy witnesses (byron if needed or shelley or both) to construct a "bogus" tx and call getMinFeeTx. We return only the fee in this case otherwise the users will have to remove the bogus witnesses if we returned the resulting tx. Constructing the dummy witnesses will allow us to avoid implementing something like calculateByronWitnessFees.

2. The user submits a signed tx or an unsigned tx body along with the signing keys they intend to validate the tx with.

   • In this case we should return the re-signed transaction. The transaction has to be re-signed after the minimum fee is updated and the user will receive a valid tx they can immediately submit.
   • Consider returning an error if the number of witnesses in the signed tx does not equal the number of signing keys (0 supplied signing keys is fine and is actually the third scenario).

3. The user submits a signed tx without the signing keys needed to validate the tx. We simply return the fee as we can't return a valid tx.

Overall I think we only want to submit valid transactions or valid transaction fees otherwise we are creating more work for the user.

[gitmachtl]

Perhaps also including an option to output the fee or the fee updated tx.

What do you mean with fee updated tx? To have the balance correct the sum of the input utxos and all the outputs including the fee must be zero. (if there is no other deposit or so). I don't understand how this would work with a "fee updated tx"? Or are you talking "build" command and not "build-raw" ?

These solutions to use witnessed or signed transactions sounds impractical to me in "real life". We have offline transactions, and we have transactions that are signed by multiple entities spread across different locations for example. Like pool owners providing a witness for a ready made txBody with there hardware wallets, etc.

[Jimbo4350]

What do you mean with fee updated tx?

We're using calculate-min-fee to get a valid fee for our transaction. We created the transaction we are interested in with build-raw so by definition we likely specified --fee 0 or some other invalid fee.

So when I say "fee updated tx" I'm referring to a tx updated with a valid fee.

[gitmachtl]

from what i have seen so far the method to generate transactions with the existing 3rd party tools (which are using cardano-cli) are:

1. do a query of the payment address and select utxos
2. build a dummy txBody with a fee set to a value in the 5byte cbor range (f.e. 200000 lovelaces). at this stage the dummyTxBody also contains multiple receivers if needed, it contains added metadata or required signers, etc. so its more or less ready to go.
3. calculating the fee for this dummyTxBody and providing the shelley and byron witness counts (the builder knows the counts)
4. generate the final tx with the corrected fee and the new values for the amounts of lovelaces for the receiver(s). this is the final tx before witnesses are added, no other mods after that point.
5. assemble the txBody with witnesses provided locally or remotely or via offline witnesses etc
6. submit the witnessed tx

so i would say that your proposal to keep

1. The user submits an unsigned tx body specifying the number of byron/shelley witnesses they desire.

is the right one for the current workflow imo.

[Jimbo4350]

from what i have seen so far the method to generate transactions with the existing 3rd party tools (which are using cardano-cli) are:

1. do a query of the payment address and select utxos

2. build a dummy txBody with a fee set to a value in the 5byte cbor range (f.e. 200000 lovelaces). at this stage the dummyTxBody also contains multiple receivers if needed, it contains added metadata or required signers, etc. so its more or less ready to go.

3. calculating the fee with for this dummyTxBody

4. generate the final tx with the corrected fee and the new values for the amounts of lovelaces for the receiver(s). this is the final tx before witnesses are added, no other mods after that point.

5. assemble the txBody with witnesses provided locally or remotely or via offline witnesses etc

6. submit the witnessed tx

so i would say that your proposal to keep 1. The user submits an unsigned tx body specifying the number of byron/shelley witnesses they desire.

Yes, this scenario is covered in item 1 of my proposal. However, all three items should be implemented. I think it is acceptable to only implement 1 in place of this PR as this covers the prior functionality but 2 and 3 would be useful.

[GeorgeFlerovsky]

@Jimbo4350 I got a bit confused by your direct edits of my comment lol.
Here's the resulting conversation we ended up having via comment edits:

George (msg 1):

Cardano Ledger already constructs dummy Shelley witnesses when you invoke Ledger.evaluateTransactionFee via Cardano.Api.evaluateTransactionFee:

https://github.com/IntersectMBO/cardano-ledger/blob/d75254dc6332b0e62c169f25ea2d6014e347dc1a/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L476

Unfortunately, it doesn't make dummy Byron witnesses, but IMO estimating Byron witness fees without dummy Byron witnesses (as I did in this PR) is reasonable given how rare they are these days.

Jimbo (msg 2):

Unfortunately, it doesn't make dummy Byron witnesses, but IMO estimating Byron witness fees without dummy Byron witnesses (as I did in this PR) is reasonable given how rare they are these days.

I agree however I'd prefer this is done via the way I outlined in item 1.. We want to avoid the size calculation function and instead rely on ledger functionality.

Can you move the golden test changes to the final commit and squash them?

[GeorgeFlerovsky]

Also, note that (assuming there are no Byron witnesses) we only need a TxBody to give to Cardano.Api.evaluateTransactionFee, not a full signed Tx.

(When you have a full Tx, you can extract the body)

[gitmachtl]

Anything that affects the size of the transaction:

• AuxData

How can AuxData be modified after witnesses are already provided for the tx hash? The Hash of the AuxData is part of the txBody. Alternating AuxData would make the whole witnessing before useless. I am confused. Or do you mean that only a hash of the AuxData is present in time of the fee calculation an not also the data for the AuxData itself too? Most unlikely to have this scenario talking cardano-cli imo. If people are messing around with self generated transactions via own tooling thats another thing. But cardano-cli shouldn't be responsible to get it right then imo.

Most common scenario for tx building on the cli is like i pointed out here: #534 (comment)

[GeorgeFlerovsky]

@Jimbo4350 Would it be OK to implement support for scenarios 2 and 3 in a subsequent PR?

Unfortunately, I have to move on to other tasks this week. I can deal with bugfixes and minor adjustments in the PR, but adding new features might be a bit much for now.

[gitmachtl]

Unfortunately, it doesn't make dummy Byron witnesses, but IMO estimating Byron witness fees without dummy Byron witnesses (as I did in this PR) is reasonable given how rare they are these days.

I agree, transactions witnessed with byron keys on the cli via cardano-cli are rare these days. I would go with the "worst case" scenario for byron witnesses and do a bit of an overestimation.

[GeorgeFlerovsky]

Unfortunately, it doesn't make dummy Byron witnesses, but IMO estimating Byron witness fees without dummy Byron witnesses (as I did in this PR) is reasonable given how rare they are these days.

I agree, transactions witnessed with byron keys on the cli via cardano-cli are rare these days. I would go with the "worst case" scenario for byron witnesses and do a bit of an overestimation.

That's what I did in the PR. 👍

[GeorgeFlerovsky]

Tools like CNTools and SPOScripts wanna have full control over who is paying what, so for that we are doing it with a "dummyTxBody" to get the fees. Than we take those fees into account and use them in the transaction with the right value. We don't use the "transaction build" command, we use the "transaction build-raw" command. So all ADA+Assets+Deposits+Claims+Fees etc are calculated by the Tools themselfs in this scenario, the only thing that is always open after building the "dummyTxBody" is the correct Fee amount. Witness-Signing ans Assemble comes after that in different scenarios.

Are you doing that because the transaction builder hasn't been handling fee calculation to begin with, or because you want to split it into two steps?

[AndrewWestberg]

@GeorgeFlerovsky I'm not sure, but there might be some edge cases where the fee isn't calculated properly. Possibly if an output utxo has inline datums or a datum hash. Haven't tested this in quite awhile though. This is the main reason a lot of us do the dummy transaction thing. It's the only way to REALLY know if the fees are correct.

I do know that the current situation is un-tennable. Just this morning I tried building a transaction and the fee it calculated came in too low. I had to manually rebuild the tx and hard-code the fee.

$ cardano-cli babbage transaction submit --tx-file /tmp/newm_tx.signed --mainnet --socket-path /home/westbam/haskell/local/db/socket
Command failed: transaction submit  Error: Error while submitting tx: ShelleyTxValidationError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (UtxoFailure (AlonzoInBabbageUtxoPredFailure (FeeTooSmallUTxO (Coin 444373) (Coin 444285))))])

Unfortunately, I didn't think to keep these files and when I re-built the hardcoded-fee tx, I overwrote them.

[GeorgeFlerovsky]

@GeorgeFlerovsky I'm not sure, but there might be some edge cases where the fee isn't calculated properly. Possibly if an output utxo has inline datums or a datum hash. Haven't tested this in quite awhile though. This is the main reason a lot of us do the dummy transaction thing. It's the only way to REALLY know if the fees are correct.

I do know that the current situation is un-tennable. Just this morning I tried building a transaction and the fee it calculated came in too low. I had to manually rebuild the tx and hard-code the fee.

$ cardano-cli babbage transaction submit --tx-file /tmp/newm_tx.signed --mainnet --socket-path /home/westbam/haskell/local/db/socket
Command failed: transaction submit  Error: Error while submitting tx: ShelleyTxValidationError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (UtxoFailure (AlonzoInBabbageUtxoPredFailure (FeeTooSmallUTxO (Coin 444373) (Coin 444285))))])

Unfortunately, I didn't think to keep these files and when I re-built the hardcoded-fee tx, I overwrote them.

It was too low with my new code in this PR, or the main branch of cardano-cli?

[gitmachtl]

Are you doing that because the transaction builder hasn't been handling fee calculation to begin with, or because you want to split it into two steps?

We are sometimes building "difficult" transactions, and the decision on who is paying for what is sometimes mixed. So utxo-1 is paying for the fee of this transaction, so the tx-out is reduced about that amount for this address. utxo-2 is paying for the key deposit fee, utxo-3 is paying for half of the pool-registration deposit, etc. So, not just a simple transaction. And for that we always used (it was the only option back in the days) the build-raw command to have full control over the outcome. Thats all good, so if the fee calculation gets some updates and we still can do it by providing a pre-composed txBody to get the correct fee out of it we would be happy.

[gitmachtl]

@GeorgeFlerovsky

I had a "fee too low" issue with your latest version of cardano-cli in combination with hw-wallet signing, because of CIP-0021 "violation" to use Tag258 everywhere or nowhere. I opened up an issue #567

Its not an issue with the fee calculation - which was on point at the time of the tx was generated, but the tx itself missed a Tag258.

[GeorgeFlerovsky]

@gitmachtl
Understood 👍

[AndrewWestberg]

@GeorgeFlerovsky I'm not sure, but there might be some edge cases where the fee isn't calculated properly. Possibly if an output utxo has inline datums or a datum hash. Haven't tested this in quite awhile though. This is the main reason a lot of us do the dummy transaction thing. It's the only way to REALLY know if the fees are correct.
I do know that the current situation is un-tennable. Just this morning I tried building a transaction and the fee it calculated came in too low. I had to manually rebuild the tx and hard-code the fee.

$ cardano-cli babbage transaction submit --tx-file /tmp/newm_tx.signed --mainnet --socket-path /home/westbam/haskell/local/db/socket
Command failed: transaction submit  Error: Error while submitting tx: ShelleyTxValidationError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (UtxoFailure (AlonzoInBabbageUtxoPredFailure (FeeTooSmallUTxO (Coin 444373) (Coin 444285))))])

Unfortunately, I didn't think to keep these files and when I re-built the hardcoded-fee tx, I overwrote them.

It was too low with my new code in this PR, or the main branch of cardano-cli?

Main branch code, not this one. I was lamenting not keeping the tx so you could test it to make sure it calculated properly. It's probably easy enough to re-create in a test setting though. It was a tx with a single utxo in with 500 ada + a big bucket of a single native asset. Then it was sent out to 56 different stake addresses with change returned.

[GeorgeFlerovsky]

@AndrewWestberg

OK, please share the reproducible example when you find it.

[AndrewWestberg]

@GeorgeFlerovsky I was not able to reproduce it. I re-ran the transaction building script I had and it came out to 444373 this time. No idea what caused it to fail before.

[AndrewWestberg]

What is left that is holding up this PR given the current calculation is broken?

[GeorgeFlerovsky]

@Jimbo4350 @gitmachtl @carbolymer cc @AndrewWestberg
I again rebased this PR to the latest main and confirmed that tests still succeed.

Can you please finish your review of this PR and merge it if it is ready?

If it's not ready, can you please fork the branch and continue working on it yourselves in a different PR? I unfortunately don't have much more bandwidth to continue monitoring this PR if it's just stalled.

[AndrewWestberg]

@disassembler What is the status of this PR? The status quo is not really acceptable given that fees are sometimes calculated wrongly.

[lehins]

evaluateTransactionFee function used in this PR has been deprecated in ledger.
There are two more accurate functions that are now available:

• calcMinFeeTx - requires access to utxo, but very accurate
• estimateMinFeeTx - does not require access to utxo, but lesss accurate

[gitmachtl]

this is not yet exposed via cardano-cli, right?

[lehins]

this is not yet exposed via cardano-cli, right?

I actually confused evaluateTransactionFee exported by cardano-api with deprecated evaluateTransactionFee function in the ledger codebase.

Turns out evaluateTransactionFee already uses estimateMinFeeTx

That being said, current implementation of evaluateTransactionFee is incomplete with respect to ConwayEra, because it assumes 0 size for reference scripts

So, I'd suggest merging this PR and then do some follow up work to figure out how cardano-api can utilize calcMinFeeTx for fee computation.

[gitmachtl]

@lehins hmm... now i am confused too. because for the current fee calculation on the cli we already create a dummyTx with the input utxo(s), the outputs, added certificates, etc.

we pass this dummyTx than to the transaction calculate-min-fee function and add the witness counts for byron and shelley ones.

so utxos and outputs in the dummyTx are actually not used to calculate the fee currently?

[lehins]

we pass this dummyTx than to the transaction calculate-min-fee function and add the witness counts for byron and shelley ones.

That is one of the main points of why ledger provides fee calculation functionality, so that you don't need to do this in cardano-node. Moreover, dummy tx approach taken here is incomplete and incorrect:

1. For Byron witnesses it assumes that they all have the same size, which is not correct and in reality they can be larger or smaller than assumed in this PR. The only reliable way to account for Byron witnesses is to actually look at the addresses in the UTxO that are being spent
2. In Conway reference scripts will also contribute to fee calculation, which means that the fee estimation function needs to look at all of the reference scripts that are supplied in reference and spending inputs, thus access to UtxO is also needed.

I understand that access to UTxO is not always possible, so we can do the best we can at guessing how big the witnesses will be, which is why estimateMinFeeTx is less accurate than calcMinFeeTx, because it does similar thing to what this PR does.

So, taking one more look at the current PR, I'd suggest removing calculateByronWitnessFees and removing this error in cardano-api, because the TODO has already been taken care of.

With respect to ref scripts (the point 2 above), that change will be present in the next ledger release.

[Jimbo4350]

@GeorgeFlerovsky We have limited bandwidth at the moment because we are focused on Conway related testing. However I will pick this up within the next 2 weeks.

[GeorgeFlerovsky]

No worries. If you need to make any administrative commits to the PR, please don't hesitate to do so.

Let me know if I need to grant you any permissions to my fork.

[Jimbo4350]

@GeorgeFlerovsky I haven't forgotten about this. I have assigned the task to my team as I will be off for essentially the rest of this month.

[GeorgeFlerovsky]

@GeorgeFlerovsky I haven't forgotten about this. I have assigned the task to my team as I will be off for essentially the rest of this month.

@Jimbo4350 Thanks. Enjoy your time off. 🙏

[newhoggy]

The code has been updated to latest cardano-api which has the changes @lehins proposed and rebased in this PR #642

[carbolymer]

Superceded by #642.