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

The maxMajorPV field of Globals is unused #3682

Closed
nfrisby opened this issue Aug 27, 2023 · 1 comment · Fixed by #4218
Closed

The maxMajorPV field of Globals is unused #3682

nfrisby opened this issue Aug 27, 2023 · 1 comment · Fixed by #4218
Assignees
Labels
enhancement New feature or request

Comments

@nfrisby
Copy link
Contributor

nfrisby commented Aug 27, 2023

Using an up-to-date commit

nfrisby@frisbycomp:~/cardano-haskell/cardano-ledger$ git show origin/master --no-patch
commit 63d98c3f8e9eb2878cec3ab71c54fc40c798ac01 (origin/master, origin/HEAD)
Author: Tim Sheard <[email protected]>
Date:   Thu Aug 24 15:45:07 2023 -0400

    Fix inactive PoolStake not counting as Drep Stake (#3676)
    
    * Changed freshDRepPulser to use map form IncrementalStake rather than SnapShot.

I see that the non-testing code propagates maxMajorPV but never actually uses it.

nfrisby@frisbycomp:~/cardano-haskell/cardano-ledger$ git grep -C1 -w -e maxMajorPV origin/master -- '*hs' | grep -v -e test-suite -e testlib
origin/master:eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs-  Globals
origin/master:eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs:mkShelleyGlobals genesis epochInfoAc maxMajorPV =
origin/master:eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs-  Globals
--
origin/master:eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs-    , maxLovelaceSupply = sgMaxLovelaceSupply genesis
origin/master:eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs:    , maxMajorPV = maxMajorPV
origin/master:eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs-    , networkId = sgNetworkId genesis
--
--
--
--
--
--
--
--
--
--
--
--
--
origin/master:libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs-  -- ^ Quorum for update system votes and MIR certificates
origin/master:libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs:  , maxMajorPV :: !Version
origin/master:libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs-  -- ^ All blocks invalid after this protocol version
--
origin/master:libs/cardano-ledger-pretty/src/Cardano/Ledger/Pretty.hs-      , ("quorum", pretty quor)
origin/master:libs/cardano-ledger-pretty/src/Cardano/Ledger/Pretty.hs:      , ("maxMajorPV", prettyA maxmaj)
origin/master:libs/cardano-ledger-pretty/src/Cardano/Ledger/Pretty.hs-      , ("maxLovelaceSupply", pretty maxlove)

I therefore propose we remove it from the Globals data type.

@nfrisby nfrisby added the enhancement New feature or request label Aug 27, 2023
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 27, 2023

The intended check is being done, just not by the ledger rules.

For TPraos, the max major version is passed as the first argument to chainChecks, which does not receive a Globals argument. It is called from Consensus code, which carries its own "config" that has the value in it. Similarly for Praos, but the check itself is also in the Consensus code base (since TPraos has not been fully relocated to the Consensus repo). See the instances of the envelopeChecks method, which is only applied to headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants