Support depending on Haskell libraries instead of pkg-depends#621
Support depending on Haskell libraries instead of pkg-depends#621hsyl20 wants to merge 1 commit intoIntersectMBO:masterfrom
Conversation
21f1fab to
49558b3
Compare
49558b3 to
67e34c4
Compare
67e34c4 to
c2b3fdd
Compare
lehins
left a comment
There was a problem hiding this comment.
I am not sure I understand the reasoning for adding this complexity.
- First of all we can't use this approach with
cardano-nodeas it is today, since none of these packages are on Hackage/CHaP and we can't be using SRPs - None of these packages are maintained by IOG or Intersect
- Most importantly, however, they are forks of original libraries, which means we/someone at https://github.com/haskell-cryptography would have to maintain forks of those crypto packages.
I don't understand what kind of control do we need for cardano-node that we don't already have today?
In my opinion using this approach adds a weak point, instead of being an improvement, since cryptographic indirection is maintained by a third party. We can trust downstream C dependencies like libsodium, blst and secp256k. But if we switch to using forks, even if the original C code doesn't change, we need to constantly keep an eye on it and do an audit every time we need to use a new release. From that perspective in my opinion it is a regression, not an improvement.
To sum it up. I am personally very much against this approach, since:
- it doesn't buy us anything new (aside form helping someone on Windows, which should not be our main goal anyways)
- it increases overhead and complexity due to the need to maintain those forks, without IOG or Intersect having total control of them either. And if we did, why should we maintain them and update for every release of crypto dependency?
I would really to see some strong motivating factors for introducing such indirection. Also I'd like hear some input on this from @angerman, @perturbing and @disassembler cause that is serious change to how we deal with cryptography and we shouldn't take it lightly.
|
I should have specified that we're interested in doing this for Plinth, not for cardano-node. For Plinth, this approach reduces installation complexity (users don't have to install the crypto libs themselves). And we care about Windows users being able to use Plinth.
I agree that maintaining forks is less than ideal.
|
perturbing
left a comment
There was a problem hiding this comment.
I appreciate the goal to improve DevX (@rober-m), as indeed plinth building is nontrivial if you are not tech-savvy.
That said, I agree with @lehins that it is not really ideal how one should maintain this (and who is responsible).
What is the impact if such an addition is not merged? Are there no alternatives?
We already ask users to install a fork of libsodium (cf https://github.com/IntersectMBO/cardano-base/blob/master/INSTALL.md) that is lagging several years behind upstream, so the situation isn't totally new.
Also note that since that was written, the VRF that is on our fork has been merged into libsodium itself. So you can either use the old fork or any new version of libsodium now.
This is the most important piece of information that was needed for this PR, which clarifies it quite a bit. I would like to ask you to document the cabal flag as such. Something along the lines: "This flag is used for Plinth users and should not be used when compiling
Exactly, that situation serves as a proof that we should try and avoid doing that in the future. I've created a ticket to abandon custom version of libsodium: #636
This still doesn't give me confidence when comes to using those for cardano-node. Fork is still a fork. It has different set of people that has access to the code that the original maintainers of crypto packages, which as I pointed out earlier creates a week link.
If these packages are not going to be used for cardano-node, it's fine to not have them in IOG or Interesect, but I am not sure what are the requirements of Plinth are. I am still quite skeptical about this approach, maybe Plint should create a lightweight fork of cardano-crypto-class and depend on that instead? For now, however, I am not going to appose merging it, since it will have no affect on cardano-node, which is my main concern. |
lehins
left a comment
There was a problem hiding this comment.
Having a flag like this that is untested on CI is a not good.
IF we are to proceed with this, we definitely need at least one CI action that runs all the tests for cardano-crypto-class and cardano-crypto-praos with use-haskell-clibs flag on
cabal.project
Outdated
| location: https://github.com/haskell-cryptography/secp256k1-clib | ||
| tag: 211b95baad422966c9e719ed70cbc189c58eaae5 | ||
|
|
||
| package secp256k1-clib |
There was a problem hiding this comment.
| package secp256k1-clib | |
| -- These configuration flags are only needed when `cardano-crypto-class` is used with `use-haskell-clibs` | |
| package secp256k1-clib |
c2b3fdd to
b8dca78
Compare
|
Thanks a lot for the feedback! I've removed the most controversial bits and updated the comments. However I agree with:
And:
So maybe our best way forward is to have Plinth use its own branch of cardano-base with this minimal patch applied (it already has its own cabal.project with the required flags and SRPs). In which case we can close this PR. Another thing I'm wondering is: does Plinth actually need any of the features provided by the C libs? If it doesn't, then maybe we could add a flag to make them optional in cardano-crypto-class/praos (no idea how possible it is). It would be the simplest: no fork to deal with at all. I'll discuss this with the Plinth team. |
|
Thank you, @perturbing, for pinning me. Dealing with Plinth is significantly more painful than with alternatives, so anything that improves devx is very welcome! @zliu41 @SeungheonOh, what do you think about:
Somewhat related issue: IntersectMBO/plutus#7145 |
Description
This MR allows the use of Haskell crypto libraries (blst-clib, sodium-clib, secp256k1-clib) instead of globally installed ones.
Pro:
Con:
I'm opening this MR because it's already been helpful to someone on Windows. It might become the recommended way to build cardano-base in the future but I'd like to have some benchmarks to ensure that performance doesn't regress while switching to the cabal-built packages.
Checklist
CHANGELOG.mdfor the affected packages.New section is never added with the code changes. (See RELEASING.md)
.cabalandCHANGELOG.mdfiles according to theversioning process.
.cabalfiles for all affected packages are updated.If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)