Skip to content
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

Introduce accurate min fee calculation function: calcMinFeeTx #3976

Merged
merged 12 commits into from
Jan 10, 2024

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Jan 6, 2024

Description

Addition of calcMinFeeTx function is a better alternative to estimateMinFeeTx introduced in #3962

In order to implement this function it was necessary to:

  • add genesisKeyHashCount, that counts up how many genesis key transaction will add
  • Properly fix getWitsVKeyNeeded to also include reqSignerHashes as it was designed in the spec.

This last change is important, because that is the only change in this PR that actually affects the rules. Currently for Babbage and Alonzo it introduces a redundant check. It is not a problem since checking presence of a few extra hashes from reqSignerHashes in a map of provided hashes is not expensive at all. However this complete functionality of getWitsVKeyNeeded according to the spec and allows us to use it for fee calculation. Also when we enter Conway, we'l be able to get rid of the redundant check, see #3972

Moreover, this new function calcMinFeeTx was utilized in the ImpTest in order to take care fixing up the fee during creation of test transactions. Fixes #3767

Some extra cleanup was also added:

  • Deprecation of poorly named txid in favor of better named txIdTxBody and txIdTx
  • Deprecation of poorly named txup function and switching of PPUP rule signal from Maybe Update to StrinctMaybe Update in order to reduce unnecessary conversions
  • Add ProtVerAtMost era 8 as a superclass constraint to ShelleyEraTxBody effectively disabling the type class from Conway onwards. This resulted in a cleanup of redundant constraints and fixup of over-constrained functions
  • Also, estimateMinFeeTx functionality that was introduced in Add estimateMinFeeTx #3962 was moved to a new module Cardano.Ledger.Tools in order to make it accessible to the test suite

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

Looks good to me, tons of improvements packed in this PR 👍

@Soupstraw
Copy link
Contributor

Soupstraw commented Jan 9, 2024

I assume it also closes #3970 (which is basically a duplicate of #3767)

Copy link
Contributor

@Soupstraw Soupstraw left a comment

Choose a reason for hiding this comment

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

Looks good overall.

I have bit of mixed feelings about losing the ShelleyEraTxBody superclass, since it makes the system of typeclasses less uniform. Sure, we can get rid of some ProtVerAtMost constraints, but we also have to add an explicit ShelleyEraTxBody constraint to some functions that didn't need it before. Then again it's nice to not have to implement ShelleyEraTxBody with a bunch of dummy methods from Conway onwards.

@lehins
Copy link
Collaborator Author

lehins commented Jan 9, 2024

I have bit of mixed feelings about losing the ShelleyEraTxBody superclass, since it makes the system of typeclasses less uniform.

It allows us to deprecate a lot of functionality in bulk. Starting with the next era that follows Conway we'll do the same thing for ShelleyEraTxCert. So, it will become a uniform patter over time: if all functionality from the era is deprecated we can deprecate the whole class.

we can get rid of some ProtVerAtMost constraints, we also have to add an explicit ShelleyEraTxBody constraint to some functions that didn't need it before

We can replace one for another. That's all that has happend in this PR.

Then again it's nice to not have to implement ShelleyEraTxBody with a bunch of dummy methods from Conway onwards.

Yeah for all of the eras that will follow.

I think it makes the deprecation process much cleaner when we can deprecate a class as a whole.

lehins added 3 commits January 9, 2024 21:58
Those functions can be useful in testing as well as for downstream
users, therefore it makes sense to start a new module where we can
collect functionality of this sorts.
…on 9.

This is a nice cleanup that allows us removing `ShelleyEraTxBody`
instance for `ConwayEra` onwards.

This commit also fixes a bunch of overconstraint functions and removes
some redundant constraints
lehins added 3 commits January 9, 2024 21:58
This allows us to deprecate the horribly named and useless `txup` function.

Also it simplifies the implementation a little bit by avoiding redundant
conversion
Using `getAlonzoWitsVKeyNeeded` for Alonzo and Babbage is safe, because
it will check `reqSignerHashes` twice, but it allows us to rely on
`getWitsVKeyNeeded` to produce all of the required witnesses, aside from
the ones that are used in native scripts. This also makes implementation
match the spec.

#3972 tracks the
removal of `MissingRequiredSigners`
@lehins lehins force-pushed the lehins/calcMinFeeTx branch from 5320a8b to f491d56 Compare January 9, 2024 20:59
lehins added 6 commits January 9, 2024 23:03
* And `getShelleyGenesisKeyHashCountTxBody`
* Deprecate `txin`
* Add `txIdTx` and `txIdTxBody`
* Organize `submitTx` functions in ImpTest
We can now remove the duplicated check for `reqSignerHashes`, however we
still need to keep around the predicate failure for NodeToClient
comunication.
@lehins lehins force-pushed the lehins/calcMinFeeTx branch from f491d56 to 8149bd6 Compare January 9, 2024 22:26
@lehins lehins enabled auto-merge January 9, 2024 22:26
@lehins lehins merged commit 9590b60 into master Jan 10, 2024
10 of 100 checks passed
@neilmayhew neilmayhew deleted the lehins/calcMinFeeTx branch March 8, 2024 21:07
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.

Improve fixupTx fee calculation
3 participants