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

ripemd_160builtin #6378

Merged
merged 20 commits into from
Aug 13, 2024
Merged

ripemd_160builtin #6378

merged 20 commits into from
Aug 13, 2024

Conversation

kwxm
Copy link
Contributor

@kwxm kwxm commented Aug 6, 2024

This updates #6252 to make it mergeable with the current state of plutus-core (including all of the bitwise work). This implements the proposed CIP for adding the RIPEMD-160 hash function to Plutus. Thanks to @paluh for doing all the hard work.

I've also updated the cost model to include ripemd_160. As a precaution I re-ran the costing benchmarks for keccak_256 to make sure that nothing had changed too much. The attached PNG shows the results. The original results for keccak_256 are the black points and the new cones s are the blue ones: there's a small difference that's probably due to randomness in our benchmarking machine, but the results aren't dratically different. The results for ripemd_160 are the red points and they don't show any unexpected behaviour. The costing function for ripemd_160 is a straight line fitted to those points.

ripemd-160

@kwxm
Copy link
Contributor Author

kwxm commented Aug 6, 2024

There were some unexpected changes in the golden files for the cardano-constitution tests, which I didn't touch at all. These look like the random kind of thing we've seen before where an addition alters the behaviour of the compiler, but someone should check.

nix/project.nix Outdated
@@ -38,6 +38,7 @@ let

sha256map = {
"https://github.com/jaccokrijnen/plutus-cert"."e814b9171398cbdfecdc6823067156a7e9fc76a3" = "0srqvx0b819b5crrbsa9hz2fnr50ahqizvvm0wdmyq2bbpk2rka7";
"https://github.com/paluh/cardano-base"."ea31e27ae4c9715232fa20e2e91f23e5bd967d65" = "sha256-R/uqcScLbkFhYL8x0FL7eQ2UIJY30o6ec312I/E7KdU=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this line? I thought this is only for SRP's and there is no such SRP in the cabal.project.

@kwxm @paluh Are you using something in your cabal.project.local that should be in the cabal.project or is this line redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why this line? I thought this is only for SRP's and there is no such SRP in the cabal.project.

Thanks for pointing those out, @bezirg. I hadn't spotted that. I'll try removing it and see what happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've removed that. Let's see if it makes any difference.

"memory": {
"arguments": 3,
"type": "constant_cost"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

VERY IMPORTANT!

This has 2 entries for RIPEMD (with different values actually), which is dangerous considering it is JSON: based on the specific implementation, a json library silently picks the first occurrence or the last occurence. Aeson I think picks the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I hope that's fixed now. I added the new costing function but forgot to delete the preliminary one (which was a duplicate of the one for keccak_256). The old one was also in the wrong place, which might not have been so good.

Comment on lines 147 to 149
"ripemd_160-cpu-arguments-intercept": 1927926,
"ripemd_160-cpu-arguments-slope": 82523,
"ripemd_160-memory-arguments": 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

These numbers do not match the builtinCostModelB&C.json but only one of the 2 duplicates in A.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grrr. I thought I'd been very careful about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That file's only for testing, so I think the actual numbers don't matter. I'll change it anyway, just to be on the safe side.

@kwxm
Copy link
Contributor Author

kwxm commented Aug 12, 2024

Somehow this seems to have become broken, possibly due to the changes in #6171. I'll try to fix it later.

@kwxm
Copy link
Contributor Author

kwxm commented Aug 12, 2024

It turned out that a lot of the tests were replicated for the Data-backed script context introduced in #6171 but there was a change for ripemd_160 in this PR that wasn't included, which caused a failure. I've fixed that and made the tests a bit more hierarchical so that it's easier to see what's going on when something goes wrong.

These tests need to be reworked anyway: see #6089.

@kwxm kwxm merged commit 970e755 into master Aug 13, 2024
6 checks passed
@kwxm kwxm deleted the kwxm/paluh/ripemd-160 branch August 13, 2024 13:32
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants