Add haddock in PlutusTx.Builtins.Internal
#7096
Conversation
| @@ -140,42 +146,56 @@ INTEGER | |||
| -- opaque, but that's going to really mess with our users' code... | |||
| type BuiltinInteger = Integer | |||
|
|
|||
| -- | Adds two integers and never fails. | |||
| addInteger :: BuiltinInteger -> BuiltinInteger -> BuiltinInteger | |||
| addInteger = coerce ((+) @Integer) | |||
There was a problem hiding this comment.
What do these coerces even do here, if BuiltinInteger is just an alias for Integer?
| emptyByteString :: BuiltinByteString | ||
| emptyByteString = BuiltinByteString BS.empty | ||
| {-# OPAQUE emptyByteString #-} | ||
|
|
||
| -- | Computes the SHA2-256 hash of the given bytestring. |
There was a problem hiding this comment.
Did you get tired of the "and never fails" part? :)
(I'm OK either way)
There was a problem hiding this comment.
... and never fails
There was a problem hiding this comment.
Strictly speaking, the addInteger and appendByteString, sliceByteString builtins (and similar) can cause a script failure if the inputs are significantly large that the costing results in it exceeding execution budget.
I understand it's not directly the result of the builtin throwing an error, however, it's extremely important to note this for any builtin that has a dynamic cost based on the size of the inputs because this is a very common attack vector for malicious actors attempting to cause deadlocks which can result in large amounts of funds being permanently lost.
Imagine the user is able to set a very large arbitrary string in the datum the dApp enforces must be included in a continuing output with a prefix appended to that large string. If the string is significantly large and the dApps design didn't account for this, the appendByteString transaction can cost a large amount of budget and if the dApp under normal operation is already quite expensive in terms of ex-units then this can push it over and result in a situation where the continuing output can never be produced because the verification of appendByteString userDatumBytes prefix == userDatumOutputBytes will cause script failure due to budget exceeding.
You probably don't need to document this for every builtin that has a non-constant costing model, but it should be documented somewhere and explained that this applies to all builtins with non-constant costing and that developers need to be aware of this, and possible even include a list of the builtins which are most susceptible to this (those that can consume the most budget with large inputs)
There was a problem hiding this comment.
I wonder if such concern can be highlighted by a static analysis tool, like the one that PlutusHA team was demoing some time ago (based on Stan)
There was a problem hiding this comment.
You probably don't need to document this for every builtin that has a non-constant costing model,
Well, you can push costs over the budget with a constant costing model too. It's kinda expected that users understand they're paying for these things and if they underpay, they'll get a failure.
I don't mind having a note about it in the code or the docs though.
There was a problem hiding this comment.
I have a PR to add this note(+ some of invariants I missed): #7109
There was a problem hiding this comment.
Yes @effectfully agreed, to be clear though it's not about underpaying, they are expected to always pay the proper fee and that's not a problem, it's about how the node imposes a maximum transaction ex-units and if your smart contract requires ex-units > to that limit imposed by the node then it will fail no matter how much you pay for tx fees, thus your funds will be permanently locked in the contract forever no matter what you do unless the cardano protocol parameters change to increase maximum ex-units enough for your contract to succeed.
There was a problem hiding this comment.
The reason this is important to document, is because a common attack is to create a very large integer / bytestring in a datum in a field that doesn't get operated on in an early state of the contracts state progression (a few transactions each with a continuing output produced that preserves that field in the datum), then after a few transactions happen, eventually at some point in the dApp state progression there comes a point where progressing to the next state requires operating on that field in the datum and it is at this point at which the UTxO becomes unspendable because the large integer / bytestring was crafted such that that the cost of performing the operation on it would always push the ex-unit budget over the ledger limit thus making it impossible to construct a transaction that can ever unlock this UTxO unless protocol parameters change to increase ex-units.
There was a problem hiding this comment.
All good points, but this is more about smart contract design than available primitives. Anyway, you're right. No point arguing here.
|
|
||
| Please note that the documentation for each function will only include operational invariants | ||
| if there are any. This documentation assumes type system correctly enforces and prevents any structural | ||
| errors on the generated UPLC. See Note [Structural vs operational errors within builtins]. |
| @@ -863,6 +1013,8 @@ writeBits (BuiltinByteString bs) (BuiltinList ixes) (BuiltinBool bit) = | |||
| BuiltinSuccessWithLogs logs bs' -> traceAll logs $ BuiltinByteString bs' | |||
| {-# OPAQUE writeBits #-} | |||
|
|
|||
| -- | Creates a bytestring of a given length by repeating the given byte. | |||
| -- Fails if the byte, second argument, is not in range @[0,255]@ or the length is negative. | |||
| replicateByte :: | |||
There was a problem hiding this comment.
| @@ -746,6 +873,9 @@ bls12_381_finalVerify (BuiltinBLS12_381_MlResult a) (BuiltinBLS12_381_MlResult b | |||
| CONVERSION | |||
| -} | |||
|
|
|||
| -- | Converts the given integer to a bytestring. The first argument specifies | |||
There was a problem hiding this comment.
closes #6782