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

Change CMTooFewParamsError to a warning #5912

Merged
merged 5 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -119,24 +119,37 @@ one plutus runtime a.k.a. `plutus-ledger-api.EvaluationContext` per plutus-versi
4) The node software will always pass the array in its entirety to each plutus-runtime,
and not partially just the updated-parameter values (in case of (b)) or just the new-parameter values (in case of (c)).

There is one complication when (c) happens and some running nodes are not updated:
these nodes are only aware of the old set of builtins, thus they expect a specific (fixed in software)
number of cost model parameters.
To guarantee smooth,continuous operation of the entire network (and not cause any splits)
we allow the old nodes to continue operating when receiving more cost model parameters
than they expected, and only issue a warning to them. When the nodes restart and update to the new node software
these warnings will go away. The overall logic for the expected number of cost model paremeters is as follows:
To make (c) work, we must allow a node to continue operating when receiving either more or
fewer cost model parameters than it expects. As an example, suppose at the beginning of
major protocol version 9 (PV9), PlutusV3 has 100 cost model parameters. During PV9, we add
some more builtins to Plutus V3 (to be enabled after the hard fork, at PV10), requiring 20
additional cost model parameters. Then, one submits a proposal updating the number of PlutusV3
cost model parameters to 120.

During PV9, both node-9.x and node-10.x must operate normally and agree on everything. This means
node-9.x must allow receiving more cost model parameters than it expects (since it may receive
120), and node-10.x must allow receiving fewer than it expects (since it may receive 100).
Node-10.x should fill in the missing parameters with a large enough number to prevent the new
builtins from being used, in case the hard fork to PV10 happens without first updating the number
of PlutusV3 cost model parameters to 120 (which is unlikely to happen, but just in case).

During PV10, node-9.x stops working.

The overall logic for the expected number of cost model paremeters is as follows:

(expected number in node software == received number by ledger) => NOWARNING & NOERROR
(expected number in node software < received number by ledger) => WARNING
(expected number in node software > received number by ledger) => ERROR
(expected number in node software > received number by ledger) => WARNING

If the received number is EQ or GT the expected (WARNING), the plutus software
If the received number is EQ or GT the expected (WARNING), we
will take the first n from the received cost model parameters (n==expected number),
and create the internal (nameful) representation of cost model parameters, by assigning a parameter name
to its value: see `PlutusLedgerApi.Common.ParamName.tagWithParamNames` and the `ParamName` datatypes in
plutus-ledger-api.

If the received number is LT the expected (WARNING), we will fill in the missing parameters
with maxBound :: Int64.

See https://github.com/IntersectMBO/cardano-ledger/issues/2902 for a discussion
of these issues and the rationale for adopting the system described above.

Expand Down Expand Up @@ -173,31 +186,28 @@ data CostModelApplyError =
-- ^ internal error when we are transforming the applyParams' input to json (should not happen)
| CMInternalWriteError !String
-- ^ internal error when we are transforming the applied params from json with given jsonstring error (should not happen)
| CMTooFewParamsError { cmTooFewExpected :: !Int, cmTooFewActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]
deriving stock (Eq, Show, Generic, Data)
deriving anyclass (Exception, NFData, NoThunks)

