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

System API for ECDSA signing #79

Merged
merged 67 commits into from
May 30, 2022
Merged

System API for ECDSA signing #79

merged 67 commits into from
May 30, 2022

Conversation

marcin-dziadus
Copy link
Contributor

@marcin-dziadus marcin-dziadus commented May 20, 2022

This PR Implements a corresponding part of the IC spec: dfinity/interface-spec#6

It also splits up IC.Test.Agent and IC.Test.Spec to reduce the excessive memory consumption of compiling those modules.

@marcin-dziadus marcin-dziadus requested review from a user and basvandijk May 20, 2022 12:35
Copy link
Contributor

@basvandijk basvandijk left a comment

Choose a reason for hiding this comment

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

LGTM!

I'll help with fixing CI.

@marcin-dziadus

This comment was marked as outdated.

@marcin-dziadus marcin-dziadus marked this pull request as ready for review May 23, 2022 09:18
.github/workflows/docs.yml Show resolved Hide resolved
src/IC/Ref.hs Outdated Show resolved Hide resolved
basvandijk added a commit that referenced this pull request May 23, 2022
We're noticing that on #79 GHC
requires massive amounts of memory to build certain
modules (`IC.Test.Agent` being the worst offender requiring more than
13GB while GitHub Runners only have 7GB causing OOM kills).

What's worse is that multiple components in the cabal file require the
same modules causing the same module to be build multiple times, often
in parallel exacerbating the memory problem.

This fixes the duplicate compilation issue by letting the components
in the cabal file only depend on the ic-hs library.

This might be deterimental to ghcid as mentioned in the cabal
file. But the associated ticket has been closed so the problem is
hopefully fixed in an upcoming GHC release.
@nomeata

This comment was marked as resolved.

@nomeata

This comment was marked as resolved.

@marcin-dziadus

This comment was marked as resolved.

@basvandijk
Copy link
Contributor

basvandijk commented May 27, 2022

Status report

For building ic-ref-dist I switched to pkgsStatic to get a static secp256k1. However pkgsStatic removes all dynamic libraries (.so). This is problematic for Haskell modules using TemplateHaskell because they require dynamic libraries to be available.

See: NixOS/nixpkgs#61575

For this reason I had to remove TemplateHaskell support from QuickCheck. But haskell-candid also uses TemplateHaskell which can't be disabled. So that package is failing now.

Not yet sure how to work around this...

@basvandijk
Copy link
Contributor

basvandijk commented May 28, 2022

The TemplateHaskell issues have been fixed by patching GHC and disabling haddock in a few libraries.

Now hopefully the last remaining issue is the following error when building ic-ref-dist:

src/IC/Crypto/BLS.hsc:23 directive strict_import cannot be handled in cross-compilation mode

@nomeata mentioned a similar issue in rethab/bindings-dsl#38.

@nomeata
Copy link
Contributor

nomeata commented May 28, 2022

So maybe 47e1274 cab be cherry-picked?

@basvandijk
Copy link
Contributor

I think IC.Crypto.BLS is simple enough to replace bindings-DSL with raw hsc.

@basvandijk
Copy link
Contributor

So maybe 47e1274 cab be cherry-picked?

Done. @nomeata thanks for the pointer!

@basvandijk
Copy link
Contributor

basvandijk commented May 28, 2022

Next problem is that ic-ref-dist refers to the warp Haskell library which is not allowed (allowedReferences= []).

The reason is that the ic-ref executable from ic-hs-static references warp:

$ grep -a /nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17 \
          /nix/store/27samy25mh98r4glq2krkmv68l9mxb5a-ic-hs-static-x86_64-unknown-linux-musl-0.0.1/bin/ic-ref
...warp_bindir/nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17/binwarp_libdir/nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17/lib/ghc-8.10.7/x86_64-linux-ghc-8.10.7/warp-3.3.17-LFuiV3JNZfpKQMWWUSmbjdwarp_dynlibdir/nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17/lib/ghc-8.10.7/x86_64-linux-ghc-8.10.7warp_datadir/nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17/share/x86_64-linux-ghc-8.10.7/warp-3.3.17warp_libexecdir/nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17/libexec/x86_64-linux-ghc-8.10.7/warp-3.3.17warp_sysconfdir/nix/store/rg01ag16m4kk1xrp6ypw6ypj0wqy5bmk-warp-static-x86_64-unknown-linux-musl-3.3.17/etc...

Note that the references to warp are the only references to the /nix/store.

@basvandijk
Copy link
Contributor

@nomeata any idea how these paths to warp end up in ic-ref?

@basvandijk
Copy link
Contributor

Just linking NixOS/nixpkgs#43795 (comment) here because I think we can simplify our static linking setup when we upgrade to release-22.05 (like removing our GHC override).

@nomeata
Copy link
Contributor

nomeata commented May 29, 2022

Next problem is that ic-ref-dist refers to the warp Haskell library which is not allowed (allowedReferences= []).

If these paths are actually unused and don’t affect whether it runs, you can use removeReferencesTo to be unblocked here, right?

I guess they are there because warp uses the Paths_warp module generated by Cabal, but only use the version number, so probably fine to remove:

/tmp/warp-3.3.19 $ grep -r -w Paths_warp 
warp.cabal:                     Paths_warp
warp.cabal:                     Paths_warp
Network/Wai/Handler/Warp/Response.hs:import qualified Paths_warp
Network/Wai/Handler/Warp/Response.hs:warpVersion = showVersion Paths_warp.version
Network/Wai/Handler/Warp/Settings.hs:import qualified Paths_warp
Network/Wai/Handler/Warp/Settings.hs:    , settingsServerName = C8.pack $ "Warp/" ++ showVersion Paths_warp.version

@basvandijk
Copy link
Contributor

basvandijk commented May 29, 2022

I guess they are there because warp uses the Paths_warp

On our way to the cinema to see Top Gun my brother @roelvandijk and I came to the same conclusion when discussing this problem ;)

Thanks for confirming our hypothesis!

I'll remove the references with removeReferencesTo.

@basvandijk basvandijk merged commit 4920ec2 into master May 30, 2022
@basvandijk basvandijk deleted the marcin/ecdsa-sig branch May 30, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants