Fix key witness count underestimation for votes in fee estimation - #1271
Fix key witness count underestimation for votes in fee estimation#1271carbolymer wants to merge 1 commit into
Conversation
eae5142 to
bf26988
Compare
There was a problem hiding this comment.
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
txVotingProceduresbased on ledger voter credential types (key-hash DReps, committee hot keys, and SPOs). - Experimental fee estimator now counts
AnyKeyWitnessPlaceholderentries in the vote witness map, andestimateTransactionKeyWitnessCountis exported fromCardano.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
left a comment
There was a problem hiding this comment.
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
| votingProcedures = | ||
| L.VotingProcedures $ | ||
| Map.fromList [(voter, Map.singleton govActionId votingProcedure) | voter <- allVoters] |
There was a problem hiding this comment.
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) -> |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
bf26988 to
599e852
Compare
6d00b7e to
21887e4
Compare
Context
Closes #722.
Both variants of
estimateTransactionKeyWitnessCount(experimentalCardano.Api.Experimental.Tx.Internal.Feeand legacyCardano.Api.Tx.Internal.Fee) ignoredtxVotingProceduresentirely, 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.VotingProceduresvoter credentials, folded into aSetof witness key hashes exactly like ledger'svoterWitnesses: aCommitteeVoter/DRepVoterneeds a key witness only when its credential is aKeyHashObj, aStakePoolVoteralways does, and a voter voting on several governance actions, or a key hash appearing under several governance roles, is counted once.estimateTransactionKeyWitnessCountis also newly exported fromCardano.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, andcalculateMinTxFeeremains the exact path when aUTxOis available.Note that the type asymmetry the issue asks about (
ScriptWitnessvsWitness) is already resolved in the experimental API viaAnyWitness; this PR closes the remaining fee-estimation gap in both APIs.How to trust this PR
Both fixes mirror ledger's own witness collection (
voterWitnessesand thegetWitsVKeyNeededunion inCardano.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 themkTxVotingProceduresmerge 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 dedupeproperty: 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 ownerproperty 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 destructuredtxVotingProcedures.Checklist
.changes/