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.

Sign up for GitHub

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

Jump to bottom

Add new builtin hashing function ripemd_160 #6252

Closed
wants to merge 1 commit into from
Closed

Add new builtin hashing function ripemd_160 #6252

wants to merge 1 commit into from

Conversation

paluh
Copy link
Contributor

@paluh paluh commented Jun 27, 2024

I'm reopening this as a separate PR because of the permissions issue (I'm not able to push to the branch under IntersectMBO/plutus anymore.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@paluh
Copy link
Contributor Author

paluh commented Jun 27, 2024 •
edited
Loading

This is preliminary work which exposes RIPEMD-160 hashing primitive as a new bulitin.

  • I closely followed changes which were made during introduction of keccak_256 and blake2b_224 functions.
  • The hashing function is included in the latest version of cardano-crypto-class so I bumped the version.
  • This draft contains an artificial costing model for the new builtin - I copied keccak_256 related values to be able to test the function.

This was referenced Jun 27, 2024
Closed
Open
@paluh paluh requested a review from kwxm June 28, 2024 07:54
@zeme-wana zeme-wana force-pushed the master branch 2 times, most recently from a161078 to db5cabb Compare July 9, 2024 09:24
kwxm
kwxm reviewed Jul 12, 2024
View reviewed changes
Copy link
Contributor

@kwxm kwxm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this all looks OK. I'm impressed that you did the Agda as well!

However, we're hoping to merge the costing code for the extra bitwise builtins (which have already been merged) shortly. Can you wait until that's done so that all of the bitwise operations are in the same place, and then rebase and put ripemd160 after those? This may require some rearrangements in your code (and also quite a bit of other stuff has been merged since you opened this PR): apologies for that.

plutus-core/cost-model/data/builtinCostModelA.json
@@ -527,6 +527,19 @@
"type": "constant_cost"
}
},
"ripemd_160": {
"cpu": {
"arguments": {
Copy link
Contributor

Choose a reason for hiding this comment

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

You must have run the benchmarks to get these numbers. Which machine did you run them on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kwxm - unfortunately I did not benchmark this as I stated above:

This draft contains an artificial costing model for the new builtin - I copied keccak_256 related values for now.

That is for sure missing part :-( Sorry for the confusion.

plutus-ledger-api/test/Spec/Versions.hs
@@ -76,7 +76,7 @@ testBuiltinVersions = testGroup "builtins"
assertBool "not in l2,Vasil" $ isRight $ V2.deserialiseScript vasilPV serialiseDataExScript
assertBool "not in l3,future" $ isRight $ V3.deserialiseScript futurePV serialiseDataExScript
, testCase "bls,keccak256,blake2b224 only available in l3,Future and after" $
for_ [blsExScript, keccak256ExScript, blake2b224ExScript] $ \script -> do
for_ [blsExScript, keccak256ExScript, blake2b224ExScript, ripemd160ExScript] $ \script -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not totally sure if ripemd160 should be in here, but these tests need to be reworked anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can drop it if you wish :-)

@paluh
Copy link
Contributor Author

paluh commented Jul 12, 2024

Can you wait until that's done so that all of the bitwise operations are in the same place, and then rebase and put ripemd160 after those? This may require some rearrangements in your code (and also quite a bit of other stuff has been merged since you opened this PR): apologies for that.

Sure. I will rebase and then we can sync how to finalize this PR. I'm really kin to have it merged - it will allow some fancy stuff on Cardano like checking Bitcoin witness under Bitcoin Tx etc.

This was referenced Aug 6, 2024
Merged
Closed
@kwxm
Copy link
Contributor

kwxm commented Aug 12, 2024

I'm going to close this in favour of #6378, which I'm just about to merge.

@kwxm kwxm closed this Aug 15, 2024
@paluh paluh mentioned this pull request Oct 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwxm kwxm kwxm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paluh @kwxm