-
Notifications
You must be signed in to change notification settings - Fork 334
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
Conversation
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. |
keccak256
into Plutus.keccak256
into Plutus
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.
@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)
Co-authored-by: Ryan Williams <[email protected]>
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. |
Yes! These changes will be addressed before the review meeting planned 2 weeks from now 👍 Thanks for addressing this CIP in today's session. |
keccak256
into Plutuskeccak256
into Plutus
keccak256
into Plutuskeccak256
into Plutus
This addition would 100% push interoperability forward. |
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. |
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.
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.
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 🤓 |
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.
new Path to Active section looks good... just one spelling fix please 🤓
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. |
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.
@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.
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'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.
Co-authored-by: Iñigo Querejeta Azurmendi <[email protected]>
Co-authored-by: Iñigo Querejeta Azurmendi <[email protected]>
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.
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):
- an argument that ... It always terminates (CIP-0101 | Integration of
keccak256
into Plutus #524 (comment)) - Discussion of how any measures and costing functions were determined.
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
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? |
@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 🤓 |
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. |
@serejke @iquerejeta (cc @michaelpj @kwxm) marking
|
@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? |
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. |
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.
According to my understanding of @michaelpj's #524 (comment), 644a12f fixes the last of the requirements so I believe this is good to go.
This comment was marked as duplicate.
This comment was marked as duplicate.
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 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. |
keccak256
into Plutuskeccak256
into Plutus
@serejke this has passed our Last Check review at the CIP Editors' Meeting today, so please rename your CIP folder to |
16c5cf3
to
f7e9619
Compare
…#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]>
This CIP proposes to add
keccak256
hash function to Plutus to unlock the potential of cross-chain integrations with Ethereum.Rendered proposal in branch