Skip to content

Fix key witness count underestimation for votes in fee estimation - #1271

Open
carbolymer wants to merge 1 commit into
masterfrom
mgalazyn/fix/vote-key-witness-count
Open

Fix key witness count underestimation for votes in fee estimation#1271
carbolymer wants to merge 1 commit into
masterfrom
mgalazyn/fix/vote-key-witness-count

Conversation

@carbolymer

@carbolymer carbolymer commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Context

Closes #722.

Both variants of estimateTransactionKeyWitnessCount (experimental Cardano.Api.Experimental.Tx.Internal.Fee and legacy Cardano.Api.Tx.Internal.Fee) ignored txVotingProcedures entirely, so a transaction carrying votes from key-credentialed voters (key-hash DReps, CC hot keys, SPOs) had its key witness count underestimated.
This understates the fee and can lead to FeeTooSmallUTxO.

Both estimators now derive the count from the ledger L.VotingProcedures voter credentials, folded into a Set of witness key hashes exactly like ledger's voterWitnesses: a CommitteeVoter/DRepVoter needs a key witness only when its credential is a KeyHashObj, a StakePoolVoter always does, and a voter voting on several governance actions, or a key hash appearing under several governance roles, is counted once.
estimateTransactionKeyWitnessCount is also newly exported from Cardano.Api.Experimental (it was previously unreachable outside the package).
The ledger requires no key witness for proposal procedures, so the vote fix deliberately adds no proposal term; the experimental estimator keeps its pre-existing conservative charge of one witness per proposal, now documented in its haddock.

Both estimators also deduplicate key witnesses across every role whose key hashes the transaction body itself pins down: certificates, withdrawals, extra key witnesses and votes are folded into a single set of witness key hashes, so a key required by several of those roles is counted once.
Pool registration certificates now count one key witness for the operator and each owner, closing another pre-existing under-estimate (the ledger requires every owner to sign, but only the operator was counted).
Inputs and collateral are still counted per input on top of that set, since their key hashes live in the spent outputs and are only known with a UTxO; the result stays an upper bound, and calculateMinTxFee remains the exact path when a UTxO is available.

Note that the type asymmetry the issue asks about (ScriptWitness vs Witness) is already resolved in the experimental API via AnyWitness; this PR closes the remaining fee-estimation gap in both APIs.

How to trust this PR

Both fixes mirror ledger's own witness collection (voterWitnesses and the getWitsVKeyNeeded union in Cardano.Ledger.Conway.UTxO), so the credential-matching and deduplication logic is not new invention, it is the same rule the ledger itself applies when building the witness set.

Two hedgehog property tests cover the fix, each generating a small pool of key-credentialed and script-credentialed voters that vote on random non-empty subsets of several governance action ids, so the same voter recurs across action ids, and asserting the exact expected key witness count:

  • Test.Cardano.Api.Experimental.Fee: estimateTransactionKeyWitnessCount counts key witnesses required by key-credentialed voters, which also exercises the mkTxVotingProcedures merge path.
  • Test.Cardano.Api.TxBody: vote key witness count, which isolates the vote contribution by comparing the estimate for a randomly generated tx body with and without the generated votes attached.

Both suites additionally gain a cross-role key witness dedupe property: a stake key that both withdraws rewards and is deregistered by a certificate in the same transaction is counted once, while an unrelated key still counts.
A pool registration counts operator and every owner property in both suites covers the owner counting, including an owner shared with a withdrawal.

Both tests fail against the pre-fix code with a bare 0 === 1 (or similar), since the old record patterns simply never destructured txVotingProcedures.

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
  • Changelog fragment added in .changes/

@carbolymer carbolymer changed the title Fix witness counting for voting transaction Fix key witness count underestimation for votes in fee estimation Jul 28, 2026
@carbolymer
carbolymer force-pushed the mgalazyn/fix/vote-key-witness-count branch 2 times, most recently from eae5142 to bf26988 Compare July 29, 2026 12:07
@carbolymer carbolymer self-assigned this Jul 29, 2026
@carbolymer
carbolymer marked this pull request as ready for review July 29, 2026 12:07
@carbolymer
carbolymer requested a review from Jimbo4350 as a code owner July 29, 2026 12:07
Copilot AI review requested due to automatic review settings July 29, 2026 12:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes fee underestimation for vote-carrying transactions by ensuring estimateTransactionKeyWitnessCount accounts for key-credentialed governance voters in both the legacy and experimental transaction-building APIs. This closes the remaining fee-estimation gap around txVotingProcedures, preventing underestimated fees that can lead to FeeTooSmallUTxO.

