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

CIP-0101 | Integration of keccak256 into Plutus #524

Merged
merged 21 commits into from
Sep 7, 2023

Conversation

serejke
Copy link
Contributor

@serejke serejke commented Jun 14, 2023

This CIP proposes to add keccak256 hash function to Plutus to unlock the potential of cross-chain integrations with Ethereum.


Rendered proposal in branch

@iquerejeta
Copy link
Contributor

Thanks for opening this! Given that the main motivation of introducing ECDSA over SECP256k1 in plutus (#250) was to be compatible with Ethereum, I believe that including keccak256 as a built-in functions is a must. Otherwise, we would not achieve our initial goal of enabling cross-chain applications.

As it is mentioned in the CIP, introducing such a built-in function should be pretty straightforward. The primitive is already part of cardano-base, and would only require one additional built-in function in Plutus.

I fully support this going forward.

@Ryun1 Ryun1 changed the title CIP: Integration of keccak256 into Plutus. CIP-???? | Integration of keccak256 into Plutus Jun 19, 2023
@Ryun1 Ryun1 added the Category: Plutus Proposals belonging to the 'Plutus' category. label Jun 19, 2023
CIP-????/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@michaelpj for the Plutus implementors to accept this, wouldn't the CIP need to include the level of detail seen in CIP-0049? (maybe not necessary since "already available in cardano-base"?) https://github.com/cardano-foundation/CIPs/blob/master/CIP-0049/README.md

@serejke ... you might have used an earlier CIP as a model but we now have a more rigorous form for CIP documents. Can you edit your header & headings so they're consistent with this specification? https://github.com/cardano-foundation/CIPs/blob/master/CIP-0001/README.md

e.g. there's no Path to Proposed since your document will be submitted with Proposed status (and should say so in the header).

Hopefully we can address these formatting & content issues in the next 2 weeks, when we've planned for Plutus representatives to be present at our next CIP meeting... I've marked it for Review in the agenda of CIP meeting # 69 after we introduce it at today's meeting (45 minutes from now).

(p.s. I see some duplication with @Ryun1 review as I was writing this)

@rphair
Copy link
Collaborator

rphair commented Jun 20, 2023

Thanks @iquerejeta for coming to discuss this at the CIP editors' meeting today. Regarding the lack of technical detail in the document compared to CIP-0049 we noted that this is just adding a hashing function rather than a full signature scheme. Even so, we agreed that something should be added to the CIP text to explain this.

Also thanks @iquerejeta for agreeing to coordinate with @serejke about updating the CIP document format according to the new specification (CIP-0001) introduced in Q4 2022. Hopefully these changes can be made before a more rigorous review in the CIP meeting planned 2 weeks from now.

@iquerejeta
Copy link
Contributor

Yes! These changes will be addressed before the review meeting planned 2 weeks from now 👍 Thanks for addressing this CIP in today's session.

@serejke serejke changed the title CIP-???? | Integration of keccak256 into Plutus CIP-? | Integration of keccak256 into Plutus Jun 20, 2023
@serejke serejke changed the title CIP-? | Integration of keccak256 into Plutus CIP-???? | Integration of keccak256 into Plutus Jun 20, 2023
@hai-kreate
Copy link

This addition would 100% push interoperability forward.
We will utilize it the moment it is available :).

@serejke
Copy link
Contributor Author

serejke commented Jun 26, 2023

Thanks @rphair for the valuable comments on the content/formatting of this CIP. I've tried to address all the issues: fixed the preamble headers, elaborated on the Specification. Look forward to the CIP editors' opinion.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Thanks for the update @serejke ... the format is almost ready for review with just a few nitpicks & more specific recommendations about making it consistent with the CIP-0001 requirements.

I believe there is popular demand for the next CIP editors' meeting being Plutus themed, after more people get back from June holidays. My own first criteria for review would be first to ensure that the Plutus implementors have enough detail in this CIP for it to meet their own requirements & I would endorse it if so.

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Jun 30, 2023

FYI @serejke @iquerejeta CIP editors are all in agreement about assigning a number to this proposal as an official candidate... though it's still awaiting review by us (and Plutus reps) & some editing by you 🤓

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

new Path to Active section looks good... just one spelling fix please 🤓

CIP-????/README.md Outdated Show resolved Hide resolved
@michaelpj
Copy link
Contributor

The implementation is in progress here, so if we can get this CIP tidied up then I think we can safely merge it as Proposed.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@serejke my reading of @michaelpj's latest #524 (comment) is that you need to confirm you've satisfied all the requirements in @KtorZ's #524 (review).

@iquerejeta if you can't modify this branch and can make these changes, just post them as a review and we can merge them here after confirming they are suitable.

I think the "costing function" requirement is satisified by confirming that it's linear in one of the more recent commits (?), so that box might be ticked right away (if you confirm these items, any editor can tick the boxes for you 🤓).

Once all that's done, I think we can merge this.

Copy link
Contributor

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

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

I've added two comments to check a few boxes requested by @KtorZ . Costing will be done by the Plutus team, so we can update this CIP once we have costing done for keccak. I believe that other checkboxes can be marked. I didn't know how to argue that the function always terminates. Matthias, let me know if the rationale is complete in your opinion, or you think we should add something else.

CIP-????/README.md Outdated Show resolved Hide resolved
CIP-????/README.md Outdated Show resolved Hide resolved
rphair and others added 2 commits July 27, 2023 15:54
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

