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

Add WT-FROST to the worker gadget #711

Merged
merged 27 commits into from
Sep 27, 2023
Merged

Add WT-FROST to the worker gadget #711

merged 27 commits into from
Sep 27, 2023

Conversation

tbraun96
Copy link
Contributor

Closes #710

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

Attention: 354 lines in your changes are missing coverage. Please review.

Comparison is base (da48f39) 17.91% compared to head (a873b3f) 17.11%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage   17.91%   17.11%   -0.79%     
==========================================
  Files          77       80       +3     
  Lines        5696     5956     +260     
==========================================
- Hits         1020     1019       -1     
- Misses       4676     4937     +261     
Files Coverage Δ
...gadget/src/async_protocols/ecdsa/keygen/handler.rs 0.00% <ø> (ø)
...et/src/async_protocols/ecdsa/sign/state_machine.rs 0.00% <ø> (ø)
dkg-gadget/src/async_protocols/mod.rs 0.00% <ø> (ø)
dkg-gadget/src/storage/proposals.rs 0.00% <ø> (ø)
dkg-gadget/src/db/offchain_storage.rs 0.00% <0.00%> (ø)
dkg-gadget/src/worker.rs 0.00% <0.00%> (ø)
dkg-gadget/src/dkg_modules/mp_ecdsa.rs 0.00% <0.00%> (ø)
.../src/async_protocols/ecdsa/keygen/state_machine.rs 0.00% <0.00%> (ø)
dkg-gadget/src/dkg_modules/mod.rs 0.00% <0.00%> (ø)
...g-gadget/src/async_protocols/ecdsa/sign/handler.rs 0.00% <0.00%> (ø)
... and 7 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@drewstone drewstone left a comment

Choose a reason for hiding this comment

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

CI needs to be fixed.

@drewstone
Copy link
Contributor

Maybe you're missing some feature flags @tbraun96 ? What ultimately changed with how our integration tests run?

@tbraun96
Copy link
Contributor Author

What ultimately changed with how our integration tests run?

So far, nothing changed in how our integration tests are run. Relevantly, what has changed is that our FROST dependency needs to link to a C library, and it appears it lacks testing for pipeline builds (cargo test does not count, as the linking process is different. What is failing for us here is when we build binaries via cargo run or cargo build). I am in contact with @xoloki about this situation and am exploring ways to fix this issue. We may need a custom build.rs script for our frost dependency.

@xoloki
Copy link

xoloki commented Sep 25, 2023

I'm really not sure what's going on here. In stacks-sbtc we link wsts with the p256k1 dependency, and CI works fine on x86_64 and arm64 for both linux and macos. All we needed to do was add libclang-dev to our build pipeline.

I'll pull this repo and try to see what's going on. p256k1 has its own build.rs, which should be adequate on all platforms.

@xoloki
Copy link

xoloki commented Sep 26, 2023

I'll pull this repo and try to see what's going on. p256k1 has its own build.rs, which should be adequate on all platforms.

Just pulled and built thomas/frost-dkg, no problems. I just had to install protobuf-compiler and libgmp-dev.

@tbraun96 does this fail for you locally, or just in GH CI?

@drewstone
Copy link
Contributor

Just in CI @xoloki

@xoloki
Copy link

xoloki commented Sep 26, 2023

@drewstone @tbraun96 there seems to be a problem in how CI is setup and run.

I would expect that the Harness CI target comes from .github/workflows/harness_stress_tests.yml. But in that file, the Install Protobuf and LLVM step should run bitcoin.sh then sudo apt-get install protobuf-compiler llvm libsecp256k1-dev libsecp256k1-0 libclang-dev.

However, that's not what happens. The bitcoin.sh script does not run, and apt-get is run twice, once for protobuf-compiler and once for llvm. That's why p256k1 is failing to link, because libclang-dev is not being installed.

@xoloki
Copy link

xoloki commented Sep 26, 2023

However, for the Integration tests / Local Chain Tests target, we are actually installing libclang-dev, but the linker problem persists.

There are a couple of things about your CI setup which I note. First, you are using sccache for both failing tests. Second, you are using cargo-make for the integration test. Maybe remove these and try a basic cargo build step in each?

@xoloki
Copy link

xoloki commented Sep 26, 2023

I'd also remove the libsecp256k1 install calls, unless you're also using those for something else.

Basically, I'd just remove everything in the failing CI .yml files and build them from scratch, installing only what you need and seeing if cargo build works. Once that works then you can add stuff back in one piece at a time.

@tbraun96
Copy link
Contributor Author

The issue, as you hinted towards @xoloki , was sccache. Thank you so much for the help.

@drewstone drewstone changed the title [WIP] WT-FROST Add WT-FROST to the worker gadget Sep 27, 2023
@drewstone drewstone merged commit 6859e4f into master Sep 27, 2023
7 checks passed
@drewstone drewstone deleted the thomas/frost-dkg branch September 27, 2023 06:55
@xoloki
Copy link

xoloki commented Sep 27, 2023

The issue, as you hinted towards @xoloki , was sccache. Thank you so much for the help.

Glad I could help 😎

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.

Implement FROST DKG into the DKG
4 participants