Changes:

  • Legacy fee estimator now counts key witnesses implied by txVotingProcedures based on ledger voter credential types (key-hash DReps, committee hot keys, and SPOs).
  • Experimental fee estimator now counts AnyKeyWitnessPlaceholder entries in the vote witness map, and estimateTransactionKeyWitnessCount is exported from Cardano.Api.Experimental.
  • Adds Hedgehog regression/property tests covering vote key-witness counting in both APIs, plus a Herald changelog fragment.

Reviewed changes

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

Show a summary per file
File Description
cardano-api/src/Cardano/Api/Tx/Internal/Fee.hs Includes txVotingProcedures in legacy key-witness estimation using ledger voter credential rules.
cardano-api/src/Cardano/Api/Experimental/Tx/Internal/Fee.hs Counts vote-related key witnesses via AnyKeyWitnessPlaceholder entries and exports the estimator from the internal module.
cardano-api/src/Cardano/Api/Experimental.hs Re-exports estimateTransactionKeyWitnessCount from the experimental top-level module.
cardano-api/test/cardano-api-test/Test/Cardano/Api/TxBody.hs Adds regression property test isolating vote contribution to legacy key-witness estimation.
cardano-api/test/cardano-api-test/Test/Cardano/Api/Experimental/Fee.hs Adds regression property test for experimental vote key-witness estimation and shared vote generators.
.changes/20260728_cardano_api_vote_key_witness_count.yml Documents the bugfix + compatible API change (new export) for Herald/CI.

@palas palas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch! This is a net improvement, but I think there are two things that could be improved, one in the implementation, and one in the tests

Comment on lines +1428 to +1430
votingProcedures =
L.VotingProcedures $
Map.fromList [(voter, Map.singleton govActionId votingProcedure) | voter <- allVoters]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should try with several govActionId and also having the same voter vote for several action ids. The easiest way would be generating a relatively small pool of potential voters and reusing them to create collisions.

OMap.size m
Nothing -> 0
+ case txVotingProcedures of
Just (TxVotingProcedures _ voteWits) ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think here it would be more reliable to look at the voting procedures (first field) instead of voteWits, like in the version of this for the traditional API. And that is what the ledger does here

Nothing -> 0
+ case txVotingProcedures of
Just (TxVotingProcedures _ voteWits) ->
length [() | AnyKeyWitnessPlaceholder <- Map.elems voteWits]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just realised we may be counting the same key witness several times over. The ledger gathers all required key witnesses into a single set, so a key that appears in more than one role only needs one witness. For example, the key witnessing the spend of a transaction input may also witness a vote and therefore it is counted twice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right about the ledger: getConwayWitsVKeyNeeded folds everything into a single Set (KeyHash Witness), so a key acting in several roles needs only one witness on chain. This function can't replicate that fully: it has no UTxO, and input key hashes live only in the spent outputs, so it can't detect your input-also-votes example even in principle. That's why its haddock documents it as a conservative overestimate; over-counting only rounds the fee up, while the under-count fixed here is what causes FeeTooSmallUTxO. For exact fees, calculateMinTxFee already goes through the ledger's calcMinFeeTx, which counts the size of the deduplicated getWitsVKeyNeeded set, votes included.

What this PR now does about it: both estimators fold every role whose key hashes the body itself pins down - certificates, withdrawals, extra key witnesses and votes - into a single deduplicated Set (KeyHash Witness), mirroring the ledger's union, so a key required by several of those roles counts once. There's a property test covering a stake key that both withdraws and is deregistered in one transaction. Only inputs and collateral are still counted per input on top of that set, which is the part that cannot be deduplicated without a UTxO; the haddock documents that remaining limitation.

@carbolymer
carbolymer force-pushed the mgalazyn/fix/vote-key-witness-count branch from bf26988 to 599e852 Compare September 8, 2026 12:10
@carbolymer
carbolymer force-pushed the mgalazyn/fix/vote-key-witness-count branch from 6d00b7e to 21887e4 Compare September 8, 2026 13:43
@carbolymer
carbolymer requested a review from Jimbo4350 September 8, 2026 13:44
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.

[FR] - TxVotingProcedures accepts ScriptWitness but not broader Witness type

4 participants