thanks @iquerejeta I will be the bean-counter for this and keep track of remaining issues so we can merge it ASAP. I've committed your suggestions in #524 (review) which leaves these two items from the checklist in #524 (review):

Since @KtorZ or another reviewer might help with the 1st item, it doesn't make sense to put this as Waiting for Author yet. I'll keep watching this & if ready we might present it again @ the 01 August CIP meeting and merge it then. cc @serejke

@iquerejeta
Copy link
Contributor

Ok. Regarding the argument that it always terminates, is it about the functionality (i.e. a hash function cannot fail as long as it receives a bytestring as input), or about the implementation itself?

@rphair
Copy link
Collaborator

rphair commented Jul 27, 2023

@michaelpj wrote those requirements (https://github.com/cardano-foundation/CIPs/tree/master/CIP-0035#additions-to-the-plutus-core-builtins) so I think he's the best one to answer that 🤓

@michaelpj
Copy link
Contributor

I think we're okay with the termination here. It would be very weird if a hash function didn't terminate. We do need to handle errors appropriately, but that's an implementation matter.

@rphair
Copy link
Collaborator

rphair commented Aug 15, 2023

@serejke @iquerejeta (cc @michaelpj @kwxm) marking Waiting for Author due to this remaining unresolved item from #524 (review):

  • Discussion of how any measures and costing functions were determined.

@rphair rphair added the State: Waiting for Author Proposal showing lack of documented progress by authors. label Aug 15, 2023
@iquerejeta
Copy link
Contributor

@michaelpj @kwxm , costing will be done (has been done?) by the Plutus team. What should we put in this section? Could you maybe point to some other CIP were we can copy the discussion on how costing will be determined?

@michaelpj
Copy link
Contributor

Well, in many cases I would expect to have some determination like "these functions are constant-time because we know crypto operations are constant time", but in this case I think it's fine to say "costing functions were determined empirically by the plutus team", since that's what actually happened.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

According to my understanding of @michaelpj's #524 (comment), 644a12f fixes the last of the requirements so I believe this is good to go.

@rphair rphair requested review from KtorZ and Ryun1 August 17, 2023 12:34
@kwxm

This comment was marked as duplicate.

@kwxm
Copy link
Contributor

kwxm commented Aug 18, 2023

@michaelpj @kwxm , costing will be done (has been done?) by the Plutus team. What should we put in this section? Could you maybe point to some other CIP were we can copy the discussion on how costing will be determined?

We have costing benchmarks for the two new functions here. Running these shows that the execution time is linear in the size of the input, as expected. However, to get sensible coefficients for the CPU cost which are consistent with the costs for the other builtins we have to run all of the benchmarks for all of the builtins on a reference machine, and our previous machine disappeared without warning when Cicero was introduced. We've been using an AWS instance for the benchmarking, but that's not very consistent. We're currently awaiting a new physical machine and when that's available we'll recalibrate the entire Plutus Core cost model, including the new hash functions. In the meantime I've reused the costing function for sha3_256 for keccak_256 since they're nearly the same algorithm, and I've re-used the costing function for blake2b_256 for blake2b_224 since the two functions are very similar, and the cost of the former should certainly be an upper bound for the cost of the latter.

We don't anticipate any difficulty in getting the final costing figures once suitable hardware is available. I think "The Plutus team determines the exact costing functions empirically" is a reasonable summary of all this.

@rphair rphair changed the title CIP-???? | Integration of keccak256 into Plutus CIP-0101 | Integration of keccak256 into Plutus Sep 5, 2023
@rphair
Copy link
Collaborator

rphair commented Sep 5, 2023

@serejke this has passed our Last Check review at the CIP Editors' Meeting today, so please rename your CIP folder to CIP-0101 and then we can provide the last required endorsing review and merge this (cc @iquerejeta).

@rphair rphair removed the State: Waiting for Author Proposal showing lack of documented progress by authors. label Sep 5, 2023
@rphair rphair merged commit af924ad into cardano-foundation:master Sep 7, 2023
Ryun1 added a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
…#524)

* CIP: Integration of `keccak256` into Plutus.

* single ? in CIP number for standard form

Co-authored-by: Ryan Williams <[email protected]>

* fix: update formatting of headers.

* fix: add a reference to CIP-49.

* fix: remove "Path to Proposed".

* fix: elaborate on the Specification.

* full title for Motivation

* full title for Rationale

* adding Copyright section in standard form

* fix: drop "Backward Compatibility" section.

* fix: drop "References" section.

* fix: add "Path to Active" subsections.

* fix: spelling of Mainnet.

* fix: add Michael Peyton Jones as an implementor.

* fix: add a real integration use case.

* fix: update the costing comment.

Co-authored-by: Iñigo Querejeta Azurmendi <[email protected]>

* obligatory language for acceptance as per cip35

Co-authored-by: Iñigo Querejeta Azurmendi <[email protected]>

* satisfying all cip35 criteria except guaranteed termination

Co-authored-by: Iñigo Querejeta Azurmendi <[email protected]>

* fix: CR suggestion on the costing functions.

* assign CIP number 101

* fix: rename folder to CIP-0101

---------

Co-authored-by: Robert Phair <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: Iñigo Querejeta Azurmendi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Plutus Proposals belonging to the 'Plutus' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants