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

Update wsts dependency to get malicious DkgPrivateShares handling #4326

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

xoloki
Copy link
Collaborator

@xoloki xoloki commented Feb 1, 2024

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@xoloki xoloki requested review from jcnelson and jferrant February 1, 2024 15:52
@xoloki xoloki self-assigned this Feb 1, 2024
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

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

Comparison is base (2dcdb02) 83.20% compared to head (20d0ddb) 72.68%.

Files Patch % Lines
libsigner/src/messages.rs 15.66% 70 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             next    #4326       +/-   ##
===========================================
- Coverage   83.20%   72.68%   -10.52%     
===========================================
  Files         446      446               
  Lines      318455   318545       +90     
===========================================
- Hits       264981   231548    -33433     
- Misses      53474    86997    +33523     

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

@ASuciuX
Copy link
Contributor

ASuciuX commented Feb 7, 2024

I was checking the mutants for different PRs. This happened on my fork, after merging the next branch on the feat/handle-dkg-private-malicious branch. https://github.com/ASuciuX/stacks-core/actions/runs/7815626949/job/21319931428#step:2:2288

I also did another test by merging this PR on my fork on the up-to-date next branch. Then I run cargo build locally and it fails the same way:

Cargo build ERROR output
error[E0599]: no method named `as_bytes` found for reference `&wsts::net::DkgFailure` in the current scope
   --> libsigner/./src/messages.rs:304:41
    |
304 |                 write_next(fd, &failure.as_bytes().to_vec())
    |                                         ^^^^^^^^ method not found in `&DkgFailure`

error[E0308]: mismatched types
   --> libsigner/./src/messages.rs:318:36
    |
318 |                 DkgStatus::Failure(failure)
    |                 ------------------ ^^^^^^^ expected `DkgFailure`, found `String`
    |                 |
    |                 arguments to this enum variant are incorrect
    |
note: tuple variant defined here
   --> /Users/asuciu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-8.0.0/src/net.rs:76:5
    |
76  |     Failure(DkgFailure),
    |     ^^^^^^^

Some errors have detailed explanations: E0308, E0599.
For more information about an error, try `rustc --explain E0308`.
error: could not compile `libsigner` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

Looks like the first error that comes up with the new wsts version is the failure variable being a DkgFailure instead of a string, and it doesn’t implement a conversion to bytes. The second error that can be seen there is the field of the DkgStatus::Failure enum argument was previously a string, but it got changed to DkgFailure - and the failure argument is defined as a string in the code.

@xoloki xoloki force-pushed the feat/handle-dkg-private-malicious branch from c650689 to 268937d Compare February 8, 2024 15:51
@xoloki
Copy link
Collaborator Author

xoloki commented Feb 8, 2024

The merge from next messed up the commit history, so I did a clean rebase and a force push.

I need to fix the build errors now by implementing StacksMessageCodecExtensions for DkgFailure and friends.

@xoloki xoloki force-pushed the feat/handle-dkg-private-malicious branch from 7678b03 to cdc8b41 Compare February 8, 2024 18:07
@xoloki
Copy link
Collaborator Author

xoloki commented Feb 8, 2024

@jcnelson are we counting on serialization to be identical for hashing or other purposes? HashMap/HashSet do not have well ordered iteration, so the existing HashMap serializations and this PR's HashSet serializations can give different byte streams depending on various factors.

@xoloki xoloki force-pushed the feat/handle-dkg-private-malicious branch from cdc8b41 to d546e9e Compare February 8, 2024 20:36
…c keys in Point format to coordinator config

fmt fixes

implement StacksMessageCodecExtensions for DkgFailure and friends

flesh out skeleton StacksMessageCodecExtensions for DkgFailure

fix test to use proper DkgFailure enum not string
@xoloki xoloki force-pushed the feat/handle-dkg-private-malicious branch from d546e9e to 20d0ddb Compare February 8, 2024 22:34
@xoloki xoloki merged commit be5ea05 into next Feb 9, 2024
1 of 2 checks passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants