Skip to content

Introduce recursive minimum fee calculation #1106

Open
Jimbo4350 wants to merge 4 commits intomasterfrom
jordan/introduce-calculate-min-fee-recursive
Open

Introduce recursive minimum fee calculation #1106
Jimbo4350 wants to merge 4 commits intomasterfrom
jordan/introduce-calculate-min-fee-recursive

Conversation

@Jimbo4350
Copy link
Contributor

Changelog

- description: |
    Introduce recursive minimum fee calculation 
  type:
  - feature     
  projects:
   - cardano-api

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

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.

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

@Jimbo4350 Jimbo4350 force-pushed the jordan/introduce-calculate-min-fee-recursive branch 3 times, most recently from 7fd4c6e to 7f0d7a8 Compare February 12, 2026 15:28
@Jimbo4350 Jimbo4350 force-pushed the jordan/introduce-calculate-min-fee-recursive branch from f42c341 to 93aa7a8 Compare February 20, 2026 18:12
@Jimbo4350 Jimbo4350 marked this pull request as ready for review February 20, 2026 18:12
@Jimbo4350 Jimbo4350 force-pushed the jordan/introduce-calculate-min-fee-recursive branch 4 times, most recently from 00bcbad to ab0d8fa Compare February 23, 2026 14:43
Three Hedgehog properties in Test.Cardano.Api.Experimental verify the
key invariants of the recursive fee calculator:

- well-funded transaction always succeeds and produces a positive fee
- fee calculation is idempotent (result is a fixed point)
- underfunded transaction (outputs exceed inputs) always returns
  NotEnoughAda with a negative deficit coin

Two lovelace-only generators drive the tests: one with
generous UTxO funding (5–20 ADA input, 1–3 ADA output) and one where
the output deliberately exceeds the input (0.5–2 ADA vs 5–10 ADA).
@Jimbo4350 Jimbo4350 force-pushed the jordan/introduce-calculate-min-fee-recursive branch from ab0d8fa to 8fabff7 Compare February 23, 2026 14:45
@carbolymer carbolymer requested a review from Copilot February 23, 2026 15:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new experimental API function to compute a minimum transaction fee via a recursive “set fee → rebalance → repeat” approach, and adds property tests intended to validate the behavior.

Changes:

  • Added calcMinFeeRecursive and RecursiveFeeCalculationError to the experimental fee API and re-exported them from Cardano.Api.Experimental.
  • Implemented recursive min-fee calculation and a helper to distribute surplus balance into outputs.
  • Added Hedgehog properties covering “well-funded succeeds”, “idempotent fixpoint”, and “insufficient funds fails”, plus updated an existing fee comparison test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
cardano-api/src/Cardano/Api/Experimental/Tx/Internal/Fee.hs Adds the recursive fee calculation implementation and its error type.
cardano-api/src/Cardano/Api/Experimental.hs Re-exports the new recursive fee API from the experimental surface.
cardano-api/test/cardano-api-test/Test/Cardano/Api/Experimental.hs Adds new property tests for calcMinFeeRecursive and extends an existing fee test.
cardano-api/cardano-api.cabal Adds test-suite dependencies used by the new tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 688 to 705
| minFee == txBodyFee && L.isZero txBalanceCoin =
-- We have reached the minimum fee but there isn't a guarantee that
-- the inputs/outputs are balanced
return unSignTx
| minFee == txBodyFee && txBalanceCoin > 0 = do
-- We have a surplus balance so we modify the outputs to include it.
balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
in return updatedTx
| txBalanceCoin < 0 = Left $ NotEnoughAda txBalanceCoin
| otherwise =
let newTx = UnsignedTx (ledgerTx & L.bodyTxL . L.feeTxBodyL .~ minFee)
in calcMinFeeRecursive newTx utxo pparams nExtraWitnesses
where
minFee = obtainCommonConstraints (useEra @era) $ L.calcMinFeeTx utxo pparams ledgerTx nExtraWitnesses
txBodyFee = ledgerTx ^. L.bodyTxL . L.feeTxBodyL
txBalanceValue = evaluateTransactionBalance pparams mempty mempty mempty utxo unSignTx
txBalanceCoin = L.coin txBalanceValue
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcMinFeeRecursive only checks L.isZero txBalanceCoin / compares txBalanceCoin against 0, which ignores any non-ADA assets in txBalanceValue. A tx can have coin == 0 while being unbalanced in multi-assets. Consider checking L.isZero txBalanceValue and handling negative (or positive) non-ADA balance explicitly (and/or extending the error type accordingly).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@palas palas Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcMinFeeRecursive only checks L.isZero txBalanceCoin / compares txBalanceCoin against 0, which ignores any non-ADA assets in txBalanceValue. A tx can have coin == 0 while being unbalanced in multi-assets. Consider checking L.isZero txBalanceValue and handling negative (or positive) non-ADA balance explicitly (and/or extending the error type accordingly).

While this is true, balancing assets is much easier than balancing Ada, as long as assets cannot be used for paying fees. So we could just balance them before starting iterating (if we balance them at all). I would actually replace L.isZero with == 0

Comment on lines 693 to 696
-- We have a surplus balance so we modify the outputs to include it.
balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
in return updatedTx
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the surplus case (minFee == txBodyFee && txBalanceCoin > 0), the code updates outputs and returns immediately. Updating output values can change the serialized tx size and therefore the minimum fee, potentially making the returned tx underfunded. It would be safer to rerun the fee/balance step on updatedTx (i.e. continue recursion until both fee and full balance reach a fixpoint).

Suggested change
-- We have a surplus balance so we modify the outputs to include it.
balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
in return updatedTx
-- We have a surplus balance so we modify the outputs to include it,
-- then recurse to ensure the fee/balance reach a fixpoint.
balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
calcMinFeeRecursive updatedTx utxo pparams nExtraWitnesses

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@palas palas Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the surplus case (minFee == txBodyFee && txBalanceCoin > 0), the code updates outputs and returns immediately. Updating output values can change the serialized tx size and therefore the minimum fee, potentially making the returned tx underfunded. It would be safer to rerun the fee/balance step on updatedTx (i.e. continue recursion until both fee and full balance reach a fixpoint).

Yes. Also, if we are not iterating both fee and balance at the same time, it would simplify the logic to separate the iteration over fee from the iteration over balance completely: here we are checking that minFee == txBodyFee and that txBalanceCoin > 0 at the same time, for example

Comment on lines +191 to +200
oldFees H.=== L.Coin 236

newFees H.=== L.Coin 236

-- Recursive fee calculation appears result in fees that are ~ 20% lower
recFee H.=== L.Coin 193

H.assert $ recFee < oldFees

H.assert $ recFee < newFees
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions hard-code exact fee values (Coin 236 / Coin 193). This makes the test brittle to any future change in protocol parameters, CBOR encoding, or ledger fee calculation. Prefer asserting relationships/invariants (e.g. oldFees == newFees, and recursive fee is a fixpoint / <= non-recursive fee) rather than pinning to specific constants.

Copilot uses AI. Check for mistakes.
prop_calcMinFeeRecursive_fee_fixpoint = H.property $ do
(unsignedTx, utxo) <- H.forAll $ genFundedSimpleTx Exp.ConwayEra
case Exp.calcMinFeeRecursive unsignedTx utxo exampleProtocolParams 0 of
Left _ -> H.success
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property currently treats a failure of calcMinFeeRecursive as success (Left _ -> H.success), which can hide real regressions. Since the generator produces a well-funded transaction, the test should fail on Left (and annotate the error) rather than passing.

Suggested change
Left _ -> H.success
Left err -> H.annotateShow err >> H.failure

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, but a comment with clarification would be nice.

minFee = obtainCommonConstraints (useEra @era) $ L.calcMinFeeTx utxo pparams ledgerTx nExtraWitnesses
txBodyFee = ledgerTx ^. L.bodyTxL . L.feeTxBodyL
txBalanceValue = evaluateTransactionBalance pparams mempty mempty mempty utxo unSignTx
txBalanceCoin = L.coin txBalanceValue
Copy link
Contributor

@carbolymer carbolymer Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about non-ada assets? Those have to be balanced as well.

Comment on lines 689 to 690
-- We have reached the minimum fee but there isn't a guarantee that
-- the inputs/outputs are balanced
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is not clear. What does it mean that there is no guarantee? Could you expand it?

Copy link
Contributor

@palas palas Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is not clear. What does it mean that there is no guarantee? Could you expand it?

Yes, do you mean other than Ada? Ada should be balanced because txBalanceCoin == 0, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the author meant non-ada assets. This should be explained.

balanceTxOuts txBalance (UnsignedTx tx) =
let outs = toList $ tx ^. L.bodyTxL . L.outputsTxBodyL
in case List.uncons outs of
Nothing -> Left NoTxOuts
Copy link
Contributor

@carbolymer carbolymer Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be a valid case, no? I imagine user could ask to balance a minting transaction with no outputs to just get one output generated: with a change.

in case List.uncons outs of
Nothing -> Left NoTxOuts
Just (h, rest) ->
let updatedout = h & L.valueTxOutL %~ (<> txBalance)
Copy link
Contributor

@carbolymer carbolymer Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you simply updating the first output with the balance difference? Shouldn't you add a change output at the end instead?

Copy link
Contributor

@carbolymer carbolymer Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that's the assumption of the function that it sends surplus change to the first output - but it's not explained anywhere. I understand that this also saves on the fees. I am not sure that everyone would want that. We should provide an option to return the change to the change address.

where
minFee = obtainCommonConstraints (useEra @era) $ L.calcMinFeeTx utxo pparams ledgerTx nExtraWitnesses
txBodyFee = ledgerTx ^. L.bodyTxL . L.feeTxBodyL
txBalanceValue = evaluateTransactionBalance pparams mempty mempty mempty utxo unSignTx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about transactions registering and deregistering stake?

-> Int
-- ^ Number of extra key hashes for native scripts
-> Either RecursiveFeeCalculationError (UnsignedTx (LedgerEra era))
calcMinFeeRecursive unSignTx@(UnsignedTx ledgerTx) utxo pparams nExtraWitnesses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haddocks missing

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (w/ help of Claude) have found a counterexample for the idempotence test. See my latest commit REMOVEME: counterexample.