-- | A non-fatal warning when trying to create a cost given some plain costmodel parameters.
data CostModelApplyWarn =
CMTooManyParamsWarn { cmTooManyExpected :: !Int, cmTooManyActual :: !Int }
{- ^ More costmodel parameters given, than expected

See Note [Cost model parameters from the ledger's point of view]
-}
CMTooManyParamsWarn { cmTooManyExpected :: !Int, cmTooManyActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]
| CMTooFewParamsWarn { cmTooFewExpected :: !Int, cmTooFewActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]
Copy link

Choose a reason for hiding this comment

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

It think it is a terrible practice to have record fields for sum types, when they have inconsistent names, because those are essentially partial functions.

In this case we could alleviate it by giving them the same names, but that is not very forward compatible if you plan to add more warnings in the future:

Suggested change
CMTooManyParamsWarn { cmTooManyExpected :: !Int, cmTooManyActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]
| CMTooFewParamsWarn { cmTooFewExpected :: !Int, cmTooFewActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]
CMTooManyParamsWarn { cmExpected :: !Int, cmActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]
| CMTooFewParamsWarn { cmExpected :: !Int, cmActual :: !Int }
-- ^ See Note [Cost model parameters from the ledger's point of view]

IMHO it is better to avoid record syntax for sum types altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point on the renaming. Regarding record fields for sum types, it is handy for construction, but certainly not great for field access.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also good for pattern matching. Ideally you just use NoFieldSelectors in this case, but we can't do that until we drop 8.10.


instance Pretty CostModelApplyError where
pretty = (preamble <+>) . \case
CMUnknownParamError k -> "Unknown cost model parameter:" <+> pretty k
CMInternalReadError -> "Internal problem occurred upon reading the given cost model parameteres"
CMInternalWriteError str -> "Internal problem occurred upon generating the applied cost model parameters with JSON error:" <+> pretty str
CMTooFewParamsError{..} -> "Too few cost model parameters passed, expected" <+> pretty cmTooFewExpected <+> "but got" <+> pretty cmTooFewActual
where
preamble = "applyParams error:"

instance Pretty CostModelApplyWarn where
pretty = (preamble <+>) . \case
CMTooManyParamsWarn{..} -> "Too many cost model parameters passed, expected" <+> pretty cmTooManyExpected <+> "but got" <+> pretty cmTooManyActual
CMTooFewParamsWarn{..} -> "Too few cost model parameters passed, expected" <+> pretty cmTooFewExpected <+> "but got" <+> pretty cmTooFewActual
where
preamble = "applyParams warn:"

Expand Down
7 changes: 5 additions & 2 deletions plutus-ledger-api/src/PlutusLedgerApi/Common/ParamName.hs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import PlutusCore.Evaluation.Machine.CostModelInterface
import Control.Monad.Except
import Control.Monad.Writer.Strict
import Data.Char (toLower)
import Data.Int (Int64)
import Data.List as List (lookup)
import Data.Map qualified as Map
import Data.Text qualified as Text
Expand Down Expand Up @@ -99,9 +100,11 @@ tagWithParamNames ledgerParams =
tell [CMTooManyParamsWarn {cmTooManyExpected = lenExpected, cmTooManyActual = lenActual}]
-- zip will truncate/ignore any extraneous parameter values
pure $ zip paramNames ledgerParams
GT ->
GT -> do
-- Too few parameters - substitute a large number for the missing parameters
-- See Note [Cost model parameters from the ledger's point of view]
throwError $ CMTooFewParamsError {cmTooFewExpected = lenExpected, cmTooFewActual = lenActual }
tell [CMTooFewParamsWarn {cmTooFewExpected = lenExpected, cmTooFewActual = lenActual}]
pure $ zip paramNames (ledgerParams ++ repeat (toInteger (maxBound :: Int64)))
Copy link

Choose a reason for hiding this comment

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

Why don't we switch the type of ledgerParams to Int64? There is no point for CostModel parameter to be specified by an arbitrary precision Integer? mkEvaluationContext will fail for values outside of Int64 range anyways.

Suggested change
pure $ zip paramNames (ledgerParams ++ repeat (toInteger (maxBound :: Int64)))
pure $ zip paramNames (ledgerParams ++ repeat (maxBound :: Int64))

Copy link
Member Author

Choose a reason for hiding this comment

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

mkEvaluationContext will fail for values outside of Int64 range anyways.

How so? Where does it fail?

Copy link

Choose a reason for hiding this comment

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

It'll fail here:

case fromJSON (Object unflattened) of
Success a -> pure a
Error str -> throwError $ CMInternalWriteError str

With something like this:

       CMInternalWriteError "parsing Int failed, value is either floating or will cause over or underflow -1.8446744073709551615e19"

To be honest with you I am really surprised to see JSON and aeson having anything to do with Plutus evaluation context.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a horrible hack, but nobody has been able to come up with a better solution yet 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing ledgerParams to [Int64] will also require some ledger changes, since the ledger currently uses [Integer] right? So this means the ledger will need to do the conversion and make sure it fails if there are overflows. Which may be fine, but I'd rather not do anything not strictly necessary right now.

Copy link

Choose a reason for hiding this comment

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

Changing ledgerParams to [Int64] will also require some ledger changes, since the ledger currently uses [Integer] right?

I've already done it on the ledger side as soon as I found out that Plutus uses aeson. I don't want to rely on aeson to protect us against invalid values.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest with you I am really surprised to see JSON and aeson having anything to do with Plutus evaluation context.

Jesus Christ, what a horrible mess, yes please, let's make mkEvaluationContext accept [Int64].

Copy link
Member Author

Choose a reason for hiding this comment

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

See #5920


-- | Untags the plutus version from the typed cost model parameters and returns their raw textual form
-- (internally used by CostModelInterface).
Expand Down