-
Notifications
You must be signed in to change notification settings - Fork 483
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
Keccak-256 and Blake2b-224 (PLT-6305, PLT-6306, PLT-6311, PLT-6820) #5431
Conversation
@@ -1,13 +1,16 @@ | |||
-- | Hash functions for lazy [[Data.ByteString.ByteString]]s | |||
{-# LANGUAGE TypeApplications #-} | |||
module Data.ByteString.Hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to rename this because the other crypto stuff is in this directory already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should probably have a CIP for blake2b-224, alas. Kenneth do you want to draft it?
@@ -21,6 +24,14 @@ sha2_256 = digest (Proxy @SHA256) | |||
sha3_256 :: BS.ByteString -> BS.ByteString | |||
sha3_256 = digest (Proxy @SHA3_256) | |||
|
|||
-- | Hash a [[BSL.ByteString]] using the Blake2B-256 hash function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- | Hash a [[BSL.ByteString]] using the Blake2B-256 hash function. | |
-- | Hash a [[BSL.ByteString]] using the Blake2B-224 hash function. |
"blake2b_256-cpu-arguments-intercept": 117366, | ||
"blake2b_256-cpu-arguments-slope": 10475, | ||
"blake2b_256-memory-arguments": 4, | ||
"bls12_381_G1_add-cpu-arguments": 1046420, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmm, weird that these were missing before??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wondered about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think that's OK. This set of cost model parameters is used by the tests for the code that updates the cost model using the flattened parameters. That's the code that uses the JSON-y stuff and I think that I wrote the tests to convince myself that it was doing the right thing. The tests are self-contained and it doesn't matter exactly what cost model is used as long as it's sensible. It won't do any harm to extend it though.
, testCase "context length" $ do | ||
let defaultCostValues = Map.elems $ fromJust defaultCostModelParams | ||
-- the defaultcostmodelparams reflects only the latest version V3, so this should succeed because the lengths match | ||
assertBool "wrong number of arguments in V2.mkContext" $ isRight $ runExcept $ runWriterT $ V3.mkEvaluationContext defaultCostValues | ||
-- currently v2 args ==v3 args | ||
-- currently v2 args == v3 args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is funny about these tests, should one of these be failing? @bezirg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think PLT-5390 (which we were talking about yesterday) should cover that. I had to add some stuff to those tests for BLS12-381 a while ago and I wasn't totally confident that I'd got it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that comment's wrong. I corrected the spacing but I should probably just have deleted it.
See #5369 (like it says in the PR description!), especially this comment. Maybe we've got the cart before the horse here, but I don't think that'll be too controversial in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, pretty straightforward.
@@ -21,6 +24,14 @@ sha2_256 = digest (Proxy @SHA256) | |||
sha3_256 :: BS.ByteString -> BS.ByteString | |||
sha3_256 = digest (Proxy @SHA3_256) | |||
|
|||
-- | Hash a [[BSL.ByteString]] using the Blake2B-224 hash function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon my ignorance, what is this [[...]]
syntax in Haddock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I asked about that before and no-one could tell me, then I just copied it without looking.
-- | Hash a [[BSL.ByteString]] using the Blake2B-224 hash function. | ||
blake2b_224 :: BS.ByteString -> BS.ByteString | ||
blake2b_224 = digest (Proxy @Blake2b_224) | ||
|
||
-- | Hash a [[BSL.ByteString]] using the Blake2B-256 hash function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it's not BSL.ByteString
but rather BS.ByteString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just changed it to ByteString
. The signature should tell us everything we need to know about the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job. I'm happy to see that there seems to be no pain in adding new builtins.
Yes, it's way easier than it used to be, so thanks for that. |
Are there any updates to Also, it is wrongly defined. The inline version is exported by PlutusTx.Builtins.
blake2b_256 is used instead of blake2b_224
I suspect this is a typo, as it is correctly exported from PlutusTx.Builtins.Internal. |
@kwxm @michaelpj |
It was a typo and has been fixed. |
…ntersectMBO#5431) * WIP * keccak256 -> keccak_256 * Update plutus-metatheory * Add blake2b_224 * Add blake2b_224 * Add blake2b_224 * Merge update * Fix a couple of omissions * Add missing imports * Add keccak_256 and blake2b_224 to V3.ParamName * Add test for keccak_256 and blake2b_224 * Fake cost model for keccak_256 and blake2b_224 * Update json file for CostModelInterface tests * Add changelog entries * Add Keccak_256 and Blake2b_224 to Versions.hs in plutus-ledger-api * Typo in comment * Typo in comment * Fix dodgy Haddock * Fix dodgy Haddock * Add property test for hash sizes * Haddock for hash size tests * Formatting * Formatting * Fix some BLS-related comments (unrelated to this PR)
This adds new builtins for
keccak_256
(see this proposed CIP), which will help with Ethereum compatiblity, andblake2b_224
, which will allow on-chain computation of ledger-compatiblePubKeyHash
hashes (see #5369, which should also lead to a CIP in the near future). This PR adds these functions toplutus-core
,plutus-tx
,plutus-metatheory
andplutus-ledger-api
.The following remain to be done
blake2b_256
forblake2b_224
and the one forsha3_256
forkeccak_256
, which is a variant ofsha3_256
.