I think Copilot is right here: https://github.com/IntersectMBO/cardano-api/pull/1106/files#r2841432882

prop_calcMinFeeRecursive_fee_fixpoint = H.property $ do
(unsignedTx, utxo) <- H.forAll $ genFundedSimpleTx Exp.ConwayEra
case Exp.calcMinFeeRecursive unsignedTx utxo exampleProtocolParams 0 of
Left _ -> H.success
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, but a comment with clarification would be nice.

Copy link
Contributor

@palas palas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is nice it is such a simple implementation, and if it gives better results, that is even better. Still, the logic is a bit odd at the moment, I give some examples of what I mean, and some ideas

Comment on lines 688 to 705
| minFee == txBodyFee && L.isZero txBalanceCoin =
-- We have reached the minimum fee but there isn't a guarantee that
-- the inputs/outputs are balanced
return unSignTx
| minFee == txBodyFee && txBalanceCoin > 0 = do
-- We have a surplus balance so we modify the outputs to include it.
balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
in return updatedTx
| txBalanceCoin < 0 = Left $ NotEnoughAda txBalanceCoin
| otherwise =
let newTx = UnsignedTx (ledgerTx & L.bodyTxL . L.feeTxBodyL .~ minFee)
in calcMinFeeRecursive newTx utxo pparams nExtraWitnesses
where
minFee = obtainCommonConstraints (useEra @era) $ L.calcMinFeeTx utxo pparams ledgerTx nExtraWitnesses
txBodyFee = ledgerTx ^. L.bodyTxL . L.feeTxBodyL
txBalanceValue = evaluateTransactionBalance pparams mempty mempty mempty utxo unSignTx
txBalanceCoin = L.coin txBalanceValue
Copy link
Contributor

@palas palas Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcMinFeeRecursive only checks L.isZero txBalanceCoin / compares txBalanceCoin against 0, which ignores any non-ADA assets in txBalanceValue. A tx can have coin == 0 while being unbalanced in multi-assets. Consider checking L.isZero txBalanceValue and handling negative (or positive) non-ADA balance explicitly (and/or extending the error type accordingly).

While this is true, balancing assets is much easier than balancing Ada, as long as assets cannot be used for paying fees. So we could just balance them before starting iterating (if we balance them at all). I would actually replace L.isZero with == 0

-- ^ Number of extra key hashes for native scripts
-> Either RecursiveFeeCalculationError (UnsignedTx (LedgerEra era))
calcMinFeeRecursive unSignTx@(UnsignedTx ledgerTx) utxo pparams nExtraWitnesses
| minFee == txBodyFee && L.isZero txBalanceCoin =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| minFee == txBodyFee && L.isZero txBalanceCoin =
| minFee == txBodyFee && txBalanceCoin == 0 =

If txBalanceCoin stops being a number we don't want this code to compile, that way we make sure to reconsider the implementation, and it seems L.isZero could potentially work with other things.
https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-core/src/Cardano.Ledger.Val.html#isZero

Also it is clearer what == 0 means exactly

Comment on lines 689 to 690
-- We have reached the minimum fee but there isn't a guarantee that
-- the inputs/outputs are balanced
Copy link
Contributor

@palas palas Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is not clear. What does it mean that there is no guarantee? Could you expand it?

Yes, do you mean other than Ada? Ada should be balanced because txBalanceCoin == 0, right?

Comment on lines 693 to 696
-- We have a surplus balance so we modify the outputs to include it.
balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
in return updatedTx
Copy link
Contributor

@palas palas Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the surplus case (minFee == txBodyFee && txBalanceCoin > 0), the code updates outputs and returns immediately. Updating output values can change the serialized tx size and therefore the minimum fee, potentially making the returned tx underfunded. It would be safer to rerun the fee/balance step on updatedTx (i.e. continue recursion until both fee and full balance reach a fixpoint).

Yes. Also, if we are not iterating both fee and balance at the same time, it would simplify the logic to separate the iteration over fee from the iteration over balance completely: here we are checking that minFee == txBodyFee and that txBalanceCoin > 0 at the same time, for example

balancedOuts <- balanceTxOuts txBalanceValue unSignTx
let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ Seq.fromList balancedOuts)
in return updatedTx
| txBalanceCoin < 0 = Left $ NotEnoughAda txBalanceCoin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I think this NotEnoughAda is not necessarily true, because it may be that fee is too large and if we reduced it then there would be enough to balance the transaction. Maybe you meant to check here minFee == txBodyFee as well

Copy link
Contributor

@palas palas Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more declarative way of thinking about this could be to separate it into functions:

  • What we are trying to optimise (the fees)
  • The constraints for the transaction to be valid (it needs to balance, and fees >= L.calcMinFeeTx)
  • The successor function (right now only trying setting the fees to L.calcMinFeeTx and balancing)

Then:

  • First, try to find one valid result
  • Then, try to iterate on that result (making sure we are not diverging)
  • Finally, return the best valid result that we can find

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants