-
Notifications
You must be signed in to change notification settings - Fork 29
Fix key witness count underestimation for votes in fee estimation #1271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| project: cardano-api | ||
| pr: 1271 | ||
| kind: | ||
| - bugfix | ||
| - compatible | ||
| description: | | ||
| Fix fee estimation for transactions containing votes: `estimateTransactionKeyWitnessCount` now accounts for the key witnesses required by key-credentialed voters (key-hash DReps, constitutional committee hot keys, and SPOs), so vote-carrying transactions no longer get underestimated fees and fail with `FeeTooSmallUTxO`. `estimateTransactionKeyWitnessCount` is now also exported from `Cardano.Api.Experimental`. See [issue #722](https://github.com/IntersectMBO/cardano-api/issues/722). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ module Cardano.Api.Experimental.Tx.Internal.Fee | |
| , calcMinFeeRecursive | ||
| , collectTxBodyScriptWitnesses | ||
| , estimateBalancedTxBody | ||
| , estimateTransactionKeyWitnessCount | ||
| , evaluateTransaction | ||
| , TxEvaluationResult (..) | ||
| , evaluateTransactionExecutionUnits | ||
|
|
@@ -1789,6 +1790,7 @@ estimateTransactionKeyWitnessCount | |
| , txWithdrawals | ||
| , txCertificates | ||
| , txProposalProcedures | ||
| , txVotingProcedures | ||
| } = | ||
| fromIntegral $ | ||
| sum (map estimateTxInWitnesses txIns) | ||
|
|
@@ -1807,6 +1809,10 @@ estimateTransactionKeyWitnessCount | |
| Just (TxProposalProcedures m) -> | ||
| OMap.size m | ||
| Nothing -> 0 | ||
| + case txVotingProcedures of | ||
| Just (TxVotingProcedures _ voteWits) -> | ||
| length [() | AnyKeyWitnessPlaceholder <- Map.elems voteWits] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| Nothing -> 0 | ||
| where | ||
| estimateTxInWitnesses :: (TxIn, AnyWitness (LedgerEra era)) -> Int | ||
| estimateTxInWitnesses (_, AnyKeyWitnessPlaceholder) = 1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| {-# LANGUAGE NumericUnderscores #-} | ||
| {-# LANGUAGE OverloadedStrings #-} | ||
| {-# LANGUAGE ScopedTypeVariables #-} | ||
| {-# LANGUAGE TypeApplications #-} | ||
| {-# LANGUAGE TypeFamilies #-} | ||
|
|
||
| module Test.Cardano.Api.Experimental.Fee | ||
|
|
@@ -38,7 +39,13 @@ import Data.Time.Clock.POSIX qualified as Time | |
| import GHC.Exts (fromList) | ||
| import Lens.Micro | ||
|
|
||
| import Test.Gen.Cardano.Api.Typed (genAddressInEra, genStakeCredential, genTxIn) | ||
| import Test.Gen.Cardano.Api.Typed | ||
| ( genAddressInEra | ||
| , genScriptHash | ||
| , genStakeCredential | ||
| , genTxIn | ||
| , genVerificationKeyHash | ||
| ) | ||
|
|
||
| import Test.Cardano.Api.Experimental (exampleProtocolParams, exampleProtocolParamsEra) | ||
|
|
||
|
|
@@ -70,6 +77,12 @@ tests = | |
| "unwitnessed certs produce no script witnesses" | ||
| prop_collectTxBodyScriptWitnesses_ignores_unwitnessed_certs | ||
| ] | ||
| , testGroup | ||
| "estimateTransactionKeyWitnessCount" | ||
| [ testProperty | ||
| "counts key witnesses required by key-credentialed voters" | ||
| prop_estimateTransactionKeyWitnessCount_counts_vote_key_witnesses | ||
| ] | ||
| , testGroup | ||
| "createCompatibleTx" | ||
| [ testProperty | ||
|
|
@@ -1288,6 +1301,24 @@ prop_createCompatibleTx_preserves_all_certs = H.property $ do | |
| let bodyCerts = ledgerTx ^. L.bodyTxL . L.certsTxBodyL | ||
| Seq.length bodyCerts H.=== expectedCount | ||
|
|
||
| -- | Regression test for: a key-credentialed voter (e.g. a key-hash DRep) | ||
| -- requires a VKey witness to satisfy the ledger, but | ||
| -- 'estimateTransactionKeyWitnessCount''s record pattern does not destructure | ||
| -- 'txVotingProcedures' at all, so a vote witnessed by | ||
| -- 'AnyKeyWitnessPlaceholder' contributes zero to the estimate. A transaction | ||
| -- containing only a generated mix of key-credentialed and script-credentialed | ||
| -- votes (and nothing else) must be estimated to need exactly one key witness | ||
| -- per key-credentialed voter - script-credentialed votes must not add to the | ||
| -- count. | ||
| prop_estimateTransactionKeyWitnessCount_counts_vote_key_witnesses :: Property | ||
| prop_estimateTransactionKeyWitnessCount_counts_vote_key_witnesses = H.property $ do | ||
| (txVotingProcedures, expectedKeyWitnessCount) <- H.forAll genVotingProceduresWithKeyWitnessCount | ||
| let txBodyContent = | ||
| Exp.defaultTxBodyContent | ||
| & Exp.setTxVotingProcedures txVotingProcedures | ||
| keyWitnessCount = Exp.estimateTransactionKeyWitnessCount @Exp.ConwayEra txBodyContent | ||
| keyWitnessCount H.=== fromIntegral expectedKeyWitnessCount | ||
|
|
||
| -- --------------------------------------------------------------------------- | ||
| -- Shared cert generators | ||
| -- --------------------------------------------------------------------------- | ||
|
|
@@ -1345,3 +1376,61 @@ genShuffledCertsWithCount = do | |
| ] | ||
| shuffled <- Gen.shuffle allCerts | ||
| pure (shuffled, length shuffled) | ||
|
|
||
| -- --------------------------------------------------------------------------- | ||
| -- Shared vote generators | ||
| -- --------------------------------------------------------------------------- | ||
|
|
||
| -- | Generate a 'TxVotingProcedures' whose witness map mixes key-credentialed | ||
| -- voters ('L.DRepVoter'/'L.CommitteeVoter' over a key hash, plus | ||
| -- 'L.StakePoolVoter', which is always key-credentialed - all witnessed by | ||
| -- 'AnyKeyWitnessPlaceholder') with script-credentialed voters | ||
| -- ('L.DRepVoter'/'L.CommitteeVoter' over a script hash, witnessed by a | ||
| -- reference-input simple script - 'L.StakePoolVoter' has no | ||
| -- script-credentialed form). Each bucket draws its hashes via 'Gen.set', so | ||
| -- voters within a bucket never collide as witness-map keys; voters across | ||
| -- buckets can never collide either, since they differ in the 'L.Voter' or | ||
| -- 'L.Credential' constructor regardless of the underlying hash bytes. | ||
| -- Returns the generated voting procedures together with the number of | ||
| -- key-credentialed voters, i.e. the key-witness count | ||
| -- 'estimateTransactionKeyWitnessCount' must report for a transaction | ||
| -- containing only these votes. | ||
| genVotingProceduresWithKeyWitnessCount | ||
| :: Gen (Exp.TxVotingProcedures (Exp.LedgerEra Exp.ConwayEra), Int) | ||
| genVotingProceduresWithKeyWitnessCount = do | ||
| drepKeyHashes <- | ||
| Gen.set (Range.linear 0 5) (Api.unDRepKeyHash <$> genVerificationKeyHash Api.AsDRepKey) | ||
| committeeKeyHashes <- | ||
| Gen.set | ||
| (Range.linear 0 5) | ||
| (Api.unCommitteeHotKeyHash <$> genVerificationKeyHash Api.AsCommitteeHotKey) | ||
| stakePoolKeyHashes <- | ||
| Gen.set (Range.linear 0 5) (Api.unStakePoolKeyHash <$> genVerificationKeyHash Api.AsStakePoolKey) | ||
| drepScriptHashes <- Gen.set (Range.linear 0 5) (Api.toShelleyScriptHash <$> genScriptHash) | ||
| committeeScriptHashes <- Gen.set (Range.linear 0 5) (Api.toShelleyScriptHash <$> genScriptHash) | ||
| refTxIn <- genTxIn | ||
|
|
||
| let keyVoters = | ||
| [L.DRepVoter (L.KeyHashObj kh) | kh <- toList drepKeyHashes] | ||
| <> [L.CommitteeVoter (L.KeyHashObj kh) | kh <- toList committeeKeyHashes] | ||
| <> [L.StakePoolVoter kh | kh <- toList stakePoolKeyHashes] | ||
| scriptVoters = | ||
| [L.DRepVoter (L.ScriptHashObj sh) | sh <- toList drepScriptHashes] | ||
| <> [L.CommitteeVoter (L.ScriptHashObj sh) | sh <- toList committeeScriptHashes] | ||
|
|
||
| govActionId = | ||
| L.GovActionId | ||
| (L.TxId (L.unsafeMakeSafeHash "0000000000000000000000000000000000000000000000000000000000000000")) | ||
| (L.GovActionIx 0) | ||
| votingProcedure = L.VotingProcedure{L.vProcVote = L.VoteYes, L.vProcAnchor = L.SNothing} | ||
| scriptWitness = Exp.AnySimpleScriptWitness (Exp.SReferenceScript refTxIn) | ||
| allVoters = keyVoters <> scriptVoters | ||
| votingProcedures = | ||
| L.VotingProcedures $ | ||
| Map.fromList [(voter, Map.singleton govActionId votingProcedure) | voter <- allVoters] | ||
|
Comment on lines
+1428
to
+1430
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should try with several |
||
| witnessMap = | ||
| Map.fromList $ | ||
| [(voter, Exp.AnyKeyWitnessPlaceholder) | voter <- keyVoters] | ||
| <> [(voter, scriptWitness) | voter <- scriptVoters] | ||
|
|
||
| pure (Exp.TxVotingProcedures votingProcedures witnessMap, length keyVoters) | ||
There was a problem hiding this comment.
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