Add gas to decimal-arithmetic #1217

jmcardon · 2023-05-03T23:29:29Z

No description provided.

pact.cabal

src/Pact/Native/Trans.hs

rsoeldner

Great work

rsoeldner · 2023-05-04T04:56:34Z

src/Pact/Native/Trans.hs

+  threshold = (10 :: Integer) ^ (80 :: Integer)
+  intValue :: Integer
+  intValue = fromIntegral coeff
+-- Todo: should there be a penalty on `NaN` or `Inf`?


I would argue no, as long as we account a constant factor for it. Do you have a particular benefit in mind?

We throw errors on NaN and Inf. That's why this comment.

There should be a penalty imo. NaN and Inf are only seen in degenerate contexts and the user should be paying to check that they're not, say, dividing by 0.

yeah i'm just not sure. They're non-observable values, and Data.Decimal can't express NaN or Inf.

rsoeldner · 2023-05-04T05:14:45Z

src/Pact/Native/Trans.hs

+    in fromIntegral (nbytes * nbytes `quot` 100)
+  where
+  threshold :: Integer
+  threshold = (10 :: Integer) ^ (80 :: Integer)


Basically, decimal with a coefficient < 10^80 is free of charge? Shouldn't our accounting incorporate the exponent?

Also, shouldn't we account for everything (gas > 0), including the case intValue < threshold?

above 10^80 it's a penalty. We do charge 1 gas for integers larger than this. It will become more granular in the future.

We don't account for the exponent atm, but there's not all that much arithmetic around it.

rsoeldner · 2023-05-04T05:15:57Z

src/Pact/Types/Gas.hs

+
+thresholdToInteger :: IntThreshold -> Integer
+thresholdToInteger = \case
+  ThresholdPrePact47 -> (10 :: Integer) ^ (30 :: Integer)


Can we comment on the rational\origins of these constants? They are unclear to me.

10^30 was an arbitrary constant from before.
10^80 is just a close number to 2^256. Essentially, allowing for cheap math on numbers below that threshold.

😃
Thank you @jmcardon

#1152

Yes, you were the motivation for increasing this limit, @CryptoPascal31 :)

rsoeldner · 2023-05-04T05:17:57Z

trans/src/Pact/Trans/Dec.hs


 {-# OPTIONS_GHC -Wno-incomplete-record-updates #-}
+{-# OPTIONS_GHC -Wno-unrecognised-pragmas #-}
+{-# HLINT ignore "Use camelCase" #-}


rsoeldner · 2023-05-04T05:18:50Z

src/Pact/Types/Gas.hs

@@ -206,7 +223,7 @@ instance Pretty GasArgs where
    GFoldDB -> "GFoldDB"
    GModuleMemory i -> "GModuleMemory: " <> pretty i
    GPrincipal i -> "GPrincipal: " <> pretty i
-    GIntegerOpCost i j -> "GIntegerOpCost:" <> pretty i <> colon <> pretty j
+    GIntegerOpCost i j ts -> "GIntegerOpCost:" <> pretty i <> colon <> pretty j <> colon <> pretty ts
    GDecimalOpCost i j -> "GDecimalOpCost:" <> pretty (show i) <> colon <> pretty (show j)


To we need the show here?

Yeah I don't see why leave it out of the debug info of what threshold we used for gas.

rsoeldner · 2023-05-04T05:20:36Z

trans/src/Pact/Trans/Dec.hs

+       (gasContext gp) of
+    (Left err, ctx) 
+      | exceptionSignal err == GasExceeded -> TransGasExceeded (ctxGas ctx)
+      | otherwise -> error $ "ln error: " ++ show err


this is pow error

honestly we need to remove all uses of error

why are we erroring here? That's a pretty hard kill. I'd prefer being in a monad here before using error.

great question, we're erroring here because this was from john's pr. We shouldn't error anymore.

rsoeldner · 2023-05-04T05:20:55Z

trans/src/Pact/Trans/Dec.hs

+       (gasContext gp) of
+    (Left err, ctx) 
+      | exceptionSignal err == GasExceeded -> TransGasExceeded (ctxGas ctx)
+      | otherwise -> error $ "ln error: " ++ show err


this should be sqrt error

should just remove error

jwiegley · 2023-05-04T17:28:31Z

cabal.project

@@ -19,5 +19,4 @@ constraints: hspec-golden <0.2,
 source-repository-package
    type: git
    location: https://github.com/kadena-io/decimal-arithmetic.git
-    tag: 5de7f3c9c93344b3932604827147ed5824fd8f45
-    --sha256: sha256-+jyOtjJIJo7ggQsNqBS/re36fXom00M71neLa8dm/dM=
+    tag: 18eb743b8cf4be3904427f82566c5b10fa9a38e4


Suggested change

tag: 18eb743b8cf4be3904427f82566c5b10fa9a38e4

tag: 18eb743b8cf4be3904427f82566c5b10fa9a38e4

--sha256: sha256-Vro3wB22oPJDiZw0k8MonwPmoDZFt2rAGU709p9Cfqk=

already on a different tag lol

jwiegley · 2023-05-04T17:30:09Z

src/Pact/Gas.hs

@@ -95,5 +95,6 @@ freeGasEnv = GasEnv 0 0.0 (constGasModel 0)
 constGasModel :: Word64 -> GasModel
 constGasModel r = GasModel
  { gasModelName = "fixed " <> tShow r
+  , gasModelType = ConstantGasModel (fromIntegral r)


Can you say more about how this addition relates to the decimal arithmetic work? It looks like you're adding new functionality that is unrelated to the decimal work per se.

See: https://github.com/kadena-io/pact/pull/1217/files#diff-cab0577f6cd5a3953dabcc04e946f3c74429edb0f0a0dea8088cba1e4bdc7061R65

or, in code:

getTransGasParams = do gUsed <- view eeGas >>= liftIO . readIORef gEnv <- view eeGasEnv let chargeFn = getCharge (view (geGasModel . to gasModelType) gEnv) gLim = view geGasLimit gEnv pure (Dec.TransGasParams (fromIntegral gUsed) (fromIntegral gLim) chargeFn) where getCharge (ConstantGasModel g) = const (chargeArithGas (fromIntegral g)) getCharge _ = chargePactArith

I think I can maybe undo this and simply use runGasModel, but I need to add an entry for the GasArgs for arithmetic functions, and the type parameters there are a bit funky. I will investigate this

src/Pact/Gas/Table.hs

jwiegley · 2023-05-04T17:31:19Z

src/Pact/Gas/Table.hs

  | otherwise =
    let !nbytes = (I# (IntLog.integerLog2# (abs a)) + 1) `quot` 8
    in fromIntegral (nbytes * nbytes `quot` 100)
  where
  threshold :: Integer
-  threshold = (10 :: Integer) ^ (30 :: Integer)


This constant should receive a clearer name, and perhaps be lifted out and up to the top of the file.

arbitraryConstant420

I disagree with retroactively changing this. I'd rather simply use the Threshold type and annotate that.

jwiegley · 2023-05-04T17:31:53Z

src/Pact/Native/Ops.hs

@@ -154,16 +154,22 @@ powImpl i as@[TLiteral a _,TLiteral b _] = do
    | otherwise = twoArgIntOpGas x x *> odds (x * x) (y `quot` 2) (x * z)
 powImpl i as = argsError i as

+getThreshold :: Eval e IntThreshold
+getThreshold = 
+  ifExecutionFlagSet' FlagDisablePact47 ThresholdPrePact47 ThresholdPact47


Can you please add a comment here to describe how exactly these flags have changed the threshold?

emilypi · 2023-05-04T20:23:42Z

src/Pact/Native/Trans.hs

+  threshold = (10 :: Integer) ^ (80 :: Integer)
+  intValue :: Integer
+  intValue = fromIntegral coeff
+-- Todo: should there be a penalty on `NaN` or `Inf`?


There should be a penalty imo. NaN and Inf are only seen in degenerate contexts and the user should be paying to check that they're not, say, dividing by 0.

emilypi · 2023-05-04T20:25:10Z

src/Pact/Types/Gas.hs

@@ -4,6 +4,7 @@
 {-# LANGUAGE OverloadedStrings #-}
 {-# LANGUAGE TypeFamilies #-}
 {-# LANGUAGE LambdaCase #-}
+{-# LANGUAGE InstanceSigs #-}


emilypi · 2023-05-04T20:25:35Z

src/Pact/Types/Gas.hs

 data GasModel = GasModel
  { gasModelName :: !Text
+  , gasModelType :: GasModelType


Suggested change

, gasModelType :: GasModelType

, gasModelType :: !GasModelType

emilypi · 2023-05-04T20:29:17Z

trans/src/Pact/Trans/Dec.hs

+       (gasContext gp) of
+    (Left err, ctx) 
+      | exceptionSignal err == GasExceeded -> TransGasExceeded (ctxGas ctx)
+      | otherwise -> error $ "ln error: " ++ show err


why are we erroring here? That's a pretty hard kill. I'd prefer being in a monad here before using error.

jmcardon · 2024-10-29T15:43:38Z

With MPFR making it into Pact-5, this can be safely closed.

jwiegley and others added 21 commits April 24, 2023 13:57

Initial version of decimal-arithmetic port

bbde73f

Move Musl files, fixes to the build

9fec931

The tests are now passing

ad8f198

add tests from lars musl branch (#1205)

b78a46a

Update docs/en/pact-functions.md

414c8d6

Add more tools to the Pact Nix developer shell

c9e8f62

Update property tests from Lars

bc13fde

Ensure precision is truncated when converting to Data.Decimal

d230026

Fix regression tests in ops.repl

f6ee3e5

Export toDecimal and fromDecimal

40b3c0a

Provide isNaN and isInfinite functions for TransResult

115cc77

Merge branch 'master' into johnw/decimal

b4df168

Setup precision of decimals to 25 digits

09d21b4

Add required library to the dependencies of trans-spec

be5f996

Fix a typo in the tests

2f45754

Merge branch 'master' into johnw/decimal

57aa963

Merge remote-tracking branch 'origin/master' into johnw/decimal

34bf3d9

wip decimal arith

eb10e6d

gas threading in pact trans

ca12c39

wip charge pact arith

9159e2a

cost update integers + cost decimals in exp and co

350b16e

jmcardon requested review from jwiegley and emilypi as code owners May 3, 2023 23:29

jmcardon commented May 3, 2023

View reviewed changes

pact.cabal Show resolved Hide resolved

jmcardon commented May 3, 2023

View reviewed changes

src/Pact/Native/Trans.hs Outdated Show resolved Hide resolved

rsoeldner reviewed May 4, 2023

View reviewed changes

fix constant gas model, decimal arithmetic gas

03c51ce

jmcardon requested a review from sirlensalot as a code owner May 4, 2023 16:36

jwiegley reviewed May 4, 2023

View reviewed changes

src/Pact/Gas/Table.hs Show resolved Hide resolved

jwiegley reviewed May 4, 2023

View reviewed changes

jwiegley mentioned this pull request May 4, 2023

Pact 4.7 Release Prep #1218

Merged

4 tasks

emilypi reviewed May 4, 2023

View reviewed changes

latest fork

853bdb0

edmundnoble mentioned this pull request May 5, 2023

Update package version to 2.19 kadena-io/chainweb-node#1630

Merged

merge with master

ce8b58c

jmcardon changed the base branch from johnw/decimal to master November 15, 2023 15:52

jwiegley mentioned this pull request Nov 27, 2023

Implement transcendental functions using decimal-arithmetic Haskell library #1204

Closed

3 tasks

jmcardon closed this Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gas to decimal-arithmetic #1217

Add gas to decimal-arithmetic #1217

jmcardon commented May 3, 2023

rsoeldner left a comment

rsoeldner May 4, 2023

jmcardon May 4, 2023

emilypi May 4, 2023

jmcardon May 4, 2023

rsoeldner May 4, 2023

jmcardon May 4, 2023

rsoeldner May 4, 2023

jmcardon May 4, 2023

CryptoPascal31 May 4, 2023

jwiegley May 4, 2023

rsoeldner May 4, 2023

rsoeldner May 4, 2023

jmcardon May 4, 2023

rsoeldner May 4, 2023

jmcardon May 4, 2023

emilypi May 4, 2023

jmcardon May 4, 2023

rsoeldner May 4, 2023

jmcardon May 4, 2023

jwiegley May 4, 2023

jmcardon May 4, 2023

jwiegley May 4, 2023

jmcardon May 4, 2023 •

edited

Loading

jwiegley May 4, 2023

jmcardon May 4, 2023

jwiegley May 4, 2023

jmcardon May 4, 2023

emilypi May 4, 2023

emilypi May 4, 2023

emilypi May 4, 2023

emilypi May 4, 2023

jmcardon commented Oct 29, 2024

	tag: 18eb743b8cf4be3904427f82566c5b10fa9a38e4
	tag: 18eb743b8cf4be3904427f82566c5b10fa9a38e4
	--sha256: sha256-Vro3wB22oPJDiZw0k8MonwPmoDZFt2rAGU709p9Cfqk=

	, gasModelType :: GasModelType
	, gasModelType :: !GasModelType

Add gas to decimal-arithmetic #1217

Add gas to decimal-arithmetic #1217

Conversation

jmcardon commented May 3, 2023

rsoeldner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcardon May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcardon commented Oct 29, 2024

jmcardon May 4, 2023 •

edited

Loading