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

Remove Crypto parametrization #4223

Open
lehins opened this issue Mar 28, 2024 · 4 comments
Open

Remove Crypto parametrization #4223

lehins opened this issue Mar 28, 2024 · 4 comments
Labels
enhancement New feature or request performance simplification A nice to have improvement that simplifies the implementation

Comments

@lehins
Copy link
Collaborator

lehins commented Mar 28, 2024

In light of recent discovery of crypto parametrization having serious overhead on the cardano-node due to lost specialization with ghc-9.x we decided that it will be a good idea to remove crypto parametrization completely.

I was very much aware of the fact that Crypto adds a significant cognitive overhead, but previously I only speculated that it could have performance overhead. Now we have confirmation that this is indeed the case.

I believe, originally the goal for crypto parametrization was to allow using different cryptographic primitives for testing, which turned out to be only a partially useful idea.
Last year while investigating potentially adding BatchVRF I questioned the usefulness of Crypto and we came to few realizations:

  • Ledger only uses HASH, ADDRHASH and DSIGN, which can never change. Therefore there is no point in them being a type family.
  • VRF and KES are used by the consensus codebase for things like block header validation, etc. Which means ledger has no need for those type families and they should only be able to change for different block header types.

These two discoveries made us realize that we do not need crypto parametrization in ledger at all. If we were to remove it, regression with performance would go away. So, not only GHC would have a much easier time with specializing, but the codebase and downstream uses of ledger would become much simpler, thus leading to less confusion about ledger types, which have enough complexity in them already. I lost count how many times I had to explain type errors when people confused era and crypto type parameters.

So, there are two major tasks that need to be done in order to get rid of Crypto:

  • Move VRF and KES from Crypto to something like HeaderCrypto and make block header parametrized by crypto, which was implemented as proof of concept a year ago: Split Crypto into Crypto and HeaderCrypto #3388
  • Remove Crypto parametrization from all ledger eras.

Both of those tasks will require significant amount of work because they would touch a lot of code, but they be would very well worth it. Therefore, we can't tackle this task until after Conway is released.

Few related notes:

  • Removal of crypto type parametrization would not require a hard fork, so we can tackle it without introducing a new era
  • Better do it sooner rather than later, cause longer we wait, the harder it will be to rip it out
  • Restricting c ~ StandardCrypto will not work as interim solution, because that would require us removing a whole bunch of tests that use MockCrypto for dealing with fake VRF and KES
@lehins lehins added enhancement New feature or request performance simplification A nice to have improvement that simplifies the implementation labels Mar 28, 2024
@doyougnu
Copy link
Contributor

doyougnu commented Apr 19, 2024

Hello!

Is there any plan to close this ticket? Is it on a roadmap somewhere? I would expect that for such a major undertaking there would be a list of packages that use c ~ StandardCrypto and then you would check them off as you finish them to complete the migration. For example, this GHC ticket identifies exactly the subsystems and modules that had to change in GHC to reach the stated goal.

@lehins
Copy link
Collaborator Author

lehins commented Apr 19, 2024

Is there any plan to close this ticket?

Yes, there is a plan. But we will not start working on this, until we have solved higher priority issues, like deploying Conway.

Is there any plan to close this ticket?

There is no point, because every single package downstream from ledger will be affected by this change.

@doyougnu
Copy link
Contributor

Perhaps I misunderstand the problem but if every package downstream is impacted then wouldn't it make sense to begin with those packages and remove the dependencies on Crypto, swapping them with StandardCrypto. Then continue until each package does not depend on class Crypto in ledger, then remove it?

@lehins
Copy link
Collaborator Author

lehins commented Apr 19, 2024

then wouldn't it make sense to begin with those packages and remove the dependencies on Crypto, swapping them with StandardCrypto.

No, the goal is to completely remove crypto parameter and StandardCrypto type altogether

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance simplification A nice to have improvement that simplifies the implementation
Projects
None yet
Development

No branches or pull requests

2 participants