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
carbolymer
changed the title
8fd6482 to
eae5142
Compare
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
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
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.The experimental estimator now counts one key witness per
AnyKeyWitnessPlaceholderentry in the vote witness map, andestimateTransactionKeyWitnessCountis newly exported fromCardano.Api.Experimental(it was previously unreachable outside the package).The legacy
TxVotingProceduresmap isScriptWitness-only, so its count is instead derived from the ledgerL.VotingProceduresvoter credentials: aCommitteeVoter/DRepVoterneeds a key witness only when its credential is aKeyHashObj, and aStakePoolVoteralways does.Proposals are deliberately not counted, since the ledger requires no key witness for them.
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
The legacy fix mirrors ledger's own
voterWitnesses(Cardano.Ledger.Conway.UTxO), so the credential-matching 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 mix of key-credentialed and script-credentialed voters and asserting the exact expected key witness count:
Test.Cardano.Api.Experimental.Fee:estimateTransactionKeyWitnessCount counts key witnesses required by key-credentialed voters.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 tests fail against the pre-fix code with a bare
0 === 1(or similar), since the old record patterns simply never destructuredtxVotingProcedures.Checklist
.